Bug 83877

Created attachment 137070 [details] Patch proposal Initial proposal, based in the geolocation provider for the Qt port. Asking for review

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

Created attachment 137301 [details] Patch proposal Addressed issues pointed out by Carlos

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.

Created attachment 137304 [details] Patch proposal

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

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

(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

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

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)

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.

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.

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

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

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)

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

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 on attachment 145056 [details] Patch proposal Attachment 145056 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12859332

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.

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 on attachment 145323 [details] Patch proposal Attachment 145323 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12862658

Created attachment 145326 [details] Patch proposal It seems git format-patch screwed up the previous patch... Uploading a new one

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

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

Created attachment 145569 [details] Patch proposal New patch addressing last issues raised by Carlos

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

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));

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 on attachment 145579 [details] Patch proposal Perfect, thanks!

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