Bug 78085

Summary: Page should have less intrusive way to associate API implementation objects.
Product: WebKit Reporter: Hajime Morrita <morrita@google.com>
Component: WebKit APIAssignee: Hajime Morrita <morrita@google.com>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth@webkit.org, ap@webkit.org, darin@apple.com, dglazkov@chromium.org, donggwan.kim@samsung.com, haraken@chromium.org, kihong.kwon@samsung.com, rakuco@webkit.org, s.choi@hackerslab.eu, webkit.review.bot@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 78471    
Bug Blocks: 72010, 78440    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

Description From 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.
------- Comment #1 From 2012-02-09 03:30:01 PST -------
Created an attachment (id=126272) [details]
Patch
------- Comment #2 From 2012-02-09 03:58:00 PST -------
(From update of attachment 126272 [details])
Attachment 126272 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11484186
------- Comment #3 From 2012-02-09 05:18:48 PST -------
(From update of attachment 126272 [details])
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 #4 From 2012-02-09 10:51:14 PST -------
(From update of attachment 126272 [details])
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.
------- Comment #5 From 2012-02-09 10:57:02 PST -------
I'm told Mr. Adler has a similar dream and might have some thoughts on the approach.
------- Comment #6 From 2012-02-09 17:21:14 PST -------
Created an attachment (id=126409) [details]
Patch
------- Comment #7 From 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.
------- Comment #8 From 2012-02-10 13:57:42 PST -------
(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.

> 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.
------- Comment #9 From 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.
------- Comment #10 From 2012-02-12 16:48:28 PST -------
Created an attachment (id=126689) [details]
Patch for landing
------- Comment #11 From 2012-02-12 16:50:03 PST -------
Created an attachment (id=126690) [details]
Patch for landing
------- Comment #12 From 2012-02-12 16:53:08 PST -------
(In reply to comment #8)
> (From update of attachment 126409 [details] [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 #13 From 2012-02-12 17:18:45 PST -------
(From update of attachment 126690 [details])
Clearing flags on attachment: 126690

Committed r107518: <http://trac.webkit.org/changeset/107518>
------- Comment #14 From 2012-02-12 17:18:52 PST -------
All reviewed patches have been landed.  Closing bug.