[webkit-reviews] review denied: [Bug 89197] [Chromium] Implements DeviceMotion : [Attachment 158834] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Amy Ousterhout
<aousterh at chromium.org>'s request for review:
Bug 89197: [Chromium] Implements DeviceMotion
https://bugs.webkit.org/show_bug.cgi?id=89197

Attachment 158834: Patch
https://bugs.webkit.org/attachment.cgi?id=158834&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.