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

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:93
> +template<typename GcObject, typename T>

GCObject

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:126
> +    // v8::Objects, this V8GcHandleMap cannot be used to map v8::Strings to T (because the public V8 API

V8GcHandleMap undefined reference.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:754
>                      return newState;

Why isn't m_serializingAccessor reset before this return?  (You explained but it would be nice to add a comment because it looks wrong.)

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:911
> +            return false;

return 0? (false doesn't go with StateBase*)

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:923
> +        greyObject(object);

Why can't the greyObject call be done as the 1st thing in this method?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:948
> +    // Marks object as having been visited by the serializer and assigns it a unique object ID.

Why do you call it a "unique object ID" here but call it am objectPoolOffset on line 976?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:975
> +    if ((value->IsObject() || value->IsDate() || value->IsRegExp())

Why are we putting things other than object, date, and regexp into m_objectPool but not writing out object offsets for them?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:984
> +        writeNull();

Why did this become a method?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1076
> +            if (refTableSize != creator.objectReferenceCount())

It would be nice to add a comment here about what likely went wrong to cause this.

A missing call to creator.pushObjectReference in the deserialization (or an extra one)  that causes a mismatch between the serialization and deserialization.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1111
> +            if (m_version > 0)

I think all of these checks for m_version > 0 in this case:

if (m_version > 0)
    creator.pushObjectReference(*value);

are simply optimizations for when we read version 0 formats. Since that will be a rare case and these checks clutter the code, why not remove them and simply have

creator.pushObjectReference(*value);

instead?

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

// ArrayBufferView serialization writes the ArrayBuffer before the ArrayBufferView.
?

Why is it importnat to know that here?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1264
> +    bool readSubTag(ArrayBufferViewSubTag* tag)

"readSubTag" sounds too generic for what it does. (It only reads ArrayBufferView sub tags).

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1857
> +    case Serializer::DataCloneError:

Why not just have:

case Serializer::InputError:
case Serializer::InputError:
 ...(same code for both)
  return;

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1867
> +    default:

case Serializer::Success:

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1869
> +        m_data = String(StringImpl::adopt(writer.data())).crossThreadString();

return;

case Serializer::JSException;
    // We should never get here because it was handled above. 
    break;
}

(By removing the default, you'll get the compiler warnings for unhandled enum values which is nice.)


ASSERT_NOT_REACHED();

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