Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13866007:
WebKit & WebCore part of the device motion implementation using the platform layer. (Closed)
|
Created:
7 years, 8 months ago by timvolodine Modified:
7 years, 4 months ago CC:
blink-reviews, abarth_chromum.org, Miguel Garcia, bulach Base URL:
https://chromium.googlesource.com/chromium/blink.git@devicemotion-webcore Visibility:
Public. |
DescriptionThis patch contains both WebCore and WebKit parts for the device motion implementation.
Webcore: refactoring of device motion to make it simpler and follow the platform-based approach. The controller classes are now singletons and do not use clients anymore. The start/stop client methods and lastDevice{Motion,Orientation}Data are implemented in the platform-specific (i.e. WebCore/platform) part of the code.
WebKit: device motion implementation using the platform layer. The calls from WebCore go to a special 'pump' singleton class which reads the motion data via the platform layer and fires the events with a certain frequency.
BUG=135804
Patch Set 1 #Patch Set 2 : added new LICENSE header #
Total comments: 86
Patch Set 3 : fixed comments from Peter and Eric, made the DeviceMotionController supplement of page instead of s… #Patch Set 4 : added unregister in destructor of DeviceMotionController #
Total comments: 46
Patch Set 5 : fixed Peter's comments #
Total comments: 20
Patch Set 6 : fixed next round of comments by Peter #
Total comments: 36
Patch Set 7 : fixed Eric's comments. #
Total comments: 12
Messages
Total messages: 22 (0 generated)
First round of comments. Thank you, Tim!! https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... File Source/Platform/chromium/public/Platform.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/Platform.h:514: Please add a separate section for Device Orientation and Motion. This would also benefit from some documentation. https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionData.h:35: namespace WTF { template <typename T> class PassRefPtr; } Can you just include include wtf/PassRefPtr.h instead? https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... File Source/Platform/chromium/public/WebDeviceMotionReader.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionReader.h:38: // FIXME(timvolodine) : abstract this into a common superclass, to be used with device orientation as well. nit: Blink style uses unattributed FIXMEs. https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionReader.h:44: virtual void lastDeviceMotionData(WebDeviceMotionData&) = 0; latestDeviceMotionData? https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionReader.h:44: virtual void lastDeviceMotionData(WebDeviceMotionData&) = 0; Since this is an API, I'd prefer some more documentation on what exactly will happen when someone calls startUpdating() or stopUpdating(), and what the "latestDeviceMotionData" means (i.e. it doesn't update live). https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... File Source/WebCore/dom/DeviceMotionController.cpp (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.cpp:29: */ We can't just change the copyright header if Apple or Samsung added this file. Best to leave it untouched. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.cpp:46: DeviceMotionController& DeviceMotionController::shared() While not that common in WebKit (yet), I'd prefer it if we can add a comment above this method which says "// static". https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.cpp:59: return lastDeviceMotionData(); Where does lastDeviceMotionData come from? https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.cpp:64: return DeviceMotionEvent::create(eventNames().devicemotionEvent, lastDeviceMotionData()); dito to line 59. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.cpp:81: startUpdating(); Unless we need to publicly expose startUpdating/stopUpdating from this class, we may as well call to {start,stop}MonitoringDeviceMotion() directly here. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... File Source/WebCore/dom/DeviceMotionController.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.h:29: */ We can't just change the copyright header if Samsung added this file. Best to leave it untouched. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.h:50: bool isActiveAt(Page* page) { return m_activePages.contains(page); } This class will now have both isActive() and isActiveAt(), which seems a bit ambiguous. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.h:53: virtual void stopUpdating(); Why are these declared as virtual? Also, do they have to be public? https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.h:55: // ------------------------------------------------- nit: the horizontal line without explanation is a bit odd. Remove it? https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.h:57: void addDeviceEventListener(DOMWindow*) OVERRIDE; Why are these declared OVERRIDE without being declared virtual? That kind of defeats the purpose of the base class. Will we be able to generalize the implementation to DeviceController again once Device Orientation also starts using this design? https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.h:63: // DeviceClient* client() { return m_client; } No commented-out code please. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.h:67: HashCountedSet<Page*> m_activePages; nit: could you add an empty line between the constructor and the member variable? https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/page/DOMWin... File Source/WebCore/page/DOMWindow.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/page/DOMWin... Source/WebCore/page/DOMWindow.h:413: Page* page(); I'm almost positive that exposing page() here isn't the way to go. Is there another, more common way people store active events for a Page? Surely Device Motion isn't the first with this problem? https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/page/Device... File Source/WebCore/page/DeviceController.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/page/Device... Source/WebCore/page/DeviceController.h:29: */ We can't just change the copyright header and this file's license if Android/Samsung touched it before. Best to just leave it. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/page/Device... Source/WebCore/page/DeviceController.h:47: // FIXME(timvolodine): remove this constructor and all references to DeviceClient nit: FIXMEs in WebKit code are unattributed. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/page/Device... Source/WebCore/page/DeviceController.h:64: DeviceController() : m_timer(this, &DeviceController::fireDeviceEvent) { } Do we have any static analysis tools which may assert that m_client is not being initialized properly? We should verify that with try-bots.. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/Pl... File Source/WebCore/platform/PlatformDeviceMotion.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/Pl... Source/WebCore/platform/PlatformDeviceMotion.h:34: #if ENABLE(DEVICE_ORIENTATION) Please don't use compile-time guards for this code. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/Pl... Source/WebCore/platform/PlatformDeviceMotion.h:42: DeviceMotionData* lastDeviceMotionData(); nit: latestDeviceMotionData https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... File Source/WebCore/platform/chromium/DeviceMotionChromium.cpp (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:32: #include "PlatformDeviceMotion.h" nit: empty line above the PlatformDeviceMotion.h include. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:34: #if ENABLE(DEVICE_ORIENTATION) Please don't use compile-time guards for this code. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:42: WebKit::WebDeviceMotionEventPump::shared().start(); We'd now end up with three singleton objects. Maybe it'd be better to keep DeviceMotionController/DeviceOrientationController as a supplement to Page (handling DOMWindow* registration), which can then register itself as a consumer of the pump? https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... File Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:27: #include <public/WebDeviceMotionData.h> nit: empty line above the public/WebDeviceMotionData.h include. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:36: m_hasAccelerationX(false), m_hasAccelerationY(false), m_hasAccelerationZ(false), Despite there not being any line length limits, I'd prefer to just list one initialization per line for readability. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:112: true, 100); It's not clear that we're hardcoding canProvideInterval and the interval (100) here, so it seems rather arbitrary. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:38: nit: two blank lines? https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:48: m_delaySeconds(delay_seconds), m_lastFireEventTimeMonotonic(0), m_stopped(true), nit: please have a single initializer per line. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:53: void WebDeviceMotionEventPump::fireEvent(Timer<WebDeviceMotionEventPump>* timer) The timer is not being used in this method, can we remove the argument? https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:60: WebDeviceMotionData data; Do we need a temporary variable for the reader? We do need a NULL check here in case it's not implemented. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:65: DeviceMotionController::shared().didChangeDeviceMotion(m_lastDeviceMotionData.get()); It's a bit odd that we update m_lastFireEventTimeMonotonic even if we don't have to fire the event. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:74: double scheduleDelay = std::max<double>(m_delaySeconds - (monotonicallyIncreasingTime() - m_lastFireEventTimeMonotonic), 0); nit: newline above this for readability. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:74: double scheduleDelay = std::max<double>(m_delaySeconds - (monotonicallyIncreasingTime() - m_lastFireEventTimeMonotonic), 0); This also assumes we'll fire at 10Hz for Device Motion. Since we don't have Chromium-side plumbing here, can we just increase this to the 50-60Hz we want? https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:78: WebCore::DeviceMotionData* WebDeviceMotionEventPump::lastDeviceMotionData() The pointer ownership situation is a bit unclear here. Should we return a const WebCore::DeviceMotionData* and clarify in this class (with a comment in the header) that the pump continues to own it? https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:85: // TODO(timvolodine): motion event should be fired only when values are different from the values nit: Blink style uses unattribute FIXMEs. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:92: WebDeviceMotionReader* reader = WebKit::Platform::current()->deviceMotionReader(); Needs a NULL check. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:100: WebDeviceMotionReader* reader = WebKit::Platform::current()->deviceMotionReader(); Needs a NULL check. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:46: // TODO(timvolodine): implement a superclass for this, because we will also use nit: Blink style uses unattributed FIXMEs. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:66: ~WebDeviceMotionEventPump() { } The order here is a bit odd. Could we define static methods first, then the constructor/destructor, then other methods, then member variables? https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:69: void fireEvent(Timer<WebDeviceMotionEventPump>*); What do we need a pointer to the timer for?
Peter, thanks for the comments, they should be fixed in this next patch I have just uploaded. https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... File Source/Platform/chromium/public/Platform.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/Platform.h:514: On 2013/04/09 17:29:53, Peter Beverloo wrote: > Please add a separate section for Device Orientation and Motion. This would also > benefit from some documentation. Done. https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionData.h:35: namespace WTF { template <typename T> class PassRefPtr; } On 2013/04/09 17:29:53, Peter Beverloo wrote: > Can you just include include wtf/PassRefPtr.h instead? This is similar to how the WebDeviceOrientation is done. This is also how the other classes in Platform/chromium/public are done i.e. WebData.h, WebDragData.h. The #include "wtf/PassRefPtr.h" does not seem to work because it is not in the search path. https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... File Source/Platform/chromium/public/WebDeviceMotionReader.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionReader.h:38: // FIXME(timvolodine) : abstract this into a common superclass, to be used with device orientation as well. On 2013/04/09 17:29:53, Peter Beverloo wrote: > nit: Blink style uses unattributed FIXMEs. Done. https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionReader.h:44: virtual void lastDeviceMotionData(WebDeviceMotionData&) = 0; On 2013/04/09 17:29:53, Peter Beverloo wrote: > latestDeviceMotionData? I've tried to keep the same naming similar to how it used to be in the clients. But latest sounds better indeed, so fixed this. https://codereview.chromium.org/13866007/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionReader.h:44: virtual void lastDeviceMotionData(WebDeviceMotionData&) = 0; On 2013/04/09 17:29:53, Peter Beverloo wrote: > Since this is an API, I'd prefer some more documentation on what exactly will > happen when someone calls startUpdating() or stopUpdating(), and what the > "latestDeviceMotionData" means (i.e. it doesn't update live). Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... File Source/WebCore/dom/DeviceMotionController.cpp (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.cpp:29: */ On 2013/04/09 17:29:53, Peter Beverloo wrote: > We can't just change the copyright header if Apple or Samsung added this file. > Best to leave it untouched. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.cpp:46: DeviceMotionController& DeviceMotionController::shared() On 2013/04/09 17:29:53, Peter Beverloo wrote: > While not that common in WebKit (yet), I'd prefer it if we can add a comment > above this method which says "// static". Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.cpp:59: return lastDeviceMotionData(); On 2013/04/09 17:29:53, Peter Beverloo wrote: > Where does lastDeviceMotionData come from? if I understand the question correctly: from the platform layer as implement in WebCore/platform/chromium/DeviceMotion.cpp https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.cpp:64: return DeviceMotionEvent::create(eventNames().devicemotionEvent, lastDeviceMotionData()); On 2013/04/09 17:29:53, Peter Beverloo wrote: > dito to line 59. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.cpp:81: startUpdating(); On 2013/04/09 17:29:53, Peter Beverloo wrote: > Unless we need to publicly expose startUpdating/stopUpdating from this class, we > may as well call to {start,stop}MonitoringDeviceMotion() directly here. we don't need start/stop to be public, I've kept them here in order to be able to easily generalize later and move the addDeviceEventListener to a super class. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... File Source/WebCore/dom/DeviceMotionController.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.h:29: */ On 2013/04/09 17:29:53, Peter Beverloo wrote: > We can't just change the copyright header if Samsung added this file. Best to > leave it untouched. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.h:50: bool isActiveAt(Page* page) { return m_activePages.contains(page); } On 2013/04/09 17:29:53, Peter Beverloo wrote: > This class will now have both isActive() and isActiveAt(), which seems a bit > ambiguous. That's how it was before as well. DeviceController implementing the isActive() and DeviceMotionController isActiveAt(page). Maybe should rename isActiveAt to isActiveAtPage(page) ? https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.h:53: virtual void stopUpdating(); On 2013/04/09 17:29:53, Peter Beverloo wrote: > Why are these declared as virtual? Also, do they have to be public? Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.h:55: // ------------------------------------------------- On 2013/04/09 17:29:53, Peter Beverloo wrote: > nit: the horizontal line without explanation is a bit odd. Remove it? Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.h:57: void addDeviceEventListener(DOMWindow*) OVERRIDE; On 2013/04/09 17:29:53, Peter Beverloo wrote: > Why are these declared OVERRIDE without being declared virtual? That kind of > defeats the purpose of the base class. Will we be able to generalize the > implementation to DeviceController again once Device Orientation also starts > using this design? right, yes the idea is to generalize the design once we switch the orientation as well. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.h:63: // DeviceClient* client() { return m_client; } On 2013/04/09 17:29:53, Peter Beverloo wrote: > No commented-out code please. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/dom/DeviceM... Source/WebCore/dom/DeviceMotionController.h:67: HashCountedSet<Page*> m_activePages; On 2013/04/09 17:29:53, Peter Beverloo wrote: > nit: could you add an empty line between the constructor and the member > variable? Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/page/DOMWin... File Source/WebCore/page/DOMWindow.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/page/DOMWin... Source/WebCore/page/DOMWindow.h:413: Page* page(); On 2013/04/09 17:29:53, Peter Beverloo wrote: > I'm almost positive that exposing page() here isn't the way to go. Is there > another, more common way people store active events for a Page? Surely Device > Motion isn't the first with this problem? ok, made the page() method private again. I switched back to the supplement to a page approach, with provideDeviceMotionTo(Page*), this is in line with the comment. Now the controller registers with the pump and is not a singleton anymore. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/page/Device... File Source/WebCore/page/DeviceController.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/page/Device... Source/WebCore/page/DeviceController.h:29: */ On 2013/04/09 17:29:53, Peter Beverloo wrote: > We can't just change the copyright header and this file's license if > Android/Samsung touched it before. Best to just leave it. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/page/Device... Source/WebCore/page/DeviceController.h:47: // FIXME(timvolodine): remove this constructor and all references to DeviceClient On 2013/04/09 17:29:53, Peter Beverloo wrote: > nit: FIXMEs in WebKit code are unattributed. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/page/Device... Source/WebCore/page/DeviceController.h:64: DeviceController() : m_timer(this, &DeviceController::fireDeviceEvent) { } On 2013/04/09 17:29:53, Peter Beverloo wrote: > Do we have any static analysis tools which may assert that m_client is not being > initialized properly? We should verify that with try-bots.. ok I've added init to null https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/Pl... File Source/WebCore/platform/PlatformDeviceMotion.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/Pl... Source/WebCore/platform/PlatformDeviceMotion.h:34: #if ENABLE(DEVICE_ORIENTATION) On 2013/04/09 17:29:53, Peter Beverloo wrote: > Please don't use compile-time guards for this code. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/Pl... Source/WebCore/platform/PlatformDeviceMotion.h:42: DeviceMotionData* lastDeviceMotionData(); On 2013/04/09 17:29:53, Peter Beverloo wrote: > nit: latestDeviceMotionData Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... File Source/WebCore/platform/chromium/DeviceMotionChromium.cpp (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:32: #include "PlatformDeviceMotion.h" On 2013/04/09 17:29:53, Peter Beverloo wrote: > nit: empty line above the PlatformDeviceMotion.h include. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:34: #if ENABLE(DEVICE_ORIENTATION) On 2013/04/09 17:29:53, Peter Beverloo wrote: > Please don't use compile-time guards for this code. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:42: WebKit::WebDeviceMotionEventPump::shared().start(); On 2013/04/09 17:29:53, Peter Beverloo wrote: > We'd now end up with three singleton objects. Maybe it'd be better to keep > DeviceMotionController/DeviceOrientationController as a supplement to Page > (handling DOMWindow* registration), which can then register itself as a consumer > of the pump? ok, switched back to the supplement to page approach. This also solves the problem with exposing the page() method as public in DOMWindow. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... File Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:27: #include <public/WebDeviceMotionData.h> On 2013/04/09 17:29:53, Peter Beverloo wrote: > nit: empty line above the public/WebDeviceMotionData.h include. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:36: m_hasAccelerationX(false), m_hasAccelerationY(false), m_hasAccelerationZ(false), On 2013/04/09 17:29:53, Peter Beverloo wrote: > Despite there not being any line length limits, I'd prefer to just list one > initialization per line for readability. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:112: true, 100); On 2013/04/09 17:29:53, Peter Beverloo wrote: > It's not clear that we're hardcoding canProvideInterval and the interval (100) > here, so it seems rather arbitrary. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:38: On 2013/04/09 17:29:53, Peter Beverloo wrote: > nit: two blank lines? Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:48: m_delaySeconds(delay_seconds), m_lastFireEventTimeMonotonic(0), m_stopped(true), On 2013/04/09 17:29:53, Peter Beverloo wrote: > nit: please have a single initializer per line. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:53: void WebDeviceMotionEventPump::fireEvent(Timer<WebDeviceMotionEventPump>* timer) On 2013/04/09 17:29:53, Peter Beverloo wrote: > The timer is not being used in this method, can we remove the argument? I've removed the name, but looks like it's not possible to remove the pointer as it is required by the Timer class, same construction is done elsewhere in the code e.g. DocumentLoader.cpp https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:60: WebDeviceMotionData data; On 2013/04/09 17:29:53, Peter Beverloo wrote: > Do we need a temporary variable for the reader? We do need a NULL check here in > case it's not implemented. added asserts. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:65: DeviceMotionController::shared().didChangeDeviceMotion(m_lastDeviceMotionData.get()); On 2013/04/09 17:29:53, Peter Beverloo wrote: > It's a bit odd that we update m_lastFireEventTimeMonotonic even if we don't have > to fire the event. the if (shouldFireEvent()) is meant as an optimization (e.g. if the values are equal), we need to update the m_lastFireEventTimeMonotonic anyway to keep properly firing https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:74: double scheduleDelay = std::max<double>(m_delaySeconds - (monotonicallyIncreasingTime() - m_lastFireEventTimeMonotonic), 0); On 2013/04/09 17:29:53, Peter Beverloo wrote: > nit: newline above this for readability. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:74: double scheduleDelay = std::max<double>(m_delaySeconds - (monotonicallyIncreasingTime() - m_lastFireEventTimeMonotonic), 0); On 2013/04/09 17:29:53, Peter Beverloo wrote: > This also assumes we'll fire at 10Hz for Device Motion. Since we don't have > Chromium-side plumbing here, can we just increase this to the 50-60Hz we want? we can change the delay value as we want, but for now I've kept it at 100ms. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:78: WebCore::DeviceMotionData* WebDeviceMotionEventPump::lastDeviceMotionData() On 2013/04/09 17:29:53, Peter Beverloo wrote: > The pointer ownership situation is a bit unclear here. Should we return a const > WebCore::DeviceMotionData* and clarify in this class (with a comment in the > header) that the pump continues to own it? ok, done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:85: // TODO(timvolodine): motion event should be fired only when values are different from the values On 2013/04/09 17:29:53, Peter Beverloo wrote: > nit: Blink style uses unattribute FIXMEs. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:92: WebDeviceMotionReader* reader = WebKit::Platform::current()->deviceMotionReader(); On 2013/04/09 17:29:53, Peter Beverloo wrote: > Needs a NULL check. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:100: WebDeviceMotionReader* reader = WebKit::Platform::current()->deviceMotionReader(); On 2013/04/09 17:29:53, Peter Beverloo wrote: > Needs a NULL check. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h (right): https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:46: // TODO(timvolodine): implement a superclass for this, because we will also use On 2013/04/09 17:29:53, Peter Beverloo wrote: > nit: Blink style uses unattributed FIXMEs. Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:66: ~WebDeviceMotionEventPump() { } On 2013/04/09 17:29:53, Peter Beverloo wrote: > The order here is a bit odd. Could we define static methods first, then the > constructor/destructor, then other methods, then member variables? Done. https://codereview.chromium.org/13866007/diff/2001/Source/WebCore/platform/ch... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:69: void fireEvent(Timer<WebDeviceMotionEventPump>*); On 2013/04/09 17:29:53, Peter Beverloo wrote: > What do we need a pointer to the timer for? this is stanard way of using the Timer class (i.e. it needs a function with pointer)
Eric, Peter, Can you spot any issues in this patch? Would appreciate your feedback. thanks, Tim
This feels like something which could move to modules/ I'm not particularly familiar with this code, but this looks sane to me. If peter isn't any more familiar than I i'm happy to take a second (closer) look. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.cpp (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:101: && !listenerVector[i]->document()->activeDOMObjectsAreSuspended() When are active dom objects stopped?
Thanks Eric, Peter said he'll have a further look, but if he is short on time I'll get back to you for help. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.cpp (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:101: && !listenerVector[i]->document()->activeDOMObjectsAreSuspended() On 2013/04/17 01:51:16, Eric Seidel (Google) wrote: > When are active dom objects stopped? What I can see from the code the three reasons to suspend active DOM objects are: JavaScriptDebuggerPaused, WillDeferLoading, DocumentWillBecomeInactive. WillDeferLoading seems to be used to avoid execution of JS code when a modal window is displayed. DocumentWillBecomeInactive appears to be used when caching a page.
Thanks Tim, this looks a lot better already! It's unfortunate that we need to provideDeviceMotionTo() call in WebViewImpl, maybe Eric knows whether there's a common way to lazily provide supplements? https://codereview.chromium.org/13866007/diff/15001/Source/Platform/chromium/... File Source/Platform/chromium/public/Platform.h (right): https://codereview.chromium.org/13866007/diff/15001/Source/Platform/chromium/... Source/Platform/chromium/public/Platform.h:517: // FIXME add WebDeviceOrientationReader* deviceOrientationReader() nit: this fixme doesn't add a lot, I'd remove it. https://codereview.chromium.org/13866007/diff/15001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionReader.h (right): https://codereview.chromium.org/13866007/diff/15001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionReader.h:45: // shared memory buffer, which is updated by the browser process. The API doesn't have to know about how the data gets in the buffer, it only cares about the fact that "the latest Device Motion data will be stored in the passed object." https://codereview.chromium.org/13866007/diff/15001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionReader.h:46: virtual void lastDeviceMotionData(WebDeviceMotionData&) = 0; In the previous patch set I suggested renaming this to "latestDeviceMotionData", which you agreed on, but you didn't change it. Is that intentional? https://codereview.chromium.org/13866007/diff/15001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionReader.h:48: // Sends a signal to the browser process to start updating the shared memory buffer. dito re: api knowledge. https://codereview.chromium.org/13866007/diff/15001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionReader.h:52: // memory buffer anymore. The buffer will still be updated if other readers are using it. dito. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.cpp (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:44: // this is a noop if the controller is not registered for updates. nit: "This is a no-op if the controller has not registered with the Device Motion pump"? https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:45: unRegisterForDeviceMotionUpdates(this); "Unregister" is one word, so no need to capitalize the "R". https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:94: void DeviceMotionController::dispatchDeviceEvent(PassRefPtr<Event> prpEvent) Please don't use acronyms and spell out the "prpEvent" name. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:120: listenerVector[i]->dispatchEvent(getLastEvent()); Since we have two paths for dispatching events, does it make sense to create a private dispatchEventToActiveDocuments method? https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.h (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:40: class DeviceMotionController : public RefCountedSupplement<Page, DeviceMotionController> { We don't inherit from DeviceController anymore. Is this temporary? You talk about it on line 56. This'd be a good thing to add a FIXME for. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:58: void dispatchDeviceEvent(PassRefPtr<Event>); Can we add the "const" modifier to the argument? It's not necessary, but it's a good way to indicate that we won't change the Event. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:77: void provideDeviceMotionTo(Page*); It's very unfortunate that we still need this. I suppose it's an artifact of using supplements to a Page.. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/P... File Source/WebCore/platform/PlatformDeviceMotion.h (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/P... Source/WebCore/platform/PlatformDeviceMotion.h:40: void unRegisterForDeviceMotionUpdates(DeviceMotionController*); As earlier said, please consider "unregister" as a single word, thus unregisterForDeviceMotionUpdates(). https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:27: #include <public/WebDeviceMotionData.h> micro nit: newline between lines 26 and 27. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:47: WebDeviceMotionEventPump::WebDeviceMotionEventPump(double delay_seconds) : nit: WebKit uses camelCase, not hacker_case (for delay_seconds). https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:64: reader->lastDeviceMotionData(data); hmm. Hypothetical situation: what if there is no latest device motion data? Should lastDeviceMotionData() maybe return a boolean to indicate whether there is anything at all? https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:95: // of the previously fired event. Is that so? In either case, I'd argue that this is more of a job for the controller to do, given that the pump just... pumps, and shouldn't have any further logic. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:43: using WebCore::Timer; All three of these forward declares also have #includes. Can we remove the includes? https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:58: // The returned pointer is owned by this class. If so, can we make the returned value const?
Supplement for what? abarth added a Supplement<T> class which I thought was lazy...
thanks Peter! https://codereview.chromium.org/13866007/diff/15001/Source/Platform/chromium/... File Source/Platform/chromium/public/Platform.h (right): https://codereview.chromium.org/13866007/diff/15001/Source/Platform/chromium/... Source/Platform/chromium/public/Platform.h:517: // FIXME add WebDeviceOrientationReader* deviceOrientationReader() On 2013/04/17 19:08:23, Peter Beverloo wrote: > nit: this fixme doesn't add a lot, I'd remove it. Done. https://codereview.chromium.org/13866007/diff/15001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionReader.h (right): https://codereview.chromium.org/13866007/diff/15001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionReader.h:45: // shared memory buffer, which is updated by the browser process. On 2013/04/17 19:08:23, Peter Beverloo wrote: > The API doesn't have to know about how the data gets in the buffer, it only > cares about the fact that "the latest Device Motion data will be stored in the > passed object." Done. https://codereview.chromium.org/13866007/diff/15001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionReader.h:46: virtual void lastDeviceMotionData(WebDeviceMotionData&) = 0; On 2013/04/17 19:08:23, Peter Beverloo wrote: > In the previous patch set I suggested renaming this to "latestDeviceMotionData", > which you agreed on, but you didn't change it. Is that intentional? no that's a bug, fixed. https://codereview.chromium.org/13866007/diff/15001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionReader.h:48: // Sends a signal to the browser process to start updating the shared memory buffer. On 2013/04/17 19:08:23, Peter Beverloo wrote: > dito re: api knowledge. Done. https://codereview.chromium.org/13866007/diff/15001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionReader.h:52: // memory buffer anymore. The buffer will still be updated if other readers are using it. On 2013/04/17 19:08:23, Peter Beverloo wrote: > dito. Done. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.cpp (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:44: // this is a noop if the controller is not registered for updates. On 2013/04/17 19:08:23, Peter Beverloo wrote: > nit: "This is a no-op if the controller has not registered with the Device > Motion pump"? Done. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:45: unRegisterForDeviceMotionUpdates(this); On 2013/04/17 19:08:23, Peter Beverloo wrote: > "Unregister" is one word, so no need to capitalize the "R". Done. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:94: void DeviceMotionController::dispatchDeviceEvent(PassRefPtr<Event> prpEvent) On 2013/04/17 19:08:23, Peter Beverloo wrote: > Please don't use acronyms and spell out the "prpEvent" name. I believe this is a guideline from WebKit on how to prefix the PassRefPtr parameters. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:120: listenerVector[i]->dispatchEvent(getLastEvent()); On 2013/04/17 19:08:23, Peter Beverloo wrote: > Since we have two paths for dispatching events, does it make sense to create a > private dispatchEventToActiveDocuments method? Done. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.h (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:40: class DeviceMotionController : public RefCountedSupplement<Page, DeviceMotionController> { On 2013/04/17 19:08:23, Peter Beverloo wrote: > We don't inherit from DeviceController anymore. Is this temporary? You talk > about it on line 56. This'd be a good thing to add a FIXME for. Done. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:58: void dispatchDeviceEvent(PassRefPtr<Event>); On 2013/04/17 19:08:23, Peter Beverloo wrote: > Can we add the "const" modifier to the argument? It's not necessary, but it's a > good way to indicate that we won't change the Event. Done. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:77: void provideDeviceMotionTo(Page*); On 2013/04/17 19:08:23, Peter Beverloo wrote: > It's very unfortunate that we still need this. I suppose it's an artifact of > using supplements to a Page.. yes, agree this is not very clean. But this is how it used to be and how the supplements appear to be done. If we get rid of supplements (and use singletons for example as in the first patch) we would probably need to change the interface of DOMWindow or add some code to make Page accessible from the controller. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/P... File Source/WebCore/platform/PlatformDeviceMotion.h (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/P... Source/WebCore/platform/PlatformDeviceMotion.h:40: void unRegisterForDeviceMotionUpdates(DeviceMotionController*); On 2013/04/17 19:08:23, Peter Beverloo wrote: > As earlier said, please consider "unregister" as a single word, thus > unregisterForDeviceMotionUpdates(). Done. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:27: #include <public/WebDeviceMotionData.h> On 2013/04/17 19:08:23, Peter Beverloo wrote: > micro nit: newline between lines 26 and 27. hmm, unsure why this is needed. Other files in this directory seem to do without the extra line (e.g. WebData.cpp, WebAudioBus.cpp, WebHTTPBody.cpp, WebSpeechSynthesisVoice.cpp, ...) https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:47: WebDeviceMotionEventPump::WebDeviceMotionEventPump(double delay_seconds) : On 2013/04/17 19:08:23, Peter Beverloo wrote: > nit: WebKit uses camelCase, not hacker_case (for delay_seconds). Done. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:64: reader->lastDeviceMotionData(data); On 2013/04/17 19:08:23, Peter Beverloo wrote: > hmm. Hypothetical situation: what if there is no latest device motion data? > Should lastDeviceMotionData() maybe return a boolean to indicate whether there > is anything at all? Done. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:95: // of the previously fired event. On 2013/04/17 19:08:23, Peter Beverloo wrote: > Is that so? In either case, I'd argue that this is more of a job for the > controller to do, given that the pump just... pumps, and shouldn't have any > further logic. hmm, I was looking at the semantics for orientation, where the event is only fired when sufficiently different from the previous one. The main reason to have the shouldFireEvent() here is because the pump owns the lastDeviceMotionData, hence in order to compare this would be the best place to do so. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:43: using WebCore::Timer; On 2013/04/17 19:08:23, Peter Beverloo wrote: > All three of these forward declares also have #includes. Can we remove the > includes? cannot remove the DeviceMotionController.h and DeviceMotionData.h includes because they are needed for RefPtr. moved <wtf/CurrentTime.h> though. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:58: // The returned pointer is owned by this class. On 2013/04/17 19:08:23, Peter Beverloo wrote: > If so, can we make the returned value const? I tried doing this, but this change in signature would lead to some significant refactoring, as the signatures of DeviceMotion.cpp, PlatformDeviceMotion.h, DeviceMotionEvent.{h,cpp} and possibly others would need to change in multiple places. Don't think this is worth the trouble, also taking into account that it is non-const throughout WebCore.
Eric: While this patch takes the Source/Platform/ approach, the DeviceMotionController is defined as a supplement and, as such, needs to have a call to provideDeviceMotionTo() in the WebViewImpl. It'd be nice to get rid of that somehow, but I don't know how to. Also, Eric, mind doing a formal review? Thank you Tim! It's looking a lot better. The following comments are mostly textual nits. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.cpp (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:94: void DeviceMotionController::dispatchDeviceEvent(PassRefPtr<Event> prpEvent) On 2013/04/18 12:37:38, timvolodine wrote: > On 2013/04/17 19:08:23, Peter Beverloo wrote: > > Please don't use acronyms and spell out the "prpEvent" name. > I believe this is a guideline from WebKit on how to prefix the PassRefPtr > parameters. Since you store it in a normal RefPtr on line 96 it doesn't matter anyway. Feel free to divert. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:95: // of the previously fired event. On 2013/04/18 12:37:38, timvolodine wrote: > On 2013/04/17 19:08:23, Peter Beverloo wrote: > > Is that so? In either case, I'd argue that this is more of a job for the > > controller to do, given that the pump just... pumps, and shouldn't have any > > further logic. > > hmm, I was looking at the semantics for orientation, where the event is only > fired when sufficiently different from the previous one. The main reason to have > the shouldFireEvent() here is because the pump owns the lastDeviceMotionData, > hence in order to compare this would be the best place to do so. Where does the Device Orientation specification say that only firing the event if it has changed enough should also apply to the devicemotion event? Do other browsers do this? I'm still not sure about the layering. It feels wrong for a pump to have logic a controller should care about. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:58: // The returned pointer is owned by this class. On 2013/04/18 12:37:38, timvolodine wrote: > On 2013/04/17 19:08:23, Peter Beverloo wrote: > > If so, can we make the returned value const? > > I tried doing this, but this change in signature would lead to some significant > refactoring, as the signatures of DeviceMotion.cpp, PlatformDeviceMotion.h, > DeviceMotionEvent.{h,cpp} and possibly others would need to change in multiple > places. Don't think this is worth the trouble, also taking into account that it > is non-const throughout WebCore. Can you file a bug on it and add a FIXME? I think it is something we'll want to address eventually. https://codereview.chromium.org/13866007/diff/30001/Source/Platform/chromium/... File Source/Platform/chromium/public/Platform.h (right): https://codereview.chromium.org/13866007/diff/30001/Source/Platform/chromium/... Source/Platform/chromium/public/Platform.h:515: // Device Motion / Orientation ----------------------------------------- micro ocd nit: this line has one more dash than the line above here. https://codereview.chromium.org/13866007/diff/30001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionReader.h (right): https://codereview.chromium.org/13866007/diff/30001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionReader.h:38: // FIXME abstract this into a common superclass, to be used with device orientation as well. nit: Colon after the FIXME. https://codereview.chromium.org/13866007/diff/30001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionReader.h:51: // Signifies that this reader will not be used to fetch data anymore, so it can stop providing motion data. s/Signifies/Indicates/, as it's a fierce signal. https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.cpp (right): https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:44: // this is a no-op if the controller has not registered with the Device Motion pump. nit: spelling (capital T). There also seems to be quite some inconsistencies in your comments in how you spell "device motion", "Device Motion", "devicemotion" et al. https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.h (right): https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:40: // FIXME This class doesn't inherit from DeviceController anymore, which is a temporary nit: Colon after the FIXME. https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:53: virtual PassRefPtr<Event> getLastEvent() OVERRIDE; Now that we're no longer inhering from DeviceController, these methods no longer override anything. As such, you should remove the OVERRIDE annotations. https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/page/DOMWi... File Source/WebCore/page/DOMWindow.h (right): https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/page/DOMWi... Source/WebCore/page/DOMWindow.h:416: Page* page(); I don't understand why this shows up as a diff? Did anything land making it public? https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp (right): https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:71: for (size_t i = 0;i < listenerVector.size(); ++i) { nit: don't need brackets around this one-line statement. https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h (right): https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:46: // FIXME implement a superclass for this, because we will also use nit: Colon after the FIXME. https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:50: static const double kPumpDelayMilliSeconds = 100; nit: milliseconds is a single word, so kPumpDelayMilliseconds.
https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.cpp (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:94: void DeviceMotionController::dispatchDeviceEvent(PassRefPtr<Event> prpEvent) On 2013/04/18 13:56:06, Peter Beverloo wrote: > On 2013/04/18 12:37:38, timvolodine wrote: > > On 2013/04/17 19:08:23, Peter Beverloo wrote: > > > Please don't use acronyms and spell out the "prpEvent" name. > > I believe this is a guideline from WebKit on how to prefix the PassRefPtr > > parameters. > > Since you store it in a normal RefPtr on line 96 it doesn't matter anyway. Feel > free to divert. Done. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:95: // of the previously fired event. On 2013/04/18 13:56:06, Peter Beverloo wrote: > On 2013/04/18 12:37:38, timvolodine wrote: > > On 2013/04/17 19:08:23, Peter Beverloo wrote: > > > Is that so? In either case, I'd argue that this is more of a job for the > > > controller to do, given that the pump just... pumps, and shouldn't have any > > > further logic. > > > > hmm, I was looking at the semantics for orientation, where the event is only > > fired when sufficiently different from the previous one. The main reason to > have > > the shouldFireEvent() here is because the pump owns the lastDeviceMotionData, > > hence in order to compare this would be the best place to do so. > > Where does the Device Orientation specification say that only firing the event > if it has changed enough should also apply to the devicemotion event? Do other > browsers do this? > > I'm still not sure about the layering. It feels wrong for a pump to have logic a > controller should care about. ok the spec does not say anything about firing the motion event, probably it expects to do so with the delay specified in the interval property.. Anyway this will be needed for device orientation, and the pump is probably the only reasonable place to do so. The controller does not own the data and is not singleton anymore, I see it playing more of a dispatcher role. https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h (right): https://codereview.chromium.org/13866007/diff/15001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:58: // The returned pointer is owned by this class. On 2013/04/18 13:56:06, Peter Beverloo wrote: > On 2013/04/18 12:37:38, timvolodine wrote: > > On 2013/04/17 19:08:23, Peter Beverloo wrote: > > > If so, can we make the returned value const? > > > > I tried doing this, but this change in signature would lead to some > significant > > refactoring, as the signatures of DeviceMotion.cpp, PlatformDeviceMotion.h, > > DeviceMotionEvent.{h,cpp} and possibly others would need to change in multiple > > places. Don't think this is worth the trouble, also taking into account that > it > > is non-const throughout WebCore. > > Can you file a bug on it and add a FIXME? I think it is something we'll want to > address eventually. Done. https://codereview.chromium.org/13866007/diff/30001/Source/Platform/chromium/... File Source/Platform/chromium/public/Platform.h (right): https://codereview.chromium.org/13866007/diff/30001/Source/Platform/chromium/... Source/Platform/chromium/public/Platform.h:515: // Device Motion / Orientation ----------------------------------------- On 2013/04/18 13:56:06, Peter Beverloo wrote: > micro ocd nit: this line has one more dash than the line above here. Done. https://codereview.chromium.org/13866007/diff/30001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionReader.h (right): https://codereview.chromium.org/13866007/diff/30001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionReader.h:38: // FIXME abstract this into a common superclass, to be used with device orientation as well. On 2013/04/18 13:56:06, Peter Beverloo wrote: > nit: Colon after the FIXME. Done. https://codereview.chromium.org/13866007/diff/30001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionReader.h:51: // Signifies that this reader will not be used to fetch data anymore, so it can stop providing motion data. On 2013/04/18 13:56:06, Peter Beverloo wrote: > s/Signifies/Indicates/, as it's a fierce signal. Done. https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.cpp (right): https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:44: // this is a no-op if the controller has not registered with the Device Motion pump. On 2013/04/18 13:56:06, Peter Beverloo wrote: > nit: spelling (capital T). There also seems to be quite some inconsistencies in > your comments in how you spell "device motion", "Device Motion", "devicemotion" > et al. Done. https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.h (right): https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:40: // FIXME This class doesn't inherit from DeviceController anymore, which is a temporary On 2013/04/18 13:56:06, Peter Beverloo wrote: > nit: Colon after the FIXME. Done. https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:53: virtual PassRefPtr<Event> getLastEvent() OVERRIDE; On 2013/04/18 13:56:06, Peter Beverloo wrote: > Now that we're no longer inhering from DeviceController, these methods no longer > override anything. As such, you should remove the OVERRIDE annotations. Done. https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/page/DOMWi... File Source/WebCore/page/DOMWindow.h (right): https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/page/DOMWi... Source/WebCore/page/DOMWindow.h:416: Page* page(); On 2013/04/18 13:56:06, Peter Beverloo wrote: > I don't understand why this shows up as a diff? Did anything land making it > public? no nothing landed, but this apparently diffs against an older patch where I did change the page() method to be public. (I am kind of confused myself as to how Rietveld does the diff..) https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp (right): https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:71: for (size_t i = 0;i < listenerVector.size(); ++i) { On 2013/04/18 13:56:06, Peter Beverloo wrote: > nit: don't need brackets around this one-line statement. Done. https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h (right): https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:46: // FIXME implement a superclass for this, because we will also use On 2013/04/18 13:56:06, Peter Beverloo wrote: > nit: Colon after the FIXME. Done. https://codereview.chromium.org/13866007/diff/30001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:50: static const double kPumpDelayMilliSeconds = 100; On 2013/04/18 13:56:06, Peter Beverloo wrote: > nit: milliseconds is a single word, so kPumpDelayMilliseconds. Done.
https://codereview.chromium.org/13866007/diff/37001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/13866007/diff/37001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:75: bool m_hasAccelerationX; If this object is at all size-sensitive, we shoudl make thes bitfields. If it's not, then ignore me. :) https://codereview.chromium.org/13866007/diff/37001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionReader.h (right): https://codereview.chromium.org/13866007/diff/37001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionReader.h:38: // FIXME: abstract this into a common superclass, to be used with device orientation as well. I'm not particularly familiar with Platform. abarth/mpilgrim know it much better than I. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.cpp (left): https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:2: * Copyright (C) 2013 Google Inc. All rights reserved. Do you mean to be deleting Google copyright? https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.cpp (right): https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:8: * * Redistributions of source code must retain the above copyright Why are you changing this license block? I would leave them alone. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:43: DeviceMotionController::~DeviceMotionController() { { goes on a new line. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:97: copyToVector(m_listeners, listenerVector); I presume this clears the list? https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:106: m_timer.stop(); Is this a repeating timer? Normally fireOnce just fires it once. :) https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:118: if (listeners[i]->document() Is there no Document::isActive() helper for these checks? https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:120: && !listeners[i]->document()->activeDOMObjectsAreStopped()) I'm confused as to when active DOM objects are ever suspended. abarth woudl know. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.h (right): https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:75: HashCountedSet<RefPtr<DOMWindow> > m_listeners; Confused why this would want to keep a ref-counted list of DOMWindows. Doens't DOMWindow control its presence on this list? https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/page/Devic... File Source/WebCore/page/DeviceController.h (right): https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/page/Devic... Source/WebCore/page/DeviceController.h:3: * Copyright (C) 2012 Samsung Electronics. All rights reserved. Why more license changes here? https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:105: return DeviceMotionData::create( Ouch. So many mallocs. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp (right): https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:59: if (m_stopped) This shoudl be an ASSERT. When you cancel a timer, it can't fire. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:81: if (m_pumpTimer.isActive()) This can be an ASSERT I believe. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:129: m_stopped = true; You should just cancel the timer here. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h (right): https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:50: static const double kPumpDelayMilliseconds = 100; Is this the delay between events? This appears to be just a default which create() uses? https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:58: // FIXME: make the return value const, see crbug/233174. Nit: Comments should start with a capital, end with a period. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:63: ~WebDeviceMotionEventPump() { } Are you sure you want this inline (and thus declared in every compilation unit which uses it?)
Eric, thanks for the quick reaction! https://codereview.chromium.org/13866007/diff/37001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/13866007/diff/37001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:75: bool m_hasAccelerationX; On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > If this object is at all size-sensitive, we shoudl make thes bitfields. If it's > not, then ignore me. :) this is definitely size-sensitive as it is used in the shared memory buffer (not in this patch). I've modified the code to use bitfield and added a compile-time check on size, however not sure how portable this will be.. bitfields seem to have a bad reputation in that respect. https://codereview.chromium.org/13866007/diff/37001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionReader.h (right): https://codereview.chromium.org/13866007/diff/37001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionReader.h:38: // FIXME: abstract this into a common superclass, to be used with device orientation as well. On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > I'm not particularly familiar with Platform. abarth/mpilgrim know it much > better than I. ok will try to ask them. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.cpp (left): https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:2: * Copyright (C) 2013 Google Inc. All rights reserved. On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > Do you mean to be deleting Google copyright? I've reverted this to the original copyright. This is the original copyright. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.cpp (right): https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:8: * * Redistributions of source code must retain the above copyright On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > Why are you changing this license block? I would leave them alone. This is the original copyright. Rietveld seems to diff against previous patch. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:43: DeviceMotionController::~DeviceMotionController() { On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > { goes on a new line. Done. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:97: copyToVector(m_listeners, listenerVector); On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > I presume this clears the list? what list? this is how it was done originally in DeviceController. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:106: m_timer.stop(); On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > Is this a repeating timer? Normally fireOnce just fires it once. :) hmm, not sure why, but this was in the original DeviceController code, so I am slightly hesitant to remove it. but can look deeper into it. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:118: if (listeners[i]->document() On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > Is there no Document::isActive() helper for these checks? No, looks like there is no such helper method. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.cpp:120: && !listeners[i]->document()->activeDOMObjectsAreStopped()) On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > I'm confused as to when active DOM objects are ever suspended. abarth woudl > know. As I understand this it's either javascript debugger paused, caching and modal dialogs. Will ask abarth about this. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.h (right): https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:75: HashCountedSet<RefPtr<DOMWindow> > m_listeners; On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > Confused why this would want to keep a ref-counted list of DOMWindows. Doens't > DOMWindow control its presence on this list? This is how it is done in the original DeviceController. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/page/Devic... File Source/WebCore/page/DeviceController.h (right): https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/page/Devic... Source/WebCore/page/DeviceController.h:3: * Copyright (C) 2012 Samsung Electronics. All rights reserved. On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > Why more license changes here? This is the original header. Rietveld diffs wrong. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:105: return DeviceMotionData::create( On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > Ouch. So many mallocs. hmm, don't see another way of doing this without severely refactoring DeviceMotionData (and all dependencies) https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp (right): https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:59: if (m_stopped) On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > This shoudl be an ASSERT. When you cancel a timer, it can't fire. Done. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:81: if (m_pumpTimer.isActive()) On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > This can be an ASSERT I believe. Done. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.cpp:129: m_stopped = true; On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > You should just cancel the timer here. Done. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h (right): https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:50: static const double kPumpDelayMilliseconds = 100; On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > Is this the delay between events? This appears to be just a default which > create() uses? yes. It is also used by WebDeviceMotionData to set the interval field in DeviceMotionData. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:58: // FIXME: make the return value const, see crbug/233174. On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > Nit: Comments should start with a capital, end with a period. Done. https://codereview.chromium.org/13866007/diff/37001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/support/WebDeviceMotionEventPump.h:63: ~WebDeviceMotionEventPump() { } On 2013/04/23 15:21:24, Eric Seidel (Google) wrote: > Are you sure you want this inline (and thus declared in every compilation unit > which uses it?) Done.
This CL has lots of problems. The existing device controller code is very bad and should likely be deleted and re-written completely. You shouldn't ever hold a RefPtr to DOMWindow. Any time you're doing that, your code is wrong. This API should be implemented as a module and should not require any integration with Core. I'm not sure how to start helping you with this CL, but I'd recommend starting fresh with a new design and eventually try to delete all the existing device controller code. https://codereview.chromium.org/13866007/diff/46001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/13866007/diff/46001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:41: #pragma pack(push, 1) Why is this needed? https://codereview.chromium.org/13866007/diff/46001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:91: COMPILE_ASSERT(sizeof(WebDeviceMotionData) == (9*8 + 2), WebDeviceMotionData_has_wrong_size); Where does this magic number come from? https://codereview.chromium.org/13866007/diff/46001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.h (right): https://codereview.chromium.org/13866007/diff/46001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:65: virtual void removeAllDeviceEventListeners(DOMWindow*); Are there subclasses of this object? Do these still need to be virtual? https://codereview.chromium.org/13866007/diff/46001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:76: HashCountedSet<RefPtr<DOMWindow> > m_lastEventListeners; This is a bad pattern. We shouldn't ref the DOMWindow from this object. https://codereview.chromium.org/13866007/diff/46001/Source/WebCore/page/Devic... File Source/WebCore/page/DeviceController.h (right): https://codereview.chromium.org/13866007/diff/46001/Source/WebCore/page/Devic... Source/WebCore/page/DeviceController.h:2: * Copyright 2010, The Android Open Source Project Why are you contributing under this copyright? Usually we contribute under the Google, Inc copyright. https://codereview.chromium.org/13866007/diff/46001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/DeviceMotion.cpp (right): https://codereview.chromium.org/13866007/diff/46001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/DeviceMotion.cpp:40: WebKit::WebDeviceMotionEventPump::shared().addListener(controller); You shouldn't need WebDeviceMotionEventPump.
Adam, thanks for the comments. I'll create a new patch and move the relevant classes to the WebCore/Modules/device_orientation directory. As we are implementing a shared-memory approach we need a "pump" on the WebKit side to actually read from shared mem and fire the events. For more details there is a design doc: goto/device-motion-design. There is also an email thread on chrome-eng-review@google.com with the title "Device Motion review request" where Darin approves of the new Platform and shared memory approach. https://codereview.chromium.org/13866007/diff/46001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/13866007/diff/46001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:41: #pragma pack(push, 1) On 2013/04/23 21:36:26, abarth wrote: > Why is this needed? Here I am using a similar approach as the Gamepad API. We basically want the shared memory buffer to be as small as possible. https://codereview.chromium.org/13866007/diff/46001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:91: COMPILE_ASSERT(sizeof(WebDeviceMotionData) == (9*8 + 2), WebDeviceMotionData_has_wrong_size); On 2013/04/23 21:36:26, abarth wrote: > Where does this magic number come from? 8 doubles + 2 bytes for the bit-packed booleans. https://codereview.chromium.org/13866007/diff/46001/Source/WebCore/dom/Device... File Source/WebCore/dom/DeviceMotionController.h (right): https://codereview.chromium.org/13866007/diff/46001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:65: virtual void removeAllDeviceEventListeners(DOMWindow*); On 2013/04/23 21:36:26, abarth wrote: > Are there subclasses of this object? Do these still need to be virtual? Done. https://codereview.chromium.org/13866007/diff/46001/Source/WebCore/dom/Device... Source/WebCore/dom/DeviceMotionController.h:76: HashCountedSet<RefPtr<DOMWindow> > m_lastEventListeners; On 2013/04/23 21:36:26, abarth wrote: > This is a bad pattern. We shouldn't ref the DOMWindow from this object. Will keep the RefPtr for now, since apparently there is no easy way to ensure we are not dispatching to deleted window pointers. https://codereview.chromium.org/13866007/diff/46001/Source/WebCore/page/Devic... File Source/WebCore/page/DeviceController.h (right): https://codereview.chromium.org/13866007/diff/46001/Source/WebCore/page/Devic... Source/WebCore/page/DeviceController.h:2: * Copyright 2010, The Android Open Source Project On 2013/04/23 21:36:26, abarth wrote: > Why are you contributing under this copyright? Usually we contribute under the > Google, Inc copyright. This is an old class originally written under this original copyright. The diff of Rietveld is confusing, but this is the original header. https://codereview.chromium.org/13866007/diff/46001/Source/WebCore/platform/c... File Source/WebCore/platform/chromium/DeviceMotion.cpp (right): https://codereview.chromium.org/13866007/diff/46001/Source/WebCore/platform/c... Source/WebCore/platform/chromium/DeviceMotion.cpp:40: WebKit::WebDeviceMotionEventPump::shared().addListener(controller); On 2013/04/23 21:36:26, abarth wrote: > You shouldn't need WebDeviceMotionEventPump. The pump is needed because we need something to read from the shared memory buffer (shared between the renderer and the browser).
On 2013/04/24 12:36:02, timvolodine wrote: > As we are implementing a shared-memory approach we need a "pump" on the WebKit > side to actually read from shared mem and fire the events. Can we put the pump on the Chromium side? It seems like an implementation detail of the multiprocess implementation of this feature that should not bleed into Blink.
I'm also very skeptical of holding RefPtr<DOMWindow>. That doesn't make any sense from an architectural point of view. I haven't studied what you're trying to do in detail, but it seems much more likely that you want to subclass ContextDestructionObserver (or potentially hold a WeakPtr<Document>).
On 2013/04/24 15:18:50, abarth wrote: > On 2013/04/24 12:36:02, timvolodine wrote: > > As we are implementing a shared-memory approach we need a "pump" on the WebKit > > side to actually read from shared mem and fire the events. > > Can we put the pump on the Chromium side? It seems like an implementation > detail of the multiprocess implementation of this feature that should not bleed > into Blink. It needs to be in the renderer process, so currently in platform/chromium/support/.. I guess it would be possible to move the pump further into the Source/Platform:: layer, i.e. the platform will instead of returning a WebDeviceMotionReader return a pump, which will then live in content/renderer/..
On 2013/04/24 15:29:42, timvolodine wrote: > It needs to be in the renderer process, so currently in > platform/chromium/support/.. > I guess it would be possible to move the pump further into the Source/Platform:: > layer, i.e. the platform will instead of returning a WebDeviceMotionReader > return a pump, which will then live in content/renderer/.. Having it in the render process is fine. The issue is having it inside Blink. Blink shouldn't know that a pump exists. It should just subscribe and unsubscribe to these events. It's up to the embedder to decide how/when to generate them.
On 2013/04/24 16:28:46, abarth wrote: > On 2013/04/24 15:29:42, timvolodine wrote: > > It needs to be in the renderer process, so currently in > > platform/chromium/support/.. > > I guess it would be possible to move the pump further into the > Source/Platform:: > > layer, i.e. the platform will instead of returning a WebDeviceMotionReader > > return a pump, which will then live in content/renderer/.. > > Having it in the render process is fine. The issue is having it inside Blink. > Blink shouldn't know that a pump exists. It should just subscribe and > unsubscribe to these events. It's up to the embedder to decide how/when to > generate them. If the pump is implemented in the content layer it would need a timer. Do you know if there is an equivalent of the WebKit Timer class (which apparently does not use extra threads) in content? Otherwise we would need to create an extra thread and we have quite a few of them already..
On 2013/04/24 17:49:01, timvolodine wrote: > If the pump is implemented in the content layer it would need a timer. Do you > know if there is an equivalent of the WebKit Timer class (which apparently does > not use extra threads) in content? Otherwise we would need to create an extra > thread and we have quite a few of them already.. You don't need any threads. You can just use PostTask or whatever other mechanism you like in Content to get scheduled periodically.
this issue is obsolete and has been resolved/landed in https://chromiumcodereview.appspot.com/14460010/. closing. |
