Change MutationObserver::observe to take an IDL dictionary, rather than WebCore::Dictionary
Created attachment 278352 [details] Patch
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.
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
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.
Comment on attachment 278352 [details] Patch r=me. mac EWS is unhappy but I have trouble believing this is because of this change.
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.
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.
Committed r200552: <http://trac.webkit.org/changeset/200552>
(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.
(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.
(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!