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

[bugzilla-daemon at webkit.org]

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





--- Comment #17 from Adam Barth <abarth at webkit.org>  2012-06-21 13:07:13 PST ---
(From update of attachment 148790)
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?

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