Bug 75641

Summary: [V8] CodeGeneration for SerializedScriptValue doesn't play nice with [Constructor]
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, haraken, japhet, levin, ojan, vitalyr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 73051    
Attachments:
Description Flags
Patch none

Adam Barth
Reported 2012-01-05 12:19:08 PST
[V8] CodeGeneration for SerializedScriptValue doesn't play nice with [Constructor]
Attachments
Patch (19.47 KB, patch)
2012-01-05 12:24 PST, Adam Barth 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2012-01-05 12:24:48 PST
Created attachment 121311 [details] Patch
Adam Barth
Comment 2 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?
Greg Billock
Comment 3 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.
David Levin
Comment 4 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.
Adam Barth
Comment 5 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.
David Levin
Comment 6 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.
Adam Barth
Comment 7 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.
WebKit Review Bot
Comment 8 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>
WebKit Review Bot
Comment 9 2012-01-05 15:57:47 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.