It might be nice to have a runtime call for attaching a PixelArray to an object from JS code to facilitate easier testing. First round of comments:
http://codereview.chromium.org/159263/diff/1012/1015 File include/v8.h (right): http://codereview.chromium.org/159263/diff/1012/1015#newcode1172 Line 1172: void SetElementsToPixelData(uint8_t* data, int length); I think this deserves a comment. Do we use the concept of 'Elements' in the API elsewhere? Maybe it could be something like AssociateWithPixelData instead? Who owns the data array? There seems to be no weak callbacks, so is that entirely up to the embedder to deal with? http://codereview.chromium.org/159263/diff/1012/1022 File src/api.h (right): http://codereview.chromium.org/159263/diff/1012/1022#newcode102 Line 102: ASSERT(value()->GetElementsKind() == Why not keep the HasFastElements checker? http://codereview.chromium.org/159263/diff/1012/1018 File src/handles.cc (right): http://codereview.chromium.org/159263/diff/1012/1018#newcode349 Line 349: *value.location() = *number; // Overwrite the value being passed in. This seems fishy to me. Why not just do 'value = number'? http://codereview.chromium.org/159263/diff/1012/1017 File src/heap.cc (right): http://codereview.chromium.org/159263/diff/1012/1017#newcode1874 Line 1874: size > MaxObjectSizeInPagedSpace() ? LO_SPACE : OLD_DATA_SPACE; Do we ever get pixel arrays in LO_SPACE? Aren't they super small? Maybe change to an assert? http://codereview.chromium.org/159263/diff/1012/1017#newcode1889 Line 1889: AllocationSpace space = Do we ever get pixel arrays in LO_SPACE? Aren't they super small? Maybe change to an assert? http://codereview.chromium.org/159263/diff/1012/1017#newcode2062 Line 2062: ASSERT(JSObject::cast(result)->GetElementsKind() == JSObject::FAST_ELEMENTS); I would keep HasFastElements. http://codereview.chromium.org/159263/diff/1012/1029 File src/heap.h (right): http://codereview.chromium.org/159263/diff/1012/1029#newcode434 Line 434: static Object* AllocatePixelArray(int lenght, uint8_t* external_pointer); lenght -> length http://codereview.chromium.org/159263/diff/1012/1029#newcode434 Line 434: static Object* AllocatePixelArray(int lenght, uint8_t* external_pointer); Do we really need this variant? Is it because it's faster than having a default value for pretenure? http://codereview.chromium.org/159263/diff/1012/1021 File src/ia32/ic-ia32.cc (right): http://codereview.chromium.org/159263/diff/1012/1021#newcode294 Line 294: __ cmp(eax, FieldOperand(ecx, FixedArray::kLengthOffset)); Shouldn't this be PixelArray::kLengthOffset? http://codereview.chromium.org/159263/diff/1012/1021#newcode422 Line 422: __ cmp(ebx, FieldOperand(ecx, FixedArray::kLengthOffset)); PixelArray::kLengthOffset? http://codereview.chromium.org/159263/diff/1012/1019 File src/objects.cc (right): http://codereview.chromium.org/159263/diff/1012/1019#newcode1153 Line 1153: break; Remove this break? http://codereview.chromium.org/159263/diff/1012/1019#newcode2249 Line 2249: CHECK(!HasPixelElements()); Maybe change to an ASSERT? http://codereview.chromium.org/159263/diff/1012/1019#newcode2336 Line 2336: CHECK(!HasPixelElements()); ASSERT? http://codereview.chromium.org/159263/diff/1012/1019#newcode2419 Line 2419: // TODO(iposva): Cleanup. What kind of cleanup is missing here? http://codereview.chromium.org/159263/diff/1012/1019#newcode2762 Line 2762: // TODO(iposva): Cleanup ? http://codereview.chromium.org/159263/diff/1012/1019#newcode2884 Line 2884: JSObject* jsObject = JSObject::cast(obj); Weird naming here? jsObject -> js_object (not your fault, but anyway) http://codereview.chromium.org/159263/diff/1012/1019#newcode3110 Line 3110: UNIMPLEMENTED(); I think you have to deal with this and add a test case that proves that the code works. http://codereview.chromium.org/159263/diff/1012/1019#newcode5323 Line 5323: break; Having a break after return seems a bit weird. http://codereview.chromium.org/159263/diff/1012/1019#newcode5370 Line 5370: } Missing break (and test case). http://codereview.chromium.org/159263/diff/1012/1019#newcode5759 Line 5759: UNIMPLEMENTED(); This needs implementing - and a test case ;-) http://codereview.chromium.org/159263/diff/1012/1019#newcode7068 Line 7068: // TODO(iposva): Check for NaN values here. Needs an implementation and a test case. http://codereview.chromium.org/159263/diff/1012/1024 File src/objects.h (right): http://codereview.chromium.org/159263/diff/1012/1024#newcode2460 Line 2460: // In particular write access do clamp the values to 0 or 255 if the value do clamp -> clamps http://codereview.chromium.org/159263/diff/1012/1024#newcode2472 Line 2472: // This accessor does apply the correct conversion and clamping on the value. does apply -> applies http://codereview.chromium.org/159263/diff/1012/1023 File src/runtime.cc (right): http://codereview.chromium.org/159263/diff/1012/1023#newcode174 Line 174: UNIMPLEMENTED(); Implementation? http://codereview.chromium.org/159263/diff/1012/1023#newcode3733 Line 3733: if (array->GetElementsKind() != JSObject::FAST_ELEMENTS) { Keep as !HasFastElements. http://codereview.chromium.org/159263/diff/1012/1023#newcode5209 Line 5209: UNIMPLEMENTED(); This probably needs an implementation too. It seems to be used by the debugger. http://codereview.chromium.org/159263/diff/1012/1013 File test/cctest/test-api.cc (right): http://codereview.chromium.org/159263/diff/1012/1013#newcode7585 Line 7585: "};" Why ;? (repeated) http://codereview.chromium.org/159263/diff/1012/1013#newcode7633 Line 7633: result = CompileRun("pixels[7] = undefined;" I might be nice to make sure this also works from an IC (add loop). http://codereview.chromium.org/159263/diff/1012/1013#newcode7638 Line 7638: result = CompileRun("pixels[6] = '2.3';" I might be nice to make sure this also works from an IC (add loop). http://codereview.chromium.org/159263 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
