Bug 42068 – Need to be able to configure Geolocation policy regarding user permissions

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Steve Block]

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.

[Yael]

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.

[Mahesh Kulkarni]

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?

[Steve Block]

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.

[Steve Block]

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

[Mahesh Kulkarni]

Created attachment 62044 [details]
patch

Incorporated changes as per Steve's comments #3

[Mahesh Kulkarni]

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

[Mahesh Kulkarni]

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

[Steve Block]

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.

[Mahesh Kulkarni]

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

[Steve Block]

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

[Mahesh Kulkarni]

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

[Mahesh Kulkarni]

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

[Mahesh Kulkarni]

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

[WebKit Review Bot]

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.

[Mahesh Kulkarni]

Created attachment 62052 [details]
style issues corrected 

Fixed style error. 

Bug #42027 will be tracked for handling startUpdate() incase of pre-emptive permission policy.

[Steve Block]

Comment on attachment 62052 [details]
style issues corrected 

r=me

[WebKit Commit Bot]

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

[Andras Becsi]

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

[Andras Becsi]

All reviewed patches landed. Closing bug.

[Andras Becsi]

Comment on attachment 62052 [details]
style issues corrected 

Clearing flags.

[Eric Seidel (no email)]

You might consider using webkit-patch land.

[WebKit Review Bot]

http://trac.webkit.org/changeset/63742 might have broken Qt Linux Release

[Steve Block]

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

[Mahesh Kulkarni]

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.

[Steve Block]

Comment on attachment 62084 [details]
build fix

r=me

[WebKit Commit Bot]

Comment on attachment 62084 [details]
build fix

Clearing flags on attachment: 62084

Committed r63761: <http://trac.webkit.org/changeset/63761>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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

[Steve Block]

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.

[Mahesh Kulkarni]

(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

[Simon Hausmann]

Revision r63742 cherry-picked into qtwebkit-2.1 with commit c12d7b744af96b4922210be46a2e7b5341cb72b6
Revision r63761 cherry-picked into qtwebkit-2.1 with commit b960aea28ce5cef3742134a7f8501188b4efd8b4