[chromium] Rename / tidy up Geolocation bridge

• Component: WebKit API
• Status: RESOLVED FIXED

[Marcus Bulach]

Follow up on Darin's comments on:
https://bugs.webkit.org/show_bug.cgi?id=32068

Summary:
+ Rename GeolocationServiceBridgeChromium.cpp to WebGeolocationServiceBridgeImpl.cpp
+ Removes Interface suffix
+ Separates classes in their own files
+ Uses a struct for geoposition instead of individual params

[Marcus Bulach]

Created attachment 51500 [details]
Patch

[Marcus Bulach]

Created attachment 51502 [details]
Patch

[Marcus Bulach]

Created attachment 51508 [details]
Patch

[Jeremy Orlow]

The change log needs to be more detailed.  Add a fixme explaining that GeolocationServiceBridgeChromium.h should go away.  And in the change log, explain how the GeolocationServiceBridgeChromium.h is just a compat layer now.

[Jeremy Orlow]

Comment on attachment 51508 [details]
Patch

> Index: WebKit/chromium/public/GeolocationServiceBridgeChromium.h
> ===================================================================
> --- WebKit/chromium/public/GeolocationServiceBridgeChromium.h	(revision 56438)
> +++ WebKit/chromium/public/GeolocationServiceBridgeChromium.h	(working copy)
> @@ -31,43 +31,15 @@
>  #ifndef GeolocationServiceBridgeChromium_h
>  #define GeolocationServiceBridgeChromium_h
>  
> -namespace WebCore {
> -class GeolocationServiceBridge;
> -class GeolocationServiceChromium;
> -}
> +#include "WebGeolocationService.h"
>  
>  namespace WebKit {
>  
> -class WebString;
> -class WebURL;
> -
> -// Provides a WebKit API called by the embedder.
> -class WebGeolocationServiceBridge {
> -public:
> -    virtual void setIsAllowed(bool allowed) = 0;
> -    virtual void setLastPosition(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) = 0;
> -    virtual void setLastError(int errorCode, const WebString& message) = 0;
> -};
> -
> -// Provides an embedder API called by WebKit.
> -class WebGeolocationServiceInterface {
> -public:
> -    virtual void requestPermissionForFrame(int bridgeId, const WebURL& url) = 0;
> -    virtual void startUpdating(int bridgeId, const WebURL& url, bool enableHighAccuracy) = 0;
> -    virtual void stopUpdating(int bridgeId) = 0;
> -    virtual void suspend(int bridgeId) = 0;
> -    virtual void resume(int bridgeId) = 0;
> -
> -    // Attaches the GeolocationBridge to the embedder and returns its id, which
> -    // should be used on subsequent calls for the methods above.
> -    virtual int attachBridge(WebKit::WebGeolocationServiceBridge* geolocationServiceBridge) = 0;
> -
> -    // Dettaches the GeolocationService from the embedder.
> -    virtual void dettachBridge(int bridgeId) = 0;
> +// TODO(bulach): remove this file, this is a temporary compatibility layer for

s/TODO(bulach)/FIXME/

> +// renaming WebGeolocationServiceInterface to WebGeolocationService.
> +class WebGeolocationServiceInterface : public WebGeolocationService {
>  };
>  
> -WebCore::GeolocationServiceBridge* createGeolocationServiceBridgeImpl(WebCore::GeolocationServiceChromium*);
> -
>  } // namespace WebKit
>  
>  #endif // GeolocationServiceBridgeChromium_h



> Index: WebKit/chromium/public/WebGeolocationServiceBridge.h
> ===================================================================
> --- WebKit/chromium/public/WebGeolocationServiceBridge.h	(revision 56438)
> +++ WebKit/chromium/public/WebGeolocationServiceBridge.h	(working copy)
> @@ -28,8 +28,8 @@
>   * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
>  
> -#ifndef GeolocationServiceBridgeChromium_h
> -#define GeolocationServiceBridgeChromium_h
> +#ifndef WebGeolocationServiceBridge_h
> +#define WebGeolocationServiceBridge_h
>  
>  namespace WebCore {
>  class GeolocationServiceBridge;
> @@ -44,30 +44,27 @@ class WebURL;
>  // Provides a WebKit API called by the embedder.
>  class WebGeolocationServiceBridge {
>  public:
> +    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;
> +    };
>      virtual void setIsAllowed(bool allowed) = 0;
> +    virtual void setLastPosition(const WebGeoposition& geoposition) = 0;
> +    // TODO(bulach): remove this method.
>      virtual void setLastPosition(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) = 0;
>      virtual void setLastError(int errorCode, const WebString& message) = 0;

Don't these need to be hooked up to each other (i.e. call each other) to keep things working?


> Index: WebKit/chromium/public/WebViewClient.h
> ===================================================================
> --- WebKit/chromium/public/WebViewClient.h	(revision 56438)
> +++ WebKit/chromium/public/WebViewClient.h	(working copy)
> @@ -31,6 +31,7 @@
>  #ifndef WebViewClient_h
>  #define WebViewClient_h
>  
> +#include "GeolocationServiceBridgeChromium.h"

Er...shouldn't you use the new name?

>  #include "WebDragOperation.h"
>  #include "WebEditingAction.h"
>  #include "WebFileChooserCompletion.h"
> @@ -46,7 +47,6 @@ class WebAccessibilityObject;
>  class WebDragData;
>  class WebFileChooserCompletion;
>  class WebFrame;
> -class WebGeolocationServiceInterface;
>  class WebNode;
>  class WebNotificationPresenter;
>  class WebRange;
> @@ -288,6 +288,9 @@ public:
>      // Geolocation ---------------------------------------------------------
>  
>      // Access the embedder API for geolocation services.
> +    virtual WebKit::WebGeolocationService* geolocationService() { return getGeolocationService(); }
> +
> +    // TODO(bulach): this is a temporary compatibility layer, remove it.
>      virtual WebKit::WebGeolocationServiceInterface* getGeolocationService() { return 0; }

This should return a geolocation service as well.  No need to break things in the middle.



> Index: WebKit/chromium/src/WebGeolocationServiceBridgeImpl.h
> ===================================================================
> --- WebKit/chromium/src/WebGeolocationServiceBridgeImpl.h	(revision 56438)
> +++ WebKit/chromium/src/WebGeolocationServiceBridgeImpl.h	(working copy)
> @@ -28,8 +28,8 @@
>   * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
>  
> -#ifndef GeolocationServiceBridgeChromium_h
> -#define GeolocationServiceBridgeChromium_h
> +#ifndef WebGeolocationServiceBridgeImpl_h
> +#define WebGeolocationServiceBridgeImpl_h
>  
>  namespace WebCore {
>  class GeolocationServiceBridge;
> @@ -37,37 +37,10 @@ class GeolocationServiceChromium;
>  }
>  
>  namespace WebKit {
> -
> -class WebString;
> -class WebURL;
> -
> -// Provides a WebKit API called by the embedder.
> -class WebGeolocationServiceBridge {
> -public:
> -    virtual void setIsAllowed(bool allowed) = 0;
> -    virtual void setLastPosition(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) = 0;
> -    virtual void setLastError(int errorCode, const WebString& message) = 0;
> -};
> -
> -// Provides an embedder API called by WebKit.
> -class WebGeolocationServiceInterface {
> -public:
> -    virtual void requestPermissionForFrame(int bridgeId, const WebURL& url) = 0;
> -    virtual void startUpdating(int bridgeId, const WebURL& url, bool enableHighAccuracy) = 0;
> -    virtual void stopUpdating(int bridgeId) = 0;
> -    virtual void suspend(int bridgeId) = 0;
> -    virtual void resume(int bridgeId) = 0;
> -
> -    // Attaches the GeolocationBridge to the embedder and returns its id, which
> -    // should be used on subsequent calls for the methods above.
> -    virtual int attachBridge(WebKit::WebGeolocationServiceBridge* geolocationServiceBridge) = 0;
> -
> -    // Dettaches the GeolocationService from the embedder.
> -    virtual void dettachBridge(int bridgeId) = 0;
> -};
> -
> +#if WEBKIT_IMPLEMENTATION
>  WebCore::GeolocationServiceBridge* createGeolocationServiceBridgeImpl(WebCore::GeolocationServiceChromium*);
> +#endif

Is this the only thing left in the file?  If so, why does it need to be in public?

[Marcus Bulach]

Created attachment 51632 [details]
Jorlow comments

[Marcus Bulach]

Thanks Jeremy!

Replies and clarifications inline, another look please?

(In reply to comment #5)
> (From update of attachment 51508 [details])
> > Index: WebKit/chromium/public/GeolocationServiceBridgeChromium.h
> > ===================================================================
> > --- WebKit/chromium/public/GeolocationServiceBridgeChromium.h	(revision 56438)
> > +++ WebKit/chromium/public/GeolocationServiceBridgeChromium.h	(working copy)
> > +// TODO(bulach): remove this file, this is a temporary compatibility layer for
> 
> s/TODO(bulach)/FIXME/
> 

Done.

> > Index: WebKit/chromium/public/WebGeolocationServiceBridge.h
> > ===================================================================
> > --- WebKit/chromium/public/WebGeolocationServiceBridge.h	(revision 56438)
> > +++ WebKit/chromium/public/WebGeolocationServiceBridge.h	(working copy)

> > +    virtual void setLastPosition(const WebGeoposition& geoposition) = 0;
> > +    // TODO(bulach): remove this method.
> >      virtual void setLastPosition(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) = 0;
> >      virtual void setLastError(int errorCode, const WebString& message) = 0;
> 
> Don't these need to be hooked up to each other (i.e. call each other) to keep
> things working?
> 

they are both implemented in the (new) WebGeolocationServiceBridgeImpl.cc.
I didn't call each other though, just changed both impl to call m_GeolocationServiceChromium->setLastPosition(geoposition);
The old one will go away as soon as the other side stops calling it.

> 
> > Index: WebKit/chromium/public/WebViewClient.h
> > ===================================================================
> > --- WebKit/chromium/public/WebViewClient.h	(revision 56438)
> > +++ WebKit/chromium/public/WebViewClient.h	(working copy)
> > @@ -31,6 +31,7 @@
> >  #ifndef WebViewClient_h
> >  #define WebViewClient_h
> >  
> > +#include "GeolocationServiceBridgeChromium.h"
> 
> Er...shouldn't you use the new name?
> 

Not yet: I added a FIXME, this is necessary so that the other side can access the old method:
virtual WebKit::WebGeolocationServiceInterface* getGeolocationService() { return 0; }


> >      // Access the embedder API for geolocation services.
> > +    virtual WebKit::WebGeolocationService* geolocationService() { return getGeolocationService(); }
> > +
> > +    // TODO(bulach): this is a temporary compatibility layer, remove it.
> >      virtual WebKit::WebGeolocationServiceInterface* getGeolocationService() { return 0; }
> 
> This should return a geolocation service as well.  No need to break things in
> the middle.
> 

As above, the other side still needs the second method. Clarified the fixme.

> > Index: WebKit/chromium/src/WebGeolocationServiceBridgeImpl.h
> > ===================================================================
> > --- WebKit/chromium/src/WebGeolocationServiceBridgeImpl.h	(revision 56438)
> > +++ WebKit/chromium/src/WebGeolocationServiceBridgeImpl.h	(working copy)
> > -
> > +#if WEBKIT_IMPLEMENTATION
> >  WebCore::GeolocationServiceBridge* createGeolocationServiceBridgeImpl(WebCore::GeolocationServiceChromium*);
> > +#endif
> 
> Is this the only thing left in the file?  If so, why does it need to be in
> public?

this in the /src directory, isn't it ok? otherwise I'd have to publish GeolocationServiceBridgeImpl class definition here..

[Jeremy Orlow]

Comment on attachment 51632 [details]
Jorlow comments

Darin, please take a final look at this?

[Darin Fisher (:fishd, Google)]

Comment on attachment 51632 [details]
Jorlow comments

> Index: WebKit/chromium/public/WebGeolocationService.h
...
>      // Attaches the GeolocationBridge to the embedder and returns its id, which
>      // should be used on subsequent calls for the methods above.
> +    virtual int attachBridge(WebGeolocationServiceBridge* geolocationServiceBridge) = 0;

nit: no need for a parameter name that just matches the type name.
only name parameters if they add information.  this is a webkit
style thing.

nit: the comment says "GeolocationBridge" but I think you mean
WebGeolocationServiceBridge.


>      // Dettaches the GeolocationService from the embedder.
>      virtual void dettachBridge(int bridgeId) = 0;

This method is misspelled, right?  It should be detachBridge, no?


> Index: WebKit/chromium/public/WebGeolocationServiceBridge.h
> +#ifndef WebGeolocationServiceBridge_h
> +#define WebGeolocationServiceBridge_h
>  
>  namespace WebCore {
>  class GeolocationServiceBridge;
> @@ -44,30 +44,27 @@ class WebURL;
>  // Provides a WebKit API called by the embedder.
>  class WebGeolocationServiceBridge {
>  public:
> +    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;
> +    };

nit: the indentation of this structure is wrong.  it should be 4
white spaces.

nit: we only use the Web prefix for toplevel types.  how about
either moving this WebGeoposition struct to the toplevel (my
preference would be to do so and put it in a new file), or rename
it to Geoposition, but then the name conflicts with the one in the
WebCore namespace.  WebGeoposition.h!  the webkit style rule is to
have one type per file, and it is a good sign that this should be
a separate file since the corresponding WebCore type is in a
separate file.


>      virtual void setIsAllowed(bool allowed) = 0;
> +    virtual void setLastPosition(const WebGeoposition& geoposition) = 0;

nit: do not name parameters if it does not add information


> Index: WebKit/chromium/public/WebViewClient.h

> +    // FIXME: this is a temporary compatibility layer, remove it.
>      virtual WebKit::WebGeolocationServiceInterface* getGeolocationService() { return 0; }

please use DEPRECATED for things like this that need to be removed.
it will be easier to find these and clean them up later if we use
consistent tagging.


> Index: WebKit/chromium/src/WebGeolocationServiceBridgeImpl.cpp
> ===================================================================
> --- WebKit/chromium/src/WebGeolocationServiceBridgeImpl.cpp	(revision 56438)
> +++ WebKit/chromium/src/WebGeolocationServiceBridgeImpl.cpp	(working copy)
> @@ -30,7 +30,7 @@
>  
>  #include "config.h"
>  
> +#include "WebGeolocationServiceBridgeImpl.h"

nit: no new line after config.h here


> Index: WebKit/chromium/src/WebGeolocationServiceBridgeImpl.h

> +#if WEBKIT_IMPLEMENTATION
>  WebCore::GeolocationServiceBridge* createGeolocationServiceBridgeImpl(WebCore::GeolocationServiceChromium*);
> +#endif

In the src/ directory, there is no need for WEBKIT_IMPLEMENTATION
guards.  They are only useful in the public/ directory.

[Marcus Bulach]

Created attachment 51898 [details]
Darin's comments.

[Marcus Bulach]

Thanks Darin!

All comments addressed, another look please?

(In reply to comment #9)
> (From update of attachment 51632 [details])
> > Index: WebKit/chromium/public/WebGeolocationService.h
> ...
> >      // Attaches the GeolocationBridge to the embedder and returns its id, which
> >      // should be used on subsequent calls for the methods above.
> > +    virtual int attachBridge(WebGeolocationServiceBridge* geolocationServiceBridge) = 0;
> 
> nit: no need for a parameter name that just matches the type name.
> only name parameters if they add information.  this is a webkit
> style thing.
Done.

> 
> nit: the comment says "GeolocationBridge" but I think you mean
> WebGeolocationServiceBridge.
Done.

> 
> 
> >      // Dettaches the GeolocationService from the embedder.
> >      virtual void dettachBridge(int bridgeId) = 0;
> 
> This method is misspelled, right?  It should be detachBridge, no?

Ops, my bad. Added a new method with the correct spelling, DEPRECATED this.
Will remove it as soon as it lands on the other side.

> 
> 
> > Index: WebKit/chromium/public/WebGeolocationServiceBridge.h
> > +#ifndef WebGeolocationServiceBridge_h
> > +#define WebGeolocationServiceBridge_h
> >  
> >  namespace WebCore {
> >  class GeolocationServiceBridge;
> > @@ -44,30 +44,27 @@ class WebURL;
> >  // Provides a WebKit API called by the embedder.
> >  class WebGeolocationServiceBridge {
> >  public:
> > +    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;
> > +    };
> 
> nit: the indentation of this structure is wrong.  it should be 4
> white spaces.
Done (in the separate file).

> 
> nit: we only use the Web prefix for toplevel types.  how about
> either moving this WebGeoposition struct to the toplevel (my
> preference would be to do so and put it in a new file), or rename
> it to Geoposition, but then the name conflicts with the one in the
> WebCore namespace.  WebGeoposition.h!  the webkit style rule is to
> have one type per file, and it is a good sign that this should be
> a separate file since the corresponding WebCore type is in a
> separate file.

Thanks for the explanation. Done as a separate WebGeoposition.h file.

> 
> 
> >      virtual void setIsAllowed(bool allowed) = 0;
> > +    virtual void setLastPosition(const WebGeoposition& geoposition) = 0;
> 
> nit: do not name parameters if it does not add information
Done.

> 
> 
> > Index: WebKit/chromium/public/WebViewClient.h
> 
> > +    // FIXME: this is a temporary compatibility layer, remove it.
> >      virtual WebKit::WebGeolocationServiceInterface* getGeolocationService() { return 0; }
> 
> please use DEPRECATED for things like this that need to be removed.
> it will be easier to find these and clean them up later if we use
> consistent tagging.

Done.

> 
> 
> > Index: WebKit/chromium/src/WebGeolocationServiceBridgeImpl.cpp
> > ===================================================================
> > --- WebKit/chromium/src/WebGeolocationServiceBridgeImpl.cpp	(revision 56438)
> > +++ WebKit/chromium/src/WebGeolocationServiceBridgeImpl.cpp	(working copy)
> > @@ -30,7 +30,7 @@
> >  
> >  #include "config.h"
> >  
> > +#include "WebGeolocationServiceBridgeImpl.h"
> 
> nit: no new line after config.h here

Done.

> 
> 
> > Index: WebKit/chromium/src/WebGeolocationServiceBridgeImpl.h
> 
> > +#if WEBKIT_IMPLEMENTATION
> >  WebCore::GeolocationServiceBridge* createGeolocationServiceBridgeImpl(WebCore::GeolocationServiceChromium*);
> > +#endif
> 
> In the src/ directory, there is no need for WEBKIT_IMPLEMENTATION
> guards.  They are only useful in the public/ directory.

Done.

[Darin Fisher (:fishd, Google)]

Comment on attachment 51898 [details]
Darin's comments.

> 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?

[Marcus Bulach]

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.

[Jeremy Orlow]

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

Unless otherwise specified, the r+ is typically contingent on you making the changes before committing.

[Marcus Bulach]

Created attachment 51943 [details]
More comments

[Marcus Bulach]

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?

[Darin Fisher (:fishd, Google)]

(In reply to comment #13)
> 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.

Sorry, r+ means you can commit.  I am happy if you address the remaining
feedback now or later or not at all pending further discussion.


> > 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?

We generally add default implementations to all methods.  Note:  There are
some older interfaces where this wasn't the case.  The decision to add
default implementations happened mid-way through the API development.  It
just makes life easier when you wish to change the API.


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

I see.  The problem seems to stem from the Coordinates constructor, which
takes a big list of arguments.  I think that was probably a poor design
choice since it suffers from the problem that led me to encourage you to
break up setLastPosition.

If Coordinates instead had a bunch of setters, then it would be easy to
create a WebGeoposition object with a similar array of setters.  I guess
the downside to that approach is that someone might forget to call one of
the setters.

My preference would be to change Coordinates to have setters with each
property having a reasonable default value.  Then, make a WebGeoposition
object that wraps Geoposition and provides similar setters (and getters).

Of course, this seems like a lot of work for the benefit of a single method.
If you want to ditch WebGeoposition and make setLastPosition have a
bazillion parameters again, I won't object.

[Marcus Bulach]

Created attachment 52020 [details]
Final round of comments. Reverts back to multiple params instead of a WebGeolocation struct.

[Marcus Bulach]

Thanks Darin!
Replies inline, would you mind a final look please?

(In reply to comment #17)
> (In reply to comment #13)
> > 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.
> 
> Sorry, r+ means you can commit.  I am happy if you address the remaining
> feedback now or later or not at all pending further discussion.

Ahn, ok, thanks for the clarification!
I went back one step and removed the struct, is now back to multiple params. More details below.

> 
> 
> > > 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?
> 
> We generally add default implementations to all methods.  Note:  There are
> some older interfaces where this wasn't the case.  The decision to add
> default implementations happened mid-way through the API development.  It
> just makes life easier when you wish to change the API.

Got it, I probably based on some older interface when implemented this, thanks for the heads up.

> 
> 
> > > 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.
> 
> I see.  The problem seems to stem from the Coordinates constructor, which
> takes a big list of arguments.  I think that was probably a poor design
> choice since it suffers from the problem that led me to encourage you to
> break up setLastPosition.
> 
> If Coordinates instead had a bunch of setters, then it would be easy to
> create a WebGeoposition object with a similar array of setters.  I guess
> the downside to that approach is that someone might forget to call one of
> the setters.
> 
> My preference would be to change Coordinates to have setters with each
> property having a reasonable default value.  Then, make a WebGeoposition
> object that wraps Geoposition and provides similar setters (and getters).
> 
> Of course, this seems like a lot of work for the benefit of a single method.
> If you want to ditch WebGeoposition and make setLastPosition have a
> bazillion parameters again, I won't object.

I reverted back to multiple params so that we can get the rest of the change in place.

I'm happy to follow up on this, but I'd appreciate your help clarifying the following:
There's a Coordinates.idl which marks all fields as read-only, afaict that's what's exposed to javascript.
Is there any constraint in diverging the .idl and the .h? That is, adding the list of setters (rather than passing all params at ctor / factory time), might introduce some confusion.
If there's no such constraint, I'll add the setters and a factory method with no params as a separate change.
Thoughts?

[Darin Fisher (:fishd, Google)]

Comment on attachment 52020 [details]
Final round of comments. Reverts back to multiple params instead of a WebGeolocation struct.

No worries about refactoring the API to introduce WebGeoposition at this point.  I'm not sure it is worth your time.  Sorry for the distraction!

[WebKit Commit Bot]

Comment on attachment 52020 [details]
Final round of comments. Reverts back to multiple params instead of a WebGeolocation struct.

Clearing flags on attachment: 52020

Committed r56803: <http://trac.webkit.org/changeset/56803>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.