Bug 163247 - Use IDLTypes in more places
Summary: Use IDLTypes in more places
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: 2016-10-10 15:38 PDT by Sam Weinig
Modified: 2016-10-11 12:16 PDT (History)
11 users (show)

See Also:


Attachments
Patch (169.14 KB, patch)
2016-10-10 15:55 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (930.15 KB, application/zip)
2016-10-10 17:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.03 MB, application/zip)
2016-10-10 17:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (1.69 MB, application/zip)
2016-10-10 17:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (13.07 MB, application/zip)
2016-10-10 17:15 PDT, Build Bot
no flags Details
Patch (172.33 KB, patch)
2016-10-10 18:09 PDT, Sam Weinig
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2016-10-10 15:38:26 PDT
Use IDLTypes in more places
Comment 1 Sam Weinig 2016-10-10 15:55:28 PDT
Created attachment 291175 [details]
Patch
Comment 2 WebKit Commit Bot 2016-10-10 15:57:36 PDT
Attachment 291175 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMConvert.h:324:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Build Bot 2016-10-10 17:02:14 PDT
Comment on attachment 291175 [details]
Patch

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

New failing tests:
fast/events/constructors/message-event-constructor.html
Comment 4 Build Bot 2016-10-10 17:02:18 PDT
Created attachment 291191 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 5 Build Bot 2016-10-10 17:03:26 PDT
Comment on attachment 291175 [details]
Patch

Attachment 291175 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2258505

New failing tests:
fast/events/constructors/message-event-constructor.html
fast/mediastream/MediaStreamConstructor.html
Comment 6 Build Bot 2016-10-10 17:03:30 PDT
Created attachment 291192 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 7 Build Bot 2016-10-10 17:06:31 PDT
Comment on attachment 291175 [details]
Patch

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

New failing tests:
fast/events/constructors/message-event-constructor.html
Comment 8 Build Bot 2016-10-10 17:06:35 PDT
Created attachment 291193 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 9 Build Bot 2016-10-10 17:15:14 PDT
Comment on attachment 291175 [details]
Patch

Attachment 291175 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2258521

New failing tests:
fast/events/constructors/message-event-constructor.html
Comment 10 Build Bot 2016-10-10 17:15:18 PDT
Created attachment 291194 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 11 Sam Weinig 2016-10-10 18:09:47 PDT
Created attachment 291203 [details]
Patch
Comment 12 WebKit Commit Bot 2016-10-10 18:11:51 PDT
Attachment 291203 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMConvert.h:324:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Darin Adler 2016-10-10 21:55:55 PDT
Comment on attachment 291203 [details]
Patch

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

I like this approach.

> Source/WebCore/bindings/scripts/IDLParser.pm:147
> -    extendedAttributes => '$', # Extended attributes
> -    type => '$', # Type of data
> +    extendedAttributes => '$',  # Extended attributes
> +    idlType => '$',             # Type of data

I don't like this kind of "lining up" and our style guide explicitly forbids it.

> LayoutTests/fast/events/constructors/message-event-constructor-expected.txt:84
> -PASS new MessageEvent('eventType', { ports: [1, 2, 3] }).ports[2] threw exception TypeError: Invalid Array element type.
> +PASS new MessageEvent('eventType', { ports: [1, 2, 3] }).ports[2] threw exception TypeError: Type error.

Seems like the new message is not quite as good.

> LayoutTests/fast/mediastream/MediaStreamConstructor-expected.txt:15
> -PASS new MediaStream([document]) threw exception TypeError: Invalid Array element type.
> -PASS new MediaStream([stream.getAudioTracks()[0], document]) threw exception TypeError: Invalid Array element type.
> -PASS new MediaStream([null]) threw exception TypeError: Invalid Array element type.
> -PASS new MediaStream([undefined]) threw exception TypeError: Invalid Array element type.
> +PASS new MediaStream([document]) threw exception TypeError: Type error.
> +PASS new MediaStream([stream.getAudioTracks()[0], document]) threw exception TypeError: Type error.
> +PASS new MediaStream([null]) threw exception TypeError: Type error.
> +PASS new MediaStream([undefined]) threw exception TypeError: Type error.

