[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 #11 from Marcus Bulach <bulach at chromium.org>  2010-03-29 04:05:52 PST ---
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.

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