[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
bugzilla-daemon at webkit.org
Fri Feb 10 13:57:42 PST 2012
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.
More information about the webkit-reviews
mailing list