Bug 24505 - Geolocation needs to support asynchronous Chrome challenges
: Geolocation needs to support asynchronous Chrome challenges
Status: RESOLVED FIXED
: WebKit
WebCore JavaScript
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-03-10 18:33 PST by
Modified: 2009-05-15 07:34 PST (History)


Attachments
Implements an asynchronous ChromeClient challenge for Geolocation (29.15 KB, patch)
2009-03-10 18:38 PST, Greg Bolsinga
simon.fraser: review-
Review Patch | Details | Formatted Diff | Diff
Updated patch addresses issues found (29.36 KB, patch)
2009-03-12 16:40 PST, Greg Bolsinga
simon.fraser: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-03-10 18:33:16 PST
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.
------- Comment #1 From 2009-03-10 18:38:14 PST -------
Created an attachment (id=28458) [details]
Implements an asynchronous ChromeClient challenge for Geolocation
------- Comment #2 From 2009-03-10 19:57:12 PST -------
(From update of attachment 28458 [details])
> 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.
------- Comment #3 From 2009-03-10 20:06:28 PST -------
Understood. I can make that return false, and keep the method.
------- Comment #4 From 2009-03-11 14:12:43 PST -------
(From update of attachment 28458 [details])
> 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.
------- Comment #5 From 2009-03-11 14:55:58 PST -------
(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.
------- Comment #6 From 2009-03-12 16:40:10 PST -------
Created an attachment (id=28566) [details]
Updated patch addresses issues found

updated
------- Comment #7 From 2009-03-12 23:05:01 PST -------
(From update of attachment 28566 [details])
> 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.
------- Comment #8 From 2009-03-13 11:57:07 PST -------
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.
------- Comment #9 From 2009-05-15 07:34:27 PST -------
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.