Bug 174140 – [WebIDL] Convert MutationCallback to be a normal generated callback

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

[Sam Weinig]

[WebIDL] Convert MutationCallback to be a normal generate callback

[Sam Weinig]

Created attachment 314579 [details]
Patch

[Build Bot]

Comment on attachment 314579 [details]
Patch

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

New failing tests:
fast/dom/MutationObserver/mutation-observer-constructor.html
fast/dom/MutationObserver/callback-arguments.html

[Build Bot]

Created attachment 314581 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314579 [details]
Patch

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

New failing tests:
fast/dom/MutationObserver/mutation-observer-constructor.html
fast/dom/MutationObserver/callback-arguments.html

[Build Bot]

Created attachment 314582 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314579 [details]
Patch

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

New failing tests:
fast/dom/MutationObserver/mutation-observer-constructor.html
fast/dom/MutationObserver/callback-arguments.html

[Build Bot]

Created attachment 314584 [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.12.5

[Build Bot]

Comment on attachment 314579 [details]
Patch

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

New failing tests:
fast/dom/MutationObserver/mutation-observer-constructor.html
fast/dom/MutationObserver/callback-arguments.html

[Build Bot]

Created attachment 314587 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314579 [details]
Patch

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

New failing tests:
fast/dom/MutationObserver/mutation-observer-constructor.html
fast/dom/MutationObserver/callback-arguments.html

[Build Bot]

Created attachment 314592 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Sam Weinig]

I need to come up with a way to pass a this object to a callback. I have two ideas.
1) Add a setThisObject(...) function that the callback needs to implement which should be called before handleEvent().
2) For callbacks that need a this object, pass it as the first argument, and offset the rest of the arguments by one.

I think I am leaning toward the second option.

[Sam Weinig]

Created attachment 314697 [details]
Patch

[Build Bot]

Comment on attachment 314697 [details]
Patch

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

New failing tests:
fast/dom/MutationObserver/mutation-observer-constructor.html

[Build Bot]

Created attachment 314700 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314697 [details]
Patch

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

New failing tests:
fast/dom/MutationObserver/mutation-observer-constructor.html

[Build Bot]

Created attachment 314701 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314697 [details]
Patch

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

New failing tests:
fast/dom/MutationObserver/mutation-observer-constructor.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 314697 [details]
Patch

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

New failing tests:
fast/dom/MutationObserver/mutation-observer-constructor.html

[Build Bot]

Created attachment 314712 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314697 [details]
Patch

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

New failing tests:
fast/dom/MutationObserver/mutation-observer-constructor.html

[Build Bot]

Created attachment 314717 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Sam Weinig]

Created attachment 314762 [details]
Patch

[Chris Dumez]

Comment on attachment 314762 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:3
> +        [WebIDL] Convert MutationCallback to be a normal generate callback

"normal generate callback" ?

> Source/WebCore/ChangeLog:9
> +        - Added the ability to for non-nullable interfaces in sequences to be passed

"to for"?

[Sam Weinig]

Committed r219361: <http://trac.webkit.org/changeset/219361>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 174434

[Sam Weinig]

Created attachment 317007 [details]
Patch

[Build Bot]

Comment on attachment 317007 [details]
Patch

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

New failing tests:
(JS) JSTestCallbackFunction.cpp
(JS) JSTestCallbackFunction.h
(JS) JSTestCallbackFunctionRethrow.cpp
(JS) JSTestCallbackFunctionRethrow.h
(JS) JSTestCallbackFunctionWithThisObject.cpp
(JS) JSTestCallbackFunctionWithThisObject.h
(JS) JSTestCallbackFunctionWithTypedefs.cpp
(JS) JSTestCallbackFunctionWithTypedefs.h
(JS) JSTestCallbackInterface.cpp
(JS) JSTestCallbackInterface.h
(JS) JSTestVoidCallbackFunction.cpp
(JS) JSTestVoidCallbackFunction.h

[Sam Weinig]

Created attachment 317020 [details]
Patch

[Build Bot]

Comment on attachment 317020 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/cssom-view/matchMedia.xht

[Build Bot]

Created attachment 317032 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 317020 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/cssom-view/matchMedia.xht

[Build Bot]

Created attachment 317046 [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.12.5

[Build Bot]

Comment on attachment 317020 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/cssom-view/matchMedia.xht

[Build Bot]

Created attachment 317053 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 317020 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/cssom-view/matchMedia.xht

[Build Bot]

Created attachment 317060 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Sam Weinig]

Created attachment 317077 [details]
Patch

[Darin Adler]

Comment on attachment 317077 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [WebIDL] Convert MutationCallback to be a normal generate callback

generated?

> Source/WebCore/Modules/geolocation/PositionCallback.h:38
> +    using ActiveDOMCallback::ActiveDOMCallback;

What does this do? I have never seen this syntax before; it looks like it would make a base class constructor public, but I not know what that means. And also, it seems that since we inherit from ActiveDOMCallback publicly that it would already be public, so what is this for?

> Source/WebCore/Modules/geolocation/PositionCallback.h:40
>      virtual ~PositionCallback() { }

This is no longer needed now that we have a base class that already makes the detstructor virtual.

> Source/WebCore/Modules/geolocation/PositionCallback.h:41
>      virtual CallbackResult<void> handleEvent(Geoposition*) = 0;

Wish this was a reference instead of a pointer.

> Source/WebCore/Modules/geolocation/PositionErrorCallback.h:40
> +    using ActiveDOMCallback::ActiveDOMCallback;
> +
>      virtual ~PositionErrorCallback() { }

Ditto. Same for all the other classes.

[Sam Weinig]

(In reply to Darin Adler from comment #40)
> Comment on attachment 317077 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317077&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [WebIDL] Convert MutationCallback to be a normal generate callback
> 
> generated?

Yep. Will fix.

> 
> > Source/WebCore/Modules/geolocation/PositionCallback.h:38
> > +    using ActiveDOMCallback::ActiveDOMCallback;
> 
> What does this do? I have never seen this syntax before; it looks like it
> would make a base class constructor public, but I not know what that means.
> And also, it seems that since we inherit from ActiveDOMCallback publicly
> that it would already be public, so what is this for?

It adds all the constructors from ActiveDOMCallback to, in this case, PositionCallback. I found the section called "Inheriting constructors" in http://en.cppreference.com/w/cpp/language/using_declaration useful and interesting.

> 
> > Source/WebCore/Modules/geolocation/PositionCallback.h:40
> >      virtual ~PositionCallback() { }
> 
> This is no longer needed now that we have a base class that already makes
> the detstructor virtual.

Good point, will remove.

> 
> > Source/WebCore/Modules/geolocation/PositionCallback.h:41
> >      virtual CallbackResult<void> handleEvent(Geoposition*) = 0;
> 
> Wish this was a reference instead of a pointer.

Me too. :(.

[Sam Weinig]

Committed r220210: <http://trac.webkit.org/changeset/220210>

[Radar WebKit Bug Importer]

<rdar://problem/33703510>

[Chris Dumez]

The changes to MutationObserver / MutationCallback may have caused crashes (rdar://problem/34062022).