Bug 42027 - [Qt] Request for permission before starting Geolocation service
Summary: [Qt] Request for permission before starting Geolocation service
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: Mahesh Kulkarni
URL:
Keywords: Qt
Depends on: 42068 42811 44179
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-10 04:15 PDT by Mahesh Kulkarni
Modified: 2010-08-25 06:16 PDT (History)
13 users (show)

See Also:


Attachments
proposed solution (4.23 KB, patch)
2010-07-10 04:31 PDT, Mahesh Kulkarni
steveblock: review-
Details | Formatted Diff | Diff
patch (2.96 KB, patch)
2010-07-21 11:57 PDT, Mahesh Kulkarni
steveblock: review-
Details | Formatted Diff | Diff
patch (2.27 KB, patch)
2010-08-18 00:15 PDT, Mahesh Kulkarni
steveblock: review-
Details | Formatted Diff | Diff
patch (2.43 KB, patch)
2010-08-18 02:51 PDT, Mahesh Kulkarni
steveblock: review-
Details | Formatted Diff | Diff
patch (3.29 KB, patch)
2010-08-18 03:21 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 Mahesh Kulkarni 2010-07-10 04:15:51 PDT
Before starting GeolocationService::startUpdate() and acquiring positions webkit must get permission for location.

As of now, requestPermission() is called after starting location device and acquiring the first position. 

This is a costly call on all platforms, On mobile devices, GPS/towerInfo is started and position is acquired even before getting permission for location.
Comment 1 Mahesh Kulkarni 2010-07-10 04:31:50 PDT
Created attachment 61152 [details]
proposed solution

Reusing m_startRequestPermissionNotifier implementation done under CLIENT_BASED_GEOLOCATION.
GeolocationService::startUpdating() is called only if permission is granted either in async/sync way.
Comment 2 Steve Block 2010-07-12 03:26:59 PDT
Comment on attachment 61152 [details]
proposed solution

For the non-client-based Geolocation implementation, the decision to start the location acquisition process before prompting the user for permission was a conscious one. It prevents unnecessary prompts when no location is available. This change will cause an unwanted change in behaviour for platforms that use the non-client-based implementation. The client-based Geolocation implementation uses opposite policy, of prompting for permission first.

If you'd like to use the second policy for the non-client-based implementation, I think we need to add this as an option behind a flag. Also, I think it makes sense to fix Bug 42068 first, which will introduce such a flag for the client-based implementation.

Finally, it's likely that the non-client-based implementation will deprecated (Bug 40373), so it might be best for you to start using the client-based implementation sooner rather than later.
Comment 3 Mahesh Kulkarni 2010-07-20 03:57:39 PDT
Fix from bug #42068 will introduce a flag under USE(). This bug will cover its functionality changes.
Comment 4 Laszlo Gombos 2010-07-21 06:15:16 PDT
As discussed with Mahesh QtWebKit seems to be the only port at the moment using GEOLOCATION with USE(PREEMPT_GEOLOCATION_PERMISSION) so I have marked this work for Qt only.
Comment 5 Steve Block 2010-07-21 07:07:49 PDT
> As discussed with Mahesh QtWebKit seems to be the only port at the moment using
> GEOLOCATION with USE(PREEMPT_GEOLOCATION_PERMISSION) so I have marked this work
> for Qt only.
Not quite.

Currently, Mac uses ENABLE(CLIENT_BASED_GEOLOCATION) and USE(PREEMPT_GEOLOCATION_PERMISSION). All other platforms use !ENABLE(CLIENT_BASED_GEOLOCATION) && !USE(PREEMPT_GEOLOCATION_PERMISSION).

Mahesh told me that Qt would like to use !ENABLE(CLIENT_BASED_GEOLOCATION) and USE(PREEMPT_GEOLOCATION_PERMISSION), which is not currently supported. This bug is to implement USE(PREEMPT_GEOLOCATION_PERMISSION) for !ENABLE(CLIENT_BASED_GEOLOCATION).

