[webkit-reviews] review requested: [Bug 83877] [GTK][WK2] Implement geolocation provider for the GTK port : [Attachment 145323] Patch proposal

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 83877: [GTK][WK2] Implement geolocation provider for the GTK port
https://bugs.webkit.org/show_bug.cgi?id=83877

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

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
I'm attaching a new patch addressing the issues pointed by Martin.

See some comments below...

(In reply to comment #22)
> (From update of attachment 145056 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=145056&action=review
> 
> Looks good, though I've listed a few issues below and it looks like the build
failed.

The build will fail until we get the patch for bug 87800 in, I'm afraid :-)

> > Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationProvider.cpp:71
> > +	     WKGeolocationManagerRef
wkGeolocationManager(WKContextGetGeolocationManager(m_wkContext.get()));
> > +	     WKGeolocationPositionRef
wkGeolocationPosition(WKGeolocationPositionCreate(timestamp, latitude,
longitude, accuracy));
> 
> I think you are leaking the position here. These are just pointers, so you
should use the assignment style, not the constructor style:

I fixed it by doing this:
WKRetainPtr<WKGeolocationPositionRef> wkGeolocationPosition(AdoptWK,
WKGeolocationPositionCreate(timestamp, latitude, longitude, accuracy));

> WKGeolocationManagerRef wkGeolocationManager =
WKContextGetGeolocationManager(m_wkContext.get());
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationProvider.cpp:80
> > +	     WKGeolocationManagerRef
wkGeolocationManager(WKContextGetGeolocationManager(m_wkContext.get()));
> 
> Ditto.

Fixed (and also above, in the other occurrence in this file).

> > Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationProvider.cpp:108
> > +	 static GeolocationProviderClientInfo clientInfo;
> 
> I don't think you want to make this static. Theoretically there may be more
than one WebKitWebContext.

Yes, you're right. Also, as per a conversation on IRC with Carlos, it seems
clientInfo should be the WebKitWebContext object and not that new class I'm
definiing, which acts also as the listener for the Geoclue-based provider in
WebCore.

So, to address both your issues and Carlos's, I moved that new class
declaration to WebKitWebContextPrivate.h and added a new function there to get
that GeolocationProviderClient object out of the WebKitWebContext object
available as clientInfo. So, from now on, startUpdating and stopUpdating
functions in WebKitGeolocationProvider.cpp will just get that object and call
startUpdating() and stopUpdating() over it, which will rely on the
Geoclue-based provider, which finally will call our functions
notifyPositionChanged() and notifyErrorOccurred() once some information has
been retrieved.