Bug 24505 – Geolocation needs to support asynchronous Chrome challenges

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Greg Bolsinga]

Right now the Geolocation API forces the Chrome to block to return whether or not the Chrome accepts the challenge.

This patch changes it to be asynchronous.

[Greg Bolsinga]

Created attachment 28458 [details]
Implements an asynchronous ChromeClient challenge for Geolocation

[Mark Rowe (bdash)]

Comment on attachment 28458 [details]
Implements an asynchronous ChromeClient challenge for Geolocation

> Index: WebCore/platform/mac/GeolocationServiceMac.mm
> ===================================================================
> --- WebCore/platform/mac/GeolocationServiceMac.mm	(revision 41572)
> +++ WebCore/platform/mac/GeolocationServiceMac.mm	(working copy)
> @@ -34,6 +34,7 @@
>  #import "PositionOptions.h"
>  #import "SoftLinking.h"
>  #import <CoreLocation/CoreLocation.h>
> +#import <CoreLocation/CoreLocationPriv.h>
>  #import <objc/objc-runtime.h>
>  #import <wtf/RefPtr.h>
>  #import <wtf/UnusedParam.h>
> @@ -121,6 +122,12 @@
>      [m_locationManager.get() startUpdatingLocation];
>  }
>  
> +bool GeolocationServiceMac::shouldCacheBeCleared()
> +{
> +    // This uses CoreLocation SPI.
> +    return m_locationManager.get() ? !m_locationManager.get().locationServicesApproved : true;
> +}
> +

We can't land something that uses SPI such as this in open source.

[Greg Bolsinga]

Understood. I can make that return false, and keep the method.

[Simon Fraser (smfr)]

Comment on attachment 28458 [details]
Implements an asynchronous ChromeClient challenge for Geolocation

> Index: WebKit/mac/WebCoreSupport/WebGeolocationPrivate.h

> +@interface WebGeolocation : NSObject {
> +@private
> +    WebGeolocationPrivate *_private;
> +    BOOL _shouldClearCache;
> +}
> +
> +- (BOOL)shouldClearCache;
> +- (void)setIsAllowed:(BOOL)allowed;
> +@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.

[Greg Bolsinga]

(In reply to comment #4)
> 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?

It is an ObjC wrapper around WebCore's Geolocation C++ class. I followed the example (and coding style) in WebSecurityOrigin/SecurityOrigin.

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

It is a challenge the user to accept Geolocation, tell me what they did later via the Geolocation object call. I'm open to naming suggestions.

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

I followed the example (and coding style) in WebSecurityOrigin/SecurityOrigin.

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

Perhaps I can add the shouldClearCache bool to Geolocation? The GeolocationService knows when the cache should be cleared, and lets the ChromeClient know this via this call. The cache is kept in the chrome (as the Chrome may have different caching policies that way, and WebCore doesn't need to know about them.

> > +    void setIsAllowed(bool);
> 
> Why no associated getter?

It's used only within the class, I can add it, but it isn't necessary.

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

These enum names were here previously, via Sam.

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

I'm open to renames for the challenge method. That was the best I came up with.

[Greg Bolsinga]

Created attachment 28566 [details]
Updated patch addresses issues found

updated

[Simon Fraser (smfr)]

Comment on attachment 28566 [details]
Updated patch addresses issues found

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

[Greg Bolsinga]

bolsinga:WebKit bolsinga$ svn commit
Sending        WebCore/ChangeLog
Sending        WebCore/WebCore.base.exp
Sending        WebCore/WebCore.xcodeproj/project.pbxproj
Sending        WebCore/page/Chrome.cpp
Sending        WebCore/page/Chrome.h
Sending        WebCore/page/ChromeClient.h
Sending        WebCore/page/Geolocation.cpp
Sending        WebCore/page/Geolocation.h
Sending        WebKit/ChangeLog
Sending        WebKit/WebKit.xcodeproj/project.pbxproj
Sending        WebKit/mac/ChangeLog
Sending        WebKit/mac/WebCoreSupport/WebChromeClient.h
Sending        WebKit/mac/WebCoreSupport/WebChromeClient.mm
Adding         WebKit/mac/WebCoreSupport/WebGeolocation.mm
Adding         WebKit/mac/WebCoreSupport/WebGeolocationInternal.h
Adding         WebKit/mac/WebCoreSupport/WebGeolocationPrivate.h
Sending        WebKit/mac/WebView/WebUIDelegatePrivate.h
Transmitting file data .................
Committed revision 41675.

[George Staikos]

I believe the ChromeClient callback should be pure virtual.  This is standard for the other callbacks and it leads to bugs in the current way.  We don't notice when things change underneath us.