Bug 170926 - [WebIDL] Make annotated types first class allowing them to be used in sequences and unions
Summary: [WebIDL] Make annotated types first class allowing them to be used in sequenc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-17 17:52 PDT by Sam Weinig
Modified: 2017-04-18 13:35 PDT (History)
5 users (show)

See Also:


Attachments
Patch (213.84 KB, patch)
2017-04-17 18:26 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (224.93 KB, patch)
2017-04-17 22:17 PDT, Sam Weinig
cdumez: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-elcapitan (1.67 MB, application/zip)
2017-04-17 23:52 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2017-04-17 17:52:45 PDT
[WebIDL] Make annotated types first class allowing them to be used in sequences and unions
Comment 1 Sam Weinig 2017-04-17 18:26:44 PDT
Created attachment 307330 [details]
Patch
Comment 2 Build Bot 2017-04-17 18:29:03 PDT
Attachment 307330 [details] did not pass style-queue:


ERROR: Source/WebCore/domjit/DOMJITIDLType.h:43:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/domjit/DOMJITIDLType.h:44:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/domjit/DOMJITIDLTypeFilter.h:42:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/domjit/DOMJITIDLTypeFilter.h:43:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/domjit/DOMJITIDLTypeFilter.h:67:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/domjit/DOMJITIDLTypeFilter.h:68:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 6 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Sam Weinig 2017-04-17 22:17:59 PDT
Created attachment 307341 [details]
Patch
Comment 4 Build Bot 2017-04-17 22:20:49 PDT
Attachment 307341 [details] did not pass style-queue:


ERROR: Source/WebCore/domjit/DOMJITIDLType.h:43:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/domjit/DOMJITIDLType.h:44:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/domjit/DOMJITIDLTypeFilter.h:42:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/domjit/DOMJITIDLTypeFilter.h:43:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/domjit/DOMJITIDLTypeFilter.h:67:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/domjit/DOMJITIDLTypeFilter.h:68:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 6 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Build Bot 2017-04-17 23:52:27 PDT
Comment on attachment 307341 [details]
Patch

Attachment 307341 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3555374

New failing tests:
http/tests/inspector/network/resource-sizes-network.html
Comment 6 Build Bot 2017-04-17 23:52:28 PDT
Created attachment 307353 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 7 Sam Weinig 2017-04-18 09:22:58 PDT
The failure looks to be a flaky test. Pay it no mind.
Comment 8 Chris Dumez 2017-04-18 12:32:34 PDT
Comment on attachment 307341 [details]
Patch

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

r=me, nice.

> Source/WebCore/bindings/js/JSDOMConvertNumbers.cpp:252
>      if (value.isInt32())

We may even want to LIKELY() this branch.

> Source/WebCore/bindings/js/JSDOMConvertNumbers.cpp:265
> +    if (value.isUInt32())

We may even want to LIKELY() this branch.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5379
> +sub GetAnnotatedIDLType

Having 2 subroutines that need to be kept in sync seems a bit unfortunate. Could we maybe drop the IsAnnotatedType() one and rely on GetAnnotatedIDLType() returning something or not?
Comment 9 Sam Weinig 2017-04-18 13:34:04 PDT
(In reply to Chris Dumez from comment #8)
> Comment on attachment 307341 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307341&action=review
> 
> r=me, nice.
> 
> > Source/WebCore/bindings/js/JSDOMConvertNumbers.cpp:252
> >      if (value.isInt32())
> 
> We may even want to LIKELY() this branch.
> 
> > Source/WebCore/bindings/js/JSDOMConvertNumbers.cpp:265
> > +    if (value.isUInt32())
> 
> We may even want to LIKELY() this branch.

I'm not sure if these are likely honestly. Since the number can often be a double, I'm not sure it makes sense here.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5379
> > +sub GetAnnotatedIDLType
> 
> Having 2 subroutines that need to be kept in sync seems a bit unfortunate.
> Could we maybe drop the IsAnnotatedType() one and rely on
> GetAnnotatedIDLType() returning something or not?

It would make the calling code a bit odd, and this is kind of the idiom in the code generator. I'll think about how to improve it more generally.

Thanks for the review.
Comment 10 Sam Weinig 2017-04-18 13:35:46 PDT
Committed r215477: <http://trac.webkit.org/changeset/215477>