[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 bugzilla-daemon at webkit.org
Wed Mar 11 14:12:43 PDT 2009

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

Attachment 28458: Implements an asynchronous ChromeClient challenge for

------- 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

> 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

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
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.

More information about the webkit-reviews mailing list