[webkit-reviews] review granted: [Bug 78085] Page should have less intrusive way to associate API implementation objects. : [Attachment 126409] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted MORITA Hajime <morrita at google.com>'s
request for review:
Bug 78085: Page should have less intrusive way to associate API implementation
objects.
https://bugs.webkit.org/show_bug.cgi?id=78085

Attachment 126409: Patch
https://bugs.webkit.org/attachment.cgi?id=126409&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=126409&action=review


This looks like a good step forward.  A couple of thoughts about naming below.

> Source/WebCore/dom/Document.cpp:1951
> -    if (page()->deviceMotionController())
> -	  
page()->deviceMotionController()->suspendEventsForAllListeners(domWindow());
> -    if (page()->deviceOrientationController())
> -	  
page()->deviceOrientationController()->suspendEventsForAllListeners(domWindow()
);
> +    if (DeviceMotionController* controller =
DeviceMotionController::requireOf(page()))
> +	   controller->suspendEventsForAllListeners(domWindow());
> +    if (DeviceOrientationController* controller =
DeviceOrientationController::requireOf(page()))
> +	   controller->suspendEventsForAllListeners(domWindow());
> +

This work should be done by an active DOM object rather than special-cased
here, but fixing that can be a later patch.

> Source/WebCore/page/DOMWindow.cpp:1565
> +	   if (DeviceMotionController* controller =
DeviceMotionController::requireOf(frame()))

I wonder if requireOf should be called "from" ?

> Source/WebCore/page/PageSupplement.cpp:40
> +    delete this;

I wonder if PageSupplement should have an OwnPtr member variable to itself. 
That would make the ownership model clearer.