Bug 160394 – [WebIDL] Implement overload resolution algorithm

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Implement overload resolution algorithm:
http://heycam.github.io/webidl/#es-overloads

[Chris Dumez]

Created attachment 284978 [details]
WIP patch

Early WIP Patch but should pass the tests.

[Chris Dumez]

Created attachment 284979 [details]
WIP patch

[Darin Adler]

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]

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]

Created attachment 285022 [details]
WIP Patch

Shaping up nicely. Implements most of the spec but now still needs some clean up / simplification.

[Chris Dumez]

Created attachment 285032 [details]
WIP Patch

[Chris Dumez]

Created attachment 285037 [details]
WIP Patch

[Chris Dumez]

Created attachment 285042 [details]
WIP Patch

[Chris Dumez]

Created attachment 285051 [details]
WIP Patch

[Chris Dumez]

Created attachment 285053 [details]
Patch

[Chris Dumez]

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]

FYI, I filed a few bugs / requests for clarification against the spec:
- https://github.com/heycam/webidl/issues/138
- https://github.com/heycam/webidl/issues/139
- https://github.com/heycam/webidl/issues/140

[Chris Dumez]

EWS is all green, patch is ready for review.

[Darin Adler]

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 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]

Created attachment 285115 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 285115 [details]
Patch

Clearing flags on attachment: 285115

Committed r204028: <http://trac.webkit.org/changeset/204028>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

*** Bug 160385 has been marked as a duplicate of this bug. ***