I don't mind whether or not the bug is labelled Qt.
Comment 6 Mahesh Kulkarni 2010-07-21 07:40:45 PDT
(In reply to comment #5)
> > As discussed with Mahesh QtWebKit seems to be the only port at the moment using
> > GEOLOCATION with USE(PREEMPT_GEOLOCATION_PERMISSION) so I have marked this work
> > for Qt only.
> Not quite.
> 
> Currently, Mac uses ENABLE(CLIENT_BASED_GEOLOCATION) and USE(PREEMPT_GEOLOCATION_PERMISSION). All other platforms use !ENABLE(CLIENT_BASED_GEOLOCATION) && !USE(PREEMPT_GEOLOCATION_PERMISSION).
> 
> Mahesh told me that Qt would like to use !ENABLE(CLIENT_BASED_GEOLOCATION) and USE(PREEMPT_GEOLOCATION_PERMISSION), which is not currently supported. This bug is to implement USE(PREEMPT_GEOLOCATION_PERMISSION) for !ENABLE(CLIENT_BASED_GEOLOCATION).
> 
> I don't mind whether or not the bug is labelled Qt.

Right. This bug is to implement USE(PREEMPT_GEOLOCATION_PERMISSION) for !ENABLE(CLIENT_BASED_GEOLOCATION). Which as of now will only be for Qt.
Comment 7 Antonio Gomes 2010-07-21 07:44:55 PDT
> Right. This bug is to implement USE(PREEMPT_GEOLOCATION_PERMISSION) for !ENABLE(CLIENT_BASED_GEOLOCATION). Which as of now will only be for Qt.

re-adding [Qt] to bug title, then.
Comment 8 Mahesh Kulkarni 2010-07-21 11:57:45 PDT
Created attachment 62214 [details]
patch

call startUpdate() when pre-emptive geolocation permission is used for non-client based implementation. No change for client-based implementations and non-pre-emptive geolocations. Qt port uses pre-emptive geolocation
Comment 9 Steve Block 2010-07-22 03:02:24 PDT
Comment on attachment 62214 [details]
patch

WebCore/page/Geolocation.cpp:408
 +  #if !ENABLE(CLIENT_BASED_GEOLOCATION)
Why reverse the order of the '#if'? It minimises the diff to keep it as it is.

WebCore/page/Geolocation.cpp:411
 +                  m_oneShots.add(m_startRequestPermissionNotifier);
This is not correct. The notifier pointed to by m_startRequestPermissionNotifier will already have been added to either m_oneShots m_watchers in getCurrentPosition() or watchPosition(). So we don't need to add it again and we can't assume it's a one-shot. I notice that existing permission denied case, below, suffers from the same problem.

Also, I see a couple of other problems with the existing pre-emptive permissions logic. I've filed Bug 42811 to track all of these, which we should fix first. Patches welcome!
Comment 10 Mahesh Kulkarni 2010-08-18 00:15:33 PDT
Created attachment 64673 [details]
patch

Fixes,

1) Starting location service when request is granted for ports using !CLIENT_BASED_GEOLOCATION and PREEMPT_GEOLOCATION_PERMISSION
2) Enables PREEMPT_GEOLOCATION_PERMISSION for QtWebkit

No new cases added as I'm not sure if there is a way GeolocationServiceMock can have PREEMPT_GEOLOCATION_PERMISSION enabled?
Comment 11 Steve Block 2010-08-18 01:40:43 PDT
Comment on attachment 64673 [details]
patch

WebCore/ChangeLog:13
 +          No new tests added because GeolocationServiceMock does not enable PREEMPT_GEOLOCATION_PERMISSION
I'm not sure what you mean by this. The choice of the preemptive vs non-preemptive permissions policy concerns the Geolocation object, not its client/service. So whether a given platform uses the preemptive policy depends on the build settings, not on whether it's using the mock in DRT. The code that you're adding is tested by the existing fast/dom/delayed-permission-* tests, so we should say so here.

It's a shame that these tests are currently skipped on Qt due to Bug 41364. Have you tested that these tests pass on Qt with your proposed fix for that bug? 

WebCore/page/Geolocation.cpp:702
 +              if (m_service && m_service->startUpdating(notifier->m_options.get()))
Why do you need to check m_service here? We don't check it when making calls to the service elsewhere.

