Summary: | Need to be able to configure Geolocation policy regarding user permissions | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Steve Block <steveblock> | ||||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, abecsi, commit-queue, eric, hausmann, laszlo.gombos, maheshk, steveblock, webkit.review.bot, yael | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Bug Depends on: | 42641 | ||||||||||||||||
Bug Blocks: | 40373, 42027 | ||||||||||||||||
Attachments: |
|
Description
Steve Block
2010-07-12 03:17:44 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. 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 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.
> 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? Created attachment 62044 [details]
patch
Incorporated changes as per Steve's comments #3
(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. (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 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. (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? > 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. (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. 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 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 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.
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 on attachment 62052 [details]
style issues corrected
r=me
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
(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. All reviewed patches landed. Closing bug. Comment on attachment 62052 [details]
style issues corrected
Clearing flags.
You might consider using webkit-patch land. http://trac.webkit.org/changeset/63742 might have broken Qt Linux Release 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
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 on attachment 62084 [details]
build fix
r=me
Comment on attachment 62084 [details] build fix Clearing flags on attachment: 62084 Committed r63761: <http://trac.webkit.org/changeset/63761> All reviewed patches have been landed. Closing bug. 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 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. (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 |