LGTM, with a few comments:
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); 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. 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); 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? 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() == Keep as HasFastElements? http://codereview.chromium.org/159263/diff/55/65#newcode109 Line 109: ASSERT(value_->GetElementsKind() == Keep as HasFastElements? 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. are -> is 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()) { It might be clearer with !JSObject::cast(pt)->HasDictionaryElements, but I'm okay either way. http://codereview.chromium.org/159263/diff/55/62#newcode5879 Line 5879: case PIXEL_ELEMENTS: { Move above the DICTIONARY_ELEMENTS case to match the more common ordering. 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; Move this to globals.h? http://codereview.chromium.org/159263/diff/55/67#newcode2472 Line 2472: // This accessor applies the correct conversion and clamping on the value. Maybe extend the comment to state that it only deals with numbers and undefined values? http://codereview.chromium.org/159263 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
