See the following code: void Geolocation::requestPermission() { ...... // Ask the chrome: it maintains the geolocation challenge policy itself. page->chrome()->requestGeolocationPermissionForFrame(m_frame, this); m_allowGeolocation = InProgress; } And the only case that requestPermission() is called: void Geolocation::geolocationServicePositionChanged(GeolocationService* service) { ASSERT(service->lastPosition()); requestPermission(); if (!isAllowed()) return; ...... } This is weird. Whatever Chrome::requestGeolocationPermissionForFrame() does on the Geolocation object, m_allowGeolocation is always InProgress. Unless it calls setAllowed by a timer. So as the result, the first call to geolocationServicePositionChanged must fail, and the subsequent calls can succeed only after the ChromeClient calls setIsAllowed(true) by a timer. Why not change it to: m_allowGeolocation = InProgress; page->chrome()->requestGeolocationPermissionForFrame(m_frame, this); Is it any reason for it?
The patch: --- a/WebCore/page/Geolocation.cpp +++ b/WebCore/page/Geolocation.cpp @@ -1,5 +1,6 @@ /* * Copyright (C) 2008, 2009 Apple Inc. All Rights Reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -264,10 +265,10 @@ void Geolocation::requestPermission() if (!page) return; + m_allowGeolocation = InProgress; + // Ask the chrome: it maintains the geolocation challenge policy itself. page->chrome()->requestGeolocationPermissionForFrame(m_frame, this); - - m_allowGeolocation = InProgress; }
Created attachment 33659 [details] the patch
*** Bug 27853 has been marked as a duplicate of this bug. ***
Is this testable somehow? It would be easy to test with a LayoutTest, but I'm not sure how we could test it with a LayoutTest...
Comment on attachment 33659 [details] the patch Let me know if you figure out a way to test this.
You also might consider adding more explanation to the ChangeLog. :)
(In reply to comment #6) > You also might consider adding more explanation to the ChangeLog. :) Thanks!
Were you able to test this out? I'd have to do quite a bit of work to get my port back to this asynchronous mode to test it out. Thanks!
Comment on attachment 33659 [details] the patch Clearing review flag on attachment: 33659 Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/page/Geolocation.cpp Committed r46663 M WebCore/ChangeLog M WebCore/page/Geolocation.cpp r46663 = 652e8a6a543e021b653c175b4640f6d8fa392af3 (trunk) No changes between current HEAD and refs/remotes/trunk Resetting to the latest refs/remotes/trunk http://trac.webkit.org/changeset/46663
All reviewed patches have been landed. Closing bug.
If I've followed the comments correctly, this change is intended to support both synchronous and asynchronous implementations of requestGeolocationPermissionForFrame(). I think, however, that this patch introduces a bug in the case where requestGeolocationPermissionForFrame() is synchronous. setIsAllowed() calls geolocationServicePositionChanged(), so if requestGeolocationPermissionForFrame() is synchronous, any watches will be called back twice, rather than once as intended. See https://bugs.webkit.org/show_bug.cgi?id=27690 (which is a duplicate of this bug) for a full explanation.
(In reply to comment #11) > If I've followed the comments correctly, this change is intended to support > both synchronous and asynchronous implementations of > requestGeolocationPermissionForFrame(). > > I think, however, that this patch introduces a bug in the case where > requestGeolocationPermissionForFrame() is synchronous. setIsAllowed() calls > geolocationServicePositionChanged(), so if > requestGeolocationPermissionForFrame() is synchronous, any watches will be > called back twice, rather than once as intended. > > See https://bugs.webkit.org/show_bug.cgi?id=27690 (which is a duplicate of this > bug) for a full explanation. I don't think it's a problem, because: void Geolocation::requestPermission() { if (m_allowGeolocation > Unknown) return; ...
> I don't think it's a problem, because: I'm pretty sure it is a problem. You're right that the check on m_allowGeolocation in requestPermission() will prevent repeated calls to requestGeolocationPermissionForFrame(), but there will still be repeated callbacks to watchers. The first callbacks are made when the call stack is ... geolocationServicePositionChanged() -> requestPermission() -> requestGeolocationPermissionForFrame() -> setIsAllowed() -> geolocationServicePositionChanged() The stack then unwinds to ... geolocationServicePositionChanged() and the callbacks are made a second time.
(In reply to comment #13) > > I don't think it's a problem, because: > I'm pretty sure it is a problem. You're right that the check on > m_allowGeolocation in requestPermission() will prevent repeated calls to > requestGeolocationPermissionForFrame(), but there will still be repeated > callbacks to watchers. > > The first callbacks are made when the call stack is ... > geolocationServicePositionChanged() -> requestPermission() -> > requestGeolocationPermissionForFrame() -> setIsAllowed() -> > geolocationServicePositionChanged() > > The stack then unwinds to ... > geolocationServicePositionChanged() > and the callbacks are made a second time. I see. It sends current position twice.
> I see. It sends current position twice. Indeed. I'll send a patch to fix this if somebody can review it. Can somebody also reopen this bug and mark Bug 27690 as a duplicate?
reopening
Created attachment 34270 [details] Patch 2 for bug 26993 Fixes bug where callbacks are called twice when permissions are granted synchronously.
Comment on attachment 34270 [details] Patch 2 for bug 26993 Why do we now have an unused argument? Seems we should at least be asserting, no? Why shoudn't makeSuccessCallbacks take an service parameter? r- for the unexplained unused argument.
> Why do we now have an unused argument? The service argument passed to geolocationServicePositionChanged() should always be equal to m_service. > Seems we should at least be asserting, no? Ideally, yes, we'd ASSERT(service == m_service). However, since service is not used in this method (nor is m_service), using service in an ASSERT only gives an 'unused argument' build warning in opt builds. I could ifdef the service argument in the method definition for debug builds only?
(In reply to comment #19) > > Seems we should at least be asserting, no? > Ideally, yes, we'd ASSERT(service == m_service). However, since service is not > used in this method (nor is m_service), using service in an ASSERT only gives > an 'unused argument' build warning in opt builds. The ASSERT_UNUSED macro is for this sort of situation. ASSERT_UNUSED(service, service == m_service); The above will do the trick.
*** Bug 27690 has been marked as a duplicate of this bug. ***
Created attachment 34323 [details] Patch 3 for bug 26993 Updated to use ASSERT_UNUSED. Thanks - I wasn't aware of that macro.
Comment on attachment 34323 [details] Patch 3 for bug 26993 Your ChangeLog shoudl have said "No new tests possible.' and explained why. Tests are always required! :)
Comment on attachment 34323 [details] Patch 3 for bug 26993 Rejecting patch 34323 from commit-queue. This patch will require manual commit. Failed to run "['git', 'svn', 'dcommit']" exit_code: 1 cwd: None
Comment on attachment 34323 [details] Patch 3 for bug 26993 Tests passed. ChangeLog was out of date by the time it went to commit. Will land manually.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/page/Geolocation.cpp M WebCore/page/Geolocation.h Committed r47152 M WebCore/ChangeLog M WebCore/page/Geolocation.cpp M WebCore/page/Geolocation.h r47152 = 27684299a4c6bc752037f37c7391cb19a17e7c60 (trunk) No changes between current HEAD and refs/remotes/trunk Resetting to the latest refs/remotes/trunk http://trac.webkit.org/changeset/47152
Comment on attachment 34323 [details] Patch 3 for bug 26993 Rejecting patch 34323 from commit-queue. This patch will require manual commit. Patch https://bugs.webkit.org/attachment.cgi?id=34323 from bug 26993 failed to download and apply.