Bug 35191 - [Gtk] use geoclue providers with don't provide update
Summary: [Gtk] use geoclue providers with don't provide update
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 35355
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-20 02:43 PST by arno.
Modified: 2010-03-02 18:21 PST (History)
3 users (show)

See Also:


Attachments
patch v1 (6.97 KB, patch)
2010-02-20 04:04 PST, arno.
eric: commit-queue-
Details | Formatted Diff | Diff
patch v1.2 (4.98 KB, patch)
2010-02-23 13:29 PST, arno.
gns: review-
Details | Formatted Diff | Diff
patch v1.3 (5.00 KB, patch)
2010-02-23 22:19 PST, arno.
no flags Details | Formatted Diff | Diff
patch v2 (2.32 KB, patch)
2010-02-26 12:10 PST, arno.
gns: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch v2.1 (1.68 KB, patch)
2010-03-01 13:29 PST, arno.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description arno. 2010-02-20 02:43:52 PST
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).
Comment 1 arno. 2010-02-20 04:04:42 PST
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).
Comment 2 arno. 2010-02-20 04:08:32 PST
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 ?
Comment 3 Eric Seidel (no email) 2010-02-22 14:19:53 PST
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.
Comment 4 arno. 2010-02-23 13:29:02 PST
Created attachment 49322 [details]
patch v1.2

I hope this one will work.
Comment 5 WebKit Review Bot 2010-02-23 13:31:46 PST
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.
Comment 6 Gustavo Noronha (kov) 2010-02-23 14:27:04 PST
(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
Comment 7 Gustavo Noronha (kov) 2010-02-23 14:57:57 PST
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?
Comment 8 arno. 2010-02-23 22:19:23 PST
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.
Comment 9 WebKit Review Bot 2010-02-23 22:21:30 PST
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.
Comment 10 arno. 2010-02-24 13:10:27 PST
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.
Comment 11 arno. 2010-02-26 12:10:25 PST
Created attachment 49610 [details]
patch v2

sorry, wrong patch. This patch depends on bug #35355 being fixed
Comment 12 WebKit Commit Bot 2010-03-01 13:17:06 PST
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
Comment 13 arno. 2010-03-01 13:29:13 PST
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 ?
Comment 14 WebKit Commit Bot 2010-03-02 18:20:56 PST
Comment on attachment 49747 [details]
patch v2.1

Clearing flags on attachment: 49747

Committed r55446: <http://trac.webkit.org/changeset/55446>
Comment 15 WebKit Commit Bot 2010-03-02 18:21:01 PST
All reviewed patches have been landed.  Closing bug.