Bug 41616 – [Chromium] DeviceOrientation plumbing

• Component: New Bugs
• Status: RESOLVED FIXED

[Hans Wennborg]

Created attachment 60534 [details]
Patch

Provide an implementation of WebCore::DeviceOrientationClient for Chromium: DeviceOrientationClientImpl. This will in turn use a WebDeviceOrientationServiceBridge (implemented by WebDeviceOrientationServiceBridgeImpl) to communicate with a WebDeviceOrientationService on the Chromium side.

[Hans Wennborg]

This is a work in progress as the patch for bug 39588 is not in yet and interfaces may change, but early comments are most welcome.

[Andrei Popescu]

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


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

> explicit WebDeviceOrientationServiceBridgeImpl(DeviceOrientationClientImpl*  deviceOrientationClientImpl);

You should leave out the param name if all it does is to repeat the type name.

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?

[Hans Wennborg]

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.

[Jeremy Orlow]

Marcus, can you please look at the logic in this?

[Jeremy Orlow]

Comment on attachment 60534 [details]
Patch

WebKit/chromium/src/WebViewImpl.h:44
 +  #if ENABLE(DEVICE_ORIENTATION)
We shoudl just turn it on now at compile time, but hide it behind a runtime flag.  Then you don't need this stuff.

WebKit/chromium/src/WebDeviceOrientationServiceBridgeImpl.h:14
 +   *     * Neither the name of Google Inc. nor the names of its
Use 2 clause license

WebKit/chromium/src/WebDeviceOrientationServiceBridgeImpl.h:42
 +      explicit WebDeviceOrientationServiceBridgeImpl(DeviceOrientationClientImpl* deviceOrientationClientImpl);
only name parameters in .h files when it adds information

WebKit/chromium/src/WebDeviceOrientationServiceBridgeImpl.cpp:89
 +      m_webDeviceOrientationService = NULL;
use 0 not NULL in webkit code

WebKit/chromium/src/WebDeviceOrientationServiceBridgeImpl.cpp:35
 +  #include "WebDeviceOrientationService.h"
alpha order

WebKit/chromium/src/WebDeviceOrientationServiceBridgeImpl.cpp:14
 +   *     * Neither the name of Google Inc. nor the names of its
2 clause

WebKit/chromium/src/DeviceOrientationClientImpl.h:40
 +  } // namespace WebCore
don't need the //

WebKit/chromium/src/DeviceOrientationClientImpl.cpp:51
 +        return;
4 spaces

[Darin Fisher (:fishd, Google)]

Comment on attachment 60534 [details]
Patch

WebKit/chromium/public/WebDeviceOrientationService.h:41
 +      virtual void attachBridge(WebDeviceOrientationServiceBridge* bridge) { WEBKIT_ASSERT_NOT_REACHED(); }
nit: no need for a parameter name.  webkit style is to exclude the parameter name
when it adds no information.

WebKit/chromium/public/WebDeviceOrientationServiceBridge.h:36
 +  // Bridge between a DeviceOrientationClientImpl and a WebDeviceOrientationService.
please avoid mentioning implementation classes in the public API.  it isn't
relevant to a consumer of the API.

