RESOLVED FIXED 78085
Page should have less intrusive way to associate API implementation objects.
https://bugs.webkit.org/show_bug.cgi?id=78085
Summary Page should have less intrusive way to associate API implementation objects.
Hajime Morrita
Reported 2012-02-08 00:32:48 PST
Currently adding a Web API which has platform access needs to modify Page and PageClient. We should have more unintrusive way to handle this.
Attachments
Patch (38.94 KB, patch)
2012-02-09 03:30 PST, Hajime Morrita
no flags
Patch (41.10 KB, patch)
2012-02-09 17:21 PST, Hajime Morrita
no flags
Patch for landing (40.87 KB, patch)
2012-02-12 16:48 PST, Hajime Morrita
no flags
Patch for landing (40.85 KB, patch)
2012-02-12 16:50 PST, Hajime Morrita
no flags
Hajime Morrita
Comment 1 2012-02-09 03:30:01 PST
Early Warning System Bot
Comment 2 2012-02-09 03:58:00 PST
WebKit Review Bot
Comment 3 2012-02-09 05:18:48 PST
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
Adam Barth
Comment 4 2012-02-09 10:51:14 PST
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.
Adam Barth
Comment 5 2012-02-09 10:57:02 PST
I'm told Mr. Adler has a similar dream and might have some thoughts on the approach.
Hajime Morrita
Comment 6 2012-02-09 17:21:14 PST
Hajime Morrita
Comment 7 2012-02-09 17:27:37 PST
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.
Adam Barth
Comment 8 2012-02-10 13:57:42 PST
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.
Adam Barth
Comment 9 2012-02-12 16:37:32 PST
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.
Hajime Morrita
Comment 10 2012-02-12 16:48:28 PST
Created attachment 126689 [details] Patch for landing
Hajime Morrita
Comment 11 2012-02-12 16:50:03 PST
Created attachment 126690 [details] Patch for landing
Hajime Morrita
Comment 12 2012-02-12 16:53:08 PST
(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.
WebKit Review Bot
Comment 13 2012-02-12 17:18:45 PST
Comment on attachment 126690 [details] Patch for landing Clearing flags on attachment: 126690 Committed r107518: <http://trac.webkit.org/changeset/107518>
WebKit Review Bot
Comment 14 2012-02-12 17:18:52 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.