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

[bugzilla-daemon at webkit.org]

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





--- Comment #36 from Amy Ousterhout <aousterh at chromium.org>  2012-08-10 11:01:34 PST ---
Thanks very much for the comments!! I made all of the changes that you suggested, except where noted below. I did not update the tests yet.

(In reply to comment #33)
> (From update of attachment 156704 [details])
> 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!!!
> 

This isn't possible because DeviceMotionData has RefPtr's as members, and RefPtrs cannot exist in a public API. DeviceMotionData should be structured this way in order to be consistent with the structure of the DeviceMotionEvent.

> 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.