[webkit-reviews] review denied: [Bug 42811] Geolocation preemptive permissions policy is buggy : [Attachment 63822] patch

[bugzilla-daemon at webkit.org]

Steve Block <steveblock at google.com> has denied Mahesh Kulkarni
<mahesh.kulkarni at nokia.com>'s request for review:
Bug 42811: Geolocation preemptive permissions policy is buggy
https://bugs.webkit.org/show_bug.cgi?id=42811

Attachment 63822: patch
https://bugs.webkit.org/attachment.cgi?id=63822&action=review

------- Additional Comments from Steve Block <steveblock at google.com>
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-allo
wed.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