WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(41.10 KB, patch)
2012-02-09 17:21 PST
,
Hajime Morrita
no flags
Details
Formatted Diff
Diff
Patch for landing
(40.87 KB, patch)
2012-02-12 16:48 PST
,
Hajime Morrita
no flags
Details
Formatted Diff
Diff
Patch for landing
(40.85 KB, patch)
2012-02-12 16:50 PST
,
Hajime Morrita
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Hajime Morrita
Comment 1
2012-02-09 03:30:01 PST
Created
attachment 126272
[details]
Patch
Early Warning System Bot
Comment 2
2012-02-09 03:58:00 PST
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 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
Created
attachment 126409
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug