Bug 89197

Created attachment 147796 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Attachment 147796 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Tools/DumpRenderTree/chromium/LayoutTestController.cpp:44: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 54 files If any of these errors are false positives, please file a bug against check-webkit-style.

I tried several different orderings of the included files and none passed the style check. In the current ordering, the two files for DeviceMotion are added in the same order as the two corresponding DeviceOrientation files. Switching the ordering of those two files and the two DeviceOrientation files still produces style errors. Using Unix sort on the whole list of included files also produces style errors. Perhaps this is a bug with the style check?

Comment on attachment 147796 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147796&action=review > Source/WebCore/ChangeLog:3 > + [Chromium] Implemented DeviceMotion s/Implemented/Implement/ I wonder if we could break this patch up into smaller peaces. Perhaps we could do the mock object separately? But I guess we need all the wrapper stuff to actually use it, though :/ > Source/WebCore/platform/mock/DeviceMotionClientMock.h:48 > + virtual void setController(DeviceMotionController*); a good idea when adding new code is to add the OVERRIDE specifier for virtual functions that are supposed to override functions in the base class. That way, the compiler will give an error if the base class function changes and the function doesn't override anymore. > Source/WebKit/chromium/public/WebDeviceMotion.h:23 > + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. this copyright blurb is slightly different than the previous ones in the patch. we should figure out which one is the best one and use the same throughout the patch > Source/WebKit/chromium/public/WebDeviceMotion.h:36 > +class WebDeviceMotion { why not WebDeviceMotionData? > Source/WebKit/chromium/public/WebDeviceMotion.h:66 > + bool isNull() { return m_isNull; } this, and all other getters in the function should be const-qualified > Source/WebKit/chromium/public/WebDeviceMotion.h:98 > + { this function could be on a single line; this goes for the other one-statement functions below as well. WebKit doesn't have an 80-col limit. > Source/WebKit/chromium/public/WebDeviceMotion.h:167 > + WebDeviceMotion(const WTF::PassRefPtr<WebCore::DeviceMotionData>&); i wonder if this should be just WebDeviceMotion(const WebCore::DeviceMotionData&); > Source/WebKit/chromium/public/WebDeviceMotion.h:168 > + WebDeviceMotion& operator=(const WTF::PassRefPtr<WebCore::DeviceMotionData>&); ditto. const-ref to a PassRef seems unusual; i don't remember if there's a good reason for it > Source/WebKit/chromium/public/WebDeviceMotionClient.h:38 > + virtual void setController(WebDeviceMotionController*) = 0; for interfaces which will be implemented by chromium-side classes, we don't use pure virtual functions anymore, they should be defined like this: virtual void setController(WebDeviceMotionController*) { WEBKIT_ASSERT_NOT_REACHED(); } This makes it easier to make changes to the classes in the future, without getting compile errors about pure virtuals not being implemented. > Source/WebKit/chromium/public/WebDeviceMotionClientMock.h:42 > + virtual void setController(WebDeviceMotionController*); these could be marked OVERRIDE > Source/WebKit/chromium/public/WebViewClient.h:332 > + virtual WebDeviceMotionClient* deviceMotionClient() { return 0; } we could probably put this in the Device Orientation section below, since they're part of the same API > Source/WebKit/chromium/src/DeviceMotionClientProxy.h:47 > + void setController(WebCore::DeviceMotionController*); since these are overriding DeviceMotionClient, they should be marked "virtual" and "OVERRIDE" > Source/WebKit/chromium/src/WebDeviceMotion.cpp:80 > + const WebCore::DeviceMotionData::Acceleration* accelerationIncludingGravity = no need for linebreak > Source/WebKit/chromium/src/WebDeviceMotionController.cpp:38 > + m_controller->didChangeDeviceMotion(static_cast<PassRefPtr<WebCore::DeviceMotionData> >(motion).get()); another way of avoiding the static_cast would be RefPtr<WebCore::DeviceMotionData> deviceMotionData = PassRefPtr<WebCore::DeviceMotionData>(motion); m_controller->didChangeDeviceMotion(deviceMotionData.get()); > Source/WebKit/chromium/src/WebDeviceOrientationController.cpp:38 > + m_controller->didChangeDeviceOrientation(static_cast<PassRefPtr<WebCore::DeviceOrientation> >(orientation).get()); same here, the trick with a local RefPtr is nicer than the static_cast (sorry, I should have mentioned that earlier). Also, this one could be a separate patch. > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:45 > +#include "WebDeviceMotionClientMock.h" yeah, we looked at this and couldn't figure out any ordering that the style script would accept :/ > Tools/DumpRenderTree/chromium/LayoutTestController.h:360 > + void setMockDeviceMotion(const CppArgumentList&, CppVariant*); maybe just put it in device orientation's section below > Tools/DumpRenderTree/chromium/WebViewHost.h:190 > + virtual WebKit::WebDeviceMotionClient* deviceMotionClient(); let's mark it OVERRIDE while we're here

(For context, this is a re-take on Bug 47078.)

Created attachment 148105 [details] Patch

