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

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

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.

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

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

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

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

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

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

Created attachment 64678 [details] patch Updated as per above comments

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?

Created attachment 64681 [details] patch Incorporate changes as per comment #14

Comment on attachment 64681 [details] patch Thanks Mahesh!

(In reply to comment #16) > (From update of attachment 64681 [details]) > Thanks Mahesh! Thank you once again for review Steve!

Comment on attachment 64681 [details] patch Clearing flags on attachment: 64681 Committed r65603: <http://trac.webkit.org/changeset/65603>

All reviewed patches have been landed. Closing bug.

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

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.

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

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

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

(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

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