Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14460010:
Implement the Blink part of the Device Motion API. (Closed)
|
Created:
7 years, 7 months ago by timvolodine Modified:
7 years, 6 months ago CC:
blink-reviews, jamesr, jeez, Miguel Garcia, dcheng Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionImplement the Blink part of the Device Motion API.
The implementation uses the Platform layer approach with push semantics, i.e. Blink registers a listener to receive updates when new motion data is available. This patch contains changes to both the device_orientation module and the WebKit plumbing.
BUG=135804
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151544
Patch Set 1 #Patch Set 2 : fixed similarity diff #
Total comments: 44
Patch Set 3 : fixed comments and removed reduntant core/platform layer classes #
Total comments: 14
Patch Set 4 : fixed Adam's comments #
Total comments: 32
Patch Set 5 : fixed comments #
Total comments: 8
Patch Set 6 : fixed re-entrancy issue #
Total comments: 29
Patch Set 7 : fixed Peter's comments #Patch Set 8 : added tests #Patch Set 9 : attempt rebase #Patch Set 10 : rebased + fixed include sorting #Patch Set 11 : fixed and extended test #Patch Set 12 : fixed add-listener-from-callback-expected.txt #
Total comments: 4
Patch Set 13 : added testing api #Patch Set 14 : added MockWebDeviceMotionHandler and methods to set in in the platform #
Total comments: 6
Patch Set 15 : made WebDeviceMotionData more "POD-ish" #
Total comments: 8
Patch Set 16 : fixed Darin's comments #
Total comments: 2
Patch Set 17 : removed spurious forward declare of provider in Platform.h #Patch Set 18 : got rid of WebDeviceMotionProvider #
Total comments: 2
Patch Set 19 : moved tests to be treated in a separate CL #
Total comments: 14
Patch Set 20 : processed comments #Patch Set 21 : similarity=90 #
Total comments: 2
Patch Set 22 : shared() -> instance() #
Total comments: 8
Patch Set 23 : fixed Adam's comments #Patch Set 24 : added a comment wrt memset in WebDeviceMotionData.cpp #Patch Set 25 : rebased #Patch Set 26 : include <string.h> for memset to compile #Patch Set 27 : added platform files to Platform.gypi #Patch Set 28 : removed files incorrectly listed in WebKit.gyp #Messages
Total messages: 72 (0 generated)
Reimplementation of the Blink part for Device Motion without using the 'pump'. The approach uses push semantics wrt Blink, so it only has to register a listener through the Platform layer to receive updates on new motion data.
I'm a bit worried about all the code that has to be added to core/platform/, which kind of defeats the point in having a module. In the WebKit world we had to do this because other ports might may want to vary on their implementation, but this is not the case anymore. I'm not really savvy about the plans for core/platform/ in the Blink world, however. In the specific case of the WebDeviceMotionDispatcher, this feels like something that can perfectly live in your module (implementing the Listener from the platform). It also looks like you haven't changed anything yet in regards to keeping RefPtr<>s on DOMWindow. A few further points below. https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionData.h:41: #pragma pack(push, 1) Is this class really that important from a memory pressure point of view? How many copies do you expect there to be? You save 8 bytes (7 + padding) by doing this. Nonetheless, it's a minor win.. https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... File Source/Platform/chromium/public/WebDeviceMotionHandler.h (right): https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionHandler.h:39: nit: no new line. https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionHandler.h:42: // Starts updating the supplied listener with device motion data at regular intervals. Will start distributing device motion data to the supplied listener. https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... File Source/Platform/chromium/public/WebDeviceMotionListener.h (right): https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionListener.h:39: nit: no new line. https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionListener.h:43: // the data is passed in the argument. "the data is passed in the argument" doesn't really add any value, since it's passed by reference that's obvious. https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionListener.h:47: } // namespace WebCore namespace WebKit https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/Platf... File Source/core/platform/PlatformDeviceMotion.h (right): https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/Platf... Source/core/platform/PlatformDeviceMotion.h:34: #include <wtf/PassRefPtr.h> PassRefPtr isn't used in this header. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/Platf... Source/core/platform/PlatformDeviceMotion.h:42: void registerForDeviceMotionUpdates(DeviceMotionController*); I wonder if it'd be acceptable now to just include WebDeviceMotionDispatcher.h and call that directly? That'd severely limit the amount of code you need to add to core/. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/Platf... Source/core/platform/PlatformDeviceMotion.h:46: } nit: // namespace WebCore https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... File Source/core/platform/chromium/support/WebDeviceMotionDispatcher.cpp (right): https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/support/WebDeviceMotionDispatcher.cpp:79: handler->stopListeningToDeviceMotion(); Shouldn't you pass the listener object here as well? https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... File Source/core/platform/chromium/support/WebDeviceMotionDispatcher.h (right): https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/support/WebDeviceMotionDispatcher.h:47: // This class listens to Device Motion data and dispatches it to all nit: s/Device Motion data/device motion data/. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/support/WebDeviceMotionDispatcher.h:49: nit: no newline. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/support/WebDeviceMotionDispatcher.h:57: // The returned pointer is owned by this class "Note that the returned object is owned by this class." https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/support/WebDeviceMotionDispatcher.h:58: // FIXME: make the return value const, see crbug/233174. Please use a full URL instead of crbug/000. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/support/WebDeviceMotionDispatcher.h:62: // which is passed in the argument. dito to my comment in WebDeviceMotionListener https://codereview.chromium.org/14460010/diff/2001/Source/modules/device_orie... File Source/modules/device_orientation/DeviceMotionController.cpp (right): https://codereview.chromium.org/14460010/diff/2001/Source/modules/device_orie... Source/modules/device_orientation/DeviceMotionController.cpp:45: // This is a no-op if the controller has not registered with the device motion pump. There no longer is a pump. https://codereview.chromium.org/14460010/diff/2001/Source/modules/device_orie... File Source/modules/device_orientation/DeviceMotionController.h (right): https://codereview.chromium.org/14460010/diff/2001/Source/modules/device_orie... Source/modules/device_orientation/DeviceMotionController.h:42: // methods in this class to the DeviceController. Is this still relevant in your new design? I'd remove the FIXME and just address this when you're moving Device Orientation to this model as well. https://codereview.chromium.org/14460010/diff/2001/Source/modules/device_orie... Source/modules/device_orientation/DeviceMotionController.h:75: HashCountedSet<RefPtr<DOMWindow> > m_listeners; Adam replied to having RefPtr<>s to DOMWindow in your previous CL, did you give that any thought?
The code in /modules/ can depend directly on Platform/chromium/public/. Why do you need new code in core/platform/... ? https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... File Source/core/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/support/WebDeviceMotionData.cpp:29: #include "modules/device_orientation/DeviceMotionData.h" you're making core/platform/ depend on the module? why?
On 2013/04/29 17:44:48, jamesr wrote: > The code in /modules/ can depend directly on Platform/chromium/public/. Why do > you need new code in core/platform/... ? > yeah, Peter also mentioned this... You are right, I should get rid of the core/platform piece. (guess I tried to reuse too much code from the previous patch ;) ).
https://codereview.chromium.org/14460010/diff/2001/Source/WebKit/chromium/src... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/14460010/diff/2001/Source/WebKit/chromium/src... Source/WebKit/chromium/src/WebViewImpl.cpp:41: #include "modules/device_orientation/DeviceMotionController.h" Please put this include in alphabetical order. https://codereview.chromium.org/14460010/diff/2001/Source/WebKit/chromium/src... Source/WebKit/chromium/src/WebViewImpl.cpp:485: provideDeviceMotionTo(m_page.get()); This shouldn't be needed. This sort of thing is only needed when you need to wire up a client, but there's not client here. (Really most of these features that use clients should be refactored to not use clients also.) https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... File Source/core/platform/chromium/DeviceMotion.cpp (right): https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/DeviceMotion.cpp:40: WebKit::WebDeviceMotionDispatcher::shared().addController(controller); I think Peter commented on this already, but you don't need to add this extra layer of abstraction anymore. Your code in modules/device_motion can just call functions in Source/Platform/chromium/public directly.
https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionData.h:41: #pragma pack(push, 1) On 2013/04/29 17:38:03, Peter Beverloo wrote: > Is this class really that important from a memory pressure point of view? How > many copies do you expect there to be? You save 8 bytes (7 + padding) by doing > this. Nonetheless, it's a minor win.. It's probably not very important, but this results in smaller mem-copy @50Hz. This is also how Gamepad API is done. https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... File Source/Platform/chromium/public/WebDeviceMotionHandler.h (right): https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionHandler.h:39: On 2013/04/29 17:38:03, Peter Beverloo wrote: > nit: no new line. Done. https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionHandler.h:42: // Starts updating the supplied listener with device motion data at regular intervals. On 2013/04/29 17:38:03, Peter Beverloo wrote: > Will start distributing device motion data to the supplied listener. Done. https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... File Source/Platform/chromium/public/WebDeviceMotionListener.h (right): https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionListener.h:39: On 2013/04/29 17:38:03, Peter Beverloo wrote: > nit: no new line. Done. https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionListener.h:43: // the data is passed in the argument. On 2013/04/29 17:38:03, Peter Beverloo wrote: > "the data is passed in the argument" doesn't really add any value, since it's > passed by reference that's obvious. I removed the comment, but strictly speaking it could be passed by reference as a container to be filled. https://codereview.chromium.org/14460010/diff/2001/Source/Platform/chromium/p... Source/Platform/chromium/public/WebDeviceMotionListener.h:47: } // namespace WebCore On 2013/04/29 17:38:03, Peter Beverloo wrote: > namespace WebKit Done. https://codereview.chromium.org/14460010/diff/2001/Source/WebKit/chromium/src... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/14460010/diff/2001/Source/WebKit/chromium/src... Source/WebKit/chromium/src/WebViewImpl.cpp:41: #include "modules/device_orientation/DeviceMotionController.h" On 2013/04/29 20:32:20, abarth wrote: > Please put this include in alphabetical order. Done. https://codereview.chromium.org/14460010/diff/2001/Source/WebKit/chromium/src... Source/WebKit/chromium/src/WebViewImpl.cpp:485: provideDeviceMotionTo(m_page.get()); On 2013/04/29 20:32:20, abarth wrote: > This shouldn't be needed. This sort of thing is only needed when you need to > wire up a client, but there's not client here. > > (Really most of these features that use clients should be refactored to not use > clients also.) DeviceMotionController is still a supplement of a page because we need to know the page it is active at for caching purposes (PageCache.cpp). Otherwise we will likely need to change the interface of DOMWindow. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/Platf... File Source/core/platform/PlatformDeviceMotion.h (right): https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/Platf... Source/core/platform/PlatformDeviceMotion.h:34: #include <wtf/PassRefPtr.h> On 2013/04/29 17:38:03, Peter Beverloo wrote: > PassRefPtr isn't used in this header. Done. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/Platf... Source/core/platform/PlatformDeviceMotion.h:42: void registerForDeviceMotionUpdates(DeviceMotionController*); On 2013/04/29 17:38:03, Peter Beverloo wrote: > I wonder if it'd be acceptable now to just include WebDeviceMotionDispatcher.h > and call that directly? That'd severely limit the amount of code you need to > add to core/. Done. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/Platf... Source/core/platform/PlatformDeviceMotion.h:46: } On 2013/04/29 17:38:03, Peter Beverloo wrote: > nit: // namespace WebCore Done. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... File Source/core/platform/chromium/DeviceMotion.cpp (right): https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/DeviceMotion.cpp:40: WebKit::WebDeviceMotionDispatcher::shared().addController(controller); On 2013/04/29 20:32:20, abarth wrote: > I think Peter commented on this already, but you don't need to add this extra > layer of abstraction anymore. Your code in modules/device_motion can just call > functions in Source/Platform/chromium/public directly. Done. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... File Source/core/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/support/WebDeviceMotionData.cpp:29: #include "modules/device_orientation/DeviceMotionData.h" On 2013/04/29 17:44:48, jamesr wrote: > you're making core/platform/ depend on the module? why? it was originally needed to perform conversion from this class to WebCore::DeviceMotionData, but I've moves that code to modules now and removed the include, so should be fine. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... File Source/core/platform/chromium/support/WebDeviceMotionDispatcher.cpp (right): https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/support/WebDeviceMotionDispatcher.cpp:79: handler->stopListeningToDeviceMotion(); On 2013/04/29 17:38:03, Peter Beverloo wrote: > Shouldn't you pass the listener object here as well? that's not necessary because can be only one listener by design. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... File Source/core/platform/chromium/support/WebDeviceMotionDispatcher.h (right): https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/support/WebDeviceMotionDispatcher.h:47: // This class listens to Device Motion data and dispatches it to all On 2013/04/29 17:38:03, Peter Beverloo wrote: > nit: s/Device Motion data/device motion data/. Done. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/support/WebDeviceMotionDispatcher.h:49: On 2013/04/29 17:38:03, Peter Beverloo wrote: > nit: no newline. Done. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/support/WebDeviceMotionDispatcher.h:57: // The returned pointer is owned by this class On 2013/04/29 17:38:03, Peter Beverloo wrote: > "Note that the returned object is owned by this class." Done. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/support/WebDeviceMotionDispatcher.h:58: // FIXME: make the return value const, see crbug/233174. On 2013/04/29 17:38:03, Peter Beverloo wrote: > Please use a full URL instead of crbug/000. Done. https://codereview.chromium.org/14460010/diff/2001/Source/core/platform/chrom... Source/core/platform/chromium/support/WebDeviceMotionDispatcher.h:62: // which is passed in the argument. On 2013/04/29 17:38:03, Peter Beverloo wrote: > dito to my comment in WebDeviceMotionListener Done. https://codereview.chromium.org/14460010/diff/2001/Source/modules/device_orie... File Source/modules/device_orientation/DeviceMotionController.cpp (right): https://codereview.chromium.org/14460010/diff/2001/Source/modules/device_orie... Source/modules/device_orientation/DeviceMotionController.cpp:45: // This is a no-op if the controller has not registered with the device motion pump. On 2013/04/29 17:38:03, Peter Beverloo wrote: > There no longer is a pump. Done. https://codereview.chromium.org/14460010/diff/2001/Source/modules/device_orie... File Source/modules/device_orientation/DeviceMotionController.h (right): https://codereview.chromium.org/14460010/diff/2001/Source/modules/device_orie... Source/modules/device_orientation/DeviceMotionController.h:42: // methods in this class to the DeviceController. On 2013/04/29 17:38:03, Peter Beverloo wrote: > Is this still relevant in your new design? I'd remove the FIXME and just > address this when you're moving Device Orientation to this model as well. Done. https://codereview.chromium.org/14460010/diff/2001/Source/modules/device_orie... Source/modules/device_orientation/DeviceMotionController.h:75: HashCountedSet<RefPtr<DOMWindow> > m_listeners; On 2013/04/29 17:38:03, Peter Beverloo wrote: > Adam replied to having RefPtr<>s to DOMWindow in your previous CL, did you give > that any thought? I would prefer to address this in a separate CL though. Adam mentioned using WeakPtr or subclass from ContextDestructionObserver, I'll have to investigate what would be better to use.
Hi Adam, Would appreciate if you could comment on this new patch - without the pump design -. I realize the RefPtr issue is still there, but I'd like to tackle that in a separate CL. thanks!
This CL looks like it's moving in the right direction. I don't understand why you need a page-level object. Instead, I'd just use a Document-level supplement (no need to be refcounted). Then you won't have the problem of needing to keep a RefPtr to the DOMWindow. https://codereview.chromium.org/14460010/diff/17001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/14460010/diff/17001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:89: double m_accelerationX; Is there a setter for this field? How does it ever get populated with data? https://codereview.chromium.org/14460010/diff/17001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:103: char m_hasAccelerationX : 1; Why char instead of bool? https://codereview.chromium.org/14460010/diff/17001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionHandler.h (right): https://codereview.chromium.org/14460010/diff/17001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionHandler.h:38: // Abstract interface for the Device Motion API. This comment doesn't really say anything interesting. I would either remove it or change it to explain what this class does. https://codereview.chromium.org/14460010/diff/17001/Source/core/platform/chro... File Source/core/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/14460010/diff/17001/Source/core/platform/chro... Source/core/platform/chromium/support/WebDeviceMotionData.cpp:47: m_hasAccelerationX = 1; Shouldn't this be "= true" ? https://codereview.chromium.org/14460010/diff/17001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionController.h (right): https://codereview.chromium.org/14460010/diff/17001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.h:59: // FIXME move these methods to DeviceController FIXME -> FIXME: https://codereview.chromium.org/14460010/diff/17001/Source/modules/device_ori... File Source/modules/device_orientation/WebDeviceMotionDispatcher.cpp (right): https://codereview.chromium.org/14460010/diff/17001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.cpp:73: if (handler) Nit: The style we typically use here is to combine these two lines. https://codereview.chromium.org/14460010/diff/17001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.cpp:110: } It's more idiomatic to put this code in a WEBKIT_IMPLEMENTATION function on WebDeviceMotionData.
Adam, thanks for the comments. I've refactored the controller to be the supplement of ScriptExecutionContext (Document itself is apparently not supplementable). That way the controller code appears significantly simpler, as we don't need to keep all the window listeners anymore. Let me know what you think of this. https://codereview.chromium.org/14460010/diff/17001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/14460010/diff/17001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:89: double m_accelerationX; On 2013/05/06 17:04:17, abarth wrote: > Is there a setter for this field? How does it ever get populated with data? Yes, there is one for each of the fields, see above e.g. setAccelerationX(). https://codereview.chromium.org/14460010/diff/17001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:103: char m_hasAccelerationX : 1; On 2013/05/06 17:04:17, abarth wrote: > Why char instead of bool? It appears that only sizeof(char)=1 is a standard, sizeof(bool) would be implementation defined. https://codereview.chromium.org/14460010/diff/17001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionHandler.h (right): https://codereview.chromium.org/14460010/diff/17001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionHandler.h:38: // Abstract interface for the Device Motion API. On 2013/05/06 17:04:17, abarth wrote: > This comment doesn't really say anything interesting. I would either remove it > or change it to explain what this class does. Done. https://codereview.chromium.org/14460010/diff/17001/Source/core/platform/chro... File Source/core/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/14460010/diff/17001/Source/core/platform/chro... Source/core/platform/chromium/support/WebDeviceMotionData.cpp:47: m_hasAccelerationX = 1; On 2013/05/06 17:04:17, abarth wrote: > Shouldn't this be "= true" ? since m_hasAccelerationX is a bitfield of type char I though this would be more straightforward to avoid implicit conversion from true to 1. https://codereview.chromium.org/14460010/diff/17001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionController.h (right): https://codereview.chromium.org/14460010/diff/17001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.h:59: // FIXME move these methods to DeviceController On 2013/05/06 17:04:17, abarth wrote: > FIXME -> FIXME: Done. https://codereview.chromium.org/14460010/diff/17001/Source/modules/device_ori... File Source/modules/device_orientation/WebDeviceMotionDispatcher.cpp (right): https://codereview.chromium.org/14460010/diff/17001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.cpp:73: if (handler) On 2013/05/06 17:04:17, abarth wrote: > Nit: The style we typically use here is to combine these two lines. Done. https://codereview.chromium.org/14460010/diff/17001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.cpp:110: } On 2013/05/06 17:04:17, abarth wrote: > It's more idiomatic to put this code in a WEBKIT_IMPLEMENTATION function on > WebDeviceMotionData. But this way the WebDeviceMotionData (in Platform/chromium/public) is independent from the Blink/device_orientation module..
This is getting close. You've got a use-after-free vulnerability that we need to take care of before landing. Hopefully the next iteration of the CL should be good. We'll also need a test. Probably the easiest way to test this feature is with a LayoutTest. You can add a function to testController that causes it to send a device motion event via the API. Your test can then listen for the event and make sure it gets delivered. Once you have that basic test working, it should be easy to write a test for the use-after-free case by having two frames listening for the device motion event and having the first one remove the other frame from the DOM. https://codereview.chromium.org/14460010/diff/27001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/14460010/diff/27001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:105: char m_hasAccelerationZ : 1; Yes, but the ": 1" should give it the proper size in the object. This is a boolean variable, we should use the type bool. https://codereview.chromium.org/14460010/diff/27001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionListener.h (right): https://codereview.chromium.org/14460010/diff/27001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionListener.h:42: virtual void didChangeDeviceMotion(WebDeviceMotionData&) = 0; Why not a const reference? https://codereview.chromium.org/14460010/diff/27001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/14460010/diff/27001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:145: #include "modules/device_orientation/DeviceMotionController.h" Is this still needed? https://codereview.chromium.org/14460010/diff/27001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/14460010/diff/27001/Source/core/core.gypi#new... Source/core/core.gypi:2692: 'platform/chromium/support/WebDeviceMotionData.cpp', Please put this in the webcore_platform_support_files section with the rest of the files in this folder. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionController.cpp (right): https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.cpp:41: m_timer(this, &DeviceMotionController::fireDeviceEvent) Blink style is to put the ","s aligned vertically below the ":". https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.cpp:82: m_document->domWindow()->dispatchEvent(event); four-space indent please. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionController.h (right): https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.h:42: class DeviceMotionController : public RefCountedSupplement<ScriptExecutionContext, DeviceMotionController> { Why RefCounted? There shouldn't be any need for this object to be RefCounted. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.h:58: DeviceMotionController(Document* document); please mark one-argument constructors "explicit" https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.h:62: Document* m_document; This raw pointer is problematic if this object is RefCounted (because then the lifetimes of this object and m_document are not coupled). When you change this object to just be a normal supplement, then it will be owned by the document and then this raw pointer will be fine. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... File Source/modules/device_orientation/WebDeviceMotionDispatcher.cpp (right): https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.cpp:42: namespace WebKit { s/WebKit/WebCore/ https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.cpp:51: WebDeviceMotionDispatcher::WebDeviceMotionDispatcher() : m_lastDeviceMotionData(0) There's no need for this initializer. RefPtr automatically initializes to 0. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.cpp:89: listenerVector[i]->didChangeDeviceMotion(m_lastDeviceMotionData.get()); This pattern isn't secure. If listenerVector was really keeping any of the controllers alive, you'd still have a use-after-free vulnerability because the controllers have a raw pointer to m_document. nit: Please add spaces around the " < " operator. It's not clear to me how to do this iteration securely... I think the way to do this is to make removeController zero out the entry in m_controllers rather than removing it. After we're done iterating m_controllers, we can compact it by removing all the zero entries. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... File Source/modules/device_orientation/WebDeviceMotionDispatcher.h (right): https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.h:44: namespace WebKit { This object should be in the WebCore namespace. (Everything in Source/modules should be in the WebCore namespace.) https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.h:50: class WebDeviceMotionDispatcher : public WebDeviceMotionListener { WebDeviceMotionDispatcher -> DeviceMotionDispatcher We only use the "web" prefix for API objects. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.h:71: PassRefPtr<WebCore::DeviceMotionData> convertToDeviceMotionData(WebDeviceMotionData&); Thought you were going to make this an operator on WebDeviceMotionData... https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.h:74: HashCountedSet<RefPtr<WebCore::DeviceMotionController> > m_controllers; There's no need to use a RefPtr here. The DeviceMotionControllers unregister themselves in their destructors, so you can't have a pointer to a dead one. If this RefPtr was really keeping a controller alive, you'd still have a use-after-free problem because the controller holds a raw pointer to m_document, which would be dead.
addressed most comments, do you want the tests in this CL or should I create a new one? https://codereview.chromium.org/14460010/diff/27001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/14460010/diff/27001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:105: char m_hasAccelerationZ : 1; On 2013/05/07 16:43:47, abarth wrote: > Yes, but the ": 1" should give it the proper size in the object. This is a > boolean variable, we should use the type bool. my reasoning behind using a char instead of bool is that it is guaranteed to have sizeof=1. As far as I understand the underlying type in bitfields is used as a granularity parameter to reserve the underlying memory, so strictly speaking if a bool > 1 bytes we could end up with a larger memory usage. This should probably not have any impact in most compilers though.. https://codereview.chromium.org/14460010/diff/27001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionListener.h (right): https://codereview.chromium.org/14460010/diff/27001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionListener.h:42: virtual void didChangeDeviceMotion(WebDeviceMotionData&) = 0; On 2013/05/07 16:43:47, abarth wrote: > Why not a const reference? Done. https://codereview.chromium.org/14460010/diff/27001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/14460010/diff/27001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:145: #include "modules/device_orientation/DeviceMotionController.h" On 2013/05/07 16:43:47, abarth wrote: > Is this still needed? Done. https://codereview.chromium.org/14460010/diff/27001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/14460010/diff/27001/Source/core/core.gypi#new... Source/core/core.gypi:2692: 'platform/chromium/support/WebDeviceMotionData.cpp', On 2013/05/07 16:43:47, abarth wrote: > Please put this in the webcore_platform_support_files section with the rest of > the files in this folder. Done. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionController.cpp (right): https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.cpp:41: m_timer(this, &DeviceMotionController::fireDeviceEvent) On 2013/05/07 16:43:47, abarth wrote: > Blink style is to put the ","s aligned vertically below the ":". Done. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.cpp:82: m_document->domWindow()->dispatchEvent(event); On 2013/05/07 16:43:47, abarth wrote: > four-space indent please. ? it is a four-space indent.. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionController.h (right): https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.h:42: class DeviceMotionController : public RefCountedSupplement<ScriptExecutionContext, DeviceMotionController> { On 2013/05/07 16:43:47, abarth wrote: > Why RefCounted? There shouldn't be any need for this object to be RefCounted. Done. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.h:58: DeviceMotionController(Document* document); On 2013/05/07 16:43:47, abarth wrote: > please mark one-argument constructors "explicit" Done. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.h:62: Document* m_document; On 2013/05/07 16:43:47, abarth wrote: > This raw pointer is problematic if this object is RefCounted (because then the > lifetimes of this object and m_document are not coupled). When you change this > object to just be a normal supplement, then it will be owned by the document and > then this raw pointer will be fine. Done. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... File Source/modules/device_orientation/WebDeviceMotionDispatcher.cpp (right): https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.cpp:42: namespace WebKit { On 2013/05/07 16:43:47, abarth wrote: > s/WebKit/WebCore/ Done. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.cpp:51: WebDeviceMotionDispatcher::WebDeviceMotionDispatcher() : m_lastDeviceMotionData(0) On 2013/05/07 16:43:47, abarth wrote: > There's no need for this initializer. RefPtr automatically initializes to 0. Done. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.cpp:89: listenerVector[i]->didChangeDeviceMotion(m_lastDeviceMotionData.get()); On 2013/05/07 16:43:47, abarth wrote: > This pattern isn't secure. If listenerVector was really keeping any of the > controllers alive, you'd still have a use-after-free vulnerability because the > controllers have a raw pointer to m_document. > > nit: Please add spaces around the " < " operator. > > It's not clear to me how to do this iteration securely... > > I think the way to do this is to make removeController zero out the entry in > m_controllers rather than removing it. After we're done iterating > m_controllers, we can compact it by removing all the zero entries. once the controllers are not ref-counted anymore this should not be a problem right? maybe I am missing a concurrency issue here. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... File Source/modules/device_orientation/WebDeviceMotionDispatcher.h (right): https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.h:44: namespace WebKit { On 2013/05/07 16:43:47, abarth wrote: > This object should be in the WebCore namespace. (Everything in Source/modules > should be in the WebCore namespace.) Done. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.h:50: class WebDeviceMotionDispatcher : public WebDeviceMotionListener { On 2013/05/07 16:43:47, abarth wrote: > WebDeviceMotionDispatcher -> DeviceMotionDispatcher > > We only use the "web" prefix for API objects. Done. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.h:71: PassRefPtr<WebCore::DeviceMotionData> convertToDeviceMotionData(WebDeviceMotionData&); On 2013/05/07 16:43:47, abarth wrote: > Thought you were going to make this an operator on WebDeviceMotionData... Done. https://codereview.chromium.org/14460010/diff/27001/Source/modules/device_ori... Source/modules/device_orientation/WebDeviceMotionDispatcher.h:74: HashCountedSet<RefPtr<WebCore::DeviceMotionController> > m_controllers; On 2013/05/07 16:43:47, abarth wrote: > There's no need to use a RefPtr here. The DeviceMotionControllers unregister > themselves in their destructors, so you can't have a pointer to a dead one. If > this RefPtr was really keeping a controller alive, you'd still have a > use-after-free problem because the controller holds a raw pointer to m_document, > which would be dead. ack, done.
https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionController.cpp (right): https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.cpp:81: m_document->domWindow()->dispatchEvent(event); This line is indented 8 spaces from the "if" on line 78 (making 12 total). It should be indented 4 spaces from the if (making 8 total). https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionController.h (right): https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.h:52: virtual PassRefPtr<Event> getLastEvent(); These probably don't need to be virtual anymore. https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionDispatcher.cpp (right): https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionDispatcher.cpp:87: listenerVector[i]->didChangeDeviceMotion(m_lastDeviceMotionData.get()); > once the controllers are not ref-counted anymore this > should not be a problem right? maybe I am missing a > concurrency issue here. It's a re-entrancy issue rather than a concurrency issue. Suppose didChangeDeviceMotion causes some script to execute which causes a Document to be destroy which causes the next DeviceMotionController in the iteration to be destroyed. Now you're going to call a function on a dead object. Copying the raw pointers into a local variable doesn't help you in this situation.... Instead, you need to iterate m_controllers directly (using indexes) and instead of removing elements in DeviceMotionDispatcher::removeController, you need to zero them out. That way you'll still iterate all the elements of the m_controllers vector and you can skip the ones that die during the iteration. After the iteration is done, you can compact m_controllers to get rid of the zero entries. https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionDispatcher.h (right): https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionDispatcher.h:72: HashCountedSet<WebCore::DeviceMotionController*> m_controllers; No need for the "WebCore::" prefix now that we're in the WebCore namespace.
uploaded a re-entrancy fix, working on tests.. https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionController.cpp (right): https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.cpp:81: m_document->domWindow()->dispatchEvent(event); On 2013/05/07 20:10:01, abarth wrote: > This line is indented 8 spaces from the "if" on line 78 (making 12 total). It > should be indented 4 spaces from the if (making 8 total). Done. https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionController.h (right): https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.h:52: virtual PassRefPtr<Event> getLastEvent(); On 2013/05/07 20:10:01, abarth wrote: > These probably don't need to be virtual anymore. Done. https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionDispatcher.cpp (right): https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionDispatcher.cpp:87: listenerVector[i]->didChangeDeviceMotion(m_lastDeviceMotionData.get()); On 2013/05/07 20:10:01, abarth wrote: > > once the controllers are not ref-counted anymore this > > should not be a problem right? maybe I am missing a > > concurrency issue here. > > It's a re-entrancy issue rather than a concurrency issue. Suppose > didChangeDeviceMotion causes some script to execute which causes a Document to > be destroy which causes the next DeviceMotionController in the iteration to be > destroyed. Now you're going to call a function on a dead object. > > Copying the raw pointers into a local variable doesn't help you in this > situation.... > > Instead, you need to iterate m_controllers directly (using indexes) and instead > of removing elements in DeviceMotionDispatcher::removeController, you need to > zero them out. That way you'll still iterate all the elements of the > m_controllers vector and you can skip the ones that die during the iteration. > > After the iteration is done, you can compact m_controllers to get rid of the > zero entries. Done. https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionDispatcher.h (right): https://codereview.chromium.org/14460010/diff/38001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionDispatcher.h:72: HashCountedSet<WebCore::DeviceMotionController*> m_controllers; On 2013/05/07 20:10:01, abarth wrote: > No need for the "WebCore::" prefix now that we're in the WebCore namespace. Done.
https://codereview.chromium.org/14460010/diff/46001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/14460010/diff/46001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:121: COMPILE_ASSERT(sizeof(WebDeviceMotionData) == (10*8 + 2), WebDeviceMotionData_has_wrong_size); nit: rather than having number magic here, it may be clearer to write the calculation like this: sizeof(WebDeviceMotionData) == 10 * sizeof(double) + 2 * sizeof(char) https://codereview.chromium.org/14460010/diff/46001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionListener.h (right): https://codereview.chromium.org/14460010/diff/46001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionListener.h:38: // Abstract interface for the Device Motion API listener. This comment doesn't add anything, I'd remove it. https://codereview.chromium.org/14460010/diff/46001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionListener.h:41: // This method is called every time new motion data is available. You call it "device motion data" elsewhere, let's stay consistent? https://codereview.chromium.org/14460010/diff/46001/Source/core/page/DOMWindo... File Source/core/page/DOMWindow.cpp (right): https://codereview.chromium.org/14460010/diff/46001/Source/core/page/DOMWindo... Source/core/page/DOMWindow.cpp:1540: if (DeviceMotionController* controller = DeviceMotionController::from(document())) I think it's confusing that you're passing document() here, while the "devicemotion" event is available on the window object and has no relationship to the document at all. You supplement the device motion controller on ScriptExecutionContext, which is a parent class of Document. I don't think the DeviceMotionController should know about Document at all, and we can instead just pass DOMWindow::scriptExecutionContext(). The passed SEC will be identical to the one you're passing now so there won't be any functional change, but I do think it's much cleaner. https://codereview.chromium.org/14460010/diff/46001/Source/core/platform/chro... File Source/core/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/14460010/diff/46001/Source/core/platform/chro... Source/core/platform/chromium/support/WebDeviceMotionData.cpp:2: * Copyright (C) 2013 Google Inc. All rights reserved. This is the wrong copyright header. https://codereview.chromium.org/14460010/diff/46001/Source/core/platform/chro... Source/core/platform/chromium/support/WebDeviceMotionData.cpp:28: #include <modules/device_orientation/DeviceMotionData.h> nit: I'd put an empty line before line 28, which is common convention. https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionController.cpp (right): https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.cpp:101: if (!m_isActive) { nit: You can use early returns here to limit indentation a bit. if (m_isActive) return; // rest of your code https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionController.h (right): https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.h:39: // This class doesn't inherit from DeviceController anymore, which is a temporary solution. This should start with FIXME: https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.h:40: // Once the device orientation switches to the client-less design, move some of the nit: s/the// https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionDispatcher.cpp (right): https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionDispatcher.cpp:71: // didChangeDeviceMotion method. Can we test this with a layout test? https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionDispatcher.cpp:92: bool needsCompactify = false; I'm not thrilled about the "compactify" name. Can we go with "needsPurge" instead? That way, "compactifyControllers" can be named "purgeControllers". https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionDispatcher.cpp:124: } // namespace WebKit This comment should read "namespace WebCore" now. https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionDispatcher.h (right): https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionDispatcher.h:49: // interested controllers. nit: s/interested/listening/. https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionDispatcher.h:61: // This method is called every time new motion data is available. nit: Please call it "device motion data" here as well.
https://codereview.chromium.org/14460010/diff/46001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/14460010/diff/46001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:121: COMPILE_ASSERT(sizeof(WebDeviceMotionData) == (10*8 + 2), WebDeviceMotionData_has_wrong_size); On 2013/05/08 13:15:01, Peter Beverloo wrote: > nit: rather than having number magic here, it may be clearer to write the > calculation like this: > > sizeof(WebDeviceMotionData) == 10 * sizeof(double) + 2 * sizeof(char) Done. https://codereview.chromium.org/14460010/diff/46001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionListener.h (right): https://codereview.chromium.org/14460010/diff/46001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionListener.h:38: // Abstract interface for the Device Motion API listener. On 2013/05/08 13:15:01, Peter Beverloo wrote: > This comment doesn't add anything, I'd remove it. Done. https://codereview.chromium.org/14460010/diff/46001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionListener.h:41: // This method is called every time new motion data is available. On 2013/05/08 13:15:01, Peter Beverloo wrote: > You call it "device motion data" elsewhere, let's stay consistent? Done. https://codereview.chromium.org/14460010/diff/46001/Source/core/page/DOMWindo... File Source/core/page/DOMWindow.cpp (right): https://codereview.chromium.org/14460010/diff/46001/Source/core/page/DOMWindo... Source/core/page/DOMWindow.cpp:1540: if (DeviceMotionController* controller = DeviceMotionController::from(document())) On 2013/05/08 13:15:01, Peter Beverloo wrote: > I think it's confusing that you're passing document() here, while the > "devicemotion" event is available on the window object and has no relationship > to the document at all. > > You supplement the device motion controller on ScriptExecutionContext, which is > a parent class of Document. I don't think the DeviceMotionController should > know about Document at all, and we can instead just pass > DOMWindow::scriptExecutionContext(). > > The passed SEC will be identical to the one you're passing now so there won't be > any functional change, but I do think it's much cleaner. hmm, the document is actually needed to dispatch the event to the window, I haven't found any domWindow() method on the SEC. https://codereview.chromium.org/14460010/diff/46001/Source/core/platform/chro... File Source/core/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/14460010/diff/46001/Source/core/platform/chro... Source/core/platform/chromium/support/WebDeviceMotionData.cpp:2: * Copyright (C) 2013 Google Inc. All rights reserved. On 2013/05/08 13:15:01, Peter Beverloo wrote: > This is the wrong copyright header. Done. https://codereview.chromium.org/14460010/diff/46001/Source/core/platform/chro... Source/core/platform/chromium/support/WebDeviceMotionData.cpp:28: #include <modules/device_orientation/DeviceMotionData.h> On 2013/05/08 13:15:01, Peter Beverloo wrote: > nit: I'd put an empty line before line 28, which is common convention. Done. https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionController.cpp (right): https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.cpp:101: if (!m_isActive) { On 2013/05/08 13:15:01, Peter Beverloo wrote: > nit: You can use early returns here to limit indentation a bit. > > if (m_isActive) > return; > > // rest of your code Done. https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionController.h (right): https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.h:39: // This class doesn't inherit from DeviceController anymore, which is a temporary solution. On 2013/05/08 13:15:01, Peter Beverloo wrote: > This should start with FIXME: Done. https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionController.h:40: // Once the device orientation switches to the client-less design, move some of the On 2013/05/08 13:15:01, Peter Beverloo wrote: > nit: s/the// Done. https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionDispatcher.cpp (right): https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionDispatcher.cpp:71: // didChangeDeviceMotion method. On 2013/05/08 13:15:01, Peter Beverloo wrote: > Can we test this with a layout test? I'll try to add this to the tests as abarth commented. https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionDispatcher.cpp:92: bool needsCompactify = false; On 2013/05/08 13:15:01, Peter Beverloo wrote: > I'm not thrilled about the "compactify" name. Can we go with "needsPurge" > instead? That way, "compactifyControllers" can be named "purgeControllers". Done. https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionDispatcher.cpp:124: } // namespace WebKit On 2013/05/08 13:15:01, Peter Beverloo wrote: > This comment should read "namespace WebCore" now. Done. https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... File Source/modules/device_orientation/DeviceMotionDispatcher.h (right): https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionDispatcher.h:49: // interested controllers. On 2013/05/08 13:15:01, Peter Beverloo wrote: > nit: s/interested/listening/. Done. https://codereview.chromium.org/14460010/diff/46001/Source/modules/device_ori... Source/modules/device_orientation/DeviceMotionDispatcher.h:61: // This method is called every time new motion data is available. On 2013/05/08 13:15:01, Peter Beverloo wrote: > nit: Please call it "device motion data" here as well. Done.
https://codereview.chromium.org/14460010/diff/46001/Source/core/page/DOMWindo... File Source/core/page/DOMWindow.cpp (right): https://codereview.chromium.org/14460010/diff/46001/Source/core/page/DOMWindo... Source/core/page/DOMWindow.cpp:1540: if (DeviceMotionController* controller = DeviceMotionController::from(document())) On 2013/05/09 10:08:28, timvolodine wrote: > hmm, the document is actually needed to dispatch the event to the window, I > haven't found any domWindow() method on the SEC. Right. Now that we're no longer storing the DOMWindow in a RefPtr, I think it would be fine to store a pointer to the DOMWindow instead of a pointer to Document in DeviceMotionController. You can get the script execution context (activeDOMObjectsAre{Stopped,Suspended} et al) from DOMWindow as well. Since the ScriptExecutionContext is owned by the DOMWindow this should be fine in terms of lifetime as well. What do you think?
On 2013/05/09 11:31:37, Peter Beverloo wrote: > https://codereview.chromium.org/14460010/diff/46001/Source/core/page/DOMWindo... > File Source/core/page/DOMWindow.cpp (right): > > https://codereview.chromium.org/14460010/diff/46001/Source/core/page/DOMWindo... > Source/core/page/DOMWindow.cpp:1540: if (DeviceMotionController* controller = > DeviceMotionController::from(document())) > On 2013/05/09 10:08:28, timvolodine wrote: > > hmm, the document is actually needed to dispatch the event to the window, I > > haven't found any domWindow() method on the SEC. > > Right. Now that we're no longer storing the DOMWindow in a RefPtr, I think it > would be fine to store a pointer to the DOMWindow instead of a pointer to > Document in DeviceMotionController. You can get the script execution context > (activeDOMObjectsAre{Stopped,Suspended} et al) from DOMWindow as well. > > Since the ScriptExecutionContext is owned by the DOMWindow this should be fine > in terms of lifetime as well. What do you think? I think the question here is mostly should device motion be a property linked to a DOMWindow or a Document. Originally Adam commented that we should use a Document level supplement. Also, DOMWindow.h contains a comment that it can be "re-used for a new Document" in rare cases. Would like to hear Adam's opinion on this before I make any changes.
Please use Document or ScriptExecutionContext rather than DOMWindow. The DOMWindow object is a slippery beast. It's very difficult to hold onto correctly. Document/ScriptExecutionContext is much more sane.
Hi Adam, I've added a layout test, which does not really inject anything, rather uses the fact that the DeviceMotionDispatcher is a singleton. However I've noticed that this is probably not the right way to go because there is now a dependency on WebCore in the TestRunner, which some platforms seem to dislike. It is not clear to me how to inject MockWebDeviceMotionHandler using the WebTestProxy, because the new interface is through the Platform layer. Instead it looks like the cleanest option is to implement a Gamepad-like approach where the mock handler will be injected into the Platform (i.e. renderer_webkitplatformsupport_impl) through the WebTestDelegate (etc..). I am going to implement this approach now.
https://codereview.chromium.org/14460010/diff/67001/Tools/DumpRenderTree/chro... File Tools/DumpRenderTree/chromium/TestRunner/src/TestInterfaces.cpp (right): https://codereview.chromium.org/14460010/diff/67001/Tools/DumpRenderTree/chro... Tools/DumpRenderTree/chromium/TestRunner/src/TestInterfaces.cpp:65: WebRuntimeFeatures::enableDeviceMotion(true); In RuntimeEnabledFeatures.in, you should mark device motion as enabled for testing rather than putting this code here. Would you also be willing to add a comment here explaining that we shouldn't put feature-specific enable flags here and that instead folks should set the "status" attribute in RuntimeEnabledFetures.in appropriately? https://codereview.chromium.org/14460010/diff/67001/Tools/DumpRenderTree/chro... File Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.cpp (right): https://codereview.chromium.org/14460010/diff/67001/Tools/DumpRenderTree/chro... Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.cpp:64: #include <modules/device_orientation/DeviceMotionDispatcher.h> This is a bad dependency. TestRunner cannot reach into this code because they exist in separate DLLs. Instead, TestRunner needs to interact with this code via the API.
Adam, thanks for comments. I've implemented the new approach by setting mock device motion handler in the platform via the WebTestDelegate. While implementing that I ended up with some minor modifications in the content/ side, see https://codereview.chromium.org/14682023/. I've tested the layout test using content_shell. Let me know what you think. https://codereview.chromium.org/14460010/diff/67001/Tools/DumpRenderTree/chro... File Tools/DumpRenderTree/chromium/TestRunner/src/TestInterfaces.cpp (right): https://codereview.chromium.org/14460010/diff/67001/Tools/DumpRenderTree/chro... Tools/DumpRenderTree/chromium/TestRunner/src/TestInterfaces.cpp:65: WebRuntimeFeatures::enableDeviceMotion(true); On 2013/05/13 18:39:56, abarth wrote: > In RuntimeEnabledFeatures.in, you should mark device motion as enabled for > testing rather than putting this code here. > > Would you also be willing to add a comment here explaining that we shouldn't put > feature-specific enable flags here and that instead folks should set the > "status" attribute in RuntimeEnabledFetures.in appropriately? Done. https://codereview.chromium.org/14460010/diff/67001/Tools/DumpRenderTree/chro... File Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.cpp (right): https://codereview.chromium.org/14460010/diff/67001/Tools/DumpRenderTree/chro... Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.cpp:64: #include <modules/device_orientation/DeviceMotionDispatcher.h> On 2013/05/13 18:39:56, abarth wrote: > This is a bad dependency. TestRunner cannot reach into this code because they > exist in separate DLLs. Instead, TestRunner needs to interact with this code > via the API. Done.
https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:48: void setAccelerationX(double); these methods need to be exported using the WEBKIT_EXPORT macro https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:93: double m_accelerationX; why isn't WebDeviceMotionData just a simple ref-counted wrapper around WebCore::DeviceMotionData? You can use WebPrivatePtr<WebCore::DeviceMotionData>, no? See for example WebNode. https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionHandler.h (right): https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionHandler.h:38: class WebDeviceMotionHandler { it seems like there are one too many interfaces here. how about just giving Platform a simple method like this: Platform::setDeviceMotionListener(WebDeviceMotionListener*); You can set it either to NULL or non-NULL. If non-NULL, then the Platform will start calling the WebDeviceMotionListener to provide it with device motion updates. Otherwise, it will do nothing. There seems to be no need for the WebDeviceMotionHandler interface.
Darin, thanks for comments! Would be great to get this patch in. So let me know if there are any concerns. https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:48: void setAccelerationX(double); On 2013/05/14 18:13:37, darin wrote: > these methods need to be exported using the WEBKIT_EXPORT macro The presubmit check complains on this: WEBKIT_EXPORT should not be used on a function with a body. https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:93: double m_accelerationX; On 2013/05/14 18:13:37, darin wrote: > why isn't WebDeviceMotionData just a simple ref-counted wrapper around > WebCore::DeviceMotionData? You can use > WebPrivatePtr<WebCore::DeviceMotionData>, no? See for example WebNode. hmm, looks like a nice construction indeed, however WebDeviceMotionData is also used in the shared memory buffer and WebCore::DeviceMotionData is not very suited for this purpose because it has Ref pointers to its members and it seems the only way to set new data is to create a new object.. https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionHandler.h (right): https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionHandler.h:38: class WebDeviceMotionHandler { On 2013/05/14 18:13:37, darin wrote: > it seems like there are one too many interfaces here. how about just giving > Platform a simple method like this: > > Platform::setDeviceMotionListener(WebDeviceMotionListener*); > > You can set it either to NULL or non-NULL. If non-NULL, then the Platform will > start calling the WebDeviceMotionListener to provide it with device motion > updates. Otherwise, it will do nothing. > > There seems to be no need for the WebDeviceMotionHandler interface. I like this NULL/non-NULL suggestion, however I just tried refactoring for this approach and noticed that WebDeviceMotionHandler is actually very handy for testing. As it is now a MockHandler is injected into the platform (similar to gamepad) and then a fire-method is called upon it. The injected mock needs to be static to facilitate calling it from WebTestDelegate. With the new Platform::setDeviceMotionListener(listener) interface we would need to cache the listener somehow, and setting/getting it for testing does not look straightforward to me. Alternatively I could use Internals:: to test the listener directly in Blink (as it is a singleton anyway). But that's a different strategy, and Adam originally suggested using TestRunner.
ping :) can we LGTM this ?
I'll let darin do the official review.
On 2013/05/15 12:34:34, timvolodine wrote: > Darin, thanks for comments! > > Would be great to get this patch in. So let me know if there are any concerns. > > https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... > File Source/Platform/chromium/public/WebDeviceMotionData.h (right): > > https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... > Source/Platform/chromium/public/WebDeviceMotionData.h:48: void > setAccelerationX(double); > On 2013/05/14 18:13:37, darin wrote: > > these methods need to be exported using the WEBKIT_EXPORT macro > > The presubmit check complains on this: > WEBKIT_EXPORT should not be used on a function with a body. Indeed. Sorry, I meant only to apply WEBKIT_EXPORT to the functions that have their body in a .cpp file. You need this in order to not break the component build. > https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... > Source/Platform/chromium/public/WebDeviceMotionData.h:93: double > m_accelerationX; > On 2013/05/14 18:13:37, darin wrote: > > why isn't WebDeviceMotionData just a simple ref-counted wrapper around > > WebCore::DeviceMotionData? You can use > > WebPrivatePtr<WebCore::DeviceMotionData>, no? See for example WebNode. > > hmm, looks like a nice construction indeed, however WebDeviceMotionData is also > used in the shared memory buffer and WebCore::DeviceMotionData is not very > suited for this purpose because it has Ref pointers to its members and it seems > the only way to set new data is to create a new object.. It sounds like you are trying to describe an IPC data structure in the Blink API. The point of the Blink API is to *thinly* wrap and expose the WebCore data structures as needed. This is why WebPrivatePtr is so useful. There are cases as in WebInputEvent and WebGamepads where we have done otherwise. Maybe those examples are guiding you to do similar? In those cases, we couldn't simply wrap a reference counted WebCore data type because there was none to wrap. By just wrapping WebCore::WebDeviceMotionData in the lightest possible way, you free yourself to evolve the IPC format independently of Blink. > > https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... > File Source/Platform/chromium/public/WebDeviceMotionHandler.h (right): > > https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... > Source/Platform/chromium/public/WebDeviceMotionHandler.h:38: class > WebDeviceMotionHandler { > On 2013/05/14 18:13:37, darin wrote: > > it seems like there are one too many interfaces here. how about just giving > > Platform a simple method like this: > > > > Platform::setDeviceMotionListener(WebDeviceMotionListener*); > > > > You can set it either to NULL or non-NULL. If non-NULL, then the Platform > will > > start calling the WebDeviceMotionListener to provide it with device motion > > updates. Otherwise, it will do nothing. > > > > There seems to be no need for the WebDeviceMotionHandler interface. > > I like this NULL/non-NULL suggestion, however I just tried refactoring for this > approach and noticed that WebDeviceMotionHandler is actually very handy for > testing. > > As it is now a MockHandler is injected into the platform (similar to gamepad) > and then a fire-method is called upon it. The injected mock needs to be static > to facilitate calling it from WebTestDelegate. > > With the new Platform::setDeviceMotionListener(listener) interface we would need > to cache the listener somehow, and setting/getting it for testing does not look > straightforward to me. I don't quite follow. The API I suggested could be used to mimic the handler API. You could just have a listener that delegates to a handler that delegates to a listener. Or you could just have a listener that chains to another listener. It seems like there is no advantage to the handler API. I'm probably missing some detail :-) > Alternatively I could use Internals:: to test the listener directly in Blink (as > it is a singleton anyway). But that's a different strategy, and Adam originally > suggested using TestRunner.
I should add that if your goal is to make WebDeviceMotionData be something suitable for IPC like WebInputEvent and WebGamepads, then please just make it be a "POD-ish" type with public data members. No need for setters and getters.
On 2013/05/16 17:53:27, darin wrote: > On 2013/05/15 12:34:34, timvolodine wrote: > > Darin, thanks for comments! > > > > Would be great to get this patch in. So let me know if there are any concerns. > > > > > https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... > > File Source/Platform/chromium/public/WebDeviceMotionData.h (right): > > > > > https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... > > Source/Platform/chromium/public/WebDeviceMotionData.h:48: void > > setAccelerationX(double); > > On 2013/05/14 18:13:37, darin wrote: > > > these methods need to be exported using the WEBKIT_EXPORT macro > > > > The presubmit check complains on this: > > WEBKIT_EXPORT should not be used on a function with a body. > > Indeed. Sorry, I meant only to apply WEBKIT_EXPORT to the functions that have > their body in a .cpp file. You need this in order to not break the component > build. > > > > > https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... > > Source/Platform/chromium/public/WebDeviceMotionData.h:93: double > > m_accelerationX; > > On 2013/05/14 18:13:37, darin wrote: > > > why isn't WebDeviceMotionData just a simple ref-counted wrapper around > > > WebCore::DeviceMotionData? You can use > > > WebPrivatePtr<WebCore::DeviceMotionData>, no? See for example WebNode. > > > > hmm, looks like a nice construction indeed, however WebDeviceMotionData is > also > > used in the shared memory buffer and WebCore::DeviceMotionData is not very > > suited for this purpose because it has Ref pointers to its members and it > seems > > the only way to set new data is to create a new object.. > > It sounds like you are trying to describe an IPC data structure in the Blink > API. > The point of the Blink API is to *thinly* wrap and expose the WebCore data > structures > as needed. This is why WebPrivatePtr is so useful. > > There are cases as in WebInputEvent and WebGamepads where we have done > otherwise. > Maybe those examples are guiding you to do similar? In those cases, we couldn't > simply wrap a reference counted WebCore data type because there was none to > wrap. > > By just wrapping WebCore::WebDeviceMotionData in the lightest possible way, you > free yourself to evolve the IPC format independently of Blink. > As you noted later, the purpose of WebDeviceMotionData is to have something suitable for shared memory buffer. I don't really see how WebCore::DeviceMotionData can be used to that end, because it is not simply 'writable' as a "POD-ish" type. > > > > > > https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... > > File Source/Platform/chromium/public/WebDeviceMotionHandler.h (right): > > > > > https://codereview.chromium.org/14460010/diff/74001/Source/Platform/chromium/... > > Source/Platform/chromium/public/WebDeviceMotionHandler.h:38: class > > WebDeviceMotionHandler { > > On 2013/05/14 18:13:37, darin wrote: > > > it seems like there are one too many interfaces here. how about just giving > > > Platform a simple method like this: > > > > > > Platform::setDeviceMotionListener(WebDeviceMotionListener*); > > > > > > You can set it either to NULL or non-NULL. If non-NULL, then the Platform > > will > > > start calling the WebDeviceMotionListener to provide it with device motion > > > updates. Otherwise, it will do nothing. > > > > > > There seems to be no need for the WebDeviceMotionHandler interface. > > > > I like this NULL/non-NULL suggestion, however I just tried refactoring for > this > > approach and noticed that WebDeviceMotionHandler is actually very handy for > > testing. > > > > As it is now a MockHandler is injected into the platform (similar to gamepad) > > and then a fire-method is called upon it. The injected mock needs to be static > > to facilitate calling it from WebTestDelegate. > > > > With the new Platform::setDeviceMotionListener(listener) interface we would > need > > to cache the listener somehow, and setting/getting it for testing does not > look > > straightforward to me. > > I don't quite follow. The API I suggested could be used to mimic the handler > API. > You could just have a listener that delegates to a handler that delegates to a > listener. Or you could just have a listener that chains to another listener. > It > seems like there is no advantage to the handler API. I'm probably missing some > detail :-) > What I basically meant is that the WebDeviceMotionHandler interface makes it easier for testing. The API you suggested is perfectly fine, but makes it harder to inject a mock object for testing the blink device motion implementation through the platform layer. > > > > Alternatively I could use Internals:: to test the listener directly in Blink > (as > > it is a singleton anyway). But that's a different strategy, and Adam > originally > > suggested using TestRunner.
On 2013/05/16 17:56:54, darin wrote: > I should add that if your goal is to make WebDeviceMotionData be something > suitable for IPC like WebInputEvent and WebGamepads, then please just make it be > a "POD-ish" type with public data members. No need for setters and getters. Ok done. I've removed the setters and getters. (the setter methods were actually handy because they set both X and canProvideX)
https://codereview.chromium.org/14460010/diff/58003/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/14460010/diff/58003/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:47: WebDeviceMotionData(); this constructor needs to be exported so Chromium side can call it. https://codereview.chromium.org/14460010/diff/58003/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:51: WEBKIT_EXPORT operator WTF::PassRefPtr<WebCore::DeviceMotionData>() const; this method does not need to be exported since it is limited by WEBKIT_IMPLEMENTATION. only Blink can call it and the code lives inside Blink so no need to export it. see third_party/WebKit/Source/WebKit/chromium/README for more details about the WEBKIT_EXPORT macro. nit: since WebDeviceMotionData is part of the Platform/ layer, instead of adding this casting operator, you can just make DeviceMotionData have a create() method that takes a WebKit::WebDeviceMotionData. things in core/ and modules/ can depend on interfaces defined in Platform/chromium/public/! :-D Doing so would be a lot cleaner and simpler. https://codereview.chromium.org/14460010/diff/58003/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionHandler.h (right): https://codereview.chromium.org/14460010/diff/58003/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionHandler.h:38: class WebDeviceMotionHandler { Based on the review thread, I'm expecting this will disappear in a future version of the CL. Let me know if I've misunderstood. https://codereview.chromium.org/14460010/diff/58003/Source/core/platform/chro... File Source/core/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/14460010/diff/58003/Source/core/platform/chro... Source/core/platform/chromium/support/WebDeviceMotionData.cpp:50: { you should probably memset the entire data structure instead. see WebInputEvent for reference. the current code leaves some of the memory uninitialized, and that will create issues for some of the valgrind-based memory tests. this constructor should have no member initializers. instead just do a big fat memset(this, 0, sizeof(*this));
https://codereview.chromium.org/14460010/diff/58003/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/14460010/diff/58003/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:47: WebDeviceMotionData(); On 2013/05/16 19:41:15, darin wrote: > this constructor needs to be exported so Chromium side can call it. Done. https://codereview.chromium.org/14460010/diff/58003/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionData.h:51: WEBKIT_EXPORT operator WTF::PassRefPtr<WebCore::DeviceMotionData>() const; On 2013/05/16 19:41:15, darin wrote: > this method does not need to be exported since it is limited by > WEBKIT_IMPLEMENTATION. only Blink can call it and the code lives inside Blink > so no need to export it. > > see third_party/WebKit/Source/WebKit/chromium/README for more details about the > WEBKIT_EXPORT macro. > > nit: since WebDeviceMotionData is part of the Platform/ layer, instead of adding > this casting operator, you can just make DeviceMotionData have a create() method > that takes a WebKit::WebDeviceMotionData. things in core/ and modules/ can > depend on interfaces defined in Platform/chromium/public/! :-D Doing so would > be a lot cleaner and simpler. moved this whole thing to WebCore::DeviceMotionData. Done. https://codereview.chromium.org/14460010/diff/58003/Source/Platform/chromium/... File Source/Platform/chromium/public/WebDeviceMotionHandler.h (right): https://codereview.chromium.org/14460010/diff/58003/Source/Platform/chromium/... Source/Platform/chromium/public/WebDeviceMotionHandler.h:38: class WebDeviceMotionHandler { On 2013/05/16 19:41:15, darin wrote: > Based on the review thread, I'm expecting this will disappear in a future > version of the CL. Let me know if I've misunderstood. OK, I've removed this class and changed the Platform interface to be Platform::setDeviceMotionListener(), as you suggested previously. However I still appear to need a new WebDeviceMotionProvider interface for testing purposes, i.e. injecting a mock provider into the platform. In this approach the actual pump implementation will implement this provider interface and be stored in renderer_webkitplatformsupport_impl.cpp (see https://codereview.chromium.org/14682023/). At this point I am not sure how to do injection without this provider construction. https://codereview.chromium.org/14460010/diff/58003/Source/core/platform/chro... File Source/core/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/14460010/diff/58003/Source/core/platform/chro... Source/core/platform/chromium/support/WebDeviceMotionData.cpp:50: { On 2013/05/16 19:41:15, darin wrote: > you should probably memset the entire data structure instead. see WebInputEvent > for reference. > > the current code leaves some of the memory uninitialized, and that will create > issues for some of the valgrind-based memory tests. > > this constructor should have no member initializers. instead just do a big fat > memset(this, 0, sizeof(*this)); Done.
It seems like the provider interface is not required by any of the interfaces under Platform/. It seems to only be required for testing. It should probably move to TestRunner/public/ https://codereview.chromium.org/14460010/diff/89001/Source/Platform/chromium/... File Source/Platform/chromium/public/Platform.h (right): https://codereview.chromium.org/14460010/diff/89001/Source/Platform/chromium/... Source/Platform/chromium/public/Platform.h:58: class WebDeviceMotionProvider; nit: no need for the provider forward declare here
On 2013/05/17 14:13:09, darin wrote: > It seems like the provider interface is not required by any of the interfaces > under Platform/. It seems to only be required for testing. It should probably > move to TestRunner/public/ > it is used by content/renderer/renderer_webkitplatformsupport_impl.cc (not in this CL but in the accompanying CL (14682023) ). So cannot really move in to TestRunner/public/..
https://codereview.chromium.org/14460010/diff/89001/Source/Platform/chromium/... File Source/Platform/chromium/public/Platform.h (right): https://codereview.chromium.org/14460010/diff/89001/Source/Platform/chromium/... Source/Platform/chromium/public/Platform.h:58: class WebDeviceMotionProvider; On 2013/05/17 14:13:09, darin wrote: > nit: no need for the provider forward declare here Done.
I see. I need to study that side. It is odd to put an interface in Platform that nothing in Platform or Source/core depends on. It ends up looking like dead code. Hmm, there has to be a better way. On May 17, 2013 7:35 AM, <timvolodine@chromium.org> wrote: > On 2013/05/17 14:13:09, darin wrote: > >> It seems like the provider interface is not required by any of the >> interfaces >> under Platform/. It seems to only be required for testing. It should >> > probably > >> move to TestRunner/public/ >> > > > it is used by content/renderer/renderer_**webkitplatformsupport_impl.cc > (not in > this CL but in the accompanying CL (14682023) ). So cannot really move in > to > TestRunner/public/.. > > > https://codereview.chromium.**org/14460010/<https://codereview.chromium.org/1... >
You should not put test code in that class. What tests need this capability and where do they live? On May 17, 2013 7:35 AM, <timvolodine@chromium.org> wrote: > On 2013/05/17 14:13:09, darin wrote: > >> It seems like the provider interface is not required by any of the >> interfaces >> under Platform/. It seems to only be required for testing. It should >> > probably > >> move to TestRunner/public/ >> > > > it is used by content/renderer/renderer_**webkitplatformsupport_impl.cc > (not in > this CL but in the accompanying CL (14682023) ). So cannot really move in > to > TestRunner/public/.. > > > https://codereview.chromium.**org/14460010/<https://codereview.chromium.org/1... >
On 2013/05/17 17:47:05, jamesr1 wrote: > You should not put test code in that class. What tests need this capability > and where do they live? > On May 17, 2013 7:35 AM, <mailto:timvolodine@chromium.org> wrote: > > > On 2013/05/17 14:13:09, darin wrote: > > > >> It seems like the provider interface is not required by any of the > >> interfaces > >> under Platform/. It seems to only be required for testing. It should > >> > > probably > > > >> move to TestRunner/public/ > >> > > > > > > it is used by content/renderer/renderer_**webkitplatformsupport_impl.cc > > (not in > > this CL but in the accompanying CL (14682023) ). So cannot really move in > > to > > TestRunner/public/.. > > > > > > > https://codereview.chromium.**org/14460010/%3Chttps://codereview.chromium.org...> > > The idea of testing in this CL is to test that blink can receive device motion updates by injecting a mock provider into the platform. To do that TestRunner implements two methods setMockDeviceMotion and fireMockDeviceMotion (which are called from Javascript). setMockDeviceMotion injects a mock provider with the WebDeviceMotionProvider interface into content/renderer/renderer_webkitplatformsupport_impl. At a later stage fireMockDeviceMotion triggers an update of the MockDeviceMotionProvider and propagates into Blink. The question here is where to put the WebDeviceMotionProvider interface as it is currently used in both renderer_webkitplatformsupport_impl and by TestRunner. The idea originates from how testing of Gamepad is done, but there the situation is slightly different as no callback is needed (i.e. no fire-method invocation). I could just switch to testing using Internals:: for simplicity, but mocking the provider (i.e. the pump) in platform seems more powerful.
On 2013/05/17 18:05:57, timvolodine wrote: > On 2013/05/17 17:47:05, jamesr1 wrote: > > You should not put test code in that class. What tests need this capability > > and where do they live? > > On May 17, 2013 7:35 AM, <mailto:timvolodine@chromium.org> wrote: > > > > > On 2013/05/17 14:13:09, darin wrote: > > > > > >> It seems like the provider interface is not required by any of the > > >> interfaces > > >> under Platform/. It seems to only be required for testing. It should > > >> > > > probably > > > > > >> move to TestRunner/public/ > > >> > > > > > > > > > it is used by content/renderer/renderer_**webkitplatformsupport_impl.cc > > > (not in > > > this CL but in the accompanying CL (14682023) ). So cannot really move in > > > to > > > TestRunner/public/.. > > > > > > > > > > > > https://codereview.chromium.**org/14460010/%253Chttps://codereview.chromium.o...> > > > > > The idea of testing in this CL is to test that blink can receive device motion > updates by injecting a mock provider into the platform. > > To do that TestRunner implements two methods setMockDeviceMotion and > fireMockDeviceMotion (which are called from Javascript). setMockDeviceMotion > injects a mock provider with the WebDeviceMotionProvider interface into > content/renderer/renderer_webkitplatformsupport_impl. At a later stage > fireMockDeviceMotion triggers an update of the MockDeviceMotionProvider and > propagates into Blink. > > The question here is where to put the WebDeviceMotionProvider interface as it is > currently used in both renderer_webkitplatformsupport_impl and by TestRunner. > > The idea originates from how testing of Gamepad is done, but there the situation > is slightly different as no callback is needed (i.e. no fire-method invocation). > > I could just switch to testing using Internals:: for simplicity, but mocking the > provider (i.e. the pump) in platform seems more powerful. ok, if the renderer_webkitplatformsupport_impl is not the preferred place to be putting testing code in, I'll reimplement the tests using internals, think it will be cleaner.
Found a way to get rid of the WebDeviceMotionProvider :), please have a look.
https://codereview.chromium.org/14460010/diff/102001/Source/Platform/chromium... File Source/Platform/chromium/public/WebDeviceMotionListener.h (right): https://codereview.chromium.org/14460010/diff/102001/Source/Platform/chromium... Source/Platform/chromium/public/WebDeviceMotionListener.h:44: virtual void setListener(WebDeviceMotionListener*) = 0; This also seems wrong. The point of WebDeviceMotionListener is to provide the Platform implementation with a way to feed new WebDeviceMotionData objects to Blink. It doesn't make sense for the Platform layer to call back into Blink to cause Blink to redirect WebDeviceMotionData objects elsewhere. The Platform layer should just do that directly, avoiding the call to didChangeDeviceMotion entirely. Platform says: "Hey, that thing I just gave you, actually, I take it back, please send it elsewhere." Blink says: "Wait, if you don't want me to have it, then why did you give it to me in the first place?"
https://codereview.chromium.org/14460010/diff/102001/Source/Platform/chromium... File Source/Platform/chromium/public/WebDeviceMotionListener.h (right): https://codereview.chromium.org/14460010/diff/102001/Source/Platform/chromium... Source/Platform/chromium/public/WebDeviceMotionListener.h:44: virtual void setListener(WebDeviceMotionListener*) = 0; On 2013/05/23 06:41:56, darin wrote: > This also seems wrong. The point of WebDeviceMotionListener is to provide the > Platform implementation with a way to feed new WebDeviceMotionData objects to > Blink. It doesn't make sense for the Platform layer to call back into Blink to > cause Blink to redirect WebDeviceMotionData objects elsewhere. The Platform > layer should just do that directly, avoiding the call to didChangeDeviceMotion > entirely. > > Platform says: "Hey, that thing I just gave you, actually, I take it back, > please send it elsewhere." > Blink says: "Wait, if you don't want me to have it, then why did you give it to > me in the first place?" I probably misunderstood one of your previous comments regarding 'chaining' of listeners. Apologies if that is the case. Let's address the testing in a separate CL if you don't mind. I've removed the test-related code from this CL for now. In general it looks like there is no very proper way of platform-based testing in this case since TestRunner uses the 'real' platform renderer_webkitsupport_impl, while ideally it should be a mock.
On 2013/05/23 11:39:26, timvolodine wrote: > https://codereview.chromium.org/14460010/diff/102001/Source/Platform/chromium... > File Source/Platform/chromium/public/WebDeviceMotionListener.h (right): > > https://codereview.chromium.org/14460010/diff/102001/Source/Platform/chromium... > Source/Platform/chromium/public/WebDeviceMotionListener.h:44: virtual void > setListener(WebDeviceMotionListener*) = 0; > On 2013/05/23 06:41:56, darin wrote: > > This also seems wrong. The point of WebDeviceMotionListener is to provide the > > Platform implementation with a way to feed new WebDeviceMotionData objects to > > Blink. It doesn't make sense for the Platform layer to call back into Blink > to > > cause Blink to redirect WebDeviceMotionData objects elsewhere. The Platform > > layer should just do that directly, avoiding the call to didChangeDeviceMotion > > entirely. > > > > Platform says: "Hey, that thing I just gave you, actually, I take it back, > > please send it elsewhere." > > Blink says: "Wait, if you don't want me to have it, then why did you give it > to > > me in the first place?" > > I probably misunderstood one of your previous comments regarding 'chaining' of > listeners. I think you can construct the chain on the Chromium side behind the Platform interface such that Blink does not need to know that chaining is happening. Does that make sense? > Apologies if that is the case. > Let's address the testing in a separate CL if you don't mind. I've removed the > test-related code from this CL for now. > > In general it looks like there is no very proper way of platform-based testing > in this case since TestRunner uses the 'real' platform > renderer_webkitsupport_impl, while ideally it should be a mock.
On 2013/05/23 14:36:49, darin wrote: > On 2013/05/23 11:39:26, timvolodine wrote: > > > https://codereview.chromium.org/14460010/diff/102001/Source/Platform/chromium... > > File Source/Platform/chromium/public/WebDeviceMotionListener.h (right): > > > > > https://codereview.chromium.org/14460010/diff/102001/Source/Platform/chromium... > > Source/Platform/chromium/public/WebDeviceMotionListener.h:44: virtual void > > setListener(WebDeviceMotionListener*) = 0; > > On 2013/05/23 06:41:56, darin wrote: > > > This also seems wrong. The point of WebDeviceMotionListener is to provide > the > > > Platform implementation with a way to feed new WebDeviceMotionData objects > to > > > Blink. It doesn't make sense for the Platform layer to call back into Blink > > to > > > cause Blink to redirect WebDeviceMotionData objects elsewhere. The Platform > > > layer should just do that directly, avoiding the call to > didChangeDeviceMotion > > > entirely. > > > > > > Platform says: "Hey, that thing I just gave you, actually, I take it back, > > > please send it elsewhere." > > > Blink says: "Wait, if you don't want me to have it, then why did you give it > > to > > > me in the first place?" > > > > I probably misunderstood one of your previous comments regarding 'chaining' of > > listeners. > > I think you can construct the chain on the Chromium side behind the Platform > interface such that Blink does not need to know that chaining is happening. > Does that make sense? So in my view the problem with chaining is that for testing I wanted to inject an object into platform, which was of type WebDeviceMotionListener and which would then need a setListener method for chaining. Anyway that was my reasoning. As it stands now I think for testing it would be event better to have a Platform::SetDeviceMotionDataForTesting(WebDeviceMotionData&) which would then create a mock pump chromium-side and for example fire that same preset data. That way nothing extra needs to be exposed in chromium/public. Is that approximately what you mean by chaining without blink knowing it? Hope that makes sense.. > > > > Apologies if that is the case. > > Let's address the testing in a separate CL if you don't mind. I've removed the > > test-related code from this CL for now. > > > > In general it looks like there is no very proper way of platform-based testing > > in this case since TestRunner uses the 'real' platform > > renderer_webkitsupport_impl, while ideally it should be a mock.
On 2013/05/23 14:57:56, timvolodine wrote: > On 2013/05/23 14:36:49, darin wrote: > > On 2013/05/23 11:39:26, timvolodine wrote: > > > > > > https://codereview.chromium.org/14460010/diff/102001/Source/Platform/chromium... > > > File Source/Platform/chromium/public/WebDeviceMotionListener.h (right): > > > > > > > > > https://codereview.chromium.org/14460010/diff/102001/Source/Platform/chromium... > > > Source/Platform/chromium/public/WebDeviceMotionListener.h:44: virtual void > > > setListener(WebDeviceMotionListener*) = 0; > > > On 2013/05/23 06:41:56, darin wrote: > > > > This also seems wrong. The point of WebDeviceMotionListener is to provide > > the > > > > Platform implementation with a way to feed new WebDeviceMotionData objects > > to > > > > Blink. It doesn't make sense for the Platform layer to call back into > Blink > > > to > > > > cause Blink to redirect WebDeviceMotionData objects elsewhere. The > Platform > > > > layer should just do that directly, avoiding the call to > > didChangeDeviceMotion > > > > entirely. > > > > > > > > Platform says: "Hey, that thing I just gave you, actually, I take it back, > > > > please send it elsewhere." > > > > Blink says: "Wait, if you don't want me to have it, then why did you give > it > > > to > > > > me in the first place?" > > > > > > I probably misunderstood one of your previous comments regarding 'chaining' > of > > > listeners. > > > > I think you can construct the chain on the Chromium side behind the Platform > > interface such that Blink does not need to know that chaining is happening. > > Does that make sense? > > So in my view the problem with chaining is that for testing I wanted to inject > an object into platform, which was of type WebDeviceMotionListener and which > would then need a setListener method for chaining. Anyway that was my reasoning. > > As it stands now I think for testing it would be event better to have a > Platform::SetDeviceMotionDataForTesting(WebDeviceMotionData&) which would then > create a mock pump chromium-side and for example fire that same preset data. > That way nothing extra needs to be exposed in chromium/public. > > Is that approximately what you mean by chaining without blink knowing it? Hope > that makes sense.. The Platform interface exists to support Blink's needs, not the TestRunner's needs. Perhaps you should add a method like that to WebTestDelegate? That would then put you inside content/shell/renderer/webkit_test_runner.cc, which would have access to content/public/renderer/ APIs. We could expose a method there to allow an override of the implementation of setDeviceMotionListener (i.e., the code in renderer_webkitplatformsupport_impl.cc could respond to that override). -Darin > > > > > > > > Apologies if that is the case. > > > Let's address the testing in a separate CL if you don't mind. I've removed > the > > > test-related code from this CL for now. > > > > > > In general it looks like there is no very proper way of platform-based > testing > > > in this case since TestRunner uses the 'real' platform > > > renderer_webkitsupport_impl, while ideally it should be a mock.
On 2013/05/23 18:31:12, darin wrote: > On 2013/05/23 14:57:56, timvolodine wrote: > > On 2013/05/23 14:36:49, darin wrote: > > > On 2013/05/23 11:39:26, timvolodine wrote: > > > > > > > > > > https://codereview.chromium.org/14460010/diff/102001/Source/Platform/chromium... > > > > File Source/Platform/chromium/public/WebDeviceMotionListener.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/14460010/diff/102001/Source/Platform/chromium... > > > > Source/Platform/chromium/public/WebDeviceMotionListener.h:44: virtual void > > > > setListener(WebDeviceMotionListener*) = 0; > > > > On 2013/05/23 06:41:56, darin wrote: > > > > > This also seems wrong. The point of WebDeviceMotionListener is to > provide > > > the > > > > > Platform implementation with a way to feed new WebDeviceMotionData > objects > > > to > > > > > Blink. It doesn't make sense for the Platform layer to call back into > > Blink > > > > to > > > > > cause Blink to redirect WebDeviceMotionData objects elsewhere. The > > Platform > > > > > layer should just do that directly, avoiding the call to > > > didChangeDeviceMotion > > > > > entirely. > > > > > > > > > > Platform says: "Hey, that thing I just gave you, actually, I take it > back, > > > > > please send it elsewhere." > > > > > Blink says: "Wait, if you don't want me to have it, then why did you > give > > it > > > > to > > > > > me in the first place?" > > > > > > > > I probably misunderstood one of your previous comments regarding > 'chaining' > > of > > > > listeners. > > > > > > I think you can construct the chain on the Chromium side behind the Platform > > > interface such that Blink does not need to know that chaining is happening. > > > Does that make sense? > > > > So in my view the problem with chaining is that for testing I wanted to inject > > an object into platform, which was of type WebDeviceMotionListener and which > > would then need a setListener method for chaining. Anyway that was my > reasoning. > > > > As it stands now I think for testing it would be event better to have a > > Platform::SetDeviceMotionDataForTesting(WebDeviceMotionData&) which would then > > create a mock pump chromium-side and for example fire that same preset data. > > That way nothing extra needs to be exposed in chromium/public. > > > > Is that approximately what you mean by chaining without blink knowing it? Hope > > that makes sense.. > > The Platform interface exists to support Blink's needs, not the TestRunner's > needs. > Perhaps you should add a method like that to WebTestDelegate? That would then > put you inside content/shell/renderer/webkit_test_runner.cc, which would have > access to content/public/renderer/ APIs. We could expose a method there to > allow > an override of the implementation of setDeviceMotionListener (i.e., the code in > renderer_webkitplatformsupport_impl.cc could respond to that override). > > -Darin > We discussed some of this with Jochen already where he pointed at the ContentRendererClient, I guess that would be roughly equivalent with the gamepad-like approach. I'll have this covered in a follow-up CL. In the meantime, could we have an lgtm for this CL? That way I could build further on checked-in code.
https://codereview.chromium.org/14460010/diff/108001/Source/Platform/chromium... File Source/Platform/chromium/public/Platform.h (right): https://codereview.chromium.org/14460010/diff/108001/Source/Platform/chromium... Source/Platform/chromium/public/Platform.h:514: // Device Motion / Orientation ---------------------------------------- nit: convention in this file is to have two new lines above section comments https://codereview.chromium.org/14460010/diff/108001/Source/Platform/chromium... File Source/Platform/chromium/public/WebDeviceMotionListener.h (right): https://codereview.chromium.org/14460010/diff/108001/Source/Platform/chromium... Source/Platform/chromium/public/WebDeviceMotionListener.h:32: #define WebDeviceMotionListener_h You might want to tweak the --similarity argument since git-cl seems to incorrectly regard this new file as a derivative of WebMediaSource.h https://codereview.chromium.org/14460010/diff/108001/Source/WebCore/WebCore.g... File Source/WebCore/WebCore.gyp/idl_files_list.tmp (right): https://codereview.chromium.org/14460010/diff/108001/Source/WebCore/WebCore.g... Source/WebCore/WebCore.gyp/idl_files_list.tmp:2: ../Modules/battery/NavigatorBattery.idl did you really mean for this file to be part of the CL? https://codereview.chromium.org/14460010/diff/108001/Source/core/platform/chr... File Source/core/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/14460010/diff/108001/Source/core/platform/chr... Source/core/platform/chromium/support/WebDeviceMotionData.cpp:32: #include <public/WebDeviceMotionData.h> ditto: you probably didn't intend for this to be a derivative of UniqueIdentifier.h https://codereview.chromium.org/14460010/diff/108001/Source/modules/device_or... File Source/modules/device_orientation/DeviceMotionData.cpp (right): https://codereview.chromium.org/14460010/diff/108001/Source/modules/device_or... Source/modules/device_orientation/DeviceMotionData.cpp:28: #include "Platform/chromium/public/WebDeviceMotionData.h" I think the convention is to write this as #include <public/WebDeviceMotionData.h> https://codereview.chromium.org/14460010/diff/108001/Source/modules/device_or... File Source/modules/device_orientation/DeviceMotionDispatcher.cpp (right): https://codereview.chromium.org/14460010/diff/108001/Source/modules/device_or... Source/modules/device_orientation/DeviceMotionDispatcher.cpp:43: DeviceMotionDispatcher& DeviceMotionDispatcher::shared() nit: "shared()" seems a little awkward for this method name. it seems like you want to use a noun (like "instance"?) here. i'm not sure what the convention in Blink is if any for methods like this that return a shared instance. sharedInstance() ? https://codereview.chromium.org/14460010/diff/108001/Source/modules/device_or... Source/modules/device_orientation/DeviceMotionDispatcher.cpp:57: void DeviceMotionDispatcher::addController(DeviceMotionController* controller) just an observation: you could easily make DeviceMotionController also implement WebDeviceMotionListener, and then DeviceMotionDispatcher could just support adding and removing listeners. i'm not sure if that level of abstraction is valuable.
https://codereview.chromium.org/14460010/diff/108001/Source/Platform/chromium... File Source/Platform/chromium/public/Platform.h (right): https://codereview.chromium.org/14460010/diff/108001/Source/Platform/chromium... Source/Platform/chromium/public/Platform.h:514: // Device Motion / Orientation ---------------------------------------- On 2013/05/23 19:08:09, darin wrote: > nit: convention in this file is to have two new lines above section comments Done. https://codereview.chromium.org/14460010/diff/108001/Source/Platform/chromium... File Source/Platform/chromium/public/WebDeviceMotionListener.h (right): https://codereview.chromium.org/14460010/diff/108001/Source/Platform/chromium... Source/Platform/chromium/public/WebDeviceMotionListener.h:32: #define WebDeviceMotionListener_h On 2013/05/23 19:08:09, darin wrote: > You might want to tweak the --similarity argument since git-cl seems to > incorrectly regard this new file as a derivative of WebMediaSource.h hmm I am already using --similarity=80 for this CL.. Done. https://codereview.chromium.org/14460010/diff/108001/Source/WebCore/WebCore.g... File Source/WebCore/WebCore.gyp/idl_files_list.tmp (right): https://codereview.chromium.org/14460010/diff/108001/Source/WebCore/WebCore.g... Source/WebCore/WebCore.gyp/idl_files_list.tmp:2: ../Modules/battery/NavigatorBattery.idl On 2013/05/23 19:08:09, darin wrote: > did you really mean for this file to be part of the CL? oops that one has squeezed in here somehow. Done. https://codereview.chromium.org/14460010/diff/108001/Source/core/platform/chr... File Source/core/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/14460010/diff/108001/Source/core/platform/chr... Source/core/platform/chromium/support/WebDeviceMotionData.cpp:32: #include <public/WebDeviceMotionData.h> On 2013/05/23 19:08:09, darin wrote: > ditto: you probably didn't intend for this to be a derivative of > UniqueIdentifier.h using similarity=80, will try higher values, but this seems a problem with rietveld in general. https://codereview.chromium.org/14460010/diff/108001/Source/modules/device_or... File Source/modules/device_orientation/DeviceMotionData.cpp (right): https://codereview.chromium.org/14460010/diff/108001/Source/modules/device_or... Source/modules/device_orientation/DeviceMotionData.cpp:28: #include "Platform/chromium/public/WebDeviceMotionData.h" On 2013/05/23 19:08:09, darin wrote: > I think the convention is to write this as #include > <public/WebDeviceMotionData.h> Done. https://codereview.chromium.org/14460010/diff/108001/Source/modules/device_or... File Source/modules/device_orientation/DeviceMotionDispatcher.cpp (right): https://codereview.chromium.org/14460010/diff/108001/Source/modules/device_or... Source/modules/device_orientation/DeviceMotionDispatcher.cpp:43: DeviceMotionDispatcher& DeviceMotionDispatcher::shared() On 2013/05/23 19:08:09, darin wrote: > nit: "shared()" seems a little awkward for this method name. it seems like you > want to use a noun (like "instance"?) here. i'm not sure what the convention in > Blink is if any for methods like this that return a shared instance. > > sharedInstance() ? it is a former convention in WebKit, static methods like this should be called shared(). https://codereview.chromium.org/14460010/diff/108001/Source/modules/device_or... Source/modules/device_orientation/DeviceMotionDispatcher.cpp:57: void DeviceMotionDispatcher::addController(DeviceMotionController* controller) On 2013/05/23 19:08:09, darin wrote: > just an observation: > > you could easily make DeviceMotionController also implement > WebDeviceMotionListener, and then DeviceMotionDispatcher could just support > adding and removing listeners. i'm not sure if that level of abstraction is > valuable. note taken, doesn't seem to add much functionality though, can explore in a follow-up CL.
On 2013/05/23 19:30:30, timvolodine wrote: ... > https://codereview.chromium.org/14460010/diff/108001/Source/modules/device_or... > Source/modules/device_orientation/DeviceMotionDispatcher.cpp:43: > DeviceMotionDispatcher& DeviceMotionDispatcher::shared() > On 2013/05/23 19:08:09, darin wrote: > > nit: "shared()" seems a little awkward for this method name. it seems like > you > > want to use a noun (like "instance"?) here. i'm not sure what the convention > in > > Blink is if any for methods like this that return a shared instance. > > > > sharedInstance() ? > > it is a former convention in WebKit, static methods like this should be called > shared(). Former convention? I found one example of "shared()" in CrossOriginPreflightResultCache.h, but I couldn't find any others in the codebase.
On 2013/05/23 19:52:53, darin wrote: > On 2013/05/23 19:30:30, timvolodine wrote: > ... > > > https://codereview.chromium.org/14460010/diff/108001/Source/modules/device_or... > > Source/modules/device_orientation/DeviceMotionDispatcher.cpp:43: > > DeviceMotionDispatcher& DeviceMotionDispatcher::shared() > > On 2013/05/23 19:08:09, darin wrote: > > > nit: "shared()" seems a little awkward for this method name. it seems like > > you > > > want to use a noun (like "instance"?) here. i'm not sure what the > convention > > in > > > Blink is if any for methods like this that return a shared instance. > > > > > > sharedInstance() ? > > > > it is a former convention in WebKit, static methods like this should be called > > shared(). > > Former convention? I found one example of "shared()" in > CrossOriginPreflightResultCache.h, but I couldn't find any others in the > codebase. There are actually more like HTMLParserThread, PageScriptDebugServer.cpp, see https://code.google.com/p/chromium/codesearch#search/&q=%22::shared()%22&sq=p... I remember getting this comment to use "shared()" from a webkit reviewer, also Adam did not complain.
ping :) ?
On 2013/05/23 20:12:51, timvolodine wrote: > I remember getting this comment to use "shared()" from a webkit reviewer, also > Adam did not complain. We don't have that established a name to use for these cases.
On 2013/05/24 16:55:25, abarth wrote: > On 2013/05/23 20:12:51, timvolodine wrote: > > I remember getting this comment to use "shared()" from a webkit reviewer, also > > Adam did not complain. > > We don't have that established a name to use for these cases. is there any preference? should I change it to ::instance() e.g. ?
https://codereview.chromium.org/14460010/diff/123001/Source/modules/device_or... File Source/modules/device_orientation/DeviceMotionDispatcher.h (right): https://codereview.chromium.org/14460010/diff/123001/Source/modules/device_or... Source/modules/device_orientation/DeviceMotionDispatcher.h:52: static DeviceMotionDispatcher& shared(); instance() is fine.
https://codereview.chromium.org/14460010/diff/123001/Source/modules/device_or... File Source/modules/device_orientation/DeviceMotionDispatcher.h (right): https://codereview.chromium.org/14460010/diff/123001/Source/modules/device_or... Source/modules/device_orientation/DeviceMotionDispatcher.h:52: static DeviceMotionDispatcher& shared(); On 2013/05/24 17:23:27, abarth wrote: > instance() is fine. Done.
LGTM. Some minor nits above. Thanks for iterating on this CL. I think we ended up in a good place with it. https://codereview.chromium.org/14460010/diff/134001/Source/Platform/chromium... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/14460010/diff/134001/Source/Platform/chromium... Source/Platform/chromium/public/WebDeviceMotionData.h:34: #include "../../../Platform/chromium/public/WebCommon.h" This can just #include "WebCommon.h" Please put this header in /public/platform/WebDeviceMotionData.h and add a forwarding header (similar to the rest of the headers in this directory). https://codereview.chromium.org/14460010/diff/134001/Source/core/platform/chr... File Source/core/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/14460010/diff/134001/Source/core/platform/chr... Source/core/platform/chromium/support/WebDeviceMotionData.cpp:38: memset(this, 0, sizeof(*this)); I probably would have initialized the members normally instead of spamming over the object with zerps. https://codereview.chromium.org/14460010/diff/134001/Source/modules/device_or... File Source/modules/device_orientation/DeviceMotionController.cpp (right): https://codereview.chromium.org/14460010/diff/134001/Source/modules/device_or... Source/modules/device_orientation/DeviceMotionController.cpp:75: void DeviceMotionController::dispatchDeviceEvent(const PassRefPtr<Event> prpEvent) No need for const here. https://codereview.chromium.org/14460010/diff/134001/Source/modules/device_or... File Source/modules/device_orientation/DeviceMotionDispatcher.cpp (right): https://codereview.chromium.org/14460010/diff/134001/Source/modules/device_or... Source/modules/device_orientation/DeviceMotionDispatcher.cpp:42: // static We usually skip these comments since they're not enforced by the compiler.
Thanks Adam and Darin for reviewing this patch! Think it looks pretty well indeed :). Hope Adam's lgtm covers all the owners, I'll come back if not. https://codereview.chromium.org/14460010/diff/134001/Source/Platform/chromium... File Source/Platform/chromium/public/WebDeviceMotionData.h (right): https://codereview.chromium.org/14460010/diff/134001/Source/Platform/chromium... Source/Platform/chromium/public/WebDeviceMotionData.h:34: #include "../../../Platform/chromium/public/WebCommon.h" On 2013/05/24 18:26:30, abarth wrote: > This can just #include "WebCommon.h" > > Please put this header in /public/platform/WebDeviceMotionData.h and add a > forwarding header (similar to the rest of the headers in this directory). Done. https://codereview.chromium.org/14460010/diff/134001/Source/core/platform/chr... File Source/core/platform/chromium/support/WebDeviceMotionData.cpp (right): https://codereview.chromium.org/14460010/diff/134001/Source/core/platform/chr... Source/core/platform/chromium/support/WebDeviceMotionData.cpp:38: memset(this, 0, sizeof(*this)); On 2013/05/24 18:26:30, abarth wrote: > I probably would have initialized the members normally instead of spamming over > the object with zerps. Darin suggested to do this so, it works, it's a POD. https://codereview.chromium.org/14460010/diff/134001/Source/modules/device_or... File Source/modules/device_orientation/DeviceMotionController.cpp (right): https://codereview.chromium.org/14460010/diff/134001/Source/modules/device_or... Source/modules/device_orientation/DeviceMotionController.cpp:75: void DeviceMotionController::dispatchDeviceEvent(const PassRefPtr<Event> prpEvent) On 2013/05/24 18:26:30, abarth wrote: > No need for const here. Done. https://codereview.chromium.org/14460010/diff/134001/Source/modules/device_or... File Source/modules/device_orientation/DeviceMotionDispatcher.cpp (right): https://codereview.chromium.org/14460010/diff/134001/Source/modules/device_or... Source/modules/device_orientation/DeviceMotionDispatcher.cpp:42: // static On 2013/05/24 18:26:30, abarth wrote: > We usually skip these comments since they're not enforced by the compiler. Done.
On 2013/05/24 19:12:41, timvolodine wrote: > Darin suggested to do this so, it works, it's a POD. ok
On 2013/05/24 18:26:29, abarth wrote: ... > https://codereview.chromium.org/14460010/diff/134001/Source/core/platform/chr... > File Source/core/platform/chromium/support/WebDeviceMotionData.cpp (right): > > https://codereview.chromium.org/14460010/diff/134001/Source/core/platform/chr... > Source/core/platform/chromium/support/WebDeviceMotionData.cpp:38: memset(this, > 0, sizeof(*this)); > I probably would have initialized the members normally instead of spamming over > the object with zerps. It is really important to zero out the memory so that we don't have some bits left uninitialized. This data is going to be memcpy'd around over IPC and such. It needs to be completely initialized. I think a comment is warranted since this is non-obvious. -Darin
On 2013/05/24 21:54:00, darin wrote: > On 2013/05/24 18:26:29, abarth wrote: > ... > > > https://codereview.chromium.org/14460010/diff/134001/Source/core/platform/chr... > > File Source/core/platform/chromium/support/WebDeviceMotionData.cpp (right): > > > > > https://codereview.chromium.org/14460010/diff/134001/Source/core/platform/chr... > > Source/core/platform/chromium/support/WebDeviceMotionData.cpp:38: memset(this, > > 0, sizeof(*this)); > > I probably would have initialized the members normally instead of spamming > over > > the object with zerps. > > It is really important to zero out the memory so that we don't have some bits > left uninitialized. This data is going to be memcpy'd around over IPC and such. > It needs to be completely initialized. I think a comment is warranted since > this is non-obvious. > > -Darin Valgrind will complain if we copy around memory that is only partially initialized.
On 2013/05/24 21:54:00, darin wrote: > On 2013/05/24 18:26:29, abarth wrote: > ... > > > https://codereview.chromium.org/14460010/diff/134001/Source/core/platform/chr... > > File Source/core/platform/chromium/support/WebDeviceMotionData.cpp (right): > > > > > https://codereview.chromium.org/14460010/diff/134001/Source/core/platform/chr... > > Source/core/platform/chromium/support/WebDeviceMotionData.cpp:38: memset(this, > > 0, sizeof(*this)); > > I probably would have initialized the members normally instead of spamming > over > > the object with zerps. > > It is really important to zero out the memory so that we don't have some bits > left uninitialized. This data is going to be memcpy'd around over IPC and such. > It needs to be completely initialized. I think a comment is warranted since > this is non-obvious. > > -Darin done. (added comment)
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/14460010/149001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_layout_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/14460010/156002
Retried try job too often on win_layout_rel for step(s) webkit_lint, webkit_tests, webkit_unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/14460010/166003
Retried try job too often on win_layout_rel for step(s) webkit_lint, webkit_tests, webkit_unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
On 2013/05/30 16:24:04, I haz the power (commit-bot) wrote: > Retried try job too often on win_layout_rel for step(s) webkit_lint, > webkit_tests, webkit_unit_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout... Adam, maybe you know why this happens only on windows build ? Exception: Missing input files: E:\b\build\slave\win_layout\build\src\third_party\WebKit\Source\WebKit\chromium\public\WebDeviceMotionData.h E:\b\build\slave\win_layout\build\src\third_party\WebKit\Source\WebKit\chromium\public\WebDeviceMotionListener.h Error: Command E:\b\depot_tools\python_bin\python.exe src/build/gyp_chromium returned non-zero exit status 1 in E:\b\build\slave\win_layout\build program finished with exit code 2 elapsedTime=108.055000
On 2013/05/30 16:34:18, timvolodine wrote: > On 2013/05/30 16:24:04, I haz the power (commit-bot) wrote: > > Retried try job too often on win_layout_rel for step(s) webkit_lint, > > webkit_tests, webkit_unit_tests > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout... > > Adam, maybe you know why this happens only on windows build ? > > Exception: Missing input files: > E:\b\build\slave\win_layout\build\src\third_party\WebKit\Source\WebKit\chromium\public\WebDeviceMotionData.h > E:\b\build\slave\win_layout\build\src\third_party\WebKit\Source\WebKit\chromium\public\WebDeviceMotionListener.h > Error: Command E:\b\depot_tools\python_bin\python.exe src/build/gyp_chromium > returned non-zero exit status 1 in E:\b\build\slave\win_layout\build > program finished with exit code 2 > elapsedTime=108.055000 You've listed these files in GYP but they don't exist. They're located in third_party\WebKit\Source\Platform\chromium\public not third_party\WebKit\Source\WebKit\chromium\public.
On 2013/05/30 17:29:24, abarth wrote: > On 2013/05/30 16:34:18, timvolodine wrote: > > On 2013/05/30 16:24:04, I haz the power (commit-bot) wrote: > > > Retried try job too often on win_layout_rel for step(s) webkit_lint, > > > webkit_tests, webkit_unit_tests > > > > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout... > > > > Adam, maybe you know why this happens only on windows build ? > > > > Exception: Missing input files: > > > E:\b\build\slave\win_layout\build\src\third_party\WebKit\Source\WebKit\chromium\public\WebDeviceMotionData.h > > > E:\b\build\slave\win_layout\build\src\third_party\WebKit\Source\WebKit\chromium\public\WebDeviceMotionListener.h > > Error: Command E:\b\depot_tools\python_bin\python.exe src/build/gyp_chromium > > returned non-zero exit status 1 in E:\b\build\slave\win_layout\build > > program finished with exit code 2 > > elapsedTime=108.055000 > > You've listed these files in GYP but they don't exist. They're located in > third_party\WebKit\Source\Platform\chromium\public not > third_party\WebKit\Source\WebKit\chromium\public. yep indeed, thanks for that :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/14460010/156004
Message was sent while issue was closed.
Change committed as 151544 |
