Bug 157456 – Change MutationObserver::observe to take an IDL dictionary, rather than WebCore::Dictionary

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

[Darin Adler]

Change MutationObserver::observe to take an IDL dictionary, rather than WebCore::Dictionary

[Darin Adler]

Created attachment 278352 [details]
Patch

[Build Bot]

Comment on attachment 278352 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 278354 [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

[Chris Dumez]

Comment on attachment 278352 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMConvert.h:76
> +    Vector<String> vector;

Do we really want to inline all this? Could we leverage toNativeArray<String>() maybe?

> Source/WebCore/bindings/scripts/test/TestObj.idl:399
> +    boolean characterData;

I don't think having all those identical members adds much value here, I would keep only 2 of the booleans (one with the default value, one without).

> LayoutTests/fast/dom/MutationObserver/observe-exceptions-expected.txt:10
> +PASS observer.observe(document.body, null) threw exception TypeError: null is not an object (evaluating 'observer.observe(document.body, null)').

Not new to your patch but this behavior seems wrong as per Web IDL. When passing null / undefined for a Dictionary parameter, we're supposed to construct an empty dictionary (and use default member values when available).
See http://heycam.github.io/webidl/#es-dictionary

We definitely want to fix this in a follow-up.

> LayoutTests/fast/dom/MutationObserver/observe-exceptions-expected.txt:11
> +PASS observer.observe(document.body, undefined) threw exception TypeError: undefined is not an object (evaluating 'observer.observe(document.body, undefined)').

Ditto.

[Chris Dumez]

Comment on attachment 278352 [details]
Patch

r=me. mac EWS is unhappy but I have trouble believing this is because of this change.

[Darin Adler]

Comment on attachment 278352 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDOMConvert.h:76
>> +    Vector<String> vector;
> 
> Do we really want to inline all this? Could we leverage toNativeArray<String>() maybe?

No, we don’t want to inline any of it. I’ll just take off the inline keyword.

As far as using toNativeArray<String>, sure, maybe. I’ll look at it.

>> Source/WebCore/bindings/scripts/test/TestObj.idl:399
>> +    boolean characterData;
> 
> I don't think having all those identical members adds much value here, I would keep only 2 of the booleans (one with the default value, one without).

Sure, I don’t really need to keep it at all. It’s just something I used when getting the bindings working. I’ll trim it. Same with the test case above with all the default strings.

>> LayoutTests/fast/dom/MutationObserver/observe-exceptions-expected.txt:10
>> +PASS observer.observe(document.body, null) threw exception TypeError: null is not an object (evaluating 'observer.observe(document.body, null)').
> 
> Not new to your patch but this behavior seems wrong as per Web IDL. When passing null / undefined for a Dictionary parameter, we're supposed to construct an empty dictionary (and use default member values when available).
> See http://heycam.github.io/webidl/#es-dictionary
> 
> We definitely want to fix this in a follow-up.

Great point. I want to fix this right away before I get any deeper.

[Chris Dumez]

Comment on attachment 278352 [details]
Patch

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

>>> Source/WebCore/bindings/js/JSDOMConvert.h:76
>>> +    Vector<String> vector;
>> 
>> Do we really want to inline all this? Could we leverage toNativeArray<String>() maybe?
> 
> No, we don’t want to inline any of it. I’ll just take off the inline keyword.
> 
> As far as using toNativeArray<String>, sure, maybe. I’ll look at it.

FYI, toNativeArray<String>() is currently what we use to convert sequence<DOMString> parameters in the bindings so it would at least be consistent.

[Darin Adler]

Committed r200552: <http://trac.webkit.org/changeset/200552>

[Darin Adler]

(In reply to comment #7)
> FYI, toNativeArray<String>() is currently what we use to convert
> sequence<DOMString> parameters in the bindings so it would at least be
> consistent.

I see. OK, I will switch to it in a newer patch.

[Chris Dumez]

(In reply to comment #8)
> Committed r200552: <http://trac.webkit.org/changeset/200552>

This broke the build:
https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20%28Build%29/builds/6168/steps/compile-webkit/logs/stdio

Probably the non-inline function in the JSDOMConvert.h header.

[Darin Adler]

(In reply to comment #10)
> Probably the non-inline function in the JSDOMConvert.h header.

Crap, I guess you are right. I thought it would be OK because it’s a template, but it’s not!