[Webkit-unassigned] [Bug 36535] [chromium] Rename / tidy up Geolocation bridge

Mon Mar 29 11:54:08 PDT 2010

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

--- Comment #16 from Marcus Bulach <bulach at chromium.org>  2010-03-29 11:54:08 PST ---
Thanks for the clarification, Jeremy!

Darin, I addressed all your comments except the Geolocation struct vs class.

Would you please take a look and let me know what's the preferred way? Thanks!

(In reply to comment #13)
> Thanks Darin!
> 
> General question: after an "r+", should I make the changes your suggested here
> or as a follow up? I'm happy to do either way, I'm just not yet familiar with
> the flow here.
> 
> More inline:
> 
> (In reply to comment #12)
> > (From update of attachment 51898 [details] [details])
> > > Index: WebKit/chromium/public/WebGeolocationService.h
> > ...
> > > +class WebGeolocationService {
> > >  public:
> > >      virtual void requestPermissionForFrame(int bridgeId, const WebURL& url) = 0;
> > >      virtual void startUpdating(int bridgeId, const WebURL& url, bool enableHighAccuracy) = 0;
> > > @@ -58,16 +47,17 @@ public:
> > >      virtual void suspend(int bridgeId) = 0;
> > >      virtual void resume(int bridgeId) = 0;
> > >  
> > > +    // Attaches the WebGeolocationServiceBridge to the embedder and returns its
> > > +    // id, which should be used on subsequent calls for the methods above.
> > > +    virtual int attachBridge(WebGeolocationServiceBridge*) = 0;
> > >  
> > > +    // Detaches the WebGeolocationServiceBridge from the embedder.
> > > +    virtual void detachBridge(int bridgeId) { dettachBridge(bridgeId); }
> > > +
> > > +    // DEPRECATED: this is a temporary compatibility layer, remove this method.
> > >      virtual void dettachBridge(int bridgeId) = 0;
> > >  };
> > 
> > Please note that we generally provide default implementations for all
> > virtual WebKit API methods so that it is easier to deprecate methods.
> > I would recommend doing so for dettachBridge at least.
> 
> Good point. As above, should I do this now or as a follow up?
> Also: should I add default impl for all methods, not just the ones being
> deprecated / changed?
> 
> > 
> > 
> > > Index: WebKit/chromium/public/WebGeoposition.h
> > ...
> > > +struct WebGeoposition {
> > > +    double latitude;
> > > +    double longitude;
> > > +    bool providesAltitude;
> > > +    double altitude;
> > > +    double accuracy;
> > > +    bool providesAltitudeAccuracy;
> > > +    double altitudeAccuracy;
> > > +    bool providesHeading;
> > > +    double heading;
> > > +    bool providesSpeed;
> > > +    double speed;
> > > +    long long timestamp;
> > > +};
> > 
> > It seems like it might be nice to have WEBKIT_IMPLEMENTATION methods
> > for converting to/from WebCore::Geoposition.
> > 
> > Also, since Geoposition is reference counted, the way we'd normally
> > expose it through the WebKit API is to leverage WebPrivatePtr, such
> > that WebGeoposition is really just a wrapper around a Geoposition
> > object.  Is there a reason why a struct as you have done is better?
> > 
> > Are you planning on serializing this struct over Chromium IPC?
> 
> Hmm, good point. On the embedder side, we have our own "Geoposition" datatype,
> which we need to pass to WebKit, then WebCore (it's one way only, there's no
> communication back to the embedder).
> 
> This struct is then only used as you suggested, to avoid a long param list for
> "setLastPosition". If we make turn this into a class aggregating Geoposition
> via WebPrivatePtr, it'd require a ctor with the same long list of params, or a
> long list of setters (for each individual field), no?
> 
> If so, wouldn't it be more straightforward to just keep the long list of param
> on setLastPosition in the first place?
> 
> Please, let me know what's the preferred way.

(In reply to comment #12)
> (From update of attachment 51898 [details])
> > Index: WebKit/chromium/public/WebGeolocationService.h
> ...
> > +class WebGeolocationService {
> >  public:
> >      virtual void requestPermissionForFrame(int bridgeId, const WebURL& url) = 0;
> >      virtual void startUpdating(int bridgeId, const WebURL& url, bool enableHighAccuracy) = 0;
> > @@ -58,16 +47,17 @@ public:
> >      virtual void suspend(int bridgeId) = 0;
> >      virtual void resume(int bridgeId) = 0;
> >  
> > +    // Attaches the WebGeolocationServiceBridge to the embedder and returns its
> > +    // id, which should be used on subsequent calls for the methods above.
> > +    virtual int attachBridge(WebGeolocationServiceBridge*) = 0;
> >  
> > +    // Detaches the WebGeolocationServiceBridge from the embedder.
> > +    virtual void detachBridge(int bridgeId) { dettachBridge(bridgeId); }
> > +
> > +    // DEPRECATED: this is a temporary compatibility layer, remove this method.
> >      virtual void dettachBridge(int bridgeId) = 0;
> >  };
> 
> Please note that we generally provide default implementations for all
> virtual WebKit API methods so that it is easier to deprecate methods.
> I would recommend doing so for dettachBridge at least.
> 
> 
> > Index: WebKit/chromium/public/WebGeoposition.h
> ...
> > +struct WebGeoposition {
> > +    double latitude;
> > +    double longitude;
> > +    bool providesAltitude;
> > +    double altitude;
> > +    double accuracy;
> > +    bool providesAltitudeAccuracy;
> > +    double altitudeAccuracy;
> > +    bool providesHeading;
> > +    double heading;
> > +    bool providesSpeed;
> > +    double speed;
> > +    long long timestamp;
> > +};
> 
> It seems like it might be nice to have WEBKIT_IMPLEMENTATION methods
> for converting to/from WebCore::Geoposition.
> 
> Also, since Geoposition is reference counted, the way we'd normally
> expose it through the WebKit API is to leverage WebPrivatePtr, such
> that WebGeoposition is really just a wrapper around a Geoposition
> object.  Is there a reason why a struct as you have done is better?
> 
> Are you planning on serializing this struct over Chromium IPC?

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