Bug 83877 - [GTK][WK2] Implement geolocation provider for the GTK port
: [GTK][WK2] Implement geolocation provider for the GTK port
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Mario Sanchez Prada
: Gtk
Depends on: 87800
Blocks: 83876 83879
  Show dependency treegraph
 
Reported: 2012-04-13 03:32 PDT by Mario Sanchez Prada
Modified: 2012-06-04 09:05 PDT (History)
8 users (show)

See Also:


Attachments
Patch proposal (13.90 KB, patch)
2012-04-13 03:50 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (14.14 KB, patch)
2012-04-16 01:37 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (14.14 KB, patch)
2012-04-16 01:53 PDT, Mario Sanchez Prada
pnormand: commit‑queue-
Details | Formatted Diff | Diff
1. Added new and reusable Geoclue-based geolocation provider to WebCore (14.86 KB, patch)
2012-05-29 17:32 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
2. Make GeolocationClient for WebKitGTK+ use the new Geoclue-based geolocation provider (10.42 KB, patch)
2012-05-29 17:33 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
3. Add a new client-based geolocation provider for WebKit2GTK+ based on the new Geoclue-based geolocation provider (13.40 KB, patch)
2012-05-29 17:34 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (13.49 KB, patch)
2012-05-31 02:54 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (13.15 KB, patch)
2012-05-31 05:20 PDT, Mario Sanchez Prada
mrobinson: review-
gns: commit‑queue-
Details | Formatted Diff | Diff
Patch proposal (14.47 KB, patch)
2012-06-01 09:08 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (13.20 KB, patch)
2012-06-01 09:22 PDT, Mario Sanchez Prada
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (527.51 KB, application/zip)
2012-06-01 13:27 PDT, WebKit Review Bot
no flags Details
Patch proposal (13.24 KB, patch)
2012-06-04 05:40 PDT, Mario Sanchez Prada
gns: commit‑queue-
Details | Formatted Diff | Diff
Patch proposal (14.76 KB, patch)
2012-06-04 07:14 PDT, Mario Sanchez Prada
cgarcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2012-04-13 03:32:09 PDT
We need to implement a Geolocation provider for WebKit2GTK+ (Geoclue based)
Comment 1 Mario Sanchez Prada 2012-04-13 03:50:53 PDT
Created attachment 137070 [details]
Patch proposal

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

Asking for review
Comment 2 Carlos Garcia Campos 2012-04-16 00:57:30 PDT
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
Comment 3 Mario Sanchez Prada 2012-04-16 01:25:27 PDT
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
Comment 4 Mario Sanchez Prada 2012-04-16 01:37:59 PDT
Created attachment 137301 [details]
Patch proposal

Addressed issues pointed out by Carlos
Comment 5 WebKit Review Bot 2012-04-16 01:42:14 PDT
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 Mario Sanchez Prada 2012-04-16 01:53:04 PDT
Created attachment 137304 [details]
Patch proposal
Comment 7 Philippe Normand 2012-04-16 02:05:37 PDT
Comment on attachment 137304 [details]
Patch proposal

Attachment 137304 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12415072
Comment 8 Carlos Garcia Campos 2012-04-17 00:11:37 PDT
(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.
Comment 9 Mario Sanchez Prada 2012-04-18 00:41:44 PDT
(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
Comment 10 Mario Sanchez Prada 2012-05-29 17:16:42 PDT
(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 Mario Sanchez Prada 2012-05-29 17:32:09 PDT
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)
Comment 12 Mario Sanchez Prada 2012-05-29 17:33:27 PDT
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.
Comment 13 Mario Sanchez Prada 2012-05-29 17:34:27 PDT
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.
Comment 14 Martin Robinson 2012-05-29 17:35:02 PDT
Do you mind splitting these out to one patch-per-bug?
Comment 15 Mario Sanchez Prada 2012-05-29 18:00:35 PDT
(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 Mario Sanchez Prada 2012-05-29 18:17:45 PDT
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
Comment 17 Mario Sanchez Prada 2012-05-29 18:17:55 PDT
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
Comment 18 Mario Sanchez Prada 2012-05-31 02:54:53 PDT
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)
Comment 19 WebKit Review Bot 2012-05-31 02:59:14 PDT
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 Mario Sanchez Prada 2012-05-31 05:20:35 PDT
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)
Comment 21 Gustavo Noronha (kov) 2012-05-31 05:31:29 PDT
Comment on attachment 145056 [details]
Patch proposal

Attachment 145056 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12859332
Comment 22 Martin Robinson 2012-05-31 10:22:10 PDT
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.
Comment 23 Mario Sanchez Prada 2012-06-01 09:08:36 PDT
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.
Comment 24 Gustavo Noronha (kov) 2012-06-01 09:14:50 PDT
Comment on attachment 145323 [details]
Patch proposal

Attachment 145323 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12862658
Comment 25 Mario Sanchez Prada 2012-06-01 09:22:57 PDT
Created attachment 145326 [details]
Patch proposal

It seems git format-patch screwed up the previous patch... Uploading a new one
Comment 26 Carlos Garcia Campos 2012-06-01 09:30:36 PDT
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.
Comment 27 WebKit Review Bot 2012-06-01 13:26:58 PDT
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
Comment 28 WebKit Review Bot 2012-06-01 13:27:02 PDT
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
Comment 29 Mario Sanchez Prada 2012-06-04 05:40:52 PDT
Created attachment 145569 [details]
Patch proposal

New patch addressing last issues raised by Carlos
Comment 30 Gustavo Noronha (kov) 2012-06-04 05:48:20 PDT
Comment on attachment 145569 [details]
Patch proposal

Attachment 145569 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12894302
Comment 31 Carlos Garcia Campos 2012-06-04 06:01:21 PDT
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));
Comment 32 Mario Sanchez Prada 2012-06-04 07:14:17 PDT
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
Comment 33 Carlos Garcia Campos 2012-06-04 08:36:48 PDT
Comment on attachment 145579 [details]
Patch proposal

Perfect, thanks!
Comment 34 Mario Sanchez Prada 2012-06-04 09:05:20 PDT
Committed r119404: <http://trac.webkit.org/changeset/119404>