[webkit-reviews] review granted: [Bug 24505] Geolocation needs to support asynchronous Chrome challenges : [Attachment 28566] Updated patch addresses issues found

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Greg Bolsinga
<bolsinga at apple.com>'s request for review:
Bug 24505: Geolocation needs to support asynchronous Chrome challenges
https://bugs.webkit.org/show_bug.cgi?id=24505

Attachment 28566: Updated patch addresses issues found
https://bugs.webkit.org/attachment.cgi?id=28566&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/page/ChromeClient.h
> ===================================================================
> --- WebCore/page/ChromeClient.h	(revision 41656)
> +++ WebCore/page/ChromeClient.h	(working copy)
> @@ -42,6 +42,7 @@ namespace WebCore {
>      class FileChooser;
>      class FloatRect;
>      class Frame;
> +    class Geolocation;
>      class HitTestResult;
>      class IntRect;
>      class Node;
> @@ -152,8 +153,8 @@ namespace WebCore {
>					     float value, float proportion,
ScrollbarControlPartMask);
>	   virtual bool paintCustomScrollCorner(GraphicsContext*, const
FloatRect&);
>  
> -	   virtual bool shouldAllowGeolocationForFrame(Frame*) { return false;
}
> -
> +	   virtual void requestGeolocationPermissionForFrame(Frame*,
Geolocation*) { }

I'd like to see a comment saying that the call is async, and describing
the code path is for giving final approval to do Geolocation
(i.e. someone calls geoloc->setIsAllowed(true));

> Index: WebCore/page/Geolocation.cpp
> ===================================================================

> +void Geolocation::displayChallengeIfNecessary()

I think this could be renamed to requestPermission() to be more similar
to requestGeolocationPermissionForFrame().

r=me with those changes.