Bug 42027 – [Qt] Request for permission before starting Geolocation service

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mahesh Kulkarni]

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.

[Mahesh Kulkarni]

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.

[Steve Block]

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.

[Mahesh Kulkarni]

Fix from bug #42068 will introduce a flag under USE(). This bug will cover its functionality changes.

[Laszlo Gombos]

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.

[Steve Block]

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

[Mahesh Kulkarni]

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

[Antonio Gomes]

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

[Mahesh Kulkarni]

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

[Steve Block]

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!

[Mahesh Kulkarni]

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?

[Steve Block]

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.

[Mahesh Kulkarni]

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

[Mahesh Kulkarni]

Created attachment 64678 [details]
patch

Updated as per above comments

[Steve Block]

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?

[Mahesh Kulkarni]

Created attachment 64681 [details]
patch

Incorporate changes as per comment #14

[Steve Block]

Comment on attachment 64681 [details]
patch

Thanks Mahesh!

[Mahesh Kulkarni]

(In reply to comment #16)
> (From update of attachment 64681 [details])
> Thanks Mahesh!

Thank you once again for review Steve!

[WebKit Commit Bot]

Comment on attachment 64681 [details]
patch

Clearing flags on attachment: 64681

Committed r65603: <http://trac.webkit.org/changeset/65603>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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

[Steve Block]

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.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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

[Mahesh Kulkarni]

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

[Eric Seidel (no email)]

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

[Csaba Osztrogonác]

(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

[Simon Hausmann]

Revision r65603 cherry-picked into qtwebkit-2.1 with commit 089151ec4f583359d129fdc3c5246d247962aa7c