Bug 163179 - Attribute getter binding generated code should use more references
Summary: Attribute getter binding generated code should use more references
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-09 04:37 PDT by youenn fablet
Modified: 2016-10-10 02:05 PDT (History)
6 users (show)

See Also:

Attachments
Patch (191.50 KB, patch)
2016-10-09 04:42 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch for landing (189.49 KB, patch)
2016-10-10 01:28 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch for landing (189.61 KB, patch)
2016-10-10 01:30 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-10-09 04:37:07 PDT 
We can use references for ExecState and thisObject.
Comment 1 youenn fablet 2016-10-09 04:42:52 PDT 
Created attachment 291042 [details]
Patch
Comment 2 Darin Adler 2016-10-09 15:25:17 PDT 
Comment on attachment 291042 [details]
Patch

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

Seems OK for now; makes things better in some ways, but worse in others with those extra conditions and variables.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3903
> +    my $useReferences = shift;

This boolean is really ugly, all the mysterious "1", all the extra variables and cases. Is there some way to avoid this?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4938
> +    my ($signature, $inFunctionCall, $interface, $value, $thisValue, $useReferences) = @_;

This boolean is really ugly, all the mysterious "1", all the extra variables and cases. Is there some way to avoid this?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4945
> +    # FIXNE: We should move to using references only

FIXME misspelled here
Comment 3 youenn fablet 2016-10-10 00:38:19 PDT 
Thanks for the review.

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3903
> > +    my $useReferences = shift;
> 
> This boolean is really ugly, all the mysterious "1", all the extra variables
> and cases. Is there some way to avoid this?

This is temporary, since it should use references once attribute setters and methods are also moved to using references.
That said, in the meantime, it might be best to introduce a NativeToJSValueWithReferences and NativeToJSValueWithPointers, that would call a sub NativeToJSValue routine.


> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4945
> > +    # FIXNE: We should move to using references only
> 
> FIXME misspelled here

OK
Comment 4 youenn fablet 2016-10-10 01:28:26 PDT 
Created attachment 291077 [details]
Patch for landing
Comment 5 youenn fablet 2016-10-10 01:30:37 PDT 
Created attachment 291078 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2016-10-10 02:05:44 PDT 
Comment on attachment 291078 [details]
Patch for landing

Clearing flags on attachment: 291078

Committed r206992: <http://trac.webkit.org/changeset/206992>
Comment 7 WebKit Commit Bot 2016-10-10 02:05:48 PDT 
All reviewed patches have been landed.  Closing bug.