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

[bugzilla-daemon at webkit.org]

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


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #156704|review?                     |review-
               Flag|                            |




--- Comment #33 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2012-08-09 11:00:29 PST ---
(From update of attachment 156704)
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;
  };

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