Ditto.
Comment 14 Sam Weinig 2016-10-11 10:05:51 PDT
(In reply to comment #13)
> Comment on attachment 291203 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291203&action=review
> 
> I like this approach.
> 
> > Source/WebCore/bindings/scripts/IDLParser.pm:147
> > -    extendedAttributes => '$', # Extended attributes
> > -    type => '$', # Type of data
> > +    extendedAttributes => '$',  # Extended attributes
> > +    idlType => '$',             # Type of data
> 
> I don't like this kind of "lining up" and our style guide explicitly forbids
> it.
> 
> > LayoutTests/fast/events/constructors/message-event-constructor-expected.txt:84
> > -PASS new MessageEvent('eventType', { ports: [1, 2, 3] }).ports[2] threw exception TypeError: Invalid Array element type.
> > +PASS new MessageEvent('eventType', { ports: [1, 2, 3] }).ports[2] threw exception TypeError: Type error.
> 
> Seems like the new message is not quite as good.
> 
> > LayoutTests/fast/mediastream/MediaStreamConstructor-expected.txt:15
> > -PASS new MediaStream([document]) threw exception TypeError: Invalid Array element type.
> > -PASS new MediaStream([stream.getAudioTracks()[0], document]) threw exception TypeError: Invalid Array element type.
> > -PASS new MediaStream([null]) threw exception TypeError: Invalid Array element type.
> > -PASS new MediaStream([undefined]) threw exception TypeError: Invalid Array element type.
> > +PASS new MediaStream([document]) threw exception TypeError: Type error.
> > +PASS new MediaStream([stream.getAudioTracks()[0], document]) threw exception TypeError: Type error.
> > +PASS new MediaStream([null]) threw exception TypeError: Type error.
> > +PASS new MediaStream([undefined]) threw exception TypeError: Type error.
> 
> Ditto.

Indeed. I want to improve the exceptions across the board here, but it will take a few iterations. Considering a few options, including passing in a lambda which creates the exception, so it can be call-site specific.
Comment 15 Chris Dumez 2016-10-11 10:08:30 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > Comment on attachment 291203 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=291203&action=review
> > 
> > I like this approach.
> > 
> > > Source/WebCore/bindings/scripts/IDLParser.pm:147
> > > -    extendedAttributes => '$', # Extended attributes
> > > -    type => '$', # Type of data
> > > +    extendedAttributes => '$',  # Extended attributes
> > > +    idlType => '$',             # Type of data
> > 
> > I don't like this kind of "lining up" and our style guide explicitly forbids
> > it.
> > 
> > > LayoutTests/fast/events/constructors/message-event-constructor-expected.txt:84
> > > -PASS new MessageEvent('eventType', { ports: [1, 2, 3] }).ports[2] threw exception TypeError: Invalid Array element type.
> > > +PASS new MessageEvent('eventType', { ports: [1, 2, 3] }).ports[2] threw exception TypeError: Type error.
> > 
> > Seems like the new message is not quite as good.
> > 
> > > LayoutTests/fast/mediastream/MediaStreamConstructor-expected.txt:15
> > > -PASS new MediaStream([document]) threw exception TypeError: Invalid Array element type.
> > > -PASS new MediaStream([stream.getAudioTracks()[0], document]) threw exception TypeError: Invalid Array element type.
> > > -PASS new MediaStream([null]) threw exception TypeError: Invalid Array element type.
> > > -PASS new MediaStream([undefined]) threw exception TypeError: Invalid Array element type.
> > > +PASS new MediaStream([document]) threw exception TypeError: Type error.
> > > +PASS new MediaStream([stream.getAudioTracks()[0], document]) threw exception TypeError: Type error.
> > > +PASS new MediaStream([null]) threw exception TypeError: Type error.
> > > +PASS new MediaStream([undefined]) threw exception TypeError: Type error.
> > 
> > Ditto.
> 
> Indeed. I want to improve the exceptions across the board here, but it will
> take a few iterations. Considering a few options, including passing in a
> lambda which creates the exception, so it can be call-site specific.

Yes, we could pass a lambda which would be super flexible and allow call-site specific messages. Alternatively, we could simply take a string literal representing the expected type so that the convert function can generate a better message on its own.
Comment 16 Sam Weinig 2016-10-11 10:36:36 PDT
Committed revision 207150
Comment 17 Alex Christensen 2016-10-11 11:51:22 PDT
Looks like this broke the bindings tests
Comment 18 Alex Christensen 2016-10-11 12:16:53 PDT
http://trac.webkit.org/changeset/207154