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

[bugzilla-daemon at webkit.org]

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





--- Comment #13 from David Levin <levin at chromium.org>  2011-07-11 16:10:31 PST ---
(From update of attachment 99884)
View in context: https://bugs.webkit.org/attachment.cgi?id=99884&action=review

Some initial (minor) comments. I need to stare at the code a bit more tonight.

> LayoutTests/ChangeLog:11
> +        

Usually there is only the bug title here (or a very short description).

> LayoutTests/ChangeLog:15
> +

This is where you would put more details relevant to this part of the chnage.

I think the majority of these comments apply to the changes done in WebCore, so I'd recommend factoring your comments appropriately.

> Source/WebCore/ChangeLog:8
> +        - Amends tests and introduces new ones to check for correctness (the bulk of the patch)

This patch is really three or four different things.

It would have been nice to structure the patches that way to reduce the size and make the reviewing potentially faster.

For example,  adding the error codes could be one patch. Adding versioning another patch.  Etc.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:124
> +public:

public nearly always is the first thing in classes and private, the second.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:164
> +// There is a ref table that maps uint32_ts to v8::Values.

uint32_t's?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1400
> +            if (shortLength * sizeof(int16_t) != byteLength)

Should be sizeof(uint16_t)

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