Bug 93597 – Add DeviceController to support DOMWindow device events

• Status: RESOLVED
• Resolution: INVALID
• Component: DOM

[Kihong Kwon]

Add DeviceProximityConroller class to support Proximity Events.(http://www.w3.org/TR/proximity/)

[Kihong Kwon]

Created attachment 157430 [details]
Patch

[WebKit Review Bot]

Comment on attachment 157430 [details]
Patch

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

[Gustavo Noronha (kov)]

Comment on attachment 157430 [details]
Patch

Attachment 157430 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13458646

[Early Warning System Bot]

Comment on attachment 157430 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 157430 [details]
Patch

Attachment 157430 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13471123

[Build Bot]

Comment on attachment 157430 [details]
Patch

Attachment 157430 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13463338

[Build Bot]

Comment on attachment 157430 [details]
Patch

Attachment 157430 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13471122

[Kihong Kwon]

Created attachment 157439 [details]
Patch

[Early Warning System Bot]

Comment on attachment 157439 [details]
Patch

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

[Build Bot]

Comment on attachment 157439 [details]
Patch

Attachment 157439 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13454889

[Early Warning System Bot]

Comment on attachment 157439 [details]
Patch

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

[Build Bot]

Comment on attachment 157439 [details]
Patch

Attachment 157439 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13455893

[WebKit Review Bot]

Comment on attachment 157439 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 157439 [details]
Patch

Attachment 157439 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13455908

[Kihong Kwon]

Created attachment 157633 [details]
Patch

[Kentaro Hara]

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.

[Kihong Kwon]

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.

[Kihong Kwon]

Created attachment 157725 [details]
Patch

[Adam Barth]

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.

[Kihong Kwon]

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.

[Kihong Kwon]

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

[Kihong Kwon]

Created attachment 158229 [details]
Patch

[WebKit Review Bot]

Comment on attachment 158229 [details]
Patch

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

[Build Bot]

Comment on attachment 158229 [details]
Patch

Attachment 158229 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13489516

[Early Warning System Bot]

Comment on attachment 158229 [details]
Patch

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

[Gustavo Noronha (kov)]

Comment on attachment 158229 [details]
Patch

Attachment 158229 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13493494

[Early Warning System Bot]

Comment on attachment 158229 [details]
Patch

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

[Kihong Kwon]

Created attachment 158275 [details]
Patch

[Adam Barth]

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.

[Adam Barth]

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.

[Adam Barth]

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.

[Peter Beverloo (cr-android ews)]

Comment on attachment 158229 [details]
Patch

Attachment 158229 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13503543

[Kihong Kwon]

Created attachment 159052 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159052 [details]
Patch

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

[Gyuyoung Kim]

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.

[Peter Beverloo (cr-android ews)]

Comment on attachment 159052 [details]
Patch

Attachment 159052 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13522072

[Early Warning System Bot]

Comment on attachment 159052 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 159052 [details]
Patch

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

[Build Bot]

Comment on attachment 159052 [details]
Patch

Attachment 159052 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13520372

[Kihong Kwon]

Created attachment 159329 [details]
Patch

[Adam Barth]

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?

[Kihong Kwon]

(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.

[Kihong Kwon]

Created attachment 159363 [details]
Patch

[Adam Barth]

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?

[Kihong Kwon]

(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. :-)

[Kihong Kwon]

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

[Adam Barth]

(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.)

[Kihong Kwon]

Created attachment 159839 [details]
Patch

[Adam Barth]

Did you mean to mark this patch for review?

[Kihong Kwon]

(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. :)

[Kihong Kwon]

Created attachment 160369 [details]
Patch

[Kihong Kwon]

Created attachment 160375 [details]
Patch

[Adam Barth]

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

[Kihong Kwon]

Created attachment 162009 [details]
Patch

[WebKit Review Bot]

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

[Kihong Kwon]

Created attachment 162456 [details]
Patch

[Adam Barth]

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

[Adam Barth]

Comment on attachment 162456 [details]
Patch

I'm sorry.  I'm tired of reviewing these patches.  We don't seem to be getting anywhere.

[Kihong Kwon]

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.

[Kihong Kwon]

Created attachment 164033 [details]
Patch

[Eric Seidel (no email)]

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."

[Eric Seidel (no email)]

Why is a m_timer needed?

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

[Kihong Kwon]

(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.

[Kihong Kwon]

This patch is move to bug 96894 because this thread is too long.