[webkit-reviews] review denied: [Bug 42068] Need to be able to configure Geolocation policy regarding user permissions : [Attachment 61985] attempt to fix

[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 42068: Need to be able to configure Geolocation policy regarding user
permissions
https://bugs.webkit.org/show_bug.cgi?id=42068

Attachment 61985: attempt to fix
https://bugs.webkit.org/attachment.cgi?id=61985&action=review

------- Additional Comments from Steve Block <steveblock at google.com>
Thanks for the patch Mahesh.

WebCore/ChangeLog:9
 +	    acquires user permission first before starting instantiating
location
extra 'instantiating'

WebCore/page/Geolocation.cpp:403
 +  
extra blank line

WebCore/WebCore.pro:2704
 +  # if geolocation is enabled, enable pre-request for permission policy
Should probably be indented

JavaScriptCore/wtf/Platform.h:1121
 +  #define WTF_USE_PREEMPT_GEOLOCATION_PERMISSION
Add a comment that currently, client-based geolocation supports only the
preemptive permission policy and that non-client-based Geolocation supports
only the non-preemptive policy.

WebCore/page/Geolocation.cpp:626
 +	// FIXME: Pass options to client.
Could you move this to the addObserver() line?

WebCore/page/Geolocation.cpp:623
 +	}
I think we should close the #if USE(PREEMPT_GEOLOCATION_PERMISSION) here

WebCore/page/Geolocation.cpp:640
 +  #endif // USE(PREEMPT_GEOLOCATION_PERMISSION)
This block should just be ifdef'ed on CLIENT_BASED_GEOLOCATION

WebCore/page/Geolocation.cpp:417
 +  #endif
I assume that when you add support for the preemptive policy to the non-client
based impl, you'll add an 'else' here? If so, you could add the else and a TODO
now to make it clear.