[webkit-reviews] review denied: [Bug 24505] Geolocation needs to support asynchronous Chrome challenges : [Attachment 28458] Implements an asynchronous ChromeClient challenge for Geolocation

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied 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 28458: Implements an asynchronous ChromeClient challenge for
Geolocation
https://bugs.webkit.org/attachment.cgi?id=28458&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebKit/mac/WebCoreSupport/WebGeolocationPrivate.h

> + at interface WebGeolocation : NSObject {
> + at private
> +    WebGeolocationPrivate *_private;
> +    BOOL _shouldClearCache;
> +}
> +
> +- (BOOL)shouldClearCache;
> +- (void)setIsAllowed:(BOOL)allowed;
> + at end

What does this class actually _do_? The name is not revealing.
It doesn't seem to actually do Geolocation, despite the name. Is it some kind
of cookie?
Or is it a WebGeolocationRequest?

> Index: WebKit/mac/WebCoreSupport/WebChromeClient.mm
> ===================================================================

> -bool WebChromeClient::shouldAllowGeolocationForFrame(Frame* frame)
> +void WebChromeClient::challengeGeolocationForFrame(Frame* frame,
Geolocation* geolocation, bool shouldClearCache)

"challenge" doesn't communicate what this method does. Is it an async "give me
my coordinates", or
"ask the user if it's OK to geolocate, then call back later", or something
else?


> Index: WebKit/mac/WebCoreSupport/WebGeolocationInternal.h
> ===================================================================

> +typedef WebCore::Geolocation WebCoreGeolocation;

Seems odd to be typedeffing a namespaced symbol to an Obj-C class. Is this done
elsewhere?

Since WebCore::Geolocation shows up in ChromeClient.h, I think it needs to be a
real, cross-platform
C++ class or struct.

> Index: WebCore/page/ChromeClient.h
> ===================================================================

> -	   virtual bool shouldAllowGeolocationForFrame(Frame*) { return false;
}
> -
> +	   virtual void challengeGeolocationForFrame(Frame*, Geolocation*,
bool) { }
> +	       

As above, I think Geolocation needs to be a concrete type here. Also, you
should keep
the name of the bool parameter, so the reader knows what the bool is for. It
also seems
a bit odd to have this method to two things. Who knows when the cache should be
cleared?
Finally, we're trying to avoid having bool params, because it makes the code at
the callsite
hard to understand.


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

> +    
> +    void setIsAllowed(bool);

Why no associated getter?


>      enum {
>	   Unknown,
> +	   InProgress,
>	   Yes,
>	   No
>      } m_allowGeolocation;

For readability, I think it would be clearer to name this enum, and use
values like GeolocationAllowedUnknown, GeolocationAllowedInProgress etc.

I think this needs some increased clarity through renaming, and some thought
about using a concrete type for Geolocation.