Bug 93597 - Add DeviceController to support DOMWindow device events
Summary: Add DeviceController to support DOMWindow device events
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://www.w3.org/TR/proximity/
Keywords:
Depends on:
Blocks: 92837
  Show dependency treegraph
 
Reported: 2012-08-09 01:42 PDT by Kihong Kwon
Modified: 2012-09-16 22:59 PDT (History)
14 users (show)

See Also:


Attachments
Patch (33.21 KB, patch)
2012-08-09 04:47 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch (33.19 KB, patch)
2012-08-09 05:16 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch (33.04 KB, patch)
2012-08-09 21:37 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch (30.52 KB, patch)
2012-08-10 06:12 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch (33.42 KB, text/plain)
2012-08-13 23:19 PDT, Kihong Kwon
gns: commit-queue-
Details
Patch (33.88 KB, patch)
2012-08-14 03:23 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch (29.76 KB, patch)
2012-08-17 02:15 PDT, Kihong Kwon
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (37.53 KB, patch)
2012-08-19 22:03 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch (36.90 KB, patch)
2012-08-20 01:29 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch (21.22 KB, patch)
2012-08-21 18:44 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch (23.21 KB, patch)
2012-08-24 02:54 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch (23.09 KB, patch)
2012-08-24 03:13 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch (38.02 KB, patch)
2012-09-04 04:42 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch (24.21 KB, patch)
2012-09-06 02:28 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch (17.07 KB, patch)
2012-09-13 20:04 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kihong Kwon 2012-08-09 01:42:04 PDT
Add DeviceProximityConroller class to support Proximity Events.(http://www.w3.org/TR/proximity/)
Comment 1 Kihong Kwon 2012-08-09 04:47:11 PDT
Created attachment 157430 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-09 04:55:06 PDT
Comment on attachment 157430 [details]
Patch

Attachment 157430 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13452978
Comment 3 Gustavo Noronha (kov) 2012-08-09 05:01:19 PDT
Comment on attachment 157430 [details]
Patch

Attachment 157430 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13458646
Comment 4 Early Warning System Bot 2012-08-09 05:01:35 PDT
Comment on attachment 157430 [details]
Patch

Attachment 157430 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13464281
Comment 5 Gyuyoung Kim 2012-08-09 05:02:25 PDT
Comment on attachment 157430 [details]
Patch

Attachment 157430 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13471123
Comment 6 Build Bot 2012-08-09 05:02:26 PDT
Comment on attachment 157430 [details]
Patch

Attachment 157430 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13463338
Comment 7 Build Bot 2012-08-09 05:13:18 PDT
Comment on attachment 157430 [details]
Patch

Attachment 157430 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13471122
Comment 8 Kihong Kwon 2012-08-09 05:16:56 PDT
Created attachment 157439 [details]
Patch
Comment 9 Early Warning System Bot 2012-08-09 09:10:33 PDT
Comment on attachment 157439 [details]
Patch

Attachment 157439 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13463378
Comment 10 Build Bot 2012-08-09 09:17:02 PDT
Comment on attachment 157439 [details]
Patch

Attachment 157439 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13454889
Comment 11 Early Warning System Bot 2012-08-09 09:42:26 PDT
Comment on attachment 157439 [details]
Patch

Attachment 157439 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13462487
Comment 12 Build Bot 2012-08-09 09:49:37 PDT
Comment on attachment 157439 [details]
Patch

Attachment 157439 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13455893
Comment 13 WebKit Review Bot 2012-08-09 10:36:22 PDT
Comment on attachment 157439 [details]
Patch

Attachment 157439 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13463398
Comment 14 Peter Beverloo (cr-android ews) 2012-08-09 10:44:35 PDT
Comment on attachment 157439 [details]
Patch

Attachment 157439 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13455908
Comment 15 Kihong Kwon 2012-08-09 21:37:08 PDT
Created attachment 157633 [details]
Patch
Comment 16 Kentaro Hara 2012-08-10 04:43:21 PDT
Comment on attachment 157633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157633&action=review

Almost looks OK to me, but I'm not familiar with the code. Could any reviewer look at this patch?

> Source/WebCore/Modules/proximity/DeviceProximityController.h:59
> +    Page* m_page;

Is it needed?

> LayoutTests/fast/dom/Proximity/add-listener-from-callback.html:6
> +<script src="script-tests/add-listener-from-callback.js"></script>

Nit: you can directly write JavaScript here.

> LayoutTests/fast/dom/Proximity/basic-operation.html:6
> +<script src="script-tests/basic-operation.js"></script>

Ditto.

> LayoutTests/fast/dom/Proximity/event-after-navigation.html:6
> +<script src="script-tests/event-after-navigation.js"></script>

Ditto.

> LayoutTests/fast/dom/Proximity/multiple-frames.html:6
> +<script src="script-tests/multiple-frames.js"></script>

Ditto.
Comment 17 Kihong Kwon 2012-08-10 05:42:09 PDT
Comment on attachment 157633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157633&action=review

>> Source/WebCore/Modules/proximity/DeviceProximityController.h:59
>> +    Page* m_page;
> 
> Is it needed?

No. It will be removed.

>> LayoutTests/fast/dom/Proximity/add-listener-from-callback.html:6
>> +<script src="script-tests/add-listener-from-callback.js"></script>
> 
> Nit: you can directly write JavaScript here.

OK, I will merge html and js files.
Comment 18 Kihong Kwon 2012-08-10 06:12:05 PDT
Created attachment 157725 [details]
Patch
Comment 19 Adam Barth 2012-08-10 09:29:22 PDT
Comment on attachment 157725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157725&action=review

> Source/WebCore/Modules/proximity/DeviceProximityClient.h:34
> +    virtual void DeviceProximityControllerDestroyed() = 0;

DeviceProximityControllerDestroyed -> deviceProximityControllerDestroyed

> Source/WebCore/Modules/proximity/DeviceProximityController.cpp:44
> +DeviceProximityController::~DeviceProximityController()
> +{
> +    m_client->DeviceProximityControllerDestroyed();
> +}

I was under the impression that most objects like this don't need this sort of callback.  Typically the client and the controller are owned by the Page and get their destructors called at the same time.

> Source/WebCore/Modules/proximity/DeviceProximityController.cpp:83
> +void DeviceProximityController::addListener(DOMWindow* window)
> +{
> +    bool isEmpty = m_listeners.isEmpty();
> +    m_listeners.add(window);
> +    if (isEmpty)
> +        m_client->startUpdating();
> +}
> +
> +void DeviceProximityController::removeListener(DOMWindow* window)
> +{
> +    m_listeners.remove(window);
> +    m_suspendedListeners.remove(window);
> +    if (m_listeners.isEmpty())
> +        m_client->stopUpdating();
> +}
> +
> +void DeviceProximityController::removeAllListeners(DOMWindow* window)
> +{
> +    if (!m_listeners.contains(window))
> +        return;
> +
> +    m_listeners.removeAll(window);
> +    m_suspendedListeners.removeAll(window);
> +    if (m_listeners.isEmpty())
> +        m_client->stopUpdating();
> +}
> +
> +void DeviceProximityController::suspendEventsForAllListeners(DOMWindow* window)
> +{
> +    if (!m_listeners.contains(window))
> +        return;
> +
> +    int count = m_listeners.count(window);
> +    removeAllListeners(window);
> +    while (count--)
> +        m_suspendedListeners.add(window);
> +}

All this stuff doesn't look specific to DeviceProximity.  There are a number of features that follow this pattern.  Should we extract a base class to share this code?

> Source/WebCore/dom/Document.cpp:2214
> +#if ENABLE(PROXIMITY_EVENTS)
> +    if (DeviceProximityController* controller = DeviceProximityController::from(page()))
> +        controller->suspendEventsForAllListeners(domWindow());
>  #endif

We shouldn't need to do this.  Perhaps DeviceProximityController should be an ActiveDOMObject?  I was looking at this code the other day and had the same thought about DeviceMotionController and DeviceOrientationController.

> Source/WebCore/dom/Document.cpp:2233
> +    if (!page())
> +        return;

At the very least, this shouldn't be copy/pasted from line 2222

> Source/WebCore/dom/Document.cpp:2235
> +    if (DeviceProximityController* controller = DeviceProximityController::from(page()))
> +        controller->resumeEventsForAllListeners(domWindow());

Ditto.
Comment 20 Kihong Kwon 2012-08-10 21:32:10 PDT
Thank you for your review, Adam.

I absolutely agree with you. There are many common parts among device controllers.
I will add a super class for device controller and after finishing Proximity Events, I will change device orientation controller and device motion controller also.
Comment 21 Kihong Kwon 2012-08-13 23:16:13 PDT
> > Source/WebCore/dom/Document.cpp:2214
> > +#if ENABLE(PROXIMITY_EVENTS)
> > +    if (DeviceProximityController* controller = DeviceProximityController::from(page()))
> > +        controller->suspendEventsForAllListeners(domWindow());
> >  #endif
> 
> We shouldn't need to do this.  Perhaps DeviceProximityController should be an ActiveDOMObject?  I was looking at this code the other day and had the same thought about DeviceMotionController and DeviceOrientationController.

IMHO, we couldn't make controller to ActiveDOMObject in current implementation structure, because the controller is shared by several DOMWindows in the page.
In addition, there is no document object when device controller is made.

I would like to get your opinion.
Thanks for advance.
Comment 22 Kihong Kwon 2012-08-13 23:19:32 PDT
Created attachment 158229 [details]
Patch
Comment 23 WebKit Review Bot 2012-08-13 23:28:02 PDT
Comment on attachment 158229 [details]
Patch

Attachment 158229 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13487595
Comment 24 Build Bot 2012-08-13 23:56:27 PDT
Comment on attachment 158229 [details]
Patch

Attachment 158229 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13489516
Comment 25 Early Warning System Bot 2012-08-13 23:59:19 PDT
Comment on attachment 158229 [details]
Patch

Attachment 158229 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13485779
Comment 26 Gustavo Noronha (kov) 2012-08-14 00:41:26 PDT
Comment on attachment 158229 [details]
Patch

Attachment 158229 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13493494
Comment 27 Early Warning System Bot 2012-08-14 00:44:06 PDT
Comment on attachment 158229 [details]
Patch

Attachment 158229 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13489542
Comment 28 Kihong Kwon 2012-08-14 03:23:08 PDT
Created attachment 158275 [details]
Patch
Comment 29 Adam Barth 2012-08-14 10:02:03 PDT
Comment on attachment 158275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158275&action=review

> Source/WebCore/dom/Document.cpp:2211
> +#if ENABLE(PROXIMITY_EVENTS)

We shouldn't have any ENABLE(PROXIMITY_EVENTS) ifdefs outside of Modules/proximity.

> Source/WebCore/testing/Internals.cpp:1007
> +#if ENABLE(PROXIMITY_EVENTS)

Well, this one is probably ok.
Comment 30 Adam Barth 2012-08-14 10:48:02 PDT
Comment on attachment 158275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158275&action=review

> Source/WebCore/Modules/proximity/DeviceProximityController.cpp:43
> +    didChangeDeviceEvent(event);

Should this have a more "verb-like" name?  For example, dispatchDeviceEvent ?

> Source/WebCore/dom/DeviceClient.h:27
> +class DeviceClient {

This should be in WebCore/page because it's per-page state.

> Source/WebCore/dom/DeviceController.cpp:31
> +    , m_timer(this, &DeviceController::timerFired)

Should we give this a more descriptive name?  What sort of timer is it?  What are we supposed to do when the timer fires?

> Source/WebCore/dom/DeviceController.cpp:72
> +void DeviceController::removeAllListeners(DOMWindow* window)

How does removeListener differ from removeAllListeners?  Can a DOMWindow be registered twice?  Does that mean it gets events fired twice?  I understand now how this works, but it's a bit confusing.  Likely we can solve this problem with better names.

> Source/WebCore/dom/DeviceController.cpp:87
> +    RefPtr<Event> rpEvent = event;

Usually we call the RefPtr version just "event" and the PassRefPtr version prpEvent

> Source/WebCore/dom/DeviceController.cpp:98
> +    int count = m_listeners.count(window);

Should count be a size_t?

> Source/WebCore/dom/DeviceController.h:32
> +class DeviceController : public Supplement<Page> {

This should be in WebCore/page because it's per-page state.
Comment 31 Adam Barth 2012-08-14 10:50:22 PDT
This iteration of the patch is much better than the earlier approach.  The comments above are all relatively minor.  In IRC we discussed a follow up patch that refactors some of the other device controllers to inherit from DeviceController.
Comment 32 Peter Beverloo (cr-android ews) 2012-08-15 01:32:54 PDT
Comment on attachment 158229 [details]
Patch

Attachment 158229 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13503543
Comment 33 Kihong Kwon 2012-08-17 02:15:53 PDT
Created attachment 159052 [details]
Patch
Comment 34 WebKit Review Bot 2012-08-17 02:36:36 PDT
Comment on attachment 159052 [details]
Patch

Attachment 159052 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13521318
Comment 35 Gyuyoung Kim 2012-08-17 02:37:40 PDT
Comment on attachment 159052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159052&action=review

> Source/WebCore/ChangeLog:9
> +        and add proximity event to DeviceController for testing.

In my humble opinion, a patch for testing can be filed to new bug. But, if this patch is acceptable, I would like to recommend to add a newline for other description.

> Source/WebCore/dom/DeviceController.h:45
> +    DeviceController(Page*);

Missing explicit keyword.
Comment 36 Peter Beverloo (cr-android ews) 2012-08-17 02:51:15 PDT
Comment on attachment 159052 [details]
Patch

Attachment 159052 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13522072
Comment 37 Early Warning System Bot 2012-08-17 02:53:03 PDT
Comment on attachment 159052 [details]
Patch

Attachment 159052 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13514748
Comment 38 Early Warning System Bot 2012-08-17 03:05:26 PDT
Comment on attachment 159052 [details]
Patch

Attachment 159052 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13509960
Comment 39 Build Bot 2012-08-17 03:35:43 PDT
Comment on attachment 159052 [details]
Patch

Attachment 159052 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13520372
Comment 40 Kihong Kwon 2012-08-19 22:03:48 PDT
Created attachment 159329 [details]
Patch
Comment 41 Adam Barth 2012-08-19 23:05:21 PDT
Comment on attachment 159329 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159329&action=review

> Source/WebCore/GNUmakefile.list.am:2672
> +	Source/WebCore/dom/DeviceClient.h \
> +	Source/WebCore/dom/DeviceController.cpp \
> +	Source/WebCore/dom/DeviceController.h \

I thought you were going to move these to the page directory?
Comment 42 Kihong Kwon 2012-08-20 01:08:53 PDT
(In reply to comment #41)
> (From update of attachment 159329 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159329&action=review
> 
> > Source/WebCore/GNUmakefile.list.am:2672
> > +	Source/WebCore/dom/DeviceClient.h \
> > +	Source/WebCore/dom/DeviceController.cpp \
> > +	Source/WebCore/dom/DeviceController.h \
> 
> I thought you were going to move these to the page directory?

I got it, You are right.
Comment 43 Kihong Kwon 2012-08-20 01:29:44 PDT
Created attachment 159363 [details]
Patch
Comment 44 Adam Barth 2012-08-20 13:42:10 PDT
Comment on attachment 159363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159363&action=review

OK.  This is getting close.  Can we remove the PROXIMITY_EVENTS-specific code from this patch?  It doesn't seem like it actually does anything because nothing inherits from DeviceClient.

Rather than adding support for PROXIMITY_EVENTS in this patch, I'd prefer if we:
1) Landed this patch
2) Changed the existing device code to use this mechanism (probably one per patch)
3) Added support for device proximity

Is that a reasonable course of action?

> Source/WebCore/page/DeviceController.cpp:44
> +#if ENABLE(PROXIMITY_EVENTS)
> +    if (eventType == eventNames().webkitdeviceproximityEvent)
> +        return true;
> +#endif

Should we do the PROXIMITY_EVENTS specific work in the next patch?

> Source/WebCore/page/DeviceController.cpp:58
> +        listeners = new ListenersCountedSet();

This is a naked new.  Presumably we want adoptPtr here.

> Source/WebCore/page/DeviceController.cpp:77
> +        delete listeners;

Raw delete.  Let's use OwnPtr instead!

> Source/WebCore/page/DeviceController.cpp:87
> +    for (int i = 0; i < m_listeners.size(); ++i) {

int -> size_t ?

> Source/WebCore/page/DeviceController.cpp:116
> +        if (!listeners[i]->document()->activeDOMObjectsAreSuspended())

Is this the right semantics for suspended documents?  It's fine to just drop the events on the floor?
Comment 45 Kihong Kwon 2012-08-20 19:16:43 PDT
(In reply to comment #44)
> (From update of attachment 159363 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159363&action=review
> 
> OK.  This is getting close.  Can we remove the PROXIMITY_EVENTS-specific code from this patch?  It doesn't seem like it actually does anything because nothing inherits from DeviceClient.
> 
> Rather than adding support for PROXIMITY_EVENTS in this patch, I'd prefer if we:
> 1) Landed this patch
> 2) Changed the existing device code to use this mechanism (probably one per patch)
> 3) Added support for device proximity
> 
> Is that a reasonable course of action?

I think so. I will do that.

> > Source/WebCore/page/DeviceController.cpp:116
> > +        if (!listeners[i]->document()->activeDOMObjectsAreSuspended())
> 
> Is this the right semantics for suspended documents?  It's fine to just drop the events on the floor?

I think there is no problem about dropping events. this is only for specific window which is suspended.

IMHO, window doesn't need to get a device event, when it's state is suspeneded.
And window can receive a device event from client if it gets back to active state because listener still alive in the listeners.

Thank you for reviewing. :-)
Comment 46 Kihong Kwon 2012-08-21 03:25:41 PDT
> > Source/WebCore/page/DeviceController.cpp:87
> > +    for (int i = 0; i < m_listeners.size(); ++i) {
> 
> int -> size_t ?

It makes warning, because size() of HashMap returns int.
Comment 47 Adam Barth 2012-08-21 07:41:52 PDT
(In reply to comment #46)
> > > Source/WebCore/page/DeviceController.cpp:87
> > > +    for (int i = 0; i < m_listeners.size(); ++i) {
> > 
> > int -> size_t ?
> 
> It makes warning, because size() of HashMap returns int.

Ok.  We should keep it as int then.  Thanks.  (I forget which of these collections use size_t and which use int.)
Comment 48 Kihong Kwon 2012-08-21 18:44:14 PDT
Created attachment 159839 [details]
Patch
Comment 49 Adam Barth 2012-08-22 12:03:19 PDT
Did you mean to mark this patch for review?
Comment 50 Kihong Kwon 2012-08-23 03:46:31 PDT
(In reply to comment #49)
> Did you mean to mark this patch for review?

Not yet, I think I need to add some more.
Thank you. :)
Comment 51 Kihong Kwon 2012-08-24 02:54:14 PDT
Created attachment 160369 [details]
Patch
Comment 52 Kihong Kwon 2012-08-24 03:13:55 PDT
Created attachment 160375 [details]
Patch
Comment 53 Adam Barth 2012-08-27 11:27:55 PDT
Comment on attachment 160375 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160375&action=review

I think we can improve this design substantially.  You're trying to have DeviceController do too much.  It's responsible for handling all types of events for all documents in a Page.  Instead, we should subclass DeviceController for each type of event and keep the per-DOMWindow state in a Supplement<DOMWindow>.  That way this state will be owned by the DOMWindow instead of needing to use a RefPtr<DOMWindow> to keep the DOMWindow alive.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28545
>  				FD537352137B651800008DCE /* ZeroPole.cpp in Sources */,
> +				CC24D64A15E212E90079DD79 /* DeviceController.cpp in Sources */,

When you modify the Xcodeproj file, you might want to run Tools/Scrips/sort-xcodeproj .  That keeps the file sorted and reduces the risk of merge conflicts.

> Source/WebCore/page/DeviceController.cpp:51
> +    if (!m_clients.contains(eventType))
> +        return;
> +
> +    DeviceClientMap::iterator iterClient = m_clients.find(eventType);

You can merge these calls together so we only do a find once.  The iterator will be end() if the eventType isn't in the map.

iterClient -> client

> Source/WebCore/page/DeviceController.cpp:52
> +    if (iterClient != m_clients.end() && iterClient->second->lastEvent()) {

iterClient != m_clients.end()   <--  This is redundant with the contains check

> Source/WebCore/page/DeviceController.cpp:53
> +        m_lastEventHandler.append(LastEventHandler(window, iterClient->second->lastEvent()));

It's strange that window is being passed in here...  I'll keep reading to see how the lifetime issues work out.

> Source/WebCore/page/DeviceController.cpp:74
> +    ListenersCountedSet* listeners = m_listeners.find(eventType)->second.get();

Similar problem as above.  It's inefficient to do the contains and the find operation separately.

> Source/WebCore/page/DeviceController.cpp:82
> +    size_t lastEventHandlerCount = m_lastEventHandler.size();
> +    for (size_t i = 0; i < lastEventHandlerCount; ++i)

You don't need the lastEventHandlerCount variable.  You can just put m_lastEventHandler.size() in the loop condition.

> Source/WebCore/page/DeviceController.cpp:90
> +    int listenersCount = m_listeners.size();
> +    for (int i = 0; i < listenersCount; ++i) {

ditto

> Source/WebCore/page/DeviceController.cpp:101
> +            size_t lastEventHandlerCount = m_lastEventHandler.size();
> +            for (size_t i = 0; i < lastEventHandlerCount; ++i)

ditto

> Source/WebCore/page/DeviceController.cpp:127
> +    Vector<RefPtr<DOMWindow> > listeners;

It's not a good idea to grab a RefPtr to the DOMWindow.  DOMWindow is only RefCounted because for some odd cases involving initial documents.  This could shouldn't be holding references to DOMWindows.

> Source/WebCore/page/Page.cpp:118
> +    , m_deviceController(DeviceController::create(this))

We should avoid spamming Page with a bunch of random objects.  Instead, DeviceController should be a Supplement<Page>.
Comment 54 Kihong Kwon 2012-09-04 04:42:18 PDT
Created attachment 162009 [details]
Patch
Comment 55 WebKit Review Bot 2012-09-04 05:03:59 PDT
Comment on attachment 162009 [details]
Patch

Attachment 162009 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13761105

New failing tests:
accessibility/aria-checkbox-checked.html
http/tests/appcache/deferred-events-delete-while-raising.html
animations/3d/matrix-transform-type-animation.html
accessibility/aria-readonly.html
accessibility/canvas-description-and-role.html
http/tests/appcache/cyrillic-uri.html
animations/animation-add-events-in-handler.html
accessibility/aria-labelledby-on-input.html
http/tests/appcache/deferred-events-delete-while-raising-timer.html
http/tests/appcache/crash-when-navigating-away-then-back.html
animations/animation-direction-alternate-reverse.html
http/tests/appcache/credential-url.html
http/tests/appcache/access-via-redirect.php
http/tests/appcache/different-scheme.html
accessibility/aria-hidden.html
http/tests/appcache/destroyed-frame.html
animations/animation-controller-drt-api.html
animations/3d/transform-perspective.html
accessibility/aria-scrollbar-role.html
accessibility/accessibility-node-reparent.html
canvas/philip/tests/2d.canvas.reference.html
animations/3d/state-at-end-event-transform.html
accessibility/aria-labelledby-stay-within.html
animations/animation-direction-reverse-hardware.html
accessibility/aria-help.html
animations/animation-direction-reverse-fill-mode.html
accessibility/adjacent-continuations-cause-assertion-failure.html
http/tests/appcache/detached-iframe.html
animations/animation-direction-reverse-timing-functions-hardware.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
Comment 56 Kihong Kwon 2012-09-06 02:28:40 PDT
Created attachment 162456 [details]
Patch
Comment 57 Adam Barth 2012-09-06 11:02:56 PDT
Comment on attachment 162456 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162456&action=review

> Source/WebCore/page/DOMWindow.cpp:1682
> +    DeviceController::removeAllDeviceEvnetListener(this, page());

Evnet -> Event

> Source/WebCore/page/DOMWindowDeviceProvider.h:31
> +class DOMWindowDeviceProvider : public Supplement<DOMWindow> {

I don't understand the point of DOMWindowDeviceProvider.  In a previous iteration of this patch, you were storing a bunch of per-DOMWindow state.  I asked you to create a Supplement<DOMWindow> to store that state, but this class doesn't actually store any state.

> Source/WebCore/page/DeviceClient.h:34
> +    virtual PassRefPtr<Event> lastEvent() const = 0;

Does this function transfer ownership of the last event?  I would have expected this to just be an getter.  If so, it should return a raw Event*

> Source/WebCore/page/DeviceController.cpp:63
> +    DOMWindowDeviceProvider* provider = static_cast<DOMWindowDeviceProvider*>(Supplement<DOMWindow>::from(window, supplementName()));
> +    if (!provider) {
> +        provider = new DOMWindowDeviceProvider(window);
> +        Supplement<DOMWindow>::provideTo(window, supplementName(), adoptPtr(provider));
> +    }
> +    m_providers.add(provider);

This work should be done by DOMWindowDeviceProvider in a from() function.

> Source/WebCore/page/DeviceController.cpp:101
> +void DeviceController::dispatchDeivceEvent(PassRefPtr<Event> event)

Deivce -> Device
Comment 58 Adam Barth 2012-09-06 11:04:11 PDT
Comment on attachment 162456 [details]
Patch

I'm sorry.  I'm tired of reviewing these patches.  We don't seem to be getting anywhere.
Comment 59 Kihong Kwon 2012-09-13 19:56:12 PDT
Comment on attachment 162456 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162456&action=review

>> Source/WebCore/page/DOMWindowDeviceProvider.h:31
>> +class DOMWindowDeviceProvider : public Supplement<DOMWindow> {
> 
> I don't understand the point of DOMWindowDeviceProvider.  In a previous iteration of this patch, you were storing a bunch of per-DOMWindow state.  I asked you to create a Supplement<DOMWindow> to store that state, but this class doesn't actually store any state.

If I make Supplement<DOMWindow>, I need to make unnecessary object for attaching DOMWindow.
But I think in this case it is enough to keep window pointer in the DeviceController.
Therefore I will remove this class.

>> Source/WebCore/page/DeviceClient.h:34
>> +    virtual PassRefPtr<Event> lastEvent() const = 0;
> 
> Does this function transfer ownership of the last event?  I would have expected this to just be an getter.  If so, it should return a raw Event*

Yes, we need to transfer ownership to EventTarget When client create a event to fire.
But, I think this function is not good to check weather there is a preserved event or not in the client, therefore I will add hasPreservedEvent() for checking.
Comment 60 Kihong Kwon 2012-09-13 20:04:31 PDT
Created attachment 164033 [details]
Patch
Comment 61 Eric Seidel (no email) 2012-09-15 09:52:36 PDT
Comment on attachment 164033 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164033&action=review

> Source/WebCore/ChangeLog:9
> +        Add DeviceController to support device related event control.
> +        DeviceController can be used by inherit to the all kind of device controller.

"Add DeviceController base-class for soon-to-be-added DeviceMotionController, DeviceOrientationController and ProximityController."
Comment 62 Eric Seidel (no email) 2012-09-15 09:53:38 PDT
Why is a m_timer needed?

It may be time to close this bug and start-over.  This one is quite long now.
Comment 63 Kihong Kwon 2012-09-16 21:55:29 PDT
(In reply to comment #62)
> Why is a m_timer needed?

We don't know exactly when event is fired by device after addEventListener called.
But if there is a event which is already set by other addEventListener, we can fire a event instantly.
In this case, a m_timer is used to fire a reserved event asynchronously.

> 
> It may be time to close this bug and start-over.  This one is quite long now.

OK, I will do that.
Thank you.
Comment 64 Kihong Kwon 2012-09-16 22:59:34 PDT
This patch is move to bug 96894 because this thread is too long.