[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
bugzilla-daemon at webkit.org
Mon Jul 19 13:53:16 PDT 2010
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.
More information about the webkit-reviews
mailing list