[Webkit-unassigned] [Bug 41616] [Chromium] DeviceOrientation plumbing

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from hans at chromium.org  2010-07-07 01:43:02 PST ---
Thanks for the review!

(In reply to comment #2)
> // Called by the service.
> //   virtual void setLastOrientation(double alpha, double beta, double gamma) = 0;
> 
> 
> What is this for? The spec doesn't define any concept of cached orientation (unlike Geolocation) so I am not sure what is this used for.
It's for communicating an orientation update back to the WebKit code (but you're right, there's no caching). The name seems ill chosen; I'll rename it to updateOrientation.

> 
> 
> //#if ENABLE(DEVICE_ORIENTATION)
>  //    if (!m_webViewImpl->page())
>  //      return;
>  //    m_webViewImpl->page()->deviceOrientation()->onDeviceOrientationChange(alpha, beta, gamma);
> // #endif
> 
> Why is just the block of code guarded by the #ifdef. I think the entire file should be guarded.
It's there because if ENABLE(DEVICE_ORIENTATION) is false, then page()->deviceOrientation() does not compile.

I'm not sure how enable guards are normally used, but I tried to follow Steve's code in bug 57646, where rather than guarding the whole files (e.g. DeviceOrientation.h and .cpp), the guards are put in place to make the code unavailable, i.e. it will never be executed if the switch is off, but it is still compiled.

I'm happy to change this if it's wrong, but I need more convincing first :)

> 
> > explicit WebDeviceOrientationServiceBridgeImpl(DeviceOrientationClientImpl*  deviceOrientationClientImpl);
> 
> You should leave out the param name if all it does is to repeat the type name.
Will fix.

> 
> One question about the architecture: You have:
> 
> DeviceOrientationClientImpl owns a WebDeviceOrientationServiceBridgeImpl object, which has a pointer back to the client and a pointer to a WebDeviceOrientationService instance.
> 
> What is the point of the bridge? Why can't the ClientImpl simply own the WebDeviceOrientationService instance?
Thanks for bringing this up. It's an area that I think is tricky, so please correct me if my reasoning seems wrong:

ClientImpl could own the WebDeviceOrientationService instance, but the WebDeviceOrientationService instance (DeviceOrientationDispatcher) would not be able to communicate back into WebKit. Since ClientImpl is not a public interface, code outside WebKit can't refer to it (and it can't be public as it relies on WebCore). So that's the role of the bridge: to provide an interface that code outside WebKit can use to communicate with the ClientImpl.

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