Bug 160356 – Enable strict type checking for Window dictionary members

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Enable strict type checking for Window dictionary members. Technically, we should do strict type checking of all wrapper types but this patch focuses on Window because it is common to pass a Window dictionary member to Event constructors.

By strict type checking, I mean that we should throw a TypeError is the value is not null/undefined and does not implement the Window interface:
- http://heycam.github.io/webidl/#es-interface

Firefox and Chrome comply with the specification already.

[Chris Dumez]

Created attachment 284901 [details]
Patch

[Build Bot]

Comment on attachment 284901 [details]
Patch

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

New failing tests:
media/track/track-remove-quickly.html

[Build Bot]

Created attachment 284918 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[Chris Dumez]

The iOS failure does not seem related to my change.

[Alexey Proskuryakov]

Yes, the iOS failure is tracked as bug 160367 (and the test causing the crash is now skipped).

[Darin Adler]

Comment on attachment 284901 [details]
Patch

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

> Source/WebCore/bindings/js/JSDictionary.cpp:184
> +    if (value.isUndefinedOrNull()) {
> +        result = nullptr;
> +        return;
> +    }

Why not merge this with the UNLIKELY(!window) case below instead of checking before calling toWrapped? I assume the undefined and null are not hot cases that are as important to optimize as the window case so it’s nice to save the branch in the normal case.

> Source/WebCore/bindings/js/JSDictionary.cpp:186
> +    if (UNLIKELY(!window)) {

Change this to:

    if (UNLIKELY(!window) && !value.isUndefinedOrNull()) {

and then we don’t need to add the check above.

[Chris Dumez]

Created attachment 284957 [details]
Patch

[Chris Dumez]

(In reply to comment #6)
> Comment on attachment 284901 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284901&action=review
> 
> > Source/WebCore/bindings/js/JSDictionary.cpp:184
> > +    if (value.isUndefinedOrNull()) {
> > +        result = nullptr;
> > +        return;
> > +    }
> 
> Why not merge this with the UNLIKELY(!window) case below instead of checking
> before calling toWrapped? I assume the undefined and null are not hot cases
> that are as important to optimize as the window case so it’s nice to save
> the branch in the normal case.
> 
> > Source/WebCore/bindings/js/JSDictionary.cpp:186
> > +    if (UNLIKELY(!window)) {
> 
> Change this to:
> 
>     if (UNLIKELY(!window) && !value.isUndefinedOrNull()) {
> 
> and then we don’t need to add the check above.

Ok, I agree that in the dictionary case, passing null/undefined is not the common case (people would likely just omit it from the dictionary) and factoring as you suggest is indeed better. I made the change, thanks.

[WebKit Commit Bot]

Comment on attachment 284957 [details]
Patch

Clearing flags on attachment: 284957

Committed r203950: <http://trac.webkit.org/changeset/203950>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.