[webkit-reviews] review denied: [Bug 47078] [Chromium] DeviceMotion plumbing : [Attachment 69908] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Hans Wennborg
<hans at chromium.org>'s request for review:
Bug 47078: [Chromium] DeviceMotion plumbing
https://bugs.webkit.org/show_bug.cgi?id=47078

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69908&action=review

> WebKit/chromium/public/WebDeviceMotionClient.h:40
> +    virtual void setController(WebDeviceMotionController*) {
WEBKIT_ASSERT_NOT_REACHED(); }

why is setController an explicit method here?  couldn't you also pass the
WebDeviceMotionController
to the method on WebViewClient used to fetch the WebDeviceMotionClient?

when does the WebDeviceMotionController pointer get freed?

> WebKit/chromium/public/WebDeviceMotionClient.h:43
> +    virtual WebDeviceMotionData* currentDeviceMotion() const

nit: currentDeviceMotion -> currentDeviceMotionData

why is currentDeviceMotion returned by pointer and not by value?  the whole
point of using WebPrivatePtr<...wrapping a RefCounted type...> is to support
passing the container type by value.

> WebKit/chromium/public/WebDeviceMotionController.h:39
> +    WebDeviceMotionController(WebCore::DeviceMotionController*);

this constructor should be wrapped with WEBKIT_IMPLEMENTATION, and i presume
that the default constructor should be declared but marked private.  i see
that WebDeviceOrientationController has the same problem.

> WebKit/chromium/public/WebDeviceMotionController.h:47
> +    WebCore::DeviceMotionController* m_controller;

i don't understand what the memory management is here?	this looks like a crash

waiting to happen.

> WebKit/chromium/public/WebDeviceMotionData.h:39
> +class WebDeviceMotionData {

Looking at the way this class is used, I'm not sure why you didn't just
do the same thing as you did for WebDeviceOrientation.	It seems like it
does not need to be a wrapper around the WebCore type.	It can just be
a simple class with member variables for the various fields.

> WebKit/chromium/public/WebDeviceMotionData.h:74
> +    WEBKIT_API void init();

what is the reason for making these private?

why aren't you providing a copy constructor and assignment operator?  those
are typically provided since WebDeviceMotionData is really just like a
RefPtr<DeviceMotionData>.  it can be copied around, and all of the memory
management happens implicitly.