Bug 163247 – Use IDLTypes in more places

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Use IDLTypes in more places

[Sam Weinig]

Created attachment 291175 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Sam Weinig]

Created attachment 291203 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

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.

[Sam Weinig]

(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.

[Chris Dumez]

(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.

[Sam Weinig]

Committed revision 207150

[Alex Christensen]

Looks like this broke the bindings tests

[Alex Christensen]

http://trac.webkit.org/changeset/207154

[Alexey Shvayka]

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