[webkit-reviews] review denied: [Bug 40835] Adds support for throwing behavior of SerializedScriptValue to be specified in the IDL. : [Attachment 62810] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Marcus Bulach <bulach at chromium.org>'s
request for review:
Bug 40835: Adds support for throwing behavior of SerializedScriptValue to be
specified in the IDL.
https://bugs.webkit.org/show_bug.cgi?id=40835

Attachment 62810: Patch
https://bugs.webkit.org/attachment.cgi?id=62810&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=62810&action=review

Looks reasonable.  Some cleanup mentioned below.  The only serious problem is
what looks like a memory leak.

> WebCore/bindings/js/SerializedScriptValue.cpp:977
> +    RefPtr<SerializedScriptValue> serializedValue = new
SerializedScriptValue(SerializedScriptValueData::serialize(exec, value));

Isn't this a memory leak?  We need to call adoptRef, right?

> WebCore/bindings/js/SerializedScriptValue.cpp:991
> +    RefPtr<SerializedScriptValue> serializedValue = new
SerializedScriptValue(SerializedScriptValueData::serialize(exec, value));

Same here.  adoptRef?

> WebCore/bindings/scripts/CodeGeneratorJS.pm:2491
> +    $ret = $ret . "	  PassRefPtr<SerializedScriptValue>
serializedScriptValue = $value;\n";

We don't usually store PassRefPtrs on the stack.  We usually us RefPtr and then
release.

> WebCore/bindings/scripts/test/V8/V8TestObj.cpp:610
> +    if (UNLIKELY(ec))
> +	   goto fail;

That's pretty nasty code.  Also, the indent is wrong here.

> WebCore/bindings/scripts/test/V8/V8TestObj.cpp:637
> +    {
> +    RefPtr<SerializedScriptValue> result =
imp->serializedValueReturnRaises(ec);
> +    if (UNLIKELY(ec))
> +	   goto fail;
> +    return result.release()->deserialize();
> +    }

Another bad indent.  Why do we need these { }  brackets?

> WebCore/bindings/v8/SerializedScriptValue.cpp:1031
> +	   didThrow = true;

Why are we using didThrow instead of the ec, like we usually do?