Bug 42811

Created attachment 62572 [details] patch maintains set of pending permission notifiers and starts service for non client based ports. No new tests added (?). layout tests for geolocations are passing with and without preemptive enabled.

Comment on attachment 62572 [details] patch > ChangeLog > + No new test content added I think this warrants a new test. Unfortunately I think that requires a fix to the Mac LayoutTestController which is being done in Bug 40002. I'll try to get that bug fixed soon. > Geolocation.cpp: 415 > page->geolocationController()->addObserver(this); This should be in the above loop. Can you also add a FIXME to pass options to the client, as at line 635. > Geolocation.cpp: 417 > // TODO: Handle startUpdate() for non-client based implementations using pre-emptive policy This patch should just fix this bug - the problems with the existing code. Let's leave adding the new code for !CLIENT_BASED && PREEMPT for Bug 42027.

(In reply to comment #2) Thanks for the review Steve. > (From update of attachment 62572 [details]) > > ChangeLog > > + No new test content added > I think this warrants a new test. Unfortunately I think that requires a fix to the Mac LayoutTestController which is being done in Bug 40002. I'll try to get that bug fixed soon. Ok few new test cases for multiple notifiers waiting permission. Do you think its good idea to raise a different bug for these new cases and mark blocked by 40002? > > > Geolocation.cpp: 415 > > page->geolocationController()->addObserver(this); > This should be in the above loop. Can you also add a FIXME to pass options to the client, as at line 635. > ok, so its number of listeners for controller. Will update this in patch. > > Geolocation.cpp: 417 > > // TODO: Handle startUpdate() for non-client based implementations using pre-emptive policy > This patch should just fix this bug - the problems with the existing code. Let's leave adding the new code for !CLIENT_BASED && PREEMPT for Bug 42027. ok make sense. Will take care of this

> Ok few new test cases for multiple notifiers waiting permission. Do you think > its good idea to raise a different bug for these new cases and mark blocked by > 40002? No, I think the tests should be part of this patch. I'll work on getting Bug 40002 fixed asap. I've marked this bug as blocked by it.

Created attachment 63822 [details] patch This patch implements a set, m_pendingForPermissionNotifiers to maintain set of pending requests. When user grands/denies permission all listener's will be notified. Added couple of new layout test cases when multiple requests are waiting for user permission.

Comment on attachment 63822 [details] patch WebCore/ChangeLog:11 + When user grands/denies permission all listener's will be notified. s/grands/grants WebCore/ChangeLog:6 + Geolocation preemptive permissions policy is buggy Should be bug title, then URL on line below WebCore/page/Geolocation.cpp:420 + // TODO: Handle startUpdate() for non-client based implementations using pre-emptive policy Indentation WebCore/page/Geolocation.cpp:417 + return; It seems odd to have this test in the loop. I think we can early-out before the loop if no frame is present. Alternatively, could you call startUpdating()? WebCore/page/Geolocation.cpp:628 + m_pendingForPermissionNotifiers.add(notifier); It might help readability to add an ASSERT(!isDenied()) above this line. WebCore/page/Geolocation.cpp:425 + for (GeoNotifierSet::const_iterator iter = m_pendingForPermissionNotifiers.begin(); iter != end; ++iter) { Would it be cleaner to use a single loop, with the 'if (isAllowed())' inside the loop? WebCore/page/Geolocation.cpp:418 + page->geolocationController()->addObserver(this, notifier->m_options->enableHighAccuracy()); This is incorrect. We should only add an observer / start the service if the request does not have a zero timeout. Also, if adding an observer / starting the service fails, we need to set a fatal error. See startRequest(). Perhaps we could share some code from that method? LayoutTests/fast/dom/Geolocation/script-tests/delayed-multiple-permissions-allowed.js:1 + description("Tests that when a position is available, no callbacks are invoked until permission is allowed."); Mention that here we test with multiple queued requests, to distinguish this from delayed-permission-allowed.html

(In reply to comment #6) Thanks for review. > WebCore/page/Geolocation.cpp:417 > + return; > It seems odd to have this test in the loop. I think we can early-out before the loop if no frame is present. Alternatively, could you call startUpdating()? StartUpdating() for PREEMPT && !CLIENT? will be covered in bug 42027. > WebCore/page/Geolocation.cpp:425 > + for (GeoNotifierSet::const_iterator iter = m_pendingForPermissionNotifiers.begin(); iter != end; ++iter) { > Would it be cleaner to use a single loop, with the 'if (isAllowed())' inside the loop? That would have repeated isAllowed() check which we could avoid. I was thinking to write a helper function to move all code under if(isAllowed()) and make it more cleaner to read? > > WebCore/page/Geolocation.cpp:418 > + page->geolocationController()->addObserver(this, notifier->m_options->enableHighAccuracy()); > This is incorrect. We should only add an observer / start the service if the request does not have a zero timeout. Also, if adding an observer / starting the service fails, we need to set a fatal error. See startRequest(). Perhaps we could share some code from that method? > A listener is appended to the m_pendingForPermissionNotifiers queue if listener doesn't have zero timeout. Perhaps we can move if (m_pendingForPermissionNotifiers.isEmpty()) to startUpdating() and add permission error's to GeoNotifier::timerFired() function?

> StartUpdating() for PREEMPT && !CLIENT? will be covered in bug 42027. I meant Geolocation::startUpdating(). Either way, I think it's neater to early-out. > A listener is appended to the m_pendingForPermissionNotifiers queue if listener > doesn't have zero timeout. Yes, you're right. Perhaps add a comment/ASSERT in startUpdating() or setIsAllowed() to make this clear. > Perhaps we can move > if (m_pendingForPermissionNotifiers.isEmpty()) to startUpdating() and add > permission error's to GeoNotifier::timerFired() function? I'm not exactly sure what you mean but I'd be happy to look at a patch.

Created attachment 63997 [details] patch Incorporates changes as per comment #6 - when notifier->hasZeroTimeout() is set, immediate call back has to be given - permission pending notifiers will be added before startUpdating() called to make starRequest() more readble. This also means that when makeCachedPositionCallbacks() calls startupdating() no redundant permission checks happen - added handlePendingPermissionNotifiers() to make setIsAllowed() more readable. - m_service->startUpdate() is called in case of !CLIENT_BASED_GEOLOCATION && PREEMPT_GEOLOCATION_PERMISSION clients on successful permission set. We could add a layout test for this but that would mean we have to enable PREEMPT_GEOLOCATION_PERMISSION for MockGeolocationService. Please comment.

Comment on attachment 63997 [details] patch Thanks for the patch Mahesh. In general, I like the new structure. A few comments below. WebCore/page/Geolocation.cpp:284 + // Only start update if we're not waiting for user permission. The indentation here is wrong. Maybe move the comment inside the 'else if' and reword as 'If we don't yet have permission, request it now rather than calling startUpdating().' WebCore/page/Geolocation.cpp:286 + ASSERT(!isDenied()); There's probably no need for this now that we test for isDenied() just above. WebCore/page/Geolocation.cpp:410 + // Permission request was made during the startUpdating process This comment is no longer accurate WebCore/page/Geolocation.cpp:662 + // start all pending notification requests as permission granted. This comment and the one above apply to both the client-based and non-client based code, so should be above the #if WebCore/page/Geolocation.cpp:664 + notifier->setFatalError(PositionError::create(PositionError::POSITION_UNAVAILABLE, failedToStartServiceErrorMessage)); Let's leave the non-client-based implementation for Bug 42027. WebCore/page/Geolocation.cpp:655 + notifier->startTimerIfNeeded(); We only need to start the timer if permission is allowed and if we successfully start the service/add the observer.

Created attachment 64027 [details] patch Incorporated changes from comment #10

Comment on attachment 64027 [details] patch WebCore/page/Geolocation.cpp:661 + #endif You could leave the TODO in place for the non-client-based impl if you like. WebCore/page/Geolocation.cpp:651 + GeoNotifierSet::const_iterator end = m_pendingForPermissionNotifiers.end(); It might be worth adding a comment that we don't need to protect against the set being modified while we iterate through it because any reentrant calls from JS callbacks to getCurrentPosition()/watchPosition() will see the permission as having been set, so won't add notifiers to the pending set.

Created attachment 64094 [details] patch Thanks Steve for r+ I don't have commit rights so submitting the patch again with changes as per comment #12. Also if you could r+ commit queue for it to pick up and land patch automatically.

Comment on attachment 64094 [details] patch r=me

Comment on attachment 64094 [details] patch Rejecting patch 64094 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 20831 test cases. fast/dom/Geolocation/delayed-multiple-permissions-allowed.html -> failed Exiting early after 1 failures. 6591 tests run. 128.40s total testing time 6590 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 5 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/3756076

Comment on attachment 64027 [details] patch Cleared Steve Block's review+ from obsolete attachment 64027 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Created attachment 64421 [details] patch test case description was mismatching in *expected.txt and js file. Which was the reason for commit bot automated testing to fail.

Comment on attachment 64421 [details] patch This patch is missing ChangeLog entries LayoutTests/fast/dom/Geolocation/script-tests/delayed-multiple-permissions-allowed.js:1 + description("Tests that when multiple positions are available, no callbacks are invoked until permission is allowed."); This is slightly misleading. There aren't multiple positions, there are multiple requests in progress, all awaiting permission. Maybe the test should be named delayed-permission-allowed-multiple-requests?

Created attachment 64483 [details] patch Incorporate changes as per comment 18. Please review

Comment on attachment 64483 [details] patch This is still missing ChangeLogs LayoutTests/fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html:10 + <script src="script-tests/delayed-permission-allowed-for-multiple-requests.html"></script> This should be the .js file. Did you run the tests locally? LayoutTests/platform/gtk/Skipped:1095 + fast/dom/Geolocation/delayed-multiple-permissions-denied.html Needs updating LayoutTests/platform/qt/Skipped:5497 + fast/dom/Geolocation/delayed-multiple-permissions-denied.html Needs updating

(In reply to comment #20) > (From update of attachment 64483 [details]) > This is still missing ChangeLogs > > LayoutTests/fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html:10 > + <script src="script-tests/delayed-permission-allowed-for-multiple-requests.html"></script> > This should be the .js file. Did you run the tests locally? > No :( I had that machine formatted. Apologies for the trouble. Will upload the tested one in a min

Created attachment 64555 [details] patch Renamed and tested new layout cases on Qt. But qt DMR fix will be checked-in through bug 41364. Skipping the new cases for now

(In reply to comment #22) > Created an attachment (id=64555) [details] > patch > > Renamed and tested new layout cases on Qt. But qt DMR fix will be checked-in through bug 41364. Skipping the new cases for now Spelling mistake. Qt s/DMR/DRT

Comment on attachment 64094 [details] patch Cleared Steve Block's review+ from obsolete attachment 64094 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Comment on attachment 64555 [details] patch WebCore/ChangeLog:8 + While waiting for permission, m_startRequestPermissionNotifier was Indentation WebCore/ChangeLog:11 + When user grants/denies permission all listener's will be notified. apostrophe

Created attachment 64579 [details] patch incorporated above comments.

Comment on attachment 64579 [details] patch r=me Thanks for making all the changes!

Comment on attachment 64579 [details] patch Clearing flags on attachment: 64579 Committed r65511: <http://trac.webkit.org/changeset/65511>

All reviewed patches have been landed. Closing bug.

The new tests fail on Chromium because the Chromium DRT lacks the functionality for these delayed permission tests. See bug 43480 A failing test expectation was added in http://trac.webkit.org/changeset/65535

(In reply to comment #27) > (From update of attachment 64579 [details]) > r=me > > Thanks for making all the changes! Thanks for the review Steve.

(In reply to comment #27) > (From update of attachment 64579 [details]) > r=me > > Thanks for making all the changes! Thanks for the review Steve. And Johnny for fixing chromium test failures :)

Revision r65511 cherry-picked into qtwebkit-2.1 with commit e3bec9eeccf5d1ebcc6d4c3ddd23df19fc9f7595