[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 #15 from David Levin <levin at chromium.org>  2011-07-12 11:58:13 PST ---
(From update of attachment 99884)
View in context: https://bugs.webkit.org/attachment.cgi?id=99884&action=review

A few comments some where giving an overview would be nice.  This would give context on why there needs to be things like GenerateFreshObjectTag, so one can read the code and get a better feel for how things are suppose to flow.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:185
> +    ArrayTag = '[', // length:uint32_t -> pops the last array from the open stack;

I wonder if it is good to have the implementation details here instead of at the serialization code (when it would help explain the code and hopefully get change more readily if the impl changes).

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:222
> +// Increment this for each change to the wire format.

For each incompatible change?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:949
> +        return objectId;

Return value never used. Consider removing.

What is the meaning of "greyObject"? I'm sure it is something familiar to folks who work on GC/VM but not to me.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1181
> +            // ArrayBuffers are always written before dependant ArrayBufferViews.

dependent (since we use color instead of colour :) ).

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1232
> +            --m_position;

This seems yucky to embed the knowledge of how many bytes were read during readTag.

Perhaps there should be a peekTag instead?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1586
> +    virtual bool consumeTopOfStack(v8::Handle<v8::Value>* obj)

Use object (avoid abbreviations in WebKit code).

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1595
> +    virtual bool finishArray(uint32_t length, v8::Handle<v8::Value>* value)

Calling this finishArray makes it sound like it is matched with a startArray but it isn't always.  When the version is 0, only finishArray will be called.

Ditto for finishObject and finishSparseArray.

This issue naming makes it harder to understand the code (and more likely for future mistake when folks naturally assume that these calls will be paired).

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1656
> +    virtual const v8::Handle<v8::Value>& pushObjectReference(const v8::Handle<v8::Value>& obj)

s/obj/object/

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1659
> +        return obj;

No one uses the return value. Why have it? (I understand the composability in theory but best to add it if/when needed).

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1662
> +    virtual bool getObjectReference(uint32_t ref, v8::Handle<v8::Value>* obj)

s/ref/reference/
s/obj/object/

This gets an object from a reference, right?


Right now the function sounds like it returns a object reference. Also, it doesn't return the object at all but a bool.

convertReferenceToObject?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1717
> +    uint32_t openNewComposite(const v8::Local<v8::Value>& obj)

s/obj/object/

Why openNewComposite as opposed to just openComposite? (The new seems implied. Also, I look for symmetry in the open and close calls and "new" throws this off.)

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1719
> +        uint32_t newid = m_objectPool.size();

newId

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1722
> +        return newid;

The return value is never used. Why have it?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1725
> +    bool closeComposite(v8::Handle<v8::Value>* obj)

s/obj/object/

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1729
> +        uint32_t oldid = m_objrefStack[m_objrefStack.size() - 1];

oldId (capital I).

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1740
> +    Vector<uint32_t> m_objrefStack;

objectReferenceStack?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1848
> +    if (status == Serializer::DataCloneError) {

Why not change this to be a "switch (status)" and collapse the Serializer::InputError and Serializer::DataCloneError cases?

Actually I'd suggest a quick separate patch to make this a switch (status).

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