Bug 35191 – [Gtk] use geoclue providers with don't provide update

• Component: WebKitGTK
• Status: RESOLVED FIXED

[arno.]

Hi,
currently, we call geoclue_master_client_set_requirements with require_updates argument set to true. It means only providers with provide updates will be taken into account. We could set this argument to false (and that would need some other code modification) to be able to use providers that don't provide updates (such as hostip).

[arno.]

Created attachment 49123 [details]
patch v1

path proposal:


after creating geocluePosition (with need_update to false), I call geoclue_position_get_position_async. Asynchronous call is needed for two things:
- not freezing the interface in case it takes a long time to have the position.
- not calling Geolocation::makeSuccessCallbacks before Geolocation::startRequest returns (otherwise, makeSuccessCallbacks is called with an empty m_oneShots).

[arno.]

Also, I've a question about GeolocationServiceGtk::startUpdating code:


g_error_free is not called after

   
    if (!result) {
        setError(PositionError::POSITION_UNAVAILABLE, error->message)

Is it because of some GOwnPtr magic ?

[Eric Seidel (no email)]

Comment on attachment 49123 [details]
patch v1

This patch fails to apply (click on any one of the purple bubbles to see why).  Thus it can't be commit-queue'd.

[arno.]

Created attachment 49322 [details]
patch v1.2

I hope this one will work.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/gtk/GeolocationServiceGtk.h:53:  get_position is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/gtk/GeolocationServiceGtk.cpp:171:  GeolocationServiceGtk::get_position is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 3 files


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

[Gustavo Noronha (kov)]

(In reply to comment #2)
> Also, I've a question about GeolocationServiceGtk::startUpdating code:
> 
> 
> g_error_free is not called after
> 
> 
>     if (!result) {
>         setError(PositionError::POSITION_UNAVAILABLE, error->message)
> 
> Is it because of some GOwnPtr magic ?

Yes. GOwnPtr knows that the reference is only relevant inside that scope, so when the function is done it will free the pointer. Here's GError's specialization of the template:

template <> void freeOwnedGPtr<GError>(GError* ptr)
{
    if (ptr)
        g_error_free(ptr);
}

You can see more details at JavaScriptCore/wtf/gobject/GOwnPtr.cpp

[Gustavo Noronha (kov)]

Comment on attachment 49322 [details]
patch v1.2

 8         No new tests. (OOPS!)

You should state here that there's no testable behaviour change, (I assume there isn't because this depends on the system). Also, you're not doing only what you claim in the changelog, in this patch. You're also making updating the location information async, which does sound like a good idea, but should be made into another patch. r- for that reason.

Also, we had many crashes happen because the WebCore object goes away between an async call, and its callback returning, so passin 'this' here worries me:

 118     geoclue_position_get_position_async(m_geocluePosition, (GeocluePositionCallback)get_position, this);

Have you checked what the life cycle of the geolocation service object is, and if it will react well if its client goes away?

[arno.]

Created attachment 49357 [details]
patch v1.3

(In reply to comment #7)
> (From update of attachment 49322 [details])
>  8         No new tests. (OOPS!)
> 
> You should state here that there's no testable behaviour change, (I assume
> there isn't because this depends on the system).

ok, here is an updated patch

> Also, you're not doing only
> what you claim in the changelog, in this patch. You're also making updating the
> location information async, which does sound like a good idea, but should be
> made into another patch. r- for that reason.

asynchronous reply is needed here. As explained in comment 1, if we return a position synchronously from startUpdating, callback fonctions will not be called. Currently (providers providing update only), position return is asynchronous. If we use providers which don't provide updates, we need to get position asynchronously (otherwise, geolocation simply won't work).

> Also, we had many crashes happen because the WebCore object goes away between
> an async call, and its callback returning, so passin 'this' here worries me:
> 
>  118     geoclue_position_get_position_async(m_geocluePosition,
> (GeocluePositionCallback)get_position, this);
> 
> Have you checked what the life cycle of the geolocation service object is, and
> if it will react well if its client goes away?

I've tested what happens when page is unloaded before position is available. GeolocationService is destroyed and get_position is not called, and no crash append.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/gtk/GeolocationServiceGtk.h:53:  get_position is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/gtk/GeolocationServiceGtk.cpp:171:  GeolocationServiceGtk::get_position is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 3 files


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

[arno.]

As seen with kon on irc, it's probably better to split this bug in two parts: first, implement the mechanism to get the position asynchronously after GeocluePosition* has been created.
Then, in a second part, set the flag to allow providers with don't provide update.

[arno.]

Created attachment 49610 [details]
patch v2

sorry, wrong patch. This patch depends on bug #35355 being fixed

[WebKit Commit Bot]

Comment on attachment 49610 [details]
patch v2

Rejecting patch 49610 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Gustavo Noronha Silva', '--force']" exit_code: 1
Last 500 characters of output:
e: Wed, 24 Feb 2010 22:17:15 +0100
Subject: [PATCH] Bug #35191

---
 WebCore/ChangeLog                              |   12 ++++++++++++
 WebCore/platform/gtk/GeolocationServiceGtk.cpp |    2 +-
 2 files changed, 13 insertions(+), 1 deletions(-)


-------------------------------------------------------------------
patching file WebCore/ChangeLog
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/ChangeLog.rej
patching file WebCore/platform/gtk/GeolocationServiceGtk.cpp

Full output: http://webkit-commit-queue.appspot.com/results/320259

[arno.]

Created attachment 49747 [details]
patch v2.1

Here is an update patch against latest Changelog. But is it frequent for patches to not apply because of changelogs modifications ? how to avoid that in the future ?

[WebKit Commit Bot]

Comment on attachment 49747 [details]
patch v2.1

Clearing flags on attachment: 49747

Committed r55446: <http://trac.webkit.org/changeset/55446>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.