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

Fri Aug 17 08:52:55 PDT 2012

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

--- Comment #52 from Amy Ousterhout <aousterh at chromium.org>  2012-08-17 08:53:24 PST ---
Thanks for the comments!

(In reply to comment #50)
> (From update of attachment 158834 [details])
> 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.
> 

Yes, the build files for mac still need to be updated.

> > Source/Platform/chromium/public/WebDeviceMotionData.h:38
> > +    WebDeviceMotionData();
> 
> Does this need WEBKIT_EXPORT ?
> 

I changed it so that it is now defined in-line and wont 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.
> 

Good point, I removed it.

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

This naming is consistent with PeerConnection00Handler, which is designed similarly. There is a default implementation, called PeerConnection00Handler.cpp/.h in WebCore/platform/mediastream, and the chromium implementation, PeerConnection00HandlerInternal.cpp/.h, in WebCore/platform/mediastream/chromium. So I guess it's "Internal" in the sense that it's chromium-specific. Since there is no default implementation for DeviceMotion right now though, we could remove the "Internal" if you prefer?

> > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:54
> > +      m_controller = controller;
> 
> Four-space indent
> 

Done.

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

Good point, this can be null.

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

I guess it is created by the WebViewImpl, but it's lifetime is managed by the Page.

> > Source/WebCore/platform/chromium/DeviceMotionDetectorInternal.cpp:88
> > +    if (m_controller) {
> 
> Prefer early return.
> 

Done.

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

This function can be called with a valid reference to a WebDeviceMotionData which is a wrapper around a NULL DeviceMotionData. I modified the function in WebDeviceMotionData that creates a PassRefPtr<DeviceMotionData> from a WebDeviceMotionData so that it creates and returns an empty DeviceMotionData rather than returning a NULL pointer in this case.

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

We probably don't need to store a pointer to this, but on first attempt it doesn't seem to work properly if you get it from the Platform each time. We'll need to investigate this further.

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

I added the "set" functions in WebCore - I think the most appropriate naming is to actually make them all "initialize" functions. That is more consistent with the rest of DeviceMotionData, which doesn't allow you to modify components of it once they are created.

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

Sorry about that, that was probably a rebasing mistake.

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

I agree that it's a bit strange for WebViewImpl to own a WebCore object, when all data is passed out through the Platform layer not through the WebView. Is there anywhere else that it could be owned instead?

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