[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 #7 from Dmitry Lomov <dslomov at google.com>  2011-06-30 17:04:47 PST ---
View in context: https://bugs.webkit.org/attachment.cgi?id=99184&action=review

Nice patch!
First round of comments - I went through the code but haven't looked at tests very closely yet.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:89
> +// address returned by a Handle's operator* is related to the handle, not to the object it holds.

I think these comments are written under the influence of another implementation of the same concept :)
I suggest talking less about the implementation details and more about how the type should be used. 

How about this rewording:
V8ObjectMap implements a map from v8 objects to arbitrary values.
V8 objects cannot be used as keys in ordinary wtf::HashMap - use this class instead.
The type argument for GcObject template parameter should be a v8::Object or its inheritor.
Suggested usage: 
   V8ObjectMap<v8::Object, int> map;
   v8::Handle<v8::Object> obj = ...; 
   map.set(obj, 42);

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:96
> +// considers a v8::String to be a v8::Primitive).

I think the implementation comments should go inside the class definition braces.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:99
> +// faster native-side maps with JavaScript objects as keys.

I do not think it is the sole purpose of this wrapper. We can add a separate comment describing possible alternate implementation. It should go inside the class definition as well.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:102
> +class V8GcHandleMap {

I do not like this name: this is not a map from handles, the keys of this maps are v8 objects. Let's call it V8ObjectMap?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:115
> +        static const bool safeToCompareToEmptyOrDeleted = false;

Unused?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:127
> +    bool tryGet(const v8::Handle<GcObject>& handle, T& valueOut)

Do not use non-const reference parameters. Use T* for valueOut type.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:130
> +            valueOut = m_map.get(*handle);

Use HashMap::find to avoid double lookup:
HandleToT::iterator result = m_map.find(*handle);
if (result != m_map.end()) {
    *valueOut = result->second();
    return true;
}
return false;

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:168
>      ObjectTag = '{',

The wire format is becoming much more complex: a comment describing it in more detail than (tag, optional data) will be very useful.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:340
> +#endif

nit: move #endif beyond the ASSERT - makes the purpose of this local variable clearer

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:974
> +        ++m_nextObjectId;

Current implementation does not "deduplicate" strings and any implementation of deduplication will bump up wire format version. Let's get rid of this objectID allocation

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1006
> +                || jsObject->IsCallable())

The condition in this if statement represents V8 bindings understanding of what is a "host object" as defined per spec. I think this should be extracted into a separate IsHostObject function in V8Bindings.h.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1021
> +    virtual bool consumeTopOfStack(v8::Handle<v8::Value>&) = 0;

use pointers instead of references for out arguments.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1025
> +    virtual bool prepareEmptyArray(uint32_t length) = 0;

nit: maybe call these startArray and startObject, to match finishArray and finishObject?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1342
> +        RefPtr<ArrayBuffer> arrayBuffer = ArrayBuffer::create(const_cast<void*>(bufferStart), byteLength);

yuck - const casts! Any reason why ArrayBuffer::create doesn't take const void *?

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

Use pointers instead of references for out arguments

> Source/WebCore/dom/ExceptionCode.h:-67
> -#endif

Why the above removed?

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