[webkit-reviews] review denied: [Bug 63481] Bring V8's SerializedScriptValue implementation up to date : [Attachment 100738] Address comments (re: code and tests both); attempt to send as text/plain.

Thu Jul 14 11:42:12 PDT 2011

David Levin <levin at chromium.org> has denied Luke Zarko
<lukezarko+bugzilla at gmail.com>'s request for review:
Bug 63481: Bring V8's SerializedScriptValue implementation up to date
https://bugs.webkit.org/show_bug.cgi?id=63481

Attachment 100738: Address comments (re: code and tests both); attempt to send
as text/plain.
https://bugs.webkit.org/attachment.cgi?id=100738&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100738&action=review

Ok only a few more comments to address and this should be good to go. Thanks!

> LayoutTests/platform/chromium/fast/canvas/webgl/array-message-passing.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

"This behavior is not currently in the standard but is there to reflect
inadvertent behavior from the previous version of the code that users have come
to expect. That being said, I fully expect that this test will apply to the
standard once it's completed; at that point we can promote it to a
WebKit-global test."

I've been thinking about this more and even if we don't follow the standard
here. One of our goals in WebKit is to have consistency among ports using
WebKit, so I don't think this should be a chromium only test. Frankly this
behavior only makes sense and we've seen some sites using it. (For example,
this site was one example:
http://juliamap.googlelabs.com/#c=0,0$z=0$p=ffffff,ffffff,ffffff,ffffff,ff0000,
ffff00,ffff00,ff00,ff$f=mandelbrot).

>
LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-pass
ing.js:124
> +    ['DataView1_ofs_end', new DataView(createBuffer(1), 1, 0),
dataViewCompare],

ofs (Please make it more readable).

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:385
> +	       append(DataViewTag);

else
  ASSERT_NOT_REACHED();

In addition, to code that can't have errors detected by the compile, awkward if
else structures, this also leads to unnecessary vtables bloat.

Do you think the ArrayBufferView::is* should change to an enum value (in a
future patch)?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1624
> +	       array = composite.As<v8::Array>();

What happens if the length given here doesn't agree with the length given when
doing "newArray"?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1629
>	   const int depth = stackDepth() - length;

This algorithm in general seems very paranoid (about possible corrupted data),
but in this case depth could go negative if given a bad length but it appears
ok with that. Should it check?