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

[bugzilla-daemon at webkit.org]

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





--- Comment #13 from Marcus Bulach <bulach at chromium.org>  2010-03-29 09:55:36 PST ---
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])
> > 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.

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