[Webkit-unassigned] [Bug 89197] [Chromium] Implements DeviceMotion

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=89197





--- Comment #19 from Amy Ousterhout <aousterh at chromium.org>  2012-06-25 10:00:56 PST ---
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.