A few comments. The requirement that the caller of SetElementsToPixelData can only delete the backing store when there are no references to the v8 object left effectively requires him to do all the bookkeeping of setting up weak callbacks etc. himself to determine when the array can be safely deleted. I assume we happen to have a weak callback already in chromium so the work is done for us, but from an api viewpoint it would be consistent with how we handle external strings and less error prone if we used an abstraction to hold the byte array that dealt explicitly with the lifetime of the data.
Is there a reason why the interface morphs an existing general object rather than define a new type of jsobject? Is the behavior of a pixel array very specific to holding pixels or could it be described as something more general -- a truncating byte array maybe? If it is more general then we might want to use a more general name for it in the api. On Tue, Jul 28, 2009 at 10:22 AM, <[email protected]> wrote: > > > http://codereview.chromium.org/159263/diff/55/58 > File include/v8.h (right): > > http://codereview.chromium.org/159263/diff/55/58#newcode1172 > Line 1172: void SetElementsToPixelData(uint8_t* data, int length); > On 2009/07/28 06:07:27, Kasper Lund wrote: >> I still think you should avoid using the Elements name here, because > it's not >> really a term we use in the API. Also a comment on what this does and > that the >> 'embedder' still owns the data would be nice. > > Changed to SetIndexedPropertiesToPixelData to be inline with other > naming within this file. > > http://codereview.chromium.org/159263/diff/55/59 > File src/api.cc (right): > > http://codereview.chromium.org/159263/diff/55/59#newcode2201 > Line 2201: i::Handle<i::PixelArray> pixels = > i::Factory::NewPixelArray(length, data); > On 2009/07/28 06:07:27, Kasper Lund wrote: >> Should we have an API check (that also checks in release mode) that > self isn't a >> JSArray? I think that would be nice given the constraints of the > current >> implementation. Maybe also check that length is a valid smi? > > Done. > > http://codereview.chromium.org/159263/diff/55/65 > File src/api.h (right): > > http://codereview.chromium.org/159263/diff/55/65#newcode102 > Line 102: ASSERT(value()->GetElementsKind() == > On 2009/07/28 06:07:27, Kasper Lund wrote: >> Keep as HasFastElements? > > Done. > > http://codereview.chromium.org/159263/diff/55/65#newcode109 > Line 109: ASSERT(value_->GetElementsKind() == > On 2009/07/28 06:07:27, Kasper Lund wrote: >> Keep as HasFastElements? > > Done. > > http://codereview.chromium.org/159263/diff/55/64 > File src/ia32/ic-ia32.cc (right): > > http://codereview.chromium.org/159263/diff/55/64#newcode287 > Line 287: // Check whether the elements are a pixel array. > On 2009/07/28 06:07:27, Kasper Lund wrote: >> are -> is > > Done. > > http://codereview.chromium.org/159263/diff/55/62 > File src/objects.cc (right): > > http://codereview.chromium.org/159263/diff/55/62#newcode1676 > Line 1676: switch (JSObject::cast(pt)->GetElementsKind()) { > On 2009/07/28 06:07:27, Kasper Lund wrote: >> It might be clearer with !JSObject::cast(pt)->HasDictionaryElements, > but I'm >> okay either way. > > Done. > > http://codereview.chromium.org/159263/diff/55/62#newcode5879 > Line 5879: case PIXEL_ELEMENTS: { > On 2009/07/28 06:07:27, Kasper Lund wrote: >> Move above the DICTIONARY_ELEMENTS case to match the more common > ordering. > > Done. > > http://codereview.chromium.org/159263/diff/55/67 > File src/objects.h (right): > > http://codereview.chromium.org/159263/diff/55/67#newcode115 > Line 115: class Array; > On 2009/07/28 06:07:27, Kasper Lund wrote: >> Move this to globals.h? > > Done. > > http://codereview.chromium.org/159263/diff/55/67#newcode2472 > Line 2472: // This accessor applies the correct conversion and clamping > on the value. > On 2009/07/28 06:07:27, Kasper Lund wrote: >> Maybe extend the comment to state that it only deals with numbers and > undefined >> values? > > Done. > > http://codereview.chromium.org/159263 > > > > --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
