Bug 75641 - [V8] CodeGeneration for SerializedScriptValue doesn't play nice with [Constructor]
Summary: [V8] CodeGeneration for SerializedScriptValue doesn't play nice with [Constru...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 73051
  Show dependency treegraph
 
Reported: 2012-01-05 12:19 PST by Adam Barth
Modified: 2012-01-05 15:57 PST (History)
7 users (show)

See Also:


Attachments
Patch (19.47 KB, patch)
2012-01-05 12:24 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-01-05 12:19:08 PST
[V8] CodeGeneration for SerializedScriptValue doesn't play nice with [Constructor]
Comment 1 Adam Barth 2012-01-05 12:24:48 PST
Created attachment 121311 [details]
Patch
Comment 2 Adam Barth 2012-01-05 12:27:25 PST
@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?
Comment 3 Greg Billock 2012-01-05 13:48:32 PST
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.
Comment 4 David Levin 2012-01-05 13:58:20 PST
(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.
Comment 5 Adam Barth 2012-01-05 14:07:51 PST
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.
Comment 6 David Levin 2012-01-05 14:22:46 PST
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.
Comment 7 Adam Barth 2012-01-05 14:26:51 PST
> > 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.
Comment 8 WebKit Review Bot 2012-01-05 15:57:43 PST
Comment on attachment 121311 [details]
Patch

Clearing flags on attachment: 121311

Committed r104233: <http://trac.webkit.org/changeset/104233>
Comment 9 WebKit Review Bot 2012-01-05 15:57:47 PST
All reviewed patches have been landed.  Closing bug.