[Webkit-unassigned] [Bug 63481] Bring V8's SerializedScriptValue implementation up to date

Thu Jul 14 11:42:13 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=63481

David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #100738|review?                     |review-
               Flag|                            |

--- Comment #25 from David Levin <levin at chromium.org>  2011-07-14 11:42:13 PST ---
(From update of attachment 100738)
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-passing.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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.