[Webkit-unassigned] [Bug 36535] Rename / tidy up Geolocation bridge

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=36535





--- Comment #7 from Marcus Bulach <bulach at chromium.org>  2010-03-25 07:01:16 PST ---
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..

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