Add DeviceProximityConroller class to support Proximity Events.(http://www.w3.org/TR/proximity/)
Created attachment 157430 [details] Patch
Comment on attachment 157430 [details] Patch Attachment 157430 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13452978
Comment on attachment 157430 [details] Patch Attachment 157430 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13458646
Comment on attachment 157430 [details] Patch Attachment 157430 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13464281
Comment on attachment 157430 [details] Patch Attachment 157430 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13471123
Comment on attachment 157430 [details] Patch Attachment 157430 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13463338
Comment on attachment 157430 [details] Patch Attachment 157430 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13471122
Created attachment 157439 [details] Patch
Comment on attachment 157439 [details] Patch Attachment 157439 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13463378
Comment on attachment 157439 [details] Patch Attachment 157439 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13454889
Comment on attachment 157439 [details] Patch Attachment 157439 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13462487
Comment on attachment 157439 [details] Patch Attachment 157439 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13455893
Comment on attachment 157439 [details] Patch Attachment 157439 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13463398
Comment on attachment 157439 [details] Patch Attachment 157439 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13455908
Created attachment 157633 [details] Patch
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 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.
Created attachment 157725 [details] Patch
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.
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.
> > 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.
Created attachment 158229 [details] Patch
Comment on attachment 158229 [details] Patch Attachment 158229 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13487595
Comment on attachment 158229 [details] Patch Attachment 158229 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13489516
Comment on attachment 158229 [details] Patch Attachment 158229 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13485779
Comment on attachment 158229 [details] Patch Attachment 158229 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13493494
Comment on attachment 158229 [details] Patch Attachment 158229 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13489542
Created attachment 158275 [details] Patch
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 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.
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 on attachment 158229 [details] Patch Attachment 158229 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13503543
Created attachment 159052 [details] Patch
Comment on attachment 159052 [details] Patch Attachment 159052 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13521318
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 on attachment 159052 [details] Patch Attachment 159052 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13522072
Comment on attachment 159052 [details] Patch Attachment 159052 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13514748
Comment on attachment 159052 [details] Patch Attachment 159052 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13509960
Comment on attachment 159052 [details] Patch Attachment 159052 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13520372
Created attachment 159329 [details] Patch
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?
(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.
Created attachment 159363 [details] Patch
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?
(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. :-)
> > 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.
(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.)
Created attachment 159839 [details] Patch
Did you mean to mark this patch for review?
(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. :)
Created attachment 160369 [details] Patch
Created attachment 160375 [details] Patch
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>.
Created attachment 162009 [details] Patch
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
Created attachment 162456 [details] Patch
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 on attachment 162456 [details] Patch I'm sorry. I'm tired of reviewing these patches. We don't seem to be getting anywhere.
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.
Created attachment 164033 [details] Patch
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."
Why is a m_timer needed? It may be time to close this bug and start-over. This one is quite long now.
(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.
This patch is move to bug 96894 because this thread is too long.