| Summary: | Geolocation needs to support asynchronous Chrome challenges | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Greg Bolsinga <bolsinga> | ||||||
| Component: | WebCore JavaScript | Assignee: | Greg Bolsinga <bolsinga> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | koivisto, mrowe, sam | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Mac | ||||||||
| OS: | OS X 10.5 | ||||||||
| Attachments: |
|
||||||||
|
Description
Greg Bolsinga
2009-03-10 18:33:16 PDT
Created attachment 28458 [details]
Implements an asynchronous ChromeClient challenge for Geolocation
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. Understood. I can make that return false, and keep the method. 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. (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. Created attachment 28566 [details]
Updated patch addresses issues found
updated
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. 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. 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. |