RESOLVED FIXED 160394
[WebIDL] Implement overload resolution algorithm
https://bugs.webkit.org/show_bug.cgi?id=160394
Summary [WebIDL] Implement overload resolution algorithm
Chris Dumez
Reported 2016-07-31 17:30:17 PDT
Implement overload resolution algorithm: http://heycam.github.io/webidl/#es-overloads
Attachments
WIP patch (72.99 KB, patch)
2016-07-31 20:59 PDT, Chris Dumez
no flags
WIP patch (60.94 KB, patch)
2016-07-31 21:00 PDT, Chris Dumez
no flags
WIP Patch (76.02 KB, patch)
2016-08-01 11:09 PDT, Chris Dumez
no flags
WIP Patch (62.75 KB, patch)
2016-08-01 12:36 PDT, Chris Dumez
no flags
WIP Patch (61.93 KB, patch)
2016-08-01 13:26 PDT, Chris Dumez
no flags
WIP Patch (61.84 KB, patch)
2016-08-01 14:09 PDT, Chris Dumez
no flags
WIP Patch (61.53 KB, patch)
2016-08-01 16:09 PDT, Chris Dumez
no flags
Patch (83.60 KB, patch)
2016-08-01 16:45 PDT, Chris Dumez
no flags
Patch (84.11 KB, patch)
2016-08-02 09:33 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2016-07-31 20:59:18 PDT
Created attachment 284978 [details] WIP patch Early WIP Patch but should pass the tests.
Chris Dumez
Comment 2 2016-07-31 21:00:30 PDT
Created attachment 284979 [details] WIP patch
Darin Adler
Comment 3 2016-07-31 21:25:41 PDT
Comment on attachment 284979 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=284979&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1648 > +# http://heycam.github.io/webidl/#dfn-effective-overload-set I like the idea of linking to the URL of an algorithm, but in this case I am concerned that this URL will not be stable and also the fact that the algorithm might change and this won’t point to what is implemented here. We should consider what URLs to put here and why we are putting them here and maybe even write some words explaining that.
youenn fablet
Comment 4 2016-07-31 22:54:40 PDT
Some github-based specs have commit-based URLS. For example, https://streams.spec.whatwg.org/commit-snapshots/ba5cc069302db2e325815af7d2cd1e743e12773f/ Maybe WebIDL GitHub spec could add it.
Chris Dumez
Comment 5 2016-08-01 11:09:36 PDT
Created attachment 285022 [details] WIP Patch Shaping up nicely. Implements most of the spec but now still needs some clean up / simplification.
Chris Dumez
Comment 6 2016-08-01 12:36:07 PDT
Created attachment 285032 [details] WIP Patch
Chris Dumez
Comment 7 2016-08-01 13:26:08 PDT
Created attachment 285037 [details] WIP Patch
Chris Dumez
Comment 8 2016-08-01 14:09:32 PDT
Created attachment 285042 [details] WIP Patch
Chris Dumez
Comment 9 2016-08-01 16:09:52 PDT
Created attachment 285051 [details] WIP Patch
Chris Dumez
Comment 10 2016-08-01 16:45:58 PDT
Chris Dumez
Comment 11 2016-08-01 16:48:33 PDT
Comment on attachment 285053 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285053&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1776 > +# http://heycam.github.io/webidl/#es-overloads I kept the spec links for now. I do feel that it helps understanding the code for as long as those links stay valid. Also, I was not able to find links to specific GIT revisions. If you feel strongly about removing those, I can move them to the ChangeLog.
Chris Dumez
Comment 12 2016-08-01 16:58:07 PDT
Chris Dumez
Comment 13 2016-08-01 18:28:27 PDT
EWS is all green, patch is ready for review.
Darin Adler
Comment 14 2016-08-02 09:11:19 PDT
Comment on attachment 285053 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285053&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1776 >> +# http://heycam.github.io/webidl/#es-overloads > > I kept the spec links for now. I do feel that it helps understanding the code for as long as those links stay valid. Also, I was not able to find links to specific GIT revisions. > If you feel strongly about removing those, I can move them to the ChangeLog. Lets talk in the future about my concern here; I don’t want to hold up this patch but I want us to leave behind good quality links that are not confusing for the person looking in the future. There’s nothing wrong with having links but it’s better if the links are not going to get stale. And also, if the link points to an evergreen document then it will be confusing if the implementation matches a particular moment in time. I am not proposing links to specific git revisions. But I am proposing adding a few well chosen words to give context to explain what the link is. I have experience with confusing 5-to-10-year-old links in WebKit code to websites that don’t even exist that used to host specifications. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1801 > + return ($type eq "Dictionary" || $codeGenerator->IsDictionaryType($type)); No need for these parentheses. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1805 > + return ($isNullable || &$isDictionaryParameter($type, $optionality, $isNullable)); Ditto. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1809 > + return ($type eq "RegExp" || $type eq "object"); Ditto. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1813 > + return ($type eq "object" || $type eq "Error"); Ditto. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1818 > + return ($type eq "DOMException") Ditto. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1822 > + return ($type eq "object" || $codeGenerator->IsFunctionOnlyCallbackInterface($type)); Ditto. > LayoutTests/webgl/1.0.2/resources/webgl_test_files/conformance/textures/tex-image-with-invalid-data.html:92 > +// We do not do strict type checkinf for ArrayBufferView type. Typo: checking with an f
Chris Dumez
Comment 15 2016-08-02 09:18:25 PDT
Comment on attachment 285053 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285053&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1801 >> + return ($type eq "Dictionary" || $codeGenerator->IsDictionaryType($type)); > > No need for these parentheses. I could have sworn I had to add them or this subroutine did not do the right thing... Anyway, I'll try and remove them and make sure it generates the same code.
Chris Dumez
Comment 16 2016-08-02 09:33:24 PDT
WebKit Commit Bot
Comment 17 2016-08-02 10:03:43 PDT
Comment on attachment 285115 [details] Patch Clearing flags on attachment: 285115 Committed r204028: <http://trac.webkit.org/changeset/204028>
WebKit Commit Bot
Comment 18 2016-08-02 10:03:49 PDT
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 19 2016-08-02 10:07:59 PDT
*** Bug 160385 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.