Bug 83877 – [GTK][WK2] Implement geolocation provider for the GTK port

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

We need to implement a Geolocation provider for WebKit2GTK+ (Geoclue based)

[Mario Sanchez Prada]

Created attachment 137070 [details]
Patch proposal

Initial proposal, based in the geolocation provider for the Qt port.

Asking for review

[Carlos Garcia Campos]

Comment on attachment 137070 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=137070&action=review

We implement C API clients in the API layer, so please move this to UIProcess/API/gtk and implement it the same way we do for other C API clients. Maybe we could move common code to WebCore so that wk1 and wk2 could use the same geolocation provider based on geoclue.

> Source/WebKit2/UIProcess/gtk/WebGeolocationProviderGtk.cpp:28
> +

Add #if ENABLE(GEOLOCATION) here.

> Source/WebKit2/UIProcess/gtk/WebGeolocationProviderGtk.cpp:29
> +#include "WKGeolocationPosition.h"

Use angle-bracket form to include WK2 C API headers.

> Source/WebKit2/UIProcess/gtk/WebGeolocationProviderGtk.cpp:80
> +    , m_geoclueClient(0)
> +    , m_geocluePosition(0)

This is already initialized to 0 by GRefPtr

> Source/WebKit2/UIProcess/gtk/WebGeolocationProviderGtk.cpp:118
> +    if (m_geoclueClient)
> +        m_geoclueClient.clear();
> +
> +    if (m_geocluePosition)
> +        m_geocluePosition.clear();

GRefPtr::clear already checks the pointer before unrefing it, so no need to check it here too.

> Source/WebKit2/UIProcess/gtk/WebGeolocationProviderGtk.cpp:128
> +    double horizontalAccuracy = 0.0;

Use 0 instead of 0.0

> Source/WebKit2/UIProcess/gtk/WebGeolocationProviderGtk.cpp:138
> +void WebGeolocationProviderGtk::errorOccured(const char* message) const
> +{
> +    WKGeolocationManagerProviderDidFailToDeterminePosition(m_manager);
> +}

The error message is ignored.

> Source/WebKit2/UIProcess/gtk/WebGeolocationProviderGtk.h:22
> +

Add #if ENABLE(GEOLOCATION) here

[Mario Sanchez Prada]

Comment on attachment 137070 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=137070&action=review

Thanks for the review. I'll address all those issues in a follow-up patch.

>> Source/WebKit2/UIProcess/gtk/WebGeolocationProviderGtk.cpp:28
>> +
> 
> Add #if ENABLE(GEOLOCATION) here.

Ok

>> Source/WebKit2/UIProcess/gtk/WebGeolocationProviderGtk.cpp:29
>> +#include "WKGeolocationPosition.h"
> 
> Use angle-bracket form to include WK2 C API headers.

Ok

>> Source/WebKit2/UIProcess/gtk/WebGeolocationProviderGtk.cpp:80
>> +    , m_geocluePosition(0)
> 
> This is already initialized to 0 by GRefPtr

Ok.

>> Source/WebKit2/UIProcess/gtk/WebGeolocationProviderGtk.cpp:118
>> +        m_geocluePosition.clear();
> 
> GRefPtr::clear already checks the pointer before unrefing it, so no need to check it here too.

Ok.

>> Source/WebKit2/UIProcess/gtk/WebGeolocationProviderGtk.cpp:128
>> +    double horizontalAccuracy = 0.0;
> 
> Use 0 instead of 0.0

Ok

>> Source/WebKit2/UIProcess/gtk/WebGeolocationProviderGtk.cpp:138
>> +}
> 
> The error message is ignored.

Ooops! Ok

>> Source/WebKit2/UIProcess/gtk/WebGeolocationProviderGtk.h:22
>> +
> 
> Add #if ENABLE(GEOLOCATION) here

Ok

[Mario Sanchez Prada]

Created attachment 137301 [details]
Patch proposal

Addressed issues pointed out by Carlos

[WebKit Review Bot]

Attachment 137301 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/gtk/WebGeolocationProviderGtk.cpp:31:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mario Sanchez Prada]

Created attachment 137304 [details]
Patch proposal

[Philippe Normand]

Comment on attachment 137304 [details]
Patch proposal