WebKit/chromium/public/WebDeviceOrientationServiceBridge.h:37
 +  class WebDeviceOrientationServiceBridge {
usually we use the term Client to describe the interface that receives
notifications from some service.

Foo has a FooClient
FooService has a FooServiceClient

Is there some reason why Bridge is better than Client?

WebKit/chromium/public/WebDeviceOrientationService.h:41
 +      virtual void attachBridge(WebDeviceOrientationServiceBridge* bridge) { WEBKIT_ASSERT_NOT_REACHED(); }
can you have more than one bridge attached?  or is it limited
to just one bridge attached at a time?

have you considered just adding these methods to WebViewClient,
maybe named a bit differently?

[Hans Wennborg]

(In reply to comment #5)
> (From update of attachment 60534 [details])
Thanks for the review.

> WebKit/chromium/src/WebViewImpl.h:44
>  +  #if ENABLE(DEVICE_ORIENTATION)
> We shoudl just turn it on now at compile time, but hide it behind a runtime flag.  Then you don't need this stuff.
Hmm, but we can't turn it on before we have landed an implementation, and we can't land this implementation without the enable guards if the switch is off, so I guess the plan would be to keep them for now and remove them eventually?

> 
> WebKit/chromium/src/WebDeviceOrientationServiceBridgeImpl.h:14
>  +   *     * Neither the name of Google Inc. nor the names of its
> Use 2 clause license
Will fix.

> 
> WebKit/chromium/src/WebDeviceOrientationServiceBridgeImpl.h:42
>  +      explicit WebDeviceOrientationServiceBridgeImpl(DeviceOrientationClientImpl* deviceOrientationClientImpl);
> only name parameters in .h files when it adds information
Will fix.

> 
> WebKit/chromium/src/WebDeviceOrientationServiceBridgeImpl.cpp:89
>  +      m_webDeviceOrientationService = NULL;
> use 0 not NULL in webkit code
Will fix.

> 
> WebKit/chromium/src/WebDeviceOrientationServiceBridgeImpl.cpp:35
>  +  #include "WebDeviceOrientationService.h"
> alpha order
Will fix.

> 
> WebKit/chromium/src/WebDeviceOrientationServiceBridgeImpl.cpp:14
>  +   *     * Neither the name of Google Inc. nor the names of its
> 2 clause
Will fix

> 
> WebKit/chromium/src/DeviceOrientationClientImpl.h:40
>  +  } // namespace WebCore
> don't need the //
Will fix. What's the convention for this? Looking at the guidelines, they seem to put a comment like this at the closing of most namespaces, but not in every case?

> 
> WebKit/chromium/src/DeviceOrientationClientImpl.cpp:51
>  +        return;
> 4 spaces
Whoops. Thanks for pointing that out.

[Jeremy Orlow]

(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 60534 [details] [details])
> Thanks for the review.
> 
> > WebKit/chromium/src/WebViewImpl.h:44
> >  +  #if ENABLE(DEVICE_ORIENTATION)
> > We shoudl just turn it on now at compile time, but hide it behind a runtime flag.  Then you don't need this stuff.
> Hmm, but we can't turn it on before we have landed an implementation, and we can't land this implementation without the enable guards if the switch is off, so I guess the plan would be to keep them for now and remove them eventually?

I never really had a problem with this in IndexedDB.  I just turned it off via the runtime enabled mechanism.  If you explicitly turn it on and try to use it in javascript, yeah, it'll crash chrome, but that's OK.  Because it can't be accessed without the flag enabled it's fine.

Will that not work for you though?  If so, I'm fine with you putting this stuff in.  It's just that I can't think of many features that have needed it.

> > 
> > WebKit/chromium/src/DeviceOrientationClientImpl.h:40
> >  +  } // namespace WebCore
> > don't need the //
> Will fix. What's the convention for this? Looking at the guidelines, they seem to put a comment like this at the closing of most namespaces, but not in every case?

Cases like these are too obvious to bother with.  Where in the style guideline does it mandate this though?

[Hans Wennborg]

