[webkit-reviews] review denied: [Bug 42811] Geolocation preemptive permissions policy is buggy : [Attachment 63997] 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 63997: patch
https://bugs.webkit.org/attachment.cgi?id=63997&action=review

------- Additional Comments from Steve Block <steveblock at google.com>
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_UNAVAILAB
LE, 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.