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

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.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 156704: Patch
https://bugs.webkit.org/attachment.cgi?id=156704&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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;
  };