Also, you could simply Geolocation::startUpdating() here for both the client-based and non-client-based impls. That would match what we do in Geolocation::startRequest() and save having to use an ifdef here.
Comment 12 Mahesh Kulkarni 2010-08-18 02:46:11 PDT
(In reply to comment #11)
> (From update of attachment 64673 [details])
> WebCore/ChangeLog:13
>  +          No new tests added because GeolocationServiceMock does not enable PREEMPT_GEOLOCATION_PERMISSION
> I'm not sure what you mean by this. The choice of the preemptive vs non-preemptive permissions policy concerns the Geolocation object, not its client/service. So whether a given platform uses the preemptive policy depends on the build settings, not on whether it's using the mock in DRT. The code that you're adding is tested by the existing fast/dom/delayed-permission-* tests, so we should say so here.
> 

Ah right. Sorry I missed to get this when I added that comment. 


> It's a shame that these tests are currently skipped on Qt due to Bug 41364. Have you tested that these tests pass on Qt with your proposed fix for that bug? 

Yes, with patch from 42811 + 42027 + 41364 all cases on Qt passes. 

> 
> WebCore/page/Geolocation.cpp:702
>  +              if (m_service && m_service->startUpdating(notifier->m_options.get()))
> Why do you need to check m_service here? We don't check it when making calls to the service elsewhere.
> 
> Also, you could simply Geolocation::startUpdating() here for both the client-based and non-client-based impls. That would match what we do in Geolocation::startRequest() and save having to use an ifdef here.

Good catch. Done.
Comment 13 Mahesh Kulkarni 2010-08-18 02:51:06 PDT
Created attachment 64678 [details]
patch

Updated as per above comments
Comment 14 Steve Block 2010-08-18 02:57:10 PDT
Comment on attachment 64678 [details]
patch

WebCore/ChangeLog:13
 +          Delayed layout cases for this bug are skipped for Qt (bug #41364 will fix)
It would be good if you could explicitly list the existing tests that test this code, as in theory, it's not specific to Qt.

WebCore/page/Geolocation.cpp:701
 +                  notifier->setFatalError(PositionError::create(PositionError::POSITION_UNAVAILABLE, failedToStartServiceErrorMessage));
Nice. Can't we now also remove the CLIENT_BASED_GEOLOCATION early-out above?
Comment 15 Mahesh Kulkarni 2010-08-18 03:21:55 PDT
Created attachment 64681 [details]
patch

Incorporate changes as per comment #14
Comment 16 Steve Block 2010-08-18 03:34:25 PDT
Comment on attachment 64681 [details]
patch

Thanks Mahesh!
Comment 17 Mahesh Kulkarni 2010-08-18 03:49:27 PDT
(In reply to comment #16)
> (From update of attachment 64681 [details])
> Thanks Mahesh!

Thank you once again for review Steve!
Comment 18 WebKit Commit Bot 2010-08-18 05:37:15 PDT
Comment on attachment 64681 [details]
patch

Clearing flags on attachment: 64681

Committed r65603: <http://trac.webkit.org/changeset/65603>
Comment 19 WebKit Commit Bot 2010-08-18 05:37:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 WebKit Review Bot 2010-08-18 06:10:47 PDT
http://trac.webkit.org/changeset/65603 might have broken Qt Linux Release
Comment 21 Steve Block 2010-08-18 08:57:14 PDT
This is being rolled out in Bug 44179 because of crashes on Qt.

Mahesh, is the crash just due to the missing Qt DRT implementation? If so, you could resubmit this without enabling the new code path on Qt.
Comment 22 Csaba Osztrogonác 2010-08-18 09:14:45 PDT
r65603 was rolled out by http://trac.webkit.org/changeset/65611

crashes can be found here:
http://build.webkit.org/results/Qt%20Linux%20Release/r65603%20%2817985%29/results.html
Comment 23 Csaba Osztrogonác 2010-08-18 11:07:03 PDT
Comment on attachment 64681 [details]
patch

Landed again: http://trac.webkit.org/changeset/65616

The patch is good, but the buildbot needed a clean build to like it. :)
Comment 24 Mahesh Kulkarni 2010-08-18 11:17:25 PDT
(In reply to comment #23)
> (From update of attachment 64681 [details] [details])
> Landed again: http://trac.webkit.org/changeset/65616
> 
> The patch is good, but the buildbot needed a clean build to like it. :)

Thank you Csaba Osztrogonac!
Comment 25 Eric Seidel 2010-08-18 11:24:43 PDT
Any time we need a clean build, that's a bug in the build system.  We should file a bug and try to fix it. :)
Comment 26 Csaba Osztrogonác 2010-08-18 12:14:06 PDT
(In reply to comment #25)
> Any time we need a clean build, that's a bug in the build system.  We should file a bug and try to fix it. :)

I absolutely agree with you, but it is a hard task now. 

http://trac.webkit.org/changeset/65603 added a new define:
DEFINES += WTF_USE_PREEMPT_GEOLOCATION_PERMISSION

Try to imagine a foo.cpp like this:
...
#ifdef WTF_USE_PREEMPT_GEOLOCATION_PERMISSION
    // do something
#endif
...

The build command in makefile before the patch:
g++ -c ... foo.cpp -o foo.o

The build command in makefile after the patch:
g++ -c ... -DWTF_USE_PREEMPT_GEOLOCATION_PERMISSION foo.cpp -o foo.o

Unfortunately modifying pri and pro files won't trigger rebuilding foo.cpp.
To fix this bug, we have to write a sophisticated script to determine
which sources should be rebuilded after adding/modifying/removing a
compiler flag.

We filed a bug about it long time ago, but there aren't a good fix yet:
https://bugs.webkit.org/show_bug.cgi?id=38054
Comment 27 Simon Hausmann 2010-08-25 06:16:20 PDT
Revision r65603 cherry-picked into qtwebkit-2.1 with commit 089151ec4f583359d129fdc3c5246d247962aa7c