[Webkit-unassigned] [Bug 83877] [GTK][WK2] Implement geolocation provider for the GTK port

[bugzilla-daemon at webkit.org]

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


Mario Sanchez Prada <msanchez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #145056|0                           |1
        is obsolete|                            |
 Attachment #145323|                            |review?
               Flag|                            |




--- Comment #23 from Mario Sanchez Prada <msanchez at igalia.com>  2012-06-01 09:08:36 PST ---
Created an attachment (id=145323)
 --> (https://bugs.webkit.org/attachment.cgi?id=145323&action=review)
Patch proposal

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.

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