Bug 75641 – [V8] CodeGeneration for SerializedScriptValue doesn't play nice with [Constructor]

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

[V8] CodeGeneration for SerializedScriptValue doesn't play nice with [Constructor]

[Adam Barth]

Created attachment 121311 [details]
Patch

[Adam Barth]

@dave_levin: You seemed to have reviewed http://trac.webkit.org/changeset/56877, when this mechanism was introduced.  Would you be willing to review this patch?

[Greg Billock]

Comment on attachment 121311 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121311&action=review

> Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:62
> +        return v8::Undefined();

This is generated at CodeGeneratorV8:1534. Should it instead call return throwError() here, to signal to the caller that the value passed did not successfully pass the serialization? That codegen is called for constructor and function generation, where I think an error would be more useful.

[David Levin]

(In reply to comment #2)
> @dave_levin: You seemed to have reviewed http://trac.webkit.org/changeset/56877, when this mechanism was introduced.  Would you be willing to review this patch?

You were suppose to say "ping? anyone? anyone? anyone?"

Looking.

[Adam Barth]

Comment on attachment 121311 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121311&action=review

>> Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:62
>> +        return v8::Undefined();
> 
> This is generated at CodeGeneratorV8:1534. Should it instead call return throwError() here, to signal to the caller that the value passed did not successfully pass the serialization? That codegen is called for constructor and function generation, where I think an error would be more useful.

Possibly, but that's something to fix in a different patch.

[David Levin]

Comment on attachment 121311 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121311&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1648
> +    push(@implContent, "    v8::Handle<v8::Object> wrapper = args.Holder();\n");

Oh I see this is needed for consistency with GenerateEagerDeserialization.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1662
> +        GenerateEagerDeserialization($serializedAttribute);

This is the only real change.

[Adam Barth]

> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1662
> > +        GenerateEagerDeserialization($serializedAttribute);
> 
> This is the only real change.

Yep.  The rest is just renaming variables to be more consistent so that GenerateEagerDeserialization makes code that builds.

[WebKit Review Bot]

Comment on attachment 121311 [details]
Patch

Clearing flags on attachment: 121311

Committed r104233: <http://trac.webkit.org/changeset/104233>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.