Bug 83877

Summary: [GTK][WK2] Implement geolocation provider for the GTK port
Product: WebKit Reporter: Mario Sanchez Prada <mario@webkit.org>
Component: WebKit GtkAssignee: Mario Sanchez Prada <mario@webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia@igalia.com, dglazkov@chromium.org, gns@gnome.org, mrobinson@webkit.org, philn@igalia.com, pnormand@igalia.com, webkit.review.bot@gmail.com, xan.lopez@gmail.com
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 87800    
Bug Blocks: 83876, 83879    
Attachments:
Description Flags
Patch proposal
none
Patch proposal
none
Patch proposal
pnormand: commit‑queue-
1. Added new and reusable Geoclue-based geolocation provider to WebCore
none
2. Make GeolocationClient for WebKitGTK+ use the new Geoclue-based geolocation provider
none
3. Add a new client-based geolocation provider for WebKit2GTK+ based on the new Geoclue-based geolocation provider
none
Patch proposal
none
Patch proposal
mrobinson: review-, gns: commit‑queue-
Patch proposal
none
Patch proposal
webkit.review.bot: commit‑queue-
Archive of layout-test-results from ec2-cr-linux-01
none
Patch proposal
gns: commit‑queue-
Patch proposal cgarcia: review+

Description From 2012-04-13 03:32:09 PST
We need to implement a Geolocation provider for WebKit2GTK+ (Geoclue based)
------- Comment #1 From 2012-04-13 03:50:53 PST -------
Created an attachment (id=137070) [details]
Patch proposal

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

Asking for review
------- Comment #2 From 2012-04-16 00:57:30 PST -------
(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.

> 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
------- Comment #3 From 2012-04-16 01:25:27 PST -------
(From update of attachment 137070 [details])
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
------- Comment #4 From 2012-04-16 01:37:59 PST -------
Created an attachment (id=137301) [details]
Patch proposal

Addressed issues pointed out by Carlos
------- Comment #5 From 2012-04-16 01:42:14 PST -------
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.
------- Comment #6 From 2012-04-16 01:53:04 PST -------
Created an attachment (id=137304) [details]
Patch proposal
------- Comment #7 From 2012-04-16 02:05:37 PST -------
(From update of attachment 137304 [details])
Attachment 137304 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12415072
------- Comment #8 From 2012-04-17 00:11:37 PST -------
(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.
------- Comment #9 From 2012-04-18 00:41:44 PST -------
(In reply to comment #2)
> (From update of attachment 137070 [details] [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] [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
------- Comment #10 From 2012-05-29 17:16:42 PST -------
(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.
------- Comment #11 From 2012-05-29 17:32:09 PST -------
Created an attachment (id=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)
------- Comment #12 From 2012-05-29 17:33:27 PST -------
Created an attachment (id=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.
------- Comment #13 From 2012-05-29 17:34:27 PST -------
Created an attachment (id=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.
------- Comment #14 From 2012-05-29 17:35:02 PST -------
Do you mind splitting these out to one patch-per-bug?
------- Comment #15 From 2012-05-29 18:00:35 PST -------
(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
------- Comment #16 From 2012-05-29 18:17:45 PST -------
(From update of attachment 144656 [details])
Obsolete, as this will be tracked in bug 87800
------- Comment #17 From 2012-05-29 18:17:55 PST -------
(From update of attachment 144657 [details])
Obsolete, as this will be tracked in bug 87801
------- Comment #18 From 2012-05-31 02:54:53 PST -------
Created an attachment (id=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)
------- Comment #19 From 2012-05-31 02:59:14 PST -------
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
------- Comment #20 From 2012-05-31 05:20:35 PST -------
Created an attachment (id=145056) [details]
Patch proposal

New patch fixing a mess with the makefile (some files added there should not be part of this patch)
------- Comment #21 From 2012-05-31 05:31:29 PST -------
(From update of attachment 145056 [details])
Attachment 145056 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12859332
------- Comment #22 From 2012-05-31 10:22:10 PST -------
(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.

> 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.
------- Comment #23 From 2012-06-01 09:08:36 PST -------
Created an attachment (id=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] [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.
------- Comment #24 From 2012-06-01 09:14:50 PST -------
(From update of attachment 145323 [details])
Attachment 145323 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12862658
------- Comment #25 From 2012-06-01 09:22:57 PST -------
Created an attachment (id=145326) [details]
Patch proposal

It seems git format-patch screwed up the previous patch... Uploading a new one
------- Comment #26 From 2012-06-01 09:30:36 PST -------
(From update of attachment 145326 [details])
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.
------- Comment #27 From 2012-06-01 13:26:58 PST -------
(From update of attachment 145326 [details])
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
------- Comment #28 From 2012-06-01 13:27:02 PST -------
Created an attachment (id=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
------- Comment #29 From 2012-06-04 05:40:52 PST -------
Created an attachment (id=145569) [details]
Patch proposal

New patch addressing last issues raised by Carlos
------- Comment #30 From 2012-06-04 05:48:20 PST -------
(From update of attachment 145569 [details])
Attachment 145569 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12894302
------- Comment #31 From 2012-06-04 06:01:21 PST -------
(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

> 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));
------- Comment #32 From 2012-06-04 07:14:17 PST -------
Created an attachment (id=145579) [details]
Patch proposal

(In reply to comment #31)
> (From update of attachment 145569 [details] [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
------- Comment #33 From 2012-06-04 08:36:48 PST -------
(From update of attachment 145579 [details])
Perfect, thanks!
------- Comment #34 From 2012-06-04 09:05:20 PST -------
Committed r119404: <http://trac.webkit.org/changeset/119404>