(In reply to comment #5) > (From update of attachment 147796 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147796&action=review > > > Source/WebCore/ChangeLog:3 > > + [Chromium] Implemented DeviceMotion > > s/Implemented/Implement/ Done. > I wonder if we could break this patch up into smaller peaces. Perhaps we could do the mock object separately? But I guess we need all the wrapper stuff to actually use it, though :/ Done. See https://bugs.webkit.org/show_bug.cgi?id=89220 for the Client Mock in WebCore, and bugs 89337 and 89339 for other small changes that can be done separately. > > Source/WebCore/platform/mock/DeviceMotionClientMock.h:48 > > + virtual void setController(DeviceMotionController*); > > a good idea when adding new code is to add the OVERRIDE specifier for virtual functions that are supposed to override functions in the base class. That way, the compiler will give an error if the base class function changes and the function doesn't override anymore. Done (see bug 89220). > > Source/WebKit/chromium/public/WebDeviceMotion.h:23 > > + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > this copyright blurb is slightly different than the previous ones in the patch. we should figure out which one is the best one and use the same throughout the patch It looks like several different copyright blurbs are commonly used within WebKit. For DeviceOrientation, one copyright blurb is always used in Source/WebCore/dom and another in Source/WebKit/chromium, so I did the same for DeviceMotion. The main difference between the blurbs is that in WebCore, it says the software is provided by "apple and its contributors" whereas in chromium it says by "the copyright holders". Is it possible that this distinction was intentional? > > Source/WebKit/chromium/public/WebDeviceMotion.h:36 > > +class WebDeviceMotion { > > why not WebDeviceMotionData? I wanted the name to be consistent with WebDeviceOrientation, but I guess it's more important for it to be consistent with the rest of the DeviceMotion implementation than with the corresponding part of the DeviceOrientation implementation. I changed it to WebDeviceMotionData. > > Source/WebKit/chromium/public/WebDeviceMotion.h:66 > > + bool isNull() { return m_isNull; } > > this, and all other getters in the function should be const-qualified Done. > > Source/WebKit/chromium/public/WebDeviceMotion.h:98 > > + { > > this function could be on a single line; this goes for the other one-statement functions below as well. WebKit doesn't have an 80-col limit. Done. > > Source/WebKit/chromium/public/WebDeviceMotion.h:167 > > + WebDeviceMotion(const WTF::PassRefPtr<WebCore::DeviceMotionData>&); > > i wonder if this should be just WebDeviceMotion(const WebCore::DeviceMotionData&); > > > Source/WebKit/chromium/public/WebDeviceMotion.h:168 > > + WebDeviceMotion& operator=(const WTF::PassRefPtr<WebCore::DeviceMotionData>&); > > ditto. const-ref to a PassRef seems unusual; i don't remember if there's a good reason for it It does seem overly-complicated in its current state. However the parameter does need to be some sort of pointer, because WebDeviceMotionData has an "isNull" field, whereas DeviceMotionData does not, and a null pointer is the easiest way to represent a null DeviceMotionData. Thus I changed it to WebDeviceMotion(const WebCore::DeviceMotionData *). > > Source/WebKit/chromium/public/WebDeviceMotionClient.h:38 > > + virtual void setController(WebDeviceMotionController*) = 0; > > for interfaces which will be implemented by chromium-side classes, we don't use pure virtual functions anymore, they should be defined like this: > > virtual void setController(WebDeviceMotionController*) { WEBKIT_ASSERT_NOT_REACHED(); } > > This makes it easier to make changes to the classes in the future, without getting compile errors about pure virtuals not being implemented. Done. > > Source/WebKit/chromium/public/WebDeviceMotionClientMock.h:42 > > + virtual void setController(WebDeviceMotionController*); > > these could be marked OVERRIDE Done. > > Source/WebKit/chromium/public/WebViewClient.h:332 > > + virtual WebDeviceMotionClient* deviceMotionClient() { return 0; } > > we could probably put this in the Device Orientation section below, since they're part of the same API Done. > > Source/WebKit/chromium/src/DeviceMotionClientProxy.h:47 > > + void setController(WebCore::DeviceMotionController*); > > since these are overriding DeviceMotionClient, they should be marked "virtual" and "OVERRIDE" Done. > > Source/WebKit/chromium/src/WebDeviceMotion.cpp:80 > > + const WebCore::DeviceMotionData::Acceleration* accelerationIncludingGravity = > > no need for linebreak Done. > > Source/WebKit/chromium/src/WebDeviceMotionController.cpp:38 > > + m_controller->didChangeDeviceMotion(static_cast<PassRefPtr<WebCore::DeviceMotionData> >(motion).get()); > > another way of avoiding the static_cast would be > > RefPtr<WebCore::DeviceMotionData> deviceMotionData = PassRefPtr<WebCore::DeviceMotionData>(motion); > m_controller->didChangeDeviceMotion(deviceMotionData.get()); > > > Source/WebKit/chromium/src/WebDeviceOrientationController.cpp:38 > > + m_controller->didChangeDeviceOrientation(static_cast<PassRefPtr<WebCore::DeviceOrientation> >(orientation).get()); > > same here, the trick with a local RefPtr is nicer than the static_cast (sorry, I should have mentioned that earlier). Also, this one could be a separate patch. Done. See bug 89337 for the change in WebDeviceOrientationController. > > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:45 > > +#include "WebDeviceMotionClientMock.h" > > yeah, we looked at this and couldn't figure out any ordering that the style script would accept :/ > > Tools/DumpRenderTree/chromium/LayoutTestController.h:360 > > + void setMockDeviceMotion(const CppArgumentList&, CppVariant*); > > maybe just put it in device orientation's section below Done. > > Tools/DumpRenderTree/chromium/WebViewHost.h:190 > > + virtual WebKit::WebDeviceMotionClient* deviceMotionClient(); > > let's mark it OVERRIDE while we're here Done.

Attachment 148105 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Tools/DumpRenderTree/chromium/LayoutTestController.cpp:44: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 53 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 148560 [details] Patch

Updated this patch now that bug 89220, bug 89337, and bug 89339 have landed.

Attachment 148560 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Tools/DumpRenderTree/chromium/LayoutTestController.cpp:44: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 148560 [details] Patch Would any of the API reviewrs like to take a look? I only have a couple of nits. View in context: https://bugs.webkit.org/attachment.cgi?id=148560&action=review > Source/WebKit/chromium/public/WebDeviceMotionData.h:150 > + WebDeviceMotionData& operator=(const WebCore::DeviceMotionData*); Hmm, I wonder if the two functions above are actually ever used or just there for "symmetry" reasons. I don't know if we ever create a WebDevicemotionData from a DeviceMotionData; i suspect we only do it the other way around. If we don't need them, maybe we should delete them because the definitions aren't too pretty. > LayoutTests/ChangeLog:9 > + Each tests corresponds to a similar existing test for DeviceOrientation, since they are part of the same spec and work very similarly. s/Each tests/Each test/ > LayoutTests/fast/dom/DeviceMotion/multiple-frames.html:6 > +<script src="script-tests/multiple-frames.js"></script> i don't know what the current WebKit policy is on this, maybe the other reviewers can comment, but if we could just put the test script in the .html file, that would be easier > LayoutTests/fast/dom/DeviceMotion/script-tests/add-listener-from-callback.js:17 > +if (window.layoutTestController) { there's an effort underway to rename layoutTestController to testRunner. Currently, both names work, but any new code should ideally use testRunner.

Created attachment 148790 [details] Patch

Attachment 148790 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Tools/DumpRenderTree/chromium/LayoutTestController.cpp:44: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.

Thanks for the comments. (In reply to comment #13) > (From update of attachment 148560 [details]) > Would any of the API reviewrs like to take a look? > > > I only have a couple of nits. > > > View in context: https://bugs.webkit.org/attachment.cgi?id=148560&action=review > > > Source/WebKit/chromium/public/WebDeviceMotionData.h:150 > > + WebDeviceMotionData& operator=(const WebCore::DeviceMotionData*); > > Hmm, I wonder if the two functions above are actually ever used or just there for "symmetry" reasons. > I don't know if we ever create a WebDevicemotionData from a DeviceMotionData; i suspect we only do it the other way around. > If we don't need them, maybe we should delete them because the definitions aren't too pretty. The first function is used by WebDeviceMotionClientMock. However, the second function is unused, so I deleted it. Done. > > LayoutTests/ChangeLog:9 > > + Each tests corresponds to a similar existing test for DeviceOrientation, since they are part of the same spec and work very similarly. > > s/Each tests/Each test/ Done. > > LayoutTests/fast/dom/DeviceMotion/multiple-frames.html:6 > > +<script src="script-tests/multiple-frames.js"></script> > > i don't know what the current WebKit policy is on this, maybe the other reviewers can comment, but if we could just put the test script in the .html file, that would be easier I couldn't find any mention of a policy regarding this. If it conforms to WebKit policy, I agree that it would be simpler. > > LayoutTests/fast/dom/DeviceMotion/script-tests/add-listener-from-callback.js:17 > > +if (window.layoutTestController) { > > there's an effort underway to rename layoutTestController to testRunner. Currently, both names work, but any new code should ideally use testRunner. Done.

Comment on attachment 148790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148790&action=review > Source/WebKit/chromium/public/WebDeviceMotionClient.h:42 > + virtual void setController(WebDeviceMotionController*) { WEBKIT_ASSERT_NOT_REACHED(); } > + virtual void startUpdating() { WEBKIT_ASSERT_NOT_REACHED(); } > + virtual void stopUpdating() { WEBKIT_ASSERT_NOT_REACHED(); } I think we typically use pure virtual functions for client interfaces. > Source/WebKit/chromium/public/WebDeviceMotionClientMock.h:37 > +class WebDeviceMotionClientMock : public WebDeviceMotionClient { Should this mock be in DumpRenderTree rather than in the production target? > Source/WebKit/chromium/public/WebDeviceMotionController.h:42 > + WebDeviceMotionController(WebCore::DeviceMotionController* c) > + : m_controller(c) > + { > + } I'm not sure I understand the role of this class. It doesn't look like it's constructible by the embedder because the embedder won't have a WebCore::DeviceMotionController*... > Source/WebKit/chromium/public/WebDeviceMotionController.h:51 > + WebCore::DeviceMotionController* m_controller; How are the lifetimes of these two objects related? This pointer looks like it could become stale quite easily. > Source/WebKit/chromium/public/WebDeviceMotionData.h:178 > + bool m_canProvideAccelerationX; > + double m_accelerationX; > + bool m_canProvideAccelerationY; > + double m_accelerationY; > + bool m_canProvideAccelerationZ; > + double m_accelerationZ; > + > + bool m_canProvideAccelerationIncludingGravityX; > + double m_accelerationIncludingGravityX; > + bool m_canProvideAccelerationIncludingGravityY; > + double m_accelerationIncludingGravityY; > + bool m_canProvideAccelerationIncludingGravityZ; > + double m_accelerationIncludingGravityZ; > + > + bool m_canProvideRotationRateAlpha; > + double m_rotationRateAlpha; > + bool m_canProvideRotationRateBeta; > + double m_rotationRateBeta; > + bool m_canProvideRotationRateGamma; > + double m_rotationRateGamma; > + > + bool m_canProvideInterval; > + double m_interval; Rather than replicating all this state, we should hold the corresponding WebCore object as a data member. > Source/WebKit/chromium/src/DeviceMotionClientProxy.cpp:46 > + m_client->setController(new WebDeviceMotionController(c)); This line doesn't look right. We generally avoid "naked news" in WebKit. Typically, we wrap calls to new in either adoptPtr or adoptRef. > Source/WebKit/chromium/src/WebDeviceMotionClientMock.cpp:37 > + return new WebDeviceMotionClientMock(); Same problem here. > Source/WebKit/chromium/src/WebDeviceMotionClientMock.cpp:43 > + delete controller; We shouldn't really use the delete operator. Instead, we should use a smart pointer that deletes things automatically. > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1876 > + if (arguments.size() < 20 || !arguments[0].isBool() || !arguments[1].isNumber() || !arguments[2].isBool() || !arguments[3].isNumber() || !arguments[4].isBool() || !arguments[5].isNumber() || !arguments[6].isBool() || !arguments[7].isNumber() || !arguments[8].isBool() || !arguments[9].isNumber() || !arguments[10].isBool() || !arguments[11].isNumber() || !arguments[12].isBool() || !arguments[13].isNumber() || !arguments[14].isBool() || !arguments[15].isNumber() || !arguments[16].isBool() || !arguments[17].isNumber() || !arguments[18].isBool() || !arguments[19].isNumber()) Woah there batman. That's kind of out of control. Is there really not a more sane way to pass arguments to this function?

Created attachment 149311 [details] Patch

Thanks for the feedback! (In reply to comment #17) > (From update of attachment 148790 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148790&action=review > > > Source/WebKit/chromium/public/WebDeviceMotionClient.h:42 > > + virtual void setController(WebDeviceMotionController*) { WEBKIT_ASSERT_NOT_REACHED(); } > > + virtual void startUpdating() { WEBKIT_ASSERT_NOT_REACHED(); } > > + virtual void stopUpdating() { WEBKIT_ASSERT_NOT_REACHED(); } > > I think we typically use pure virtual functions for client interfaces. The other web clients in WebKit/chromium/public are inconsistent - some use pure virtual functions and some don't. In comment 4 of the previous take on DeviceMotion (bug 47078) it says that webkit api's should not be pure virtual. So there's not really a consensus on this - do you think pure virtual functions are better? > > Source/WebKit/chromium/public/WebDeviceMotionClientMock.h:37 > > +class WebDeviceMotionClientMock : public WebDeviceMotionClient { > > Should this mock be in DumpRenderTree rather than in the production target? If we put the mock in DumpRenderTree instead, it would replace DeviceMotionClientMock in WebCore/platform/mock. Then we also wouldn't need the wrapper WebDeviceMotionClientMock in WebKit/chromium/public, which is nice. However, then other platforms would have to implement their own complete mocks instead of being able to use the one in WebCore/platform/mock (with their own wrappers). So it's a trade-off. Which do you prefer? > > Source/WebKit/chromium/public/WebDeviceMotionController.h:42 > > + WebDeviceMotionController(WebCore::DeviceMotionController* c) > > + : m_controller(c) > > + { > > + } > > I'm not sure I understand the role of this class. It doesn't look like it's constructible by the embedder because the embedder won't have a WebCore::DeviceMotionController*... The DeviceMotionController is constructed with a pointer to the (already constructed) DeviceMotionClientProxy. During construction, the DeviceMotionController instructs the DeviceMotionClientProxy to construct the WebDeviceMotionController from itself. This allows the WebDeviceMotionController to maintain a pointer to the DeviceMotionController. Then, when the device motion is updated, the new motion data can be passed from a WebDeviceMotionClient to the WebDeviceMotionController to the DeviceMotionController (where the DeviceMotionEvent is created). > > Source/WebKit/chromium/public/WebDeviceMotionController.h:51 > > + WebCore::DeviceMotionController* m_controller; > > How are the lifetimes of these two objects related? This pointer looks like it could become stale quite easily. The lifetime of the DeviceMotionController is controlled by the Page (the lifetime of which is controlled by the WebViewImpl). The lifetime of the WebDeviceMotionController is controlled by the WebDeviceMotionClient. So I guess it is the client's responsibility to make sure that the WebDeviceMotionController isn't deleted before the DeviceMotionController. > > Source/WebKit/chromium/public/WebDeviceMotionData.h:178 > > + bool m_canProvideAccelerationX; > > + double m_accelerationX; > > + bool m_canProvideAccelerationY; > > + double m_accelerationY; > > + bool m_canProvideAccelerationZ; > > + double m_accelerationZ; > > + > > + bool m_canProvideAccelerationIncludingGravityX; > > + double m_accelerationIncludingGravityX; > > + bool m_canProvideAccelerationIncludingGravityY; > > + double m_accelerationIncludingGravityY; > > + bool m_canProvideAccelerationIncludingGravityZ; > > + double m_accelerationIncludingGravityZ; > > + > > + bool m_canProvideRotationRateAlpha; > > + double m_rotationRateAlpha; > > + bool m_canProvideRotationRateBeta; > > + double m_rotationRateBeta; > > + bool m_canProvideRotationRateGamma; > > + double m_rotationRateGamma; > > + > > + bool m_canProvideInterval; > > + double m_interval; > > Rather than replicating all this state, we should hold the corresponding WebCore object as a data member. In comment 11 in the previous take on DeviceMotion (bug 47078), it was suggested that this should be a simple class with member variables rather than a wrapper around the WebCore type. What's the consensus on this? > > Source/WebKit/chromium/src/DeviceMotionClientProxy.cpp:46 > > + m_client->setController(new WebDeviceMotionController(c)); > > This line doesn't look right. We generally avoid "naked news" in WebKit. Typically, we wrap calls to new in either adoptPtr or adoptRef. Done - fixed as a consequence of removing the "delete", as discussed below. > > Source/WebKit/chromium/src/WebDeviceMotionClientMock.cpp:37 > > + return new WebDeviceMotionClientMock(); > > Same problem here. In this case the raw pointer returned by the "naked new" is returned by a function in a public API (in WebDeviceMotionClient.h), and the public API shouldn't return an OwnPtr or a RefPtr. > > Source/WebKit/chromium/src/WebDeviceMotionClientMock.cpp:43 > > + delete controller; > > We shouldn't really use the delete operator. Instead, we should use a smart pointer that deletes things automatically. WebDeviceMotionController is in the public API and shouldn't be RefCounted, so we can't use a smart pointer. However, to avoid having to explicitly delete the controller, I changed DeviceMotionClientProxy so that it creates the WebDeviceMotionController on the stack and passes a reference to it as an argument to setController. This also fixes one instance of the "naked new" problem discussed above. > > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1876 > > + if (arguments.size() < 20 || !arguments[0].isBool() || !arguments[1].isNumber() || !arguments[2].isBool() || !arguments[3].isNumber() || !arguments[4].isBool() || !arguments[5].isNumber() || !arguments[6].isBool() || !arguments[7].isNumber() || !arguments[8].isBool() || !arguments[9].isNumber() || !arguments[10].isBool() || !arguments[11].isNumber() || !arguments[12].isBool() || !arguments[13].isNumber() || !arguments[14].isBool() || !arguments[15].isNumber() || !arguments[16].isBool() || !arguments[17].isNumber() || !arguments[18].isBool() || !arguments[19].isNumber()) > > Woah there batman. That's kind of out of control. Is there really not a more sane way to pass arguments to this function? One alternative is to change setMockDeviceMotion so that it sets one property of the motion at a time. For example: testRunner.setMockDeviceMotion("acceleration_x", 22.3). Then WebDeviceMotionClientMock would have a similar setMotion function, which would then call setMotion on the DeviceMotion, with the same arguments. Then setMotion in DeviceMotionClientMock would finally update the appropriate property of the DeviceMotionData. The javascript tests would call the setMockDeviceMotion function once for each property of the motion that they wanted to set. This would replace each call to the large function with many calls to the smaller function - do you think that would be better?

Attachment 149311 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Tools/DumpRenderTree/chromium/LayoutTestController.cpp:44: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.

@fishd: Would you be willing to review this patch? There's a large amount of WebKit API in the patch and I think Amy would benefit from some reviewer continuity from DeviceOrientation.

@fishd: could you take a look at this? Thanks!

I will do a more thorough review shortly, but just a comment on the design. I feel like the design for "device orientation" may not have been done correctly. Put simply, things that are properties of the device instead of properties of the WebView should be accessed through the platform instead through the WebView. Is device orientation really something WebView-specific? Maybe it is possible to have two monitors that are oriented differently? Hmm...

(In reply to comment #23) > I will do a more thorough review shortly, but just a comment on the design. I feel like the design for "device orientation" may not have been done correctly. Put simply, things that are properties of the device instead of properties of the WebView should be accessed through the platform instead through the WebView. Is device orientation really something WebView-specific? Maybe it is possible to have two monitors that are oriented differently? Hmm... Geolocation was originally accessed through the platform, but was eventually switched to the WebView, because the embedder needed to have close control over it (to handle permissions, to suspend it to save power, to control the polling frequency, etc.). It makes sense for DeviceOrientation and DeviceMotion to be implemented similarly, so that the embedder can have close control over them. This e-mail thread (https://lists.webkit.org/pipermail/webkit-dev/2010-December/015313.html) discusses the pros and cons of switching Geolocation from the platform-based approach to the client-based approach.

(In reply to comment #24) > (In reply to comment #23) > > I will do a more thorough review shortly, but just a comment on the design. I feel like the design for "device orientation" may not have been done correctly. Put simply, things that are properties of the device instead of properties of the WebView should be accessed through the platform instead through the WebView. Is device orientation really something WebView-specific? Maybe it is possible to have two monitors that are oriented differently? Hmm... > > Geolocation was originally accessed through the platform, but was eventually switched to the WebView, because the embedder needed to have close control over it (to handle permissions, to suspend it to save power, to control the polling frequency, etc.). It makes sense for DeviceOrientation and DeviceMotion to be implemented similarly, so that the embedder can have close control over them. > > This e-mail thread (https://lists.webkit.org/pipermail/webkit-dev/2010-December/015313.html) discusses the pros and cons of switching Geolocation from the platform-based approach to the client-based approach. Our standard design approach is to have policy APIs at the WebView layer combined with functional APIs at the platform layer. I think Geolocation would likely benefit from this separation of concerns too.

(In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #23) > > > I will do a more thorough review shortly, but just a comment on the design. I feel like the design for "device orientation" may not have been done correctly. Put simply, things that are properties of the device instead of properties of the WebView should be accessed through the platform instead through the WebView. Is device orientation really something WebView-specific? Maybe it is possible to have two monitors that are oriented differently? Hmm... > > > > Geolocation was originally accessed through the platform, but was eventually switched to the WebView, because the embedder needed to have close control over it (to handle permissions, to suspend it to save power, to control the polling frequency, etc.). It makes sense for DeviceOrientation and DeviceMotion to be implemented similarly, so that the embedder can have close control over them. > > > > This e-mail thread (https://lists.webkit.org/pipermail/webkit-dev/2010-December/015313.html) discusses the pros and cons of switching Geolocation from the platform-based approach to the client-based approach. > > Our standard design approach is to have policy APIs at the WebView layer combined with functional APIs at the platform layer. I think Geolocation would likely benefit from this separation of concerns too. I agree with the principle of "policy APIs at the WebView layer combined with functional APIs at the platform layer". However, for Geolocation, Device Orientation, and Device Motion, there isn't really a functional component - all three require policy decisions. Geolocation involves policy decisions for user permissions and to determine where to obtain data from (GPS, network location, etc.). All three kinds of data involve policy decisions regarding polling frequency and suspension when a tab is in the background to reduce power consumption. Implementing these APIs in the platform layer would require a layering violation in order to obtain the necessary policy information from the embedder. In the long run, if there end up being several types of device data that similarly require policy decisions, they can be consolidated into one pathway through WebKit and demultiplexed in WebCore, so in case you're worried about there being a large number of these APIs in the future, that can be avoided.

(In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #24) > > > (In reply to comment #23) > > > > I will do a more thorough review shortly, but just a comment on the design. I feel like the design for "device orientation" may not have been done correctly. Put simply, things that are properties of the device instead of properties of the WebView should be accessed through the platform instead through the WebView. Is device orientation really something WebView-specific? Maybe it is possible to have two monitors that are oriented differently? Hmm... > > > > > > Geolocation was originally accessed through the platform, but was eventually switched to the WebView, because the embedder needed to have close control over it (to handle permissions, to suspend it to save power, to control the polling frequency, etc.). It makes sense for DeviceOrientation and DeviceMotion to be implemented similarly, so that the embedder can have close control over them. > > > > > > This e-mail thread (https://lists.webkit.org/pipermail/webkit-dev/2010-December/015313.html) discusses the pros and cons of switching Geolocation from the platform-based approach to the client-based approach. > > > > Our standard design approach is to have policy APIs at the WebView layer combined with functional APIs at the platform layer. I think Geolocation would likely benefit from this separation of concerns too. > > I agree with the principle of "policy APIs at the WebView layer combined with functional APIs at the platform layer". However, for Geolocation, Device Orientation, and Device Motion, there isn't really a functional component - all three require policy decisions. Geolocation involves policy decisions for user permissions and to determine where to obtain data from (GPS, network location, etc.). All three kinds of data involve policy decisions regarding polling frequency and suspension when a tab is in the background to reduce power consumption. Implementing these APIs in the platform layer would require a layering violation in order to obtain the necessary policy information from the embedder. My point was that you could break all of these up into two pieces: 1) Do I have permission to request [device capability]? 2) Request [device capability]. These only need to be coupled in cases where we don't trust the renderer to prefix #2 with #1. My understanding is that we aren't trying to prevent a rogue renderer from abusing #2. Or, am I mistaken? > In the long run, if there end up being several types of device data that similarly require policy decisions, they can be consolidated into one pathway through WebKit and demultiplexed in WebCore, so in case you're worried about there being a large number of these APIs in the future, that can be avoided. In general, I think that sort of encapsulation just adds overhead. We don't need two IPC systems or nested IPC systems in Chrome. The WebKit API is supposed to be a thin wrapper around WebCore. There's not much reason to minimize the number of WebKit API methods in other words.

(In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #25) > > > (In reply to comment #24) > > > > (In reply to comment #23) > > > > > I will do a more thorough review shortly, but just a comment on the design. I feel like the design for "device orientation" may not have been done correctly. Put simply, things that are properties of the device instead of properties of the WebView should be accessed through the platform instead through the WebView. Is device orientation really something WebView-specific? Maybe it is possible to have two monitors that are oriented differently? Hmm... > > > > > > > > Geolocation was originally accessed through the platform, but was eventually switched to the WebView, because the embedder needed to have close control over it (to handle permissions, to suspend it to save power, to control the polling frequency, etc.). It makes sense for DeviceOrientation and DeviceMotion to be implemented similarly, so that the embedder can have close control over them. > > > > > > > > This e-mail thread (https://lists.webkit.org/pipermail/webkit-dev/2010-December/015313.html) discusses the pros and cons of switching Geolocation from the platform-based approach to the client-based approach. > > > > > > Our standard design approach is to have policy APIs at the WebView layer combined with functional APIs at the platform layer. I think Geolocation would likely benefit from this separation of concerns too. > > > > I agree with the principle of "policy APIs at the WebView layer combined with functional APIs at the platform layer". However, for Geolocation, Device Orientation, and Device Motion, there isn't really a functional component - all three require policy decisions. Geolocation involves policy decisions for user permissions and to determine where to obtain data from (GPS, network location, etc.). All three kinds of data involve policy decisions regarding polling frequency and suspension when a tab is in the background to reduce power consumption. Implementing these APIs in the platform layer would require a layering violation in order to obtain the necessary policy information from the embedder. > > My point was that you could break all of these up into two pieces: > > 1) Do I have permission to request [device capability]? > > 2) Request [device capability]. > > These only need to be coupled in cases where we don't trust the renderer to prefix #2 with #1. My understanding is that we aren't trying to prevent a rogue renderer from abusing #2. Or, am I mistaken? That’s correct - we do trust the renderer to prefix #2 with #1, so the two don’t need to be coupled. However, decoupling them would require WebCore to maintain information about the embedder's state, which is a layering violation. For example, in the current design, if the embedder wants to turn off GPS because a tab is in the background, or if the embedder wants to change its data source from GPS to network location, it can change those properties directly without interacting with WebCore. If Webcore instead were to obtain device data through the platform layer, the embedder would have to pass this information down through WebKit into WebCore (something along the lines of device_orientation_controller->suspend(), which would then eventually call platform_impl->suspend(), and similar for changing other properties). When something similar was attempted with Geolocation, the feedback was that we shouldn't clutter WebCore with stuff like that if the embedder can handle it instead. In addition, it's not possible for WebCore to prefix #2 with #1 every time it obtains device data, since an embedder's preferences about how to obtain data could change at any time, and the arrival of device data is event-driven. What would be the advantages to decoupling them (besides adhering to the policy mentioned above)? @fishd, hans, steveblock: Can we set up a VC to discuss this? I don't think that we're completely understanding each others' arguments, and it would be helpful to have this discussion in one sitting instead of strung-out over multiple days. > > In the long run, if there end up being several types of device data that similarly require policy decisions, they can be consolidated into one pathway through WebKit and demultiplexed in WebCore, so in case you're worried about there being a large number of these APIs in the future, that can be avoided. > > In general, I think that sort of encapsulation just adds overhead. We don't need two IPC systems or nested IPC systems in Chrome. The WebKit API is supposed to be a thin wrapper around WebCore. There's not much reason to minimize the number of WebKit API methods in other words.

Created attachment 156704 [details] Patch

Comment on attachment 156704 [details] Patch Attachment 156704 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13445323

This patch set redesigns DeviceMotion so that data is passed through the Platform layer rather than through WebKit. Can you take a look and let me know if this is on the right track? A couple of notes: -I have not yet updated the tests to reflect this change - the mock should probably be moved to Tools/DumpRenderTree/chromium. -We cannot wrap DeviceMotionData using WebPrivatePtr because DeviceMotionData includes RefPtrs as members (Acceleration and RotationRate), which cannot be exposed in the public API. Instead, I created WebDeviceMotionData in Platform/chromium/public, and copy data from it to DeviceMotionData.

Comment on attachment 156704 [details] Patch Attachment 156704 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13447827

Comment on attachment 156704 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156704&action=review > Source/Platform/chromium/public/Platform.h:142 > + virtual WebDeviceMotionClient* createWebDeviceMotionClient() { return 0; } nit: methods that return "Web" types do not have "Web" in their name. note: it seems incorrect to instantiate a "Client" interface from the platform. there are no other examples of this in the current API. this may just be about finding better names for the interfaces here. does this need to be a "create" function? can it instead be a singleton getter? why would we need to create multiple device motion interfaces? device motion is a singleton property right? see for example Platform::clipboard() or Platform::mimeRegistry(). those are also singletons, and they don't return a newly heap allocated object. they return a pointer to a singleton. > Source/Platform/chromium/public/WebDeviceMotionClient.h:40 > + virtual void setController(WebDeviceMotionController&) { WEBKIT_ASSERT_NOT_REACHED(); } WebDeviceMotionController should be passed to the create* function instead. it should be passed by pointer instead of by reference. or, if this interface becomes a singleton, then i would pass the callback interface to startUpdating. > Source/Platform/chromium/public/WebDeviceMotionController.h:37 > +class WebDeviceMotionController { this looks a lot more like a Client interface to me. notice how it's only public method is an event notification? fits the client pattern perfectly! this should also most likely be a pure virtual interface. > Source/Platform/chromium/public/WebDeviceMotionData.h:149 > + WebDeviceMotionData(const WebCore::DeviceMotionData*); Since DeviceMotionData is reference counted, I think you should change WebDeviceMotionData to just be a wrapper using WebPrivatePtr<DeviceMotionData>. See for example WebNode. You don't need to copy all of the member variables!!! Here's a proposal for a Platform interface: class WebDeviceMotionData; class WebDeviceMotionDetectorClient { public: virtual void didDetectDeviceMotion(const WebDeviceMotionData&) = 0; }; class WebDeviceMotionDetector { public: virtual void start(WebDeviceMotionDetectorClient*) = 0; virtual void stop() = 0; };

(In reply to comment #33) ... > Here's a proposal for a Platform interface: > > class WebDeviceMotionData; > > class WebDeviceMotionDetectorClient { > public: > virtual void didDetectDeviceMotion(const WebDeviceMotionData&) = 0; > }; > > class WebDeviceMotionDetector { > public: > virtual void start(WebDeviceMotionDetectorClient*) = 0; > virtual void stop() = 0; > }; Oh, and add the following function to Platform: class Platform { public: ... virtual WebDeviceMotionDetector* deviceMotionDetector() { return 0; } };

Created attachment 157758 [details] Patch

Thanks very much for the comments!! I made all of the changes that you suggested, except where noted below. I did not update the tests yet. (In reply to comment #33) > (From update of attachment 156704 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156704&action=review > > > Source/Platform/chromium/public/Platform.h:142 > > + virtual WebDeviceMotionClient* createWebDeviceMotionClient() { return 0; } > > nit: methods that return "Web" types do not have "Web" in their name. > > note: it seems incorrect to instantiate a "Client" interface from the platform. there are no other examples of this in the current API. this may just be about finding better names for the interfaces here. > > does this need to be a "create" function? can it instead be a singleton getter? > why would we need to create multiple device motion interfaces? device motion > is a singleton property right? see for example Platform::clipboard() or > Platform::mimeRegistry(). those are also singletons, and they don't return > a newly heap allocated object. they return a pointer to a singleton. > > > Source/Platform/chromium/public/WebDeviceMotionClient.h:40 > > + virtual void setController(WebDeviceMotionController&) { WEBKIT_ASSERT_NOT_REACHED(); } > > WebDeviceMotionController should be passed to the create* function instead. it should be passed by pointer instead of by reference. > > or, if this interface becomes a singleton, then i would pass the callback interface > to startUpdating. > > > Source/Platform/chromium/public/WebDeviceMotionController.h:37 > > +class WebDeviceMotionController { > > this looks a lot more like a Client interface to me. notice how it's only > public method is an event notification? fits the client pattern perfectly! > > this should also most likely be a pure virtual interface. > > > Source/Platform/chromium/public/WebDeviceMotionData.h:149 > > + WebDeviceMotionData(const WebCore::DeviceMotionData*); > > Since DeviceMotionData is reference counted, I think you should change > WebDeviceMotionData to just be a wrapper using WebPrivatePtr<DeviceMotionData>. > See for example WebNode. You don't need to copy all of the member variables!!! > This isn't possible because DeviceMotionData has RefPtr's as members, and RefPtrs cannot exist in a public API. DeviceMotionData should be structured this way in order to be consistent with the structure of the DeviceMotionEvent. > Here's a proposal for a Platform interface: > > class WebDeviceMotionData; > > class WebDeviceMotionDetectorClient { > public: > virtual void didDetectDeviceMotion(const WebDeviceMotionData&) = 0; > }; > > class WebDeviceMotionDetector { > public: > virtual void start(WebDeviceMotionDetectorClient*) = 0; > virtual void stop() = 0; > };

Created attachment 158780 [details] Patch

This patch set modifies WebDeviceMotionData to be a wrapper around DeviceMotionData using WebPrivatePtr, as suggested. This patch also moves the MockWebDeviceMotionDetector to Tools/DumpRenderTree/chromium. All 11 LayoutTests for DeviceMotion pass. The tests require some changes in chromium (to webkit/support/test_webkit_platform_support.*), so this will probably need to be landed in a 3-sided patch.

Comment on attachment 158780 [details] Patch Attachment 158780 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13506702

Comment on attachment 158780 [details] Patch Attachment 158780 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13508607

Comment on attachment 158780 [details] Patch Thanks Amy, this looks great. I have two tiny nits, and I suppose the Mac build files need an update, but I'm mostly interested to hear if Darin is happy with the overall design now. View in context: https://bugs.webkit.org/attachment.cgi?id=158780&action=review > Source/Platform/chromium/public/WebDeviceMotionDetector.h:41 > + virtual void stopUpdating() = 0; i think these two should also use WEBKIT_ASSERT_NOT_REACHED(); instead of "= 0". > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:51 > + // TODO(aousterh): allow for multiple DeviceMotionControllers or make nit: in webkit, i think "// FIXME: Foo bar." is the commonly used syntax.

Comment on attachment 158780 [details] Patch Attachment 158780 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13516364 New failing tests: fast/dom/DeviceMotion/basic-operation.html fast/dom/DeviceMotion/no-page-cache.html fast/dom/DeviceMotion/event-after-navigation.html fast/dom/DeviceMotion/add-listener-from-callback.html fast/dom/DeviceMotion/multiple-frames.html fast/dom/DeviceMotion/null-values.html fast/dom/DeviceMotion/no-synchronous-events.html fast/dom/DeviceMotion/updates.html

Created attachment 158790 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 158834 [details] Patch

Thanks for the comments Hans, I made the changes you suggested. I also uploaded the Chromium patch that is necessary for the LayoutTests to run. Once everyone is happy with this patch, we can split it into 2 pieces and land the 3-sided patch: (1) The files from this patch in Source/ (2) The chromium patch (https://chromiumcodereview.appspot.com/10823366/) (3) The remaining files in this patch, in Tools/ and LayoutTests/

Comment on attachment 158834 [details] Patch Attachment 158834 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13509649

Comment on attachment 158834 [details] Patch Attachment 158834 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13506790 New failing tests: fast/dom/DeviceMotion/basic-operation.html fast/dom/DeviceMotion/no-page-cache.html fast/dom/DeviceMotion/event-after-navigation.html fast/dom/DeviceMotion/add-listener-from-callback.html fast/dom/DeviceMotion/multiple-frames.html fast/dom/DeviceMotion/null-values.html fast/dom/DeviceMotion/no-synchronous-events.html fast/dom/DeviceMotion/updates.html

Created attachment 158844 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 158834 [details] Patch Attachment 158834 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13517392

Comment on attachment 158834 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158834&action=review I haven't reviewed the tests yet. It looks like you've also got a compile failure on apple-mac. > Source/Platform/chromium/public/WebDeviceMotionData.h:38 > + WebDeviceMotionData(); Does this need WEBKIT_EXPORT ? > Source/Platform/chromium/public/WebDeviceMotionData.h:46 > + static WebDeviceMotionData nullMotion() { return WebDeviceMotionData(); } What's the point of this API? It seems redundant with the default constructor. > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:39 > +DeviceMotionDetectorInternal::DeviceMotionDetectorInternal() DeviceMotionDetectorInternal -> What does "Internal" mean in this name? Is there a non-internal version? If not, we should probably just call this class DeviceMotionDetector.... > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:54 > + m_controller = controller; Four-space indent > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:59 > + m_detector = WebKit::Platform::current()->deviceMotionDetector(); Is this always non-0? If not, we should probably add a null check. > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:83 > + // Our lifetime is bound to the WebViewImpl. There is no such thing as a WebViewImpl in WebCore. Do you mean the Page? Do other ports use deviceMotionControllerDestroyed? It seems like we should use the same mechanism for managing the lifetime of these classes across ports. > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:88 > + if (m_controller) { Prefer early return. > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:91 > + if (!deviceMotionData) > + deviceMotionData = DeviceMotionData::create(); Can this happen? Why can this function be called with a null pointer but m_controller->didChangeDeviceMotion cannot? > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.h:62 > + WebKit::WebDeviceMotionDetector* m_detector; How is the lifetime of this object managed? Do we even need to store a pointer to this object? It seems like we can just get it from the Platform whenever we like. > Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:50 > +void WebDeviceMotionData::initializeAcceleration(bool canProvideAccelerationX, double accelerationX, bool canProvideAccelerationY, double accelerationY, bool canProvideAccelerationZ, double accelerationZ) Should this be called setAcceleration instead? Is it legal to call this function more than once? If not, we should ASSERT that we don't do that. If so, we should change the name to "set" rather than "initialize". > Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:55 > + m_private->setAcceleration(acceleration.release()); Notice that the WebCore function name is "set". We like to align the API names with the WebCore names to cut down on confusion whenever possible. > Source/WebKit/chromium/src/WebViewImpl.h:840 > + // If we attempt to fetch the on-screen GraphicsContext3D before > + // the compositor has been turned on, we need to instantiate it > + // early. This member holds on to the GC3D in this case. > + OwnPtr<WebGraphicsContext3D> m_temporaryOnscreenGraphicsContext3D; This seems unrelated to device motion. > Source/WebKit/chromium/src/WebViewImpl.h:841 > + OwnPtr<WebCore::DeviceMotionDetectorInternal> m_deviceMotionDetectorInternal; It seems strange to own a WebCore object here. Perhaps DeviceMotionDetectorInternal should be in the WebKit layer rather than WebCore? It implements DeviceMotionClient, which is a WebCore client. Typically, we implement WebCore clients in the WebKit layer, not in the WebCore layer. You can see there are a bunch of FooClientImpl objects here. Perhaps DeviceMotionDetectorInternal should be called DeviceMotionClientImpl? It's not entirely clear to me.

Created attachment 159132 [details] Patch

Thanks for the comments! (In reply to comment #50) > (From update of attachment 158834 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158834&action=review > > I haven't reviewed the tests yet. It looks like you've also got a compile failure on apple-mac. > Yes, the build files for mac still need to be updated. > > Source/Platform/chromium/public/WebDeviceMotionData.h:38 > > + WebDeviceMotionData(); > > Does this need WEBKIT_EXPORT ? > I changed it so that it is now defined in-line and wont need WEBKIT_EXPORT. > > Source/Platform/chromium/public/WebDeviceMotionData.h:46 > > + static WebDeviceMotionData nullMotion() { return WebDeviceMotionData(); } > > What's the point of this API? It seems redundant with the default constructor. > Good point, I removed it. > > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:39 > > +DeviceMotionDetectorInternal::DeviceMotionDetectorInternal() > > DeviceMotionDetectorInternal -> What does "Internal" mean in this name? Is there a non-internal version? If not, we should probably just call this class DeviceMotionDetector.... > This naming is consistent with PeerConnection00Handler, which is designed similarly. There is a default implementation, called PeerConnection00Handler.cpp/.h in WebCore/platform/mediastream, and the chromium implementation, PeerConnection00HandlerInternal.cpp/.h, in WebCore/platform/mediastream/chromium. So I guess it's "Internal" in the sense that it's chromium-specific. Since there is no default implementation for DeviceMotion right now though, we could remove the "Internal" if you prefer? > > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:54 > > + m_controller = controller; > > Four-space indent > Done. > > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:59 > > + m_detector = WebKit::Platform::current()->deviceMotionDetector(); > > Is this always non-0? If not, we should probably add a null check. > Good point, this can be null. > > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:83 > > + // Our lifetime is bound to the WebViewImpl. > > There is no such thing as a WebViewImpl in WebCore. Do you mean the Page? Do other ports use deviceMotionControllerDestroyed? It seems like we should use the same mechanism for managing the lifetime of these classes across ports. > I guess it is created by the WebViewImpl, but it's lifetime is managed by the Page. > > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:88 > > + if (m_controller) { > > Prefer early return. > Done. > > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:91 > > + if (!deviceMotionData) > > + deviceMotionData = DeviceMotionData::create(); > > Can this happen? Why can this function be called with a null pointer but m_controller->didChangeDeviceMotion cannot? > This function can be called with a valid reference to a WebDeviceMotionData which is a wrapper around a NULL DeviceMotionData. I modified the function in WebDeviceMotionData that creates a PassRefPtr<DeviceMotionData> from a WebDeviceMotionData so that it creates and returns an empty DeviceMotionData rather than returning a NULL pointer in this case. > > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.h:62 > > + WebKit::WebDeviceMotionDetector* m_detector; > > How is the lifetime of this object managed? Do we even need to store a pointer to this object? It seems like we can just get it from the Platform whenever we like. > We probably don't need to store a pointer to this, but on first attempt it doesn't seem to work properly if you get it from the Platform each time. We'll need to investigate this further. > > Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:50 > > +void WebDeviceMotionData::initializeAcceleration(bool canProvideAccelerationX, double accelerationX, bool canProvideAccelerationY, double accelerationY, bool canProvideAccelerationZ, double accelerationZ) > > Should this be called setAcceleration instead? Is it legal to call this function more than once? If not, we should ASSERT that we don't do that. If so, we should change the name to "set" rather than "initialize". > > > Source/WebCore/platform/chromium/support/WebDeviceMotionData.cpp:55 > > + m_private->setAcceleration(acceleration.release()); > > Notice that the WebCore function name is "set". We like to align the API names with the WebCore names to cut down on confusion whenever possible. > I added the "set" functions in WebCore - I think the most appropriate naming is to actually make them all "initialize" functions. That is more consistent with the rest of DeviceMotionData, which doesn't allow you to modify components of it once they are created. > > Source/WebKit/chromium/src/WebViewImpl.h:840 > > + // If we attempt to fetch the on-screen GraphicsContext3D before > > + // the compositor has been turned on, we need to instantiate it > > + // early. This member holds on to the GC3D in this case. > > + OwnPtr<WebGraphicsContext3D> m_temporaryOnscreenGraphicsContext3D; > > This seems unrelated to device motion. > Sorry about that, that was probably a rebasing mistake. > > Source/WebKit/chromium/src/WebViewImpl.h:841 > > + OwnPtr<WebCore::DeviceMotionDetectorInternal> m_deviceMotionDetectorInternal; > > It seems strange to own a WebCore object here. Perhaps DeviceMotionDetectorInternal should be in the WebKit layer rather than WebCore? It implements DeviceMotionClient, which is a WebCore client. Typically, we implement WebCore clients in the WebKit layer, not in the WebCore layer. You can see there are a bunch of FooClientImpl objects here. Perhaps DeviceMotionDetectorInternal should be called DeviceMotionClientImpl? It's not entirely clear to me. I agree that it's a bit strange for WebViewImpl to own a WebCore object, when all data is passed out through the Platform layer not through the WebView. Is there anywhere else that it could be owned instead?

Comment on attachment 159132 [details] Patch Attachment 159132 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13527095

Comment on attachment 159132 [details] Patch Attachment 159132 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13514866

Comment on attachment 159132 [details] Patch Attachment 159132 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13516861 New failing tests: fast/dom/DeviceMotion/basic-operation.html fast/dom/DeviceMotion/add-listener-from-callback.html fast/dom/DeviceMotion/multiple-frames.html fast/dom/DeviceMotion/null-values.html fast/dom/DeviceMotion/no-synchronous-events.html fast/dom/DeviceMotion/updates.html

Created attachment 159151 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 159450 [details] Patch

I'm taking over this patch after Amy. I've uploaded a new version that should hopefully turn the EWS bots green: - Mac build files are updated - Accidentally placed #if ENABLE(MEDIA_STREAM) fixed; should make Android happy - Disable the layout tests for Chromium for now. (In reply to comment #50) > It seems strange to own a WebCore object here. Perhaps DeviceMotionDetectorInternal should be in the WebKit layer rather than WebCore? It implements DeviceMotionClient, which is a WebCore client. Typically, we implement WebCore clients in the WebKit layer, not in the WebCore layer. You can see there are a bunch of FooClientImpl objects here. Perhaps DeviceMotionDetectorInternal should be called DeviceMotionClientImpl? It's not entirely clear to me. Right, this is exactly how DeviceOrientation is implemented, but Darin suggested that that approach was bad and that we should go through the platform layer instead. That's what we're trying to do with this patch. I'm not entirely convinced that this makes things simpler, but I'm happy to do whatever it takes to get it landed. I'm very interested to hear comments about if we're happy with this approach before we start getting into reviewing the details. Once we're happy with the overall structure, I'd like to split this into smaller patches so we can review them properly piece by piece.

Comment on attachment 159450 [details] Patch Going "down" to the platform rather than "up" to the client is fine. However, if you're going to adopt that approach, you should adopt it entirely. The current patch is a confused mix of the two approaches. For example, the "down to the platform" approach, we should get rid of DeviceMotionClient and the WebKit layer shouldn't have any knowledge of DeviceMotionDetectorInternal. The reason classes like DeviceMotionClient exist is to call "up" from WebCore to the WebKit layer. In the platform-based approach, there isn't any need to call up to the WebKit layer as you can just talk directly down to the platform layer. Consequently, you don't need DeviceMotionClient. Similarly, WebViewImpl shouldn't be involved in interactions with the platform layer. WebViewImpl exists to mediate interactions with the client.

Here's a somewhat simplified diagram that might or might not be helpful: https://docs.google.com/drawings/d/1GJXd6XSLEehvuqA7Xbvbz0OH6Znnr2iTijihjrEY0ts/edit

(In reply to comment #59) > (From update of attachment 159450 [details]) > Going "down" to the platform rather than "up" to the client is fine. However, if you're going to adopt that approach, you should adopt it entirely. The current patch is a confused mix of the two approaches. For example, the "down to the platform" approach, we should get rid of DeviceMotionClient and the WebKit layer shouldn't have any knowledge of DeviceMotionDetectorInternal. So it seems the current patch is somewhere halfway in-between the client and platform ways of doing things. To do the platform approach entirely, it seems we should rewrite DeviceMotionController not to rely on a DeviceMotionClient, but call directly to the platform layer. The problem with this is that several ports already implement the client-based approach: Qt, Blackberry, iOS (though I guess that's not in the main repository?), and EFL and GTK (those two are just stubs so not a real problem). If we kill DeviceMotionClient, these ports would need to be updated too. I do like a good yak shave, but I'm not sure this one is really worth the effort, and I'm not entirely sure how to motivate it to the other ports.

> To do the platform approach entirely, it seems we should rewrite DeviceMotionController not to rely on a DeviceMotionClient, but call directly to the platform layer. Correct. > The problem with this is that several ports already implement the client-based approach: Qt, Blackberry, iOS (though I guess that's not in the main repository?), and EFL and GTK (those two are just stubs so not a real problem). If we kill DeviceMotionClient, these ports would need to be updated too. Sounds like we would need to update Qt and Blackberry. We can't update iOS because it's not in trunk (that's the price Apple pays for keeping it sekret). > I do like a good yak shave, but I'm not sure this one is really worth the effort, and I'm not entirely sure how to motivate it to the other ports. I don't have a strong opinion one way or another. It sounds like fishd was asking for a platform-based approach. Let's ask him if he's ok with a client-based approach in light of this new information.

Created attachment 166015 [details] Platform-based patch Here is a stab at what a full-blown platform-based approach might look like. This patch ignores the other ports, layout tests, hooking up in DRT, and changelog - so I expect red EWS-bots - but it does work end-to-end in Chromium. Please take a look at the patch and let me know if I understood correctly that this is the kind of plumbing you had in mind, and I can go on to make it a full-blown patch, update the other ports or ping their mantainers, etc.

Attachment 166015 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/Platform.gypi', u'Source/P..." exit_code: 1 Source/Platform/chromium/public/WebDeviceMotionDetector.h:26: #ifndef header guard has wrong style, please use: WebDeviceMotionDetector_h [build/header_guard] [5] Source/WebCore/dom/DeviceMotionController.h:29: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 166015 [details] Platform-based patch Attachment 166015 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14034856

Comment on attachment 166015 [details] Platform-based patch Attachment 166015 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14037843

Comment on attachment 166015 [details] Platform-based patch Attachment 166015 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14027967

Comment on attachment 166015 [details] Platform-based patch Attachment 166015 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13964092

Comment on attachment 166015 [details] Platform-based patch Attachment 166015 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14037848

Comment on attachment 166015 [details] Platform-based patch View in context: https://bugs.webkit.org/attachment.cgi?id=166015&action=review > Source/Platform/chromium/public/WebDeviceMotionDetector.h:35 > + virtual void start(void* handle, void (*callback)(void*, const WebDeviceMotionData&)) { WEBKIT_ASSERT_NOT_REACHED(); } we don't usually do callbacks this way. we usually define a Client interface. why does stop() take a pointer? WebDeviceMotionListener would also be a reasonable choice for this interface name as it matches up with the DeviceMotionListener interface name. > Source/WebCore/platform/DeviceMotion.h:43 > +void stopDetectingDeviceMotion(DeviceMotionListener*); why do you need to pass the listener to this stop method?

(In reply to comment #70) > (From update of attachment 166015 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166015&action=review > > > Source/Platform/chromium/public/WebDeviceMotionDetector.h:35 > > + virtual void start(void* handle, void (*callback)(void*, const WebDeviceMotionData&)) { WEBKIT_ASSERT_NOT_REACHED(); } > > we don't usually do callbacks this way. we usually define a Client interface. Right, that's just a hack to get something up and running. I'm mostly interested in hearing if the way this is hooked up is how you want it: - Are the files in the right places? - Is the flow correct? I.e. DeviceMotionController -> function declared in WebCore/platform/ -> function definition in WebCore/platform/chromium -> interface in Platform/chromium/public which is implemented Chromium-side (and in DRT) It would be great you could comment on these, and then we can work out the details later. > why does stop() take a pointer? Because the platform would need to know which of the multiple listeners wants to stop receiving updates. I might be wrong, but there might be multiple DeviceMotionControllers asking for updates, one for each Page. (If I'm wrong here that would make things much easier :) > WebDeviceMotionListener would also be a > reasonable choice for this interface name as it matches up with the > DeviceMotionListener interface name. Right. I'm uploading a new patch that removes the callback hack and introduces a WebDeviceMotionListener class. > > Source/WebCore/platform/DeviceMotion.h:43 > > +void stopDetectingDeviceMotion(DeviceMotionListener*); > > why do you need to pass the listener to this stop method? See "why does stop() take a pointer?" above.

Created attachment 166259 [details] Patch

Attachment 166259 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1 Source/WebCore/platform/chromium/support/WebDeviceMotionListener.cpp:29: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/dom/DeviceMotionController.h:29: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 166259 [details] Patch Attachment 166259 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14050741

Comment on attachment 166259 [details] Patch Attachment 166259 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14071180

Comment on attachment 166259 [details] Patch Attachment 166259 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14050743

Comment on attachment 166259 [details] Patch Attachment 166259 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14060348

Comment on attachment 166259 [details] Patch Attachment 166259 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13980061

Comment on attachment 166259 [details] Patch Attachment 166259 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14059384

Comment on attachment 166259 [details] Patch Attachment 166259 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14070286

Comment on attachment 166259 [details] Patch Attachment 166259 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14060375

(In reply to comment #71) > (In reply to comment #70) > > (From update of attachment 166015 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=166015&action=review > > > > > Source/Platform/chromium/public/WebDeviceMotionDetector.h:35 > > > + virtual void start(void* handle, void (*callback)(void*, const WebDeviceMotionData&)) { WEBKIT_ASSERT_NOT_REACHED(); } > > > > we don't usually do callbacks this way. we usually define a Client interface. > > Right, that's just a hack to get something up and running. OK > I'm mostly interested in hearing if the way this is hooked up is how you want it: > > - Are the files in the right places? > - Is the flow correct? I.e. DeviceMotionController -> function declared in WebCore/platform/ -> function definition in WebCore/platform/chromium -> interface in Platform/chromium/public which is implemented Chromium-side (and in DRT) Yes, that seems right. > > why does stop() take a pointer? > > Because the platform would need to know which of the multiple listeners wants to stop receiving updates. > > I might be wrong, but there might be multiple DeviceMotionControllers asking for updates, one for each Page. (If I'm wrong here that would make things much easier :) It seems like there is one source of device motion, and that gets broadcast to all interested parties (each Page). I think that broadcasting can happen in WebCore. It doesn't have to happen at the platform layer.

(In reply to comment #82) > > > why does stop() take a pointer? > > > > Because the platform would need to know which of the multiple listeners wants to stop receiving updates. > > > > I might be wrong, but there might be multiple DeviceMotionControllers asking for updates, one for each Page. (If I'm wrong here that would make things much easier :) > > It seems like there is one source of device motion, and that gets broadcast to all interested parties (each Page). I think that broadcasting can happen in WebCore. It doesn't have to happen at the platform layer. Ah, I think I see what you mean. I'm still not entirely sure I've got the terminology right, though. When you say "It doesn't have to happen at the platform layer.", do you mean you don't want it in the implementation of WebKit::Platform (which is implemented in Chromium), or do you mean you don't want it in Source/WebCore/platform either? One idea would be to replace the {start,stop}DetectingDeviceMotion() functions that I put in WebCore/platform/DeviceMotion.h, with a singleton class like this: class DeviceMotionDetector { public: static DeviceMotionDetector& instance(); void addListener(Listener*); void removeListener(Listener*); }; And then that class would do the "broadcasting", and call into "WebKit::Platform::current()->deviceMotionDetector()->start(this)" and "->stop()" behind the scenes. Does this sound like a good approach? Or would you prefer the broadcasting to happen in WebCore-not-WebCore/platform?