[webkit-reviews] review denied: [Bug 36535] Rename / tidy up Geolocation bridge : [Attachment 51508] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Marcus Bulach
<bulach at chromium.org>'s request for review:
Bug 36535: Rename / tidy up Geolocation bridge
https://bugs.webkit.org/show_bug.cgi?id=36535

Attachment 51508: Patch
https://bugs.webkit.org/attachment.cgi?id=51508&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
> 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?