Attachment 137304 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12415072

[Carlos Garcia Campos]

(In reply to comment #6)
> Created an attachment (id=137304) [details]
> Patch proposal

What I'm proposing is to wrap geoclue API into a common class in WebCore and use that class from GeolocationClientGtk in wk1 and WebKitGeolocationProvider in wk2. 
Instead of implementing the C API in UIProcess/gtk, move it to the API layer, adding WebKitGeolocationProvider implemented the same way we implement all other C API clients.

[Mario Sanchez Prada]

(In reply to comment #2)
> (From update of attachment 137070 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137070&action=review
> 
> We implement C API clients in the API layer, so please move this to UIProcess/API/gtk and implement it the 
> same way we do for other C API clients. Maybe we could move common code to WebCore so that wk1 and 
> wk2 could use the same geolocation provider based on geoclue.

Sorry, I missed this part. Will do that in a follow up patch.

> (In reply to comment #8)
> (In reply to comment #6)
> > Created an attachment (id=137304) [details] [details]
> > Patch proposal
> 
> What I'm proposing is to wrap geoclue API into a common class in WebCore and use that class from 
> GeolocationClientGtk in wk1 and WebKitGeolocationProvider in wk2. 
> Instead of implementing the C API in UIProcess/gtk, move it to the API layer, adding 
> WebKitGeolocationProvider implemented the same way we implement all other C API clients.

Sounds good to me. That's what I will do

[Mario Sanchez Prada]

(In reply to comment #9)
> [...]
> > What I'm proposing is to wrap geoclue API into a common class in WebCore and use that class from 
> > GeolocationClientGtk in wk1 and WebKitGeolocationProvider in wk2. 
> > Instead of implementing the C API in UIProcess/gtk, move it to the API layer, adding 
> > WebKitGeolocationProvider implemented the same way we implement all other C API clients.
> 
> Sounds good to me. That's what I will do

I've been working today on this, and as a result I have the following:

 - Moved geoclue dependant code from WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp
   to WebCore/platform/geoclue, so we can reuse it from WK2 too.

 - Implement WK2GTK+'s geolocation provider in terms of that new class in WebCore, and
   place it in UIProcess/API/gtk, instead of UIProcess/gtk.

I'll be attaching separate patches for these changes soon.

[Mario Sanchez Prada]

Created attachment 144656 [details]
1. Added new and reusable Geoclue-based geolocation provider to WebCore

This is the new class for WebCore, and the interface definition for a Listener that must be implemented from the API layers to get notified when an event happens (new position got, error ocurred)

[Mario Sanchez Prada]

Created attachment 144657 [details]
2. Make GeolocationClient for WebKitGTK+ use the new Geoclue-based geolocation provider

Update code in WebKitGTK's API layer to use the new provider instead of dealing with Geoclue on its own.

[Mario Sanchez Prada]

Created attachment 144660 [details]
3. Add a new client-based geolocation provider for WebKit2GTK+ based on the new Geoclue-based geolocation provider 

New geolocation provider for WebKit2GTK+, using the new geoclue-based provided in WebCore.

[Martin Robinson]

Do you mind splitting these out to one patch-per-bug?

[Mario Sanchez Prada]

(In reply to comment #14)
> Do you mind splitting these out to one patch-per-bug?

No problem:

 * [GTK] Add a new and reusable Geoclue-based geolocation provider in WebCore
   https://bugs.webkit.org/show_bug.cgi?id=87800

 * [GTK] Remove geoclue dependency from WebKit API Layer
   https://bugs.webkit.org/post_bug.cgi

Now making this bug depend on bug 87800 too

[Mario Sanchez Prada]

Comment on attachment 144656 [details]
1. Added new and reusable Geoclue-based geolocation provider to WebCore

Obsolete, as this will be tracked in bug 87800

[Mario Sanchez Prada]

Comment on attachment 144657 [details]
2. Make GeolocationClient for WebKitGTK+ use the new Geoclue-based geolocation provider

Obsolete, as this will be tracked in bug 87801

[Mario Sanchez Prada]

Created attachment 145034 [details]
Patch proposal

New patch, addressing a similar issue to that one spotted by Martin in bug 87801 (https://bugs.webkit.org/show_bug.cgi?id=87801#c2)

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Mario Sanchez Prada]

Created attachment 145056 [details]
Patch proposal

New patch fixing a mess with the makefile (some files added there should not be part of this patch)

[Gustavo Noronha (kov)]

Comment on attachment 145056 [details]
Patch proposal

Attachment 145056 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12859332

[Martin Robinson]

Comment on attachment 145056 [details]
Patch proposal

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.

> 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:

WKGeolocationManagerRef wkGeolocationManager = WKContextGetGeolocationManager(m_wkContext.get());

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

Ditto.

> 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.

[Mario Sanchez Prada]

Created attachment 145323 [details]
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.

[Gustavo Noronha (kov)]

Comment on attachment 145323 [details]
Patch proposal

Attachment 145323 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12862658

[Mario Sanchez Prada]

Created attachment 145326 [details]
Patch proposal

It seems git format-patch screwed up the previous patch... Uploading a new one

[Carlos Garcia Campos]

Comment on attachment 145326 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=145326&action=review

hmm, I think that GeolocationProviderClient could be moved to its own file. It could be a member of the web context private struct, and it could implement the WKGeolocationProvider using it's own pointer as clientInfo. Maybe I'm missing something.

> Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationProvider.cpp:46
> +    static WKGeolocationProvider wkGeolocationProvider = {

Even though webContext is a singleton in this moment, that might change in the future, so don't make this static.

> Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationProvider.cpp:50
> +        startUpdating, // startUpdating
> +        stopUpdating // stopUpdating

Don't ned to keep the comments if you provide an implementation, it's redundant

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:356
> +#if ENABLE(GEOLOCATION)
> +GeolocationProviderClient::GeolocationProviderClient(WebKitWebContext* webContext)

I think we could move this to its won file.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:357
> +    : m_provider(this)

Why do you need this? why not just using the methods directly?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:380
> +    if (!m_wkContext)
> +        return;

Is this really possible?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:402
> +    if (!priv->geolocationProviderClient)
> +        priv->geolocationProviderClient = adoptRef(new GeolocationProviderClient(context));

Do you need to allocate this on the heap? This could just be a member of the private struct.

[WebKit Review Bot]

Comment on attachment 145326 [details]
Patch proposal

Attachment 145326 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12873373

New failing tests:
http/tests/media/media-source/video-media-source-event-attributes.html

[WebKit Review Bot]

Created attachment 145366 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Mario Sanchez Prada]

Created attachment 145569 [details]
Patch proposal

New patch addressing last issues raised by Carlos

[Gustavo Noronha (kov)]

Comment on attachment 145569 [details]
Patch proposal

Attachment 145569 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12894302

[Carlos Garcia Campos]

Comment on attachment 145569 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=145569&action=review

You need to add WebKitGeolocationProvider to the list fo files ignored by gtk-doc, or rename it to WebKitGeolocationClient, since *Client is ignored

> Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationProvider.cpp:34
> +    const WebKitGeolocationProvider* providerClientInfo = static_cast<const WebKitGeolocationProvider*>(clientInfo);
> +    return const_cast<WebKitGeolocationProvider*>(providerClientInfo);

I think this could be just return static_cast<WebKitGeolocationProvider*>(const_cast<void*>(clientInfo));

[Mario Sanchez Prada]

Created attachment 145579 [details]
Patch proposal

(In reply to comment #31)
> (From update of attachment 145569 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145569&action=review
> 
> You need to add WebKitGeolocationProvider to the list fo files ignored by gtk-doc, or rename it to WebKitGeolocationClient, since *Client is ignored

Fixed. Sorry.

> > Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationProvider.cpp:34
> > +    const WebKitGeolocationProvider* providerClientInfo = static_cast<const WebKitGeolocationProvider*>(clientInfo);
> > +    return const_cast<WebKitGeolocationProvider*>(providerClientInfo);
> 
> I think this could be just return static_cast<WebKitGeolocationProvider*>(const_cast<void*>(clientInfo));

You're right. Fixed too

[Carlos Garcia Campos]

Comment on attachment 145579 [details]
Patch proposal

Perfect, thanks!

[Mario Sanchez Prada]

Committed r119404: <http://trac.webkit.org/changeset/119404>