Bug 42068 - Need to be able to configure Geolocation policy regarding user permissions
Summary: Need to be able to configure Geolocation policy regarding user permissions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 42641
Blocks: 40373 42027
  Show dependency treegraph
 
Reported: 2010-07-12 03:17 PDT by Steve Block
Modified: 2010-08-03 08:39 PDT (History)
10 users (show)

See Also:


Attachments
attempt to fix (5.47 KB, patch)
2010-07-19 13:21 PDT, Mahesh Kulkarni
steveblock: review-
Details | Formatted Diff | Diff
patch (5.93 KB, patch)
2010-07-20 02:13 PDT, Mahesh Kulkarni
steveblock: review-
Details | Formatted Diff | Diff
patch (5.80 KB, patch)
2010-07-20 03:04 PDT, Mahesh Kulkarni
no flags Details | Formatted Diff | Diff
patch (5.77 KB, patch)
2010-07-20 03:06 PDT, Mahesh Kulkarni
no flags Details | Formatted Diff | Diff
style issues corrected (5.77 KB, patch)
2010-07-20 03:12 PDT, Mahesh Kulkarni
no flags Details | Formatted Diff | Diff
build fix (5.77 KB, patch)
2010-07-20 09:39 PDT, Mahesh Kulkarni
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2010-07-12 03:17:44 PDT
When writing the initial Geolocation implementation, a decision was made to start the location acquisition process before prompting the user for permission. Only once a position has been successfully obtained do we prompt the user. This avoids unnecessary prompting in the case when a location is unavailable. Note that this complies with the Geolocation spec (http://dev.w3.org/geo/api/spec-source.html), which requires that a UA must obtain user permission before disclosing location information. See also the discussion in Bug 39434.

When the client-based Geolocation implementation was added, the opposite decision was taken. Permission is always obtained before the location acquisition process is started.

As a result, the selection of a Geolocation implementation (with ENABLE(CLIENT_BASED_GEOLOCATION) also determines the user permission policy. Instead, the choice of user permission policy should be set independently, probably with a new USE flag.

Note that plans are in place to deprecate the non-client-based Geolocation implementation. See Bug 40373.
Comment 1 Yael 2010-07-12 05:32:12 PDT
After this re-factor, it would be great if Geolocation and Notifications could share the same approach in their API for prompting the client for permission.
Comment 2 Mahesh Kulkarni 2010-07-19 13:21:04 PDT
Created attachment 61985 [details]
attempt to fix

Introducing a USE flag and making it enable by default for all CLIENT_BASED_GEOLOCATION ports. 
Enabling it separately for QtWebkit. 

Please review. Am I missing something obvious here?
Comment 3 Steve Block 2010-07-19 13:53:17 PDT
Comment on attachment 61985 [details]
attempt to fix

Thanks for the patch Mahesh.

WebCore/ChangeLog:9
 +          acquires user permission first before starting instantiating location
extra 'instantiating'

WebCore/page/Geolocation.cpp:403
 +  
extra blank line

WebCore/WebCore.pro:2704
 +  # if geolocation is enabled, enable pre-request for permission policy
Should probably be indented

JavaScriptCore/wtf/Platform.h:1121
 +  #define WTF_USE_PREEMPT_GEOLOCATION_PERMISSION
Add a comment that currently, client-based geolocation supports only the preemptive permission policy and that non-client-based Geolocation supports only the non-preemptive policy.

WebCore/page/Geolocation.cpp:626
 +      // FIXME: Pass options to client.
Could you move this to the addObserver() line?

WebCore/page/Geolocation.cpp:623
 +      }
I think we should close the #if USE(PREEMPT_GEOLOCATION_PERMISSION) here

WebCore/page/Geolocation.cpp:640
 +  #endif // USE(PREEMPT_GEOLOCATION_PERMISSION)
This block should just be ifdef'ed on CLIENT_BASED_GEOLOCATION

WebCore/page/Geolocation.cpp:417
 +  #endif
I assume that when you add support for the preemptive policy to the non-client based impl, you'll add an 'else' here? If so, you could add the else and a TODO now to make it clear.
Comment 4 Steve Block 2010-07-19 14:39:05 PDT
> WebCore/ChangeLog
Add a line explaining that this change does not introduce any change in behaviour for any platforms currently using Geolocation, so there are no new tests.

> JavaScriptCore/wtf/Platform.h:1121
>  +  #define WTF_USE_PREEMPT_GEOLOCATION_PERMISSION
> Add a comment that currently, client-based geolocation supports only the preemptive permission
> policy and that non-client-based Geolocation supports only the non-preemptive policy.
On second thoughts, I think that with the changes I suggested, the client-based implementation should work with both policies. Can you confirm this?

Do you still plan to use the non-client-based implementation?
Comment 5 Mahesh Kulkarni 2010-07-20 02:13:20 PDT
Created attachment 62044 [details]
patch

Incorporated changes as per Steve's comments #3
Comment 6 Mahesh Kulkarni 2010-07-20 02:15:01 PDT
(In reply to comment #4)
> > WebCore/ChangeLog
> Add a line explaining that this change does not introduce any change in behaviour for any platforms currently using Geolocation, so there are no new tests.
> 

Done

> > JavaScriptCore/wtf/Platform.h:1121
> >  +  #define WTF_USE_PREEMPT_GEOLOCATION_PERMISSION
> > Add a comment that currently, client-based geolocation supports only the preemptive permission
> > policy and that non-client-based Geolocation supports only the non-preemptive policy.
> On second thoughts, I think that with the changes I suggested, the client-based implementation should work with both policies. Can you confirm this?
> 
> Do you still plan to use the non-client-based implementation?

Yes. The latest patch changes should enable client-based implementation to utilize both the policies.
Comment 7 Mahesh Kulkarni 2010-07-20 02:18:56 PDT
(In reply to comment #4)

> Do you still plan to use the non-client-based implementation?

Steve using preemptive permission policy for qt port is for a intermediate release. Will track the old bug  #42027, for implementing client-based solution for qt port.
Comment 8 Steve Block 2010-07-20 02:29:14 PDT
Comment on attachment 62044 [details]
patch

WebCore/page/Geolocation.cpp:643
 +  #endif 
Trailing whitespace

WebCore/page/Geolocation.cpp:421
 +              m_startRequestPermissionNotifier = 0;
This new block wasn't in the first patch and introduces a change in behaviour. This bug was intended to add the new USE flag and not introduce any functional changes. This block should be part of the fix for your original Bug 42027.

JavaScriptCore/wtf/Platform.h:1120
 +  #if ENABLE(CLIENT_BASED_GEOLOCATION)
I still think a comment describing the currently supported permutations would be good.

It looks like you still plan to use the non-client-based implementation on Qt? It would be great of you could go straight for the client-based approach.
Comment 9 Mahesh Kulkarni 2010-07-20 02:45:16 PDT
(In reply to comment #8)
> (From update of attachment 62044 [details])
> WebCore/page/Geolocation.cpp:421
>  +              m_startRequestPermissionNotifier = 0;
> This new block wasn't in the first patch and introduces a change in behaviour. This bug was intended to add the new USE flag and not introduce any functional changes. This block should be part of the fix for your original Bug 42027.
> 

This new block will not be available for non-preemptive permission policy. No change in behavior for existing implementations (for both client based and non-client based). 
Because we are introducing preemptive permission policy, startUpdate() call has never instantiated the service, resulting in ignoring the first request received for position. 
This block implements the functionality of USE() flag. I can move this to bug #42027 if you would like?
Comment 10 Steve Block 2010-07-20 02:58:38 PDT
> This new block will not be available for non-preemptive permission policy. No
> change in behavior for existing implementations (for both client based and non
> client based). 
It's true that currently no platform uses USE(PREEMPT_GEOLOCATION_PERMISSION) && !ENABLE(CLIENT_BASED_GEOLOCATION), but this is still new functionality. The purpose of this bug was just to add the new flag with no functional change.

Your intent with Bug 42027 was to implement USE(PREEMPT_GEOLOCATION_PERMISSION) && !ENABLE(CLIENT_BASED_GEOLOCATION), right? This is exactly what this block does, so should be in that bug.

You could update the ChangeLog description to drop 'for any platforms currently using geolocation' to make it clear that this patch has no functional change at all.
Comment 11 Mahesh Kulkarni 2010-07-20 03:01:55 PDT
(In reply to comment #10)
> > This new block will not be available for non-preemptive permission policy. No
> > change in behavior for existing implementations (for both client based and non
> > client based). 
> It's true that currently no platform uses USE(PREEMPT_GEOLOCATION_PERMISSION) && !ENABLE(CLIENT_BASED_GEOLOCATION), but this is still new functionality. The purpose of this bug was just to add the new flag with no functional change.
> 
> Your intent with Bug 42027 was to implement USE(PREEMPT_GEOLOCATION_PERMISSION) && !ENABLE(CLIENT_BASED_GEOLOCATION), right? This is exactly what this block does, so should be in that bug.
> 
> You could update the ChangeLog description to drop 'for any platforms currently using geolocation' to make it clear that this patch has no functional change at all.

Sorry I spoke bit soon. This but not introducing behavior changes is proper solution and we need not update test content. New patch on the way.
Comment 12 Mahesh Kulkarni 2010-07-20 03:04:19 PDT
Created attachment 62049 [details]
patch

Incorporate changes from comment #8

bug #4207 will be tracked to implement the preemptive permission policy behavior. I will raise a new bug for qt port to implement client-based solution
Comment 13 Mahesh Kulkarni 2010-07-20 03:06:24 PDT
Created attachment 62050 [details]
patch

(In reply to comment #12)
> Created an attachment (id=62049) [details]
> patch
> 
> Incorporate changes from comment #8
> 
> bug #4207 will be tracked to implement the preemptive permission policy behavior. I will raise a new bug for qt port to implement client-based solution

Sorry missed one change from comment #8
Comment 14 WebKit Review Bot 2010-07-20 03:08:20 PDT
Attachment 62050 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/page/Geolocation.cpp:417:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Mahesh Kulkarni 2010-07-20 03:12:24 PDT
Created attachment 62052 [details]
style issues corrected 

Fixed style error. 

Bug #42027 will be tracked for handling startUpdate() incase of pre-emptive permission policy.
Comment 16 Steve Block 2010-07-20 05:48:34 PDT
Comment on attachment 62052 [details]
style issues corrected 

r=me
Comment 17 WebKit Commit Bot 2010-07-20 06:42:42 PDT
Comment on attachment 62052 [details]
style issues corrected 

Rejecting patch 62052 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 62052, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
date_status
    return NetworkTransaction().run(lambda: self._post_status_to_server(queue_name, status, patch, results_file))
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 56, in run
    self._check_for_timeout()
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 63, in _check_for_timeout
    raise NetworkTimeout()
webkitpy.common.net.networktransaction.NetworkTimeout
Comment 18 Andras Becsi 2010-07-20 07:25:59 PDT
(In reply to comment #16)
> (From update of attachment 62052 [details])
> r=me
Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/wtf/Platform.h
Sending        WebCore/ChangeLog
Sending        WebCore/WebCore.pro
Sending        WebCore/page/Geolocation.cpp
Sending        WebCore/page/Geolocation.h
Transmitting file data ......
Committed revision 63742.
Comment 19 Andras Becsi 2010-07-20 07:26:53 PDT
All reviewed patches landed. Closing bug.
Comment 20 Andras Becsi 2010-07-20 07:27:32 PDT
Comment on attachment 62052 [details]
style issues corrected 

Clearing flags.
Comment 21 Eric Seidel (no email) 2010-07-20 07:28:38 PDT
You might consider using webkit-patch land.
Comment 22 WebKit Review Bot 2010-07-20 07:59:54 PDT
http://trac.webkit.org/changeset/63742 might have broken Qt Linux Release
Comment 23 Steve Block 2010-07-20 08:13:31 PDT
Patch was rolled back for breaking build

> JavaScriptCore/wtf/Platform.h:1123
> +  #define WTF_USE_PREEMPT_GEOLOCATION_PERMISSION
I think this should be ...
#define WTF_USE_PREEMPT_GEOLOCATION_PERMISSION 1
Comment 24 Mahesh Kulkarni 2010-07-20 09:39:39 PDT
Created attachment 62084 [details]
build fix

(In reply to comment #23)
> Patch was rolled back for breaking build
> 
> > JavaScriptCore/wtf/Platform.h:1123
> > +  #define WTF_USE_PREEMPT_GEOLOCATION_PERMISSION
> I think this should be ...
> #define WTF_USE_PREEMPT_GEOLOCATION_PERMISSION 1

Apologies about the build break. Resubmitting the patch.
Comment 25 Steve Block 2010-07-20 09:47:25 PDT
Comment on attachment 62084 [details]
build fix

r=me
Comment 26 WebKit Commit Bot 2010-07-20 11:59:22 PDT
Comment on attachment 62084 [details]
build fix

Clearing flags on attachment: 62084

Committed r63761: <http://trac.webkit.org/changeset/63761>
Comment 27 WebKit Commit Bot 2010-07-20 11:59:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 WebKit Review Bot 2010-07-20 12:33:18 PDT
http://trac.webkit.org/changeset/63761 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/63761
http://trac.webkit.org/changeset/63764
Comment 29 Steve Block 2010-07-20 16:23:38 PDT
Fixed failing Qt tests by not using PREEMPT_GEOLOCATION_PERMISSION on Qt until Bug 42027 is fixed.
See http://trac.webkit.org/changeset/63771

The buildbot is still not happy but I think this is just because it needs to have a clean build forced.
Comment 30 Mahesh Kulkarni 2010-07-20 19:13:44 PDT
(In reply to comment #29)
> Fixed failing Qt tests by not using PREEMPT_GEOLOCATION_PERMISSION on Qt until Bug 42027 is fixed.
> See http://trac.webkit.org/changeset/63771
> 
> The buildbot is still not happy but I think this is just because it needs to have a clean build forced.

Thank you Steve! Will take care of those cases in 42027
Comment 31 Simon Hausmann 2010-08-03 08:39:27 PDT
Revision r63742 cherry-picked into qtwebkit-2.1 with commit c12d7b744af96b4922210be46a2e7b5341cb72b6
Revision r63761 cherry-picked into qtwebkit-2.1 with commit b960aea28ce5cef3742134a7f8501188b4efd8b4