Currently adding a Web API which has platform access needs to modify Page and PageClient. We should have more unintrusive way to handle this.
Created attachment 126272 [details] Patch
Comment on attachment 126272 [details] Patch Attachment 126272 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11484186
Comment on attachment 126272 [details] Patch Attachment 126272 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11480234 New failing tests: fast/dom/DeviceOrientation/no-synchronous-events.html fast/dom/DeviceOrientation/basic-operation.html fast/dom/DeviceOrientation/multiple-frames.html fast/dom/DeviceOrientation/updates.html fast/dom/DeviceOrientation/null-values.html fast/dom/DeviceOrientation/add-listener-from-callback.html
Comment on attachment 126272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126272&action=review I really like the approach. A few comments below. It looks like you've got some test failures to work through. > Source/WebCore/page/Page.cpp:230 > + notifyDestroyedToSupplements(); We might also want a PageDestructionObserver, similar to FrameDestructionObserver, but we can add that in a future patch. > Source/WebCore/page/PageSupplement.h:41 > +class PageSupplementKey { > + WTF_MAKE_NONCOPYABLE(PageSupplementKey); > +public: > + PageSupplementKey() { } > +}; You might consider using AtomicStrings as the keys.
I'm told Mr. Adler has a similar dream and might have some thoughts on the approach.
Created attachment 126409 [details] Patch
Hi Adam, thanks for taking a look! I polished the patch a bit and fixed broken tests. > > > Source/WebCore/page/Page.cpp:230 > > + notifyDestroyedToSupplements(); > > We might also want a PageDestructionObserver, similar to FrameDestructionObserver, but we can add that in a future patch. Sure. We also have something like DocumentSupplement/WindowSupplment. > > +class PageSupplementKey { > > + WTF_MAKE_NONCOPYABLE(PageSupplementKey); > > +public: > > + PageSupplementKey() { } > > +}; > > You might consider using AtomicStrings as the keys. Fixed. (In reply to comment #5) > I'm told Mr. Adler has a similar dream and might have some thoughts on the approach. This is great! I'd love to hear his opinion.
Comment on attachment 126409 [details] Patch 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.
I was thinking about the ownership model here a bit. I wonder if would be better if the Page should own these supplements in PageSupplementMap.
Created attachment 126689 [details] Patch for landing
Created attachment 126690 [details] Patch for landing
(In reply to comment #8) > (From update of attachment 126409 [details]) > 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. I feel same. Filed Bug 78442. > > > Source/WebCore/page/DOMWindow.cpp:1565 > > + if (DeviceMotionController* controller = DeviceMotionController::requireOf(frame())) > > I wonder if requireOf should be called "from" ? > Done. > > 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. I notice that most of these supplement objects are captured by OwnPtr. So I changed m_supplements' type to HashMap<AtomiStringImpl*,OwnPtr<PageSupplement>> and removed pageDestroyed() virtual function for now. I filed use Bug 78443 to provide such a hook.
Comment on attachment 126690 [details] Patch for landing Clearing flags on attachment: 126690 Committed r107518: <http://trac.webkit.org/changeset/107518>
All reviewed patches have been landed. Closing bug.