[webkit-reviews] review granted: [Bug 36535] [chromium] Rename / tidy up Geolocation bridge : [Attachment 51898] Darin's comments.

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has granted Marcus Bulach
<bulach at chromium.org>'s request for review:
Bug 36535: [chromium] Rename / tidy up Geolocation bridge
https://bugs.webkit.org/show_bug.cgi?id=36535

Attachment 51898: Darin's comments.
https://bugs.webkit.org/attachment.cgi?id=51898&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> 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?