(In reply to comment #6)
> (From update of attachment 60534 [details])
Thanks for the review.

> WebKit/chromium/public/WebDeviceOrientationService.h:41
>  +      virtual void attachBridge(WebDeviceOrientationServiceBridge* bridge) { WEBKIT_ASSERT_NOT_REACHED(); }
> nit: no need for a parameter name.  webkit style is to exclude the parameter name
> when it adds no information.
Fixed.

> 
> WebKit/chromium/public/WebDeviceOrientationServiceBridge.h:36
>  +  // Bridge between a DeviceOrientationClientImpl and a WebDeviceOrientationService.
> please avoid mentioning implementation classes in the public API.  it isn't
> relevant to a consumer of the API.
Hmm, removing this comment as I have trouble finding a sensible alternative.

> 
> WebKit/chromium/public/WebDeviceOrientationServiceBridge.h:37
>  +  class WebDeviceOrientationServiceBridge {
> usually we use the term Client to describe the interface that receives
> notifications from some service.
> 
> Foo has a FooClient
> FooService has a FooServiceClient
> 
> Is there some reason why Bridge is better than Client?
Well, there is already a DeviceOrientationClient, but on the WebCore side. And that's why we provide a bridge: so the service can send notifications to it (the bridge is being renamed to just WebDeviceOrientationBridge, btw). I guess we could also have a WebKit::DeviceOrientationClient, but I think that would be more confusing?

> 
> WebKit/chromium/public/WebDeviceOrientationService.h:41
>  +      virtual void attachBridge(WebDeviceOrientationServiceBridge* bridge) { WEBKIT_ASSERT_NOT_REACHED(); }
> can you have more than one bridge attached?  or is it limited
> to just one bridge attached at a time?
It's built to be able to have multiple bridges attached at the same time.

> 
> have you considered just adding these methods to WebViewClient,
> maybe named a bit differently?
I hadn't, as I started off mimicking the geolocation design. I guess one could certainly put it in there, but would it be preferable? I kind of like having it in its own class, as WebViewClient is quite large, but it's not something I feel strongly about.



(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > (From update of attachment 60534 [details] [details] [details])
> > Thanks for the review.
> > 
> > > WebKit/chromium/src/WebViewImpl.h:44
> > >  +  #if ENABLE(DEVICE_ORIENTATION)
> > > We shoudl just turn it on now at compile time, but hide it behind a runtime flag.  Then you don't need this stuff.
> > Hmm, but we can't turn it on before we have landed an implementation, and we can't land this implementation without the enable guards if the switch is off, so I guess the plan would be to keep them for now and remove them eventually?
> 
> I never really had a problem with this in IndexedDB.  I just turned it off via the runtime enabled mechanism.  If you explicitly turn it on and try to use it in javascript, yeah, it'll crash chrome, but that's OK.  Because it can't be accessed without the flag enabled it's fine.
> 
> Will that not work for you though?  If so, I'm fine with you putting this stuff in.  It's just that I can't think of many features that have needed it.
I will look into this.

> 
> > > 
> > > WebKit/chromium/src/DeviceOrientationClientImpl.h:40
> > >  +  } // namespace WebCore
> > > don't need the //
> > Will fix. What's the convention for this? Looking at the guidelines, they seem to put a comment like this at the closing of most namespaces, but not in every case?
> 
> Cases like these are too obvious to bother with.  Where in the style guideline does it mandate this though?
I don't think it's mandated, but they seem to do it like this in the examples involving namespaces.

[Darin Fisher (:fishd, Google)]

> > WebKit/chromium/public/WebDeviceOrientationServiceBridge.h:37
> >  +  class WebDeviceOrientationServiceBridge {
> > usually we use the term Client to describe the interface that receives
> > notifications from some service.
> > 
> > Foo has a FooClient
> > FooService has a FooServiceClient
> > 
> > Is there some reason why Bridge is better than Client?
> Well, there is already a DeviceOrientationClient, but on the WebCore side. And 
> that's why we provide a bridge: so the service can send notifications to it (the 
> bridge is being renamed to just WebDeviceOrientationBridge, btw). I guess we could 
> also have a WebKit::DeviceOrientationClient, but I think that would be more 
> confusing?

My suggestion is to name this interface WebKit::WebDeviceOrientationClient.  It is
a reflection of WebCore::DeviceOrientationClient, so naming it consistently makes
good sense.  DeviceOrientationClientImpl is an adapter between DeviceOrientationClient
and WebDeviceOrientationClient.  We have several examples of this very pattern already.


> > WebKit/chromium/public/WebDeviceOrientationService.h:41
> >  +      virtual void attachBridge(WebDeviceOrientationServiceBridge* bridge) { WEBKIT_ASSERT_NOT_REACHED(); }
> > can you have more than one bridge attached?  or is it limited
> > to just one bridge attached at a time?
> It's built to be able to have multiple bridges attached at the same time.

I guess I was wondering why that is necessary.


> > have you considered just adding these methods to WebViewClient,
> > maybe named a bit differently?
> I hadn't, as I started off mimicking the geolocation design. I guess one could certainly put 
> it in there, but would it be preferable? I kind of like having it in its own class, as 
> WebViewClient is quite large, but it's not something I feel strongly about.

Notice that geolocation uses the terms Bridge even at the WebCore level.

I'm concerned that this patch is way more complicated than it needs to be.  I'm not even
convinced that WebDeviceOrientationClient is necessary now.

When I look at the WebCore interfaces, I just see DeviceOrientationClient and
DeviceOrientationController.  It seems like you want to route the client methods to the
embedder and have the embedder call the controller method when the orientation changes.
Given that there are only 3 methods in question, perhaps it would be simpler to just
add those to WebView and WebViewClient.

If you wanted to create sub-interfaces to help group things, then I think you can do so like so:

  class WebDeviceOrientationClient {
  public:
      virtual void startUpdating() = 0;
      virtual void stopUpdating() = 0;
  };

  class WebDeviceOrientationController {
  public:
      virtual void setClient(WebDeviceOrientationClient*) = 0;
      virtual void didChangeDeviceOrientation(double, double, double) = 0;
  };

  class WebView {
  public:
      ...
      virtual WebDeviceOrientationController* deviceOrientationController() = 0;
  };

But to be honest, I would prefer that you just add the methods to WebView and WebViewClient.  I don't
think there are enough methods here to warrant the additional interfaces.  Or, do you plan to add
more in the future?

Implementation wise, you can make DeviceOrientationClientImpl implement both the 
WebDeviceOrientationClient and WebDeviceOrientationController interfaces.  It can
be owned by the WebViewImpl just like the EditorClientImpl and friends.

NOTE:  I'm surprised by the name "onDeviceOrientationChange" used by DeviceOrientationController.  That
doesn't match WebKit conventions, which is usually to avoid "on" in favor of "did".  That's why I
suggested calling the corresponding WebKit API method didChangeDeviceOrientation.

[Hans Wennborg]

Sorry for the delay. The underlying code is now all in place. Thank you very much for all comments so far.

Uploading rebased patch, refactored to use Darin's suggestion of just adding the methods to WebView and WebViewClient which simplified the design a great deal.

[Hans Wennborg]

Created attachment 62295 [details]
Patch

[WebKit Review Bot]

Attachment 62295 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3413726

[Hans Wennborg]

Rebasing and hoping it will build on the cr-linux bot (WebKit/chromium/DEPS referred to too old Chromium revision last time).

Also: ping?

[Hans Wennborg]

Created attachment 62811 [details]
Patch

[WebKit Review Bot]

Attachment 62811 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3610365

[Jeremy Orlow]

Darin, are you satisfied?

Hans, can you please rebase this (since Steve just refactored the page clien code a bit)?

[Steve Block]

You might want to hold of on submitting this until Bug 39589 is fixed. That bug attempts to add a shared mock for testing DeviceOrientation and may affect the plumbing for the DeviceOrientationClient in WebKit. Unfortunately, it's still embroiled in significant debate. Comments welcome!

[Hans Wennborg]

Rebasing.

The reason it doesn't build on cr-linux is I forgot to add the compile-time enable flag in features.gypi. That is on its way in in Bug 43122.

[Hans Wennborg]

Created attachment 62840 [details]
Patch

[WebKit Review Bot]

Attachment 62840 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3399978

[Dean Jackson]

I have some (probably stupid) questions about the design that will help me do the same for AccelerometerEvents. I expect I misunderstand the way WebKit approaches these things, so sorry in advance.

Why do you need the didChangeDeviceOrientation method on WebView? My thought was that your real implementation would be the ClientImpl (or an instance of it), and when the orientation actually changes it gets to the controller via the view -> page -> controller link. It seems weird (to me) that you have the ClientImpl called by WebView, and then have it go back to the view to call the controller. 

In other words, why doesn't your platform implementation of the client just call directly into the controller?

Here's what I was thinking of doing (for Accelerometer):

A WebAccelerometerClient class in WebKit/WebCoreSupport that implements the WebCore::AccelerometerClient interface. Then the real implementation is a WebAccelerometerClientCoreMotion object. This is instantiated in the WebView platform code, and passed in as you construct the Page (and thus given to the AccelerometerController). 

That way we don't need the start/stopDeviceOrientationUpdates methods on the WebView client either. The controller already tells the client when to start or stop updating. Or is there some reason you want WebView to be able to start/stop?

Again, sorry if these questions show my lack of understanding!

[Dean Jackson]

Partially answering my own question: you need the API on WebView because the Page is private (at least it is in the mac build).

[Hans Wennborg]

Comment on attachment 62840 [details]
Patch

Cancelling review request for now as the design might change a bit after Bug 39589 went in.

[Hans Wennborg]

Bug 43258 ended up doing the whole plumbing, so marking this fixed.