Bug 78085 – Page should have less intrusive way to associate API implementation objects.

• Component: WebKit API
• Status: RESOLVED FIXED

[Hajime Morrita]

Currently adding a Web API which has platform access needs to modify Page and PageClient.
We should have more unintrusive way to handle this.

[Hajime Morrita]

Created attachment 126272 [details]
Patch

[Early Warning System Bot]

Comment on attachment 126272 [details]
Patch

Attachment 126272 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11484186

[WebKit Review Bot]

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 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]

I'm told Mr. Adler has a similar dream and might have some thoughts on the approach.

[Hajime Morrita]

Created attachment 126409 [details]
Patch

[Hajime Morrita]

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 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]

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]

Created attachment 126689 [details]
Patch for landing

[Hajime Morrita]

Created attachment 126690 [details]
Patch for landing

[Hajime Morrita]

(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 on attachment 126690 [details]
Patch for landing

Clearing flags on attachment: 126690

Committed r107518: <http://trac.webkit.org/changeset/107518>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.