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

Tue Jul 13 08:55:54 PDT 2010

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


--- Comment #9 from hans at chromium.org  2010-07-13 08:55:54 PST ---
(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.

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