[webkit-reviews] review denied: [Bug 36535] [chromium] Rename / tidy up Geolocation bridge : [Attachment 51632] Jorlow comments

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Jeremy Orlow
<jorlow 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 51632: Jorlow comments
https://bugs.webkit.org/attachment.cgi?id=51632&action=review

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