Add CoreLocation support to Chrome

Add CoreLocation support to Chrome.

Patch from Jorge Villatoro <jorge@tomatocannon.com>;

This add core location support to the Geolocation code.  Included are the base 
location provider class, a data provider class that allows CoreLocation to run 
on the UI thread, and an Objective C wrapper for CoreLocation.  Hints were taken 
from the CoreWLAN Api class in the same directory for dynamically loading the 
framework, since CoreLocation is only available on Snow Leopard (Mac OS X 10.6)

This new provider has also been added to the location arbitrator in the same way
that the libgps provider was added.

BUG=45548
TEST=Open the browser with the --enable-geolocation argument on 10.6 and visit
http://maps.google.com/maps/m

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=59213

[Nico, 2010-08-07 19:10:39]

You said on the bug that this changes the UI how the user is asked if he wants
to grant location permissions? Is there a way around this? We really want to
keep our current UI here.

http://codereview.chromium.org/3092015/diff/1/3
File chrome/browser/geolocation/core_location_data_provider_mac.h (right):

http://codereview.chromium.org/3092015/diff/1/3#newcode46
chrome/browser/geolocation/core_location_data_provider_mac.h:46:
CoreLocationWrapperMac* wrapper_;
Use a scoped_nsobject for this (from base/scoped_nsobject.h)

[jorgevillatoro, 2010-08-07 19:15:58]

I couldn't find any way to change the way that CoreLocation deals with allowing
access to location information.  Essentially what happens is that when you
request updates from CoreLocation, OS X springs up a dialog box that says
something to the effect of "Would you like to allow "Chromium" to access your
location information?" with a check box that says "don't ask me again", and
yes/no options.

You can see the same dialog box with Safari by visiting
http://maps.google.com/maps/m

I'll do some more research to find out if there's any way around this.

On 2010/08/07 19:10:39, Nico wrote:
> You said on the bug that this changes the UI how the user is asked if he wants
> to grant location permissions? Is there a way around this? We really want to
> keep our current UI here.
> 
> http://codereview.chromium.org/3092015/diff/1/3
> File chrome/browser/geolocation/core_location_data_provider_mac.h (right):
> 
> http://codereview.chromium.org/3092015/diff/1/3#newcode46
> chrome/browser/geolocation/core_location_data_provider_mac.h:46:
> CoreLocationWrapperMac* wrapper_;
> Use a scoped_nsobject for this (from base/scoped_nsobject.h)

[joth, 2010-08-09 11:49:14]

On 2010/08/07 19:15:58, jorgevillatoro wrote:
> I couldn't find any way to change the way that CoreLocation deals with
allowing
> access to location information.  Essentially what happens is that when you
> request updates from CoreLocation, OS X springs up a dialog box that says
> something to the effect of "Would you like to allow "Chromium" to access your
> location information?" with a check box that says "don't ask me again", and
> yes/no options.
> 
> You can see the same dialog box with Safari by visiting
> http://maps.google.com/maps/m
> 

When I try this in Safari, I don't get any OS popup. I get an app modal dialog
inside safari that states:
 "The website xxx would like to use your current location. [ ] Request
permission only once every 24 hours.
Don't Allow / Allow"

This would appear to be app originated, not OS.

> I'll do some more research to find out if there's any way around this.
> 

Please do,
Thanks!

[joth, 2010-08-09 12:17:32]

Many thanks for contributing this.
A few comments on the code, but also need to work through the issues Nico
mentioned about UI.

Also, could you describe a little more about the conflicting wifi? Does the
location ping-pong? Would be nice if we can work out a way to avoid this without
special casing or out-right disabling either of them (to maximise coverage it's
good to keep both enabled if possible)

Cheers!

http://codereview.chromium.org/3092015/diff/1/3
File chrome/browser/geolocation/core_location_data_provider_mac.h (right):

http://codereview.chromium.org/3092015/diff/1/3#newcode66
chrome/browser/geolocation/core_location_data_provider_mac.h:66: // them to
Geoposition objects, and passes them on to the data provider class
Mention what the threading restrictions are on this interface. e.g. must be used
in UI thread / mention why it's ok to call locationDataAvailable from any thread

http://codereview.chromium.org/3092015/diff/1/4
File chrome/browser/geolocation/core_location_data_provider_mac.mm (right):

http://codereview.chromium.org/3092015/diff/1/4#newcode1
chrome/browser/geolocation/core_location_data_provider_mac.mm:1: #include
"chrome/browser/geolocation/core_location_data_provider_mac.h"
copyright banner needed

http://codereview.chromium.org/3092015/diff/1/4#newcode5
chrome/browser/geolocation/core_location_data_provider_mac.mm:5: 
file level comment. could you outline the seperation of cpp and obj C class, and
specifically mention that and explain why the obj C class needs to run on the UI
class.

http://codereview.chromium.org/3092015/diff/1/4#newcode21
chrome/browser/geolocation/core_location_data_provider_mac.mm:21:
CoreLocationProviderMac* provider) {
strange indenting

http://codereview.chromium.org/3092015/diff/1/4#newcode22
chrome/browser/geolocation/core_location_data_provider_mac.mm:22:
DCHECK(provider);
DCHECK(!provider_) << "StartUpdating called twice";

http://codereview.chromium.org/3092015/diff/1/4#newcode45
chrome/browser/geolocation/core_location_data_provider_mac.mm:45: void
CoreLocationDataProviderMac::StartUpdatingTask() {
DCHECK(ChromeThread::CurrentlyOn(UI));

http://codereview.chromium.org/3092015/diff/1/4#newcode50
chrome/browser/geolocation/core_location_data_provider_mac.mm:50: void
CoreLocationDataProviderMac::StopUpdatingTask() {
DCHECK(ChromeThread::CurrentlyOn(UI));

http://codereview.chromium.org/3092015/diff/1/4#newcode54
chrome/browser/geolocation/core_location_data_provider_mac.mm:54: void
CoreLocationDataProviderMac::PositionUpdated(Geoposition position) {
DCHECK(ChromeThread::CurrentlyOn(origin_thread_id_));

http://codereview.chromium.org/3092015/diff/1/4#newcode62
chrome/browser/geolocation/core_location_data_provider_mac.mm:62: // This idea
was borrowed from wifi_data_provider_corewlan_mac.mm
TODO(you): Remove these declarations when we build using 10.6 SDK.

http://codereview.chromium.org/3092015/diff/1/4#newcode143
chrome/browser/geolocation/core_location_data_provider_mac.mm:143:
dataProvider_->UpdatePosition(&position);
how does the location manager indicate a loss of location fix? (e.g. loss of GPS
reception)

http://codereview.chromium.org/3092015/diff/1/6
File chrome/browser/geolocation/core_location_provider_mac.mm (right):

http://codereview.chromium.org/3092015/diff/1/6#newcode4
chrome/browser/geolocation/core_location_provider_mac.mm:4: 
this could be a .cc file

http://codereview.chromium.org/3092015/diff/1/7
File chrome/browser/geolocation/location_arbitrator.cc (right):

http://codereview.chromium.org/3092015/diff/1/7#newcode116
chrome/browser/geolocation/location_arbitrator.cc:116: return
::NewCoreLocationProvider();
See http://codereview.chromium.org/3015053/show for the parallel patch for
windows 7.

Over there, we have decided to rename NewGpslocationProvider to
NewSystemLocationProvider, which will make this change obsolete (just implement
CoreLocation as the 'system' location provider)

[jorgevillatoro, 2010-08-09 17:52:39]

On 2010/08/09 12:17:32, joth wrote:

I'll make those changes code changes this evening, thanks for the feedback.

When I mentioned a conflict between the two, I was only referring to the UI
issue.  Right now when CoreLocation is started, it doesn't matter much what the
user's response is to the dialog box.  CoreWlan will provide location data
either way.  It makes sense to use Chrome's current UI for location data
permissions in lieu of CoreLocation's dialog though.

The UI issue looks like it might be difficult to solve.  There's definitely not
anything in the public CoreLocation API for this.  Here are a couple of the
possibilities that occurred to me:

- Find out where the settings for this warning dialog are and pre-emptively set
the permission
- Try class-dump on the framework to see if there are any private APIs that
could make this possible

So far I haven't been able to locate where the settings are stored.  This also
doesn't seem like the best way to go about things though.

Using class-dump on the framework yielded a possibility, but I don't know what
the project's stance is on using private frameworks (although it looks like the
80211 framework is used if CoreWLAN isn't available in the wifi provider).  Here
are the relevant bits:

@interface CLLocationManager : NSObject
...
+ (BOOL)locationServicesEnabled;
+ (void)setLocationServicesEnabled:(BOOL)arg1 withAuthorization:(id)arg2;
+ (void)resetLocationWarningsWithAuthorization:(id)arg1;
...
@property(readonly) BOOL locationServicesEnabled; // @dynamic
locationServicesEnabled;
@property(readonly) BOOL locationServicesApproved; // @dynamic
locationServicesApproved;
...
@end

That setLocationServicesEnabled method is possibly expecting a SFAuthorization
object, but finding out how that object should be created/configured seems like
a difficult problem.

Another possibility is this CoreLocationAgent.app that I found under
/System/Library/CoreServices, but there's not really much information out there
about it.  Running class-dump reveals some method names that appear to be
related to authorizing location data, but nothing more than that.

I'll keep on looking into this, do you have any ideas?

--Jorge

> Many thanks for contributing this.
> A few comments on the code, but also need to work through the issues Nico
> mentioned about UI.
> 
> Also, could you describe a little more about the conflicting wifi? Does the
> location ping-pong? Would be nice if we can work out a way to avoid this
without
> special casing or out-right disabling either of them (to maximise coverage
it's
> good to keep both enabled if possible)
> 
> Cheers!
> 
> http://codereview.chromium.org/3092015/diff/1/3
> File chrome/browser/geolocation/core_location_data_provider_mac.h (right):
> 
> http://codereview.chromium.org/3092015/diff/1/3#newcode66
> chrome/browser/geolocation/core_location_data_provider_mac.h:66: // them to
> Geoposition objects, and passes them on to the data provider class
> Mention what the threading restrictions are on this interface. e.g. must be
used
> in UI thread / mention why it's ok to call locationDataAvailable from any
thread
> 
> http://codereview.chromium.org/3092015/diff/1/4
> File chrome/browser/geolocation/core_location_data_provider_mac.mm (right):
> 
> http://codereview.chromium.org/3092015/diff/1/4#newcode1
> chrome/browser/geolocation/core_location_data_provider_mac.mm:1: #include
> "chrome/browser/geolocation/core_location_data_provider_mac.h"
> copyright banner needed
> 
> http://codereview.chromium.org/3092015/diff/1/4#newcode5
> chrome/browser/geolocation/core_location_data_provider_mac.mm:5: 
> file level comment. could you outline the seperation of cpp and obj C class,
and
> specifically mention that and explain why the obj C class needs to run on the
UI
> class.
> 
> http://codereview.chromium.org/3092015/diff/1/4#newcode21
> chrome/browser/geolocation/core_location_data_provider_mac.mm:21:
> CoreLocationProviderMac* provider) {
> strange indenting
> 
> http://codereview.chromium.org/3092015/diff/1/4#newcode22
> chrome/browser/geolocation/core_location_data_provider_mac.mm:22:
> DCHECK(provider);
> DCHECK(!provider_) << "StartUpdating called twice";
> 
> http://codereview.chromium.org/3092015/diff/1/4#newcode45
> chrome/browser/geolocation/core_location_data_provider_mac.mm:45: void
> CoreLocationDataProviderMac::StartUpdatingTask() {
> DCHECK(ChromeThread::CurrentlyOn(UI));
> 
> http://codereview.chromium.org/3092015/diff/1/4#newcode50
> chrome/browser/geolocation/core_location_data_provider_mac.mm:50: void
> CoreLocationDataProviderMac::StopUpdatingTask() {
> DCHECK(ChromeThread::CurrentlyOn(UI));
> 
> http://codereview.chromium.org/3092015/diff/1/4#newcode54
> chrome/browser/geolocation/core_location_data_provider_mac.mm:54: void
> CoreLocationDataProviderMac::PositionUpdated(Geoposition position) {
> DCHECK(ChromeThread::CurrentlyOn(origin_thread_id_));
> 
> http://codereview.chromium.org/3092015/diff/1/4#newcode62
> chrome/browser/geolocation/core_location_data_provider_mac.mm:62: // This idea
> was borrowed from wifi_data_provider_corewlan_mac.mm
> TODO(you): Remove these declarations when we build using 10.6 SDK.
> 
> http://codereview.chromium.org/3092015/diff/1/4#newcode143
> chrome/browser/geolocation/core_location_data_provider_mac.mm:143:
> dataProvider_->UpdatePosition(&position);
> how does the location manager indicate a loss of location fix? (e.g. loss of
GPS
> reception)
> 
> http://codereview.chromium.org/3092015/diff/1/6
> File chrome/browser/geolocation/core_location_provider_mac.mm (right):
> 
> http://codereview.chromium.org/3092015/diff/1/6#newcode4
> chrome/browser/geolocation/core_location_provider_mac.mm:4: 
> this could be a .cc file
> 
> http://codereview.chromium.org/3092015/diff/1/7
> File chrome/browser/geolocation/location_arbitrator.cc (right):
> 
> http://codereview.chromium.org/3092015/diff/1/7#newcode116
> chrome/browser/geolocation/location_arbitrator.cc:116: return
> ::NewCoreLocationProvider();
> See http://codereview.chromium.org/3015053/show for the parallel patch for
> windows 7.
> 
> Over there, we have decided to rename NewGpslocationProvider to
> NewSystemLocationProvider, which will make this change obsolete (just
implement
> CoreLocation as the 'system' location provider)

[jorgevillatoro, 2010-08-10 00:44:14]

I've made the changes requested by Joth, as well as fixed a few issues with the
first patch set.  The wrapper class has been fixed so that the CoreLocation
framework is actually loaded now (some debugging code ended up in the wrapper
accidentally), and an error handler for the location manager has been added.

--Jorge

http://codereview.chromium.org/3092015/diff/1/3
File chrome/browser/geolocation/core_location_data_provider_mac.h (right):

http://codereview.chromium.org/3092015/diff/1/3#newcode46
chrome/browser/geolocation/core_location_data_provider_mac.h:46:
CoreLocationWrapperMac* wrapper_;
On 2010/08/07 19:10:40, Nico wrote:
> Use a scoped_nsobject for this (from base/scoped_nsobject.h)

Done.

http://codereview.chromium.org/3092015/diff/1/3#newcode66
chrome/browser/geolocation/core_location_data_provider_mac.h:66: // them to
Geoposition objects, and passes them on to the data provider class
I've clarified some more, let me know if this is what you meant

On 2010/08/09 12:17:33, joth wrote:
> Mention what the threading restrictions are on this interface. e.g. must be
used
> in UI thread / mention why it's ok to call locationDataAvailable from any
thread

http://codereview.chromium.org/3092015/diff/1/4
File chrome/browser/geolocation/core_location_data_provider_mac.mm (right):

http://codereview.chromium.org/3092015/diff/1/4#newcode1
chrome/browser/geolocation/core_location_data_provider_mac.mm:1: #include
"chrome/browser/geolocation/core_location_data_provider_mac.h"
On 2010/08/09 12:17:33, joth wrote:
> copyright banner needed

Done.

http://codereview.chromium.org/3092015/diff/1/4#newcode5
chrome/browser/geolocation/core_location_data_provider_mac.mm:5: 
On 2010/08/09 12:17:33, joth wrote:
> file level comment. could you outline the seperation of cpp and obj C class,
and
> specifically mention that and explain why the obj C class needs to run on the
UI
> class.

Done.

http://codereview.chromium.org/3092015/diff/1/4#newcode21
chrome/browser/geolocation/core_location_data_provider_mac.mm:21:
CoreLocationProviderMac* provider) {
Agreed, I was trying to break the line up because it was longer than 80
characters.  I changed it to match something I found in a couple of other files.

On 2010/08/09 12:17:33, joth wrote:
> strange indenting

http://codereview.chromium.org/3092015/diff/1/4#newcode22
chrome/browser/geolocation/core_location_data_provider_mac.mm:22:
DCHECK(provider);
On 2010/08/09 12:17:33, joth wrote:
> DCHECK(!provider_) << "StartUpdating called twice";

Done.

http://codereview.chromium.org/3092015/diff/1/4#newcode45
chrome/browser/geolocation/core_location_data_provider_mac.mm:45: void
CoreLocationDataProviderMac::StartUpdatingTask() {
On 2010/08/09 12:17:33, joth wrote:
> DCHECK(ChromeThread::CurrentlyOn(UI));

Done.

http://codereview.chromium.org/3092015/diff/1/4#newcode50
chrome/browser/geolocation/core_location_data_provider_mac.mm:50: void
CoreLocationDataProviderMac::StopUpdatingTask() {
On 2010/08/09 12:17:33, joth wrote:
> DCHECK(ChromeThread::CurrentlyOn(UI));

Done.

http://codereview.chromium.org/3092015/diff/1/4#newcode54
chrome/browser/geolocation/core_location_data_provider_mac.mm:54: void
CoreLocationDataProviderMac::PositionUpdated(Geoposition position) {
On 2010/08/09 12:17:33, joth wrote:
> DCHECK(ChromeThread::CurrentlyOn(origin_thread_id_));

Done.

http://codereview.chromium.org/3092015/diff/1/4#newcode62
chrome/browser/geolocation/core_location_data_provider_mac.mm:62: // This idea
was borrowed from wifi_data_provider_corewlan_mac.mm
On 2010/08/09 12:17:33, joth wrote:
> TODO(you): Remove these declarations when we build using 10.6 SDK.
> 

Done.

http://codereview.chromium.org/3092015/diff/1/4#newcode143
chrome/browser/geolocation/core_location_data_provider_mac.mm:143:
dataProvider_->UpdatePosition(&position);
Errors are reported through a separate callback,
locationManager:didFailWithError

At first, I didn't see any way of reporting these through the Provider API, so
it went unimplemented.  But I realized that these errors should be reported
through the geoposition.  I've added a new handler in the wrapper for this.

On 2010/08/09 12:17:33, joth wrote:
> how does the location manager indicate a loss of location fix? (e.g. loss of
GPS
> reception)

http://codereview.chromium.org/3092015/diff/1/6
File chrome/browser/geolocation/core_location_provider_mac.mm (right):

http://codereview.chromium.org/3092015/diff/1/6#newcode4
chrome/browser/geolocation/core_location_provider_mac.mm:4: 
Could it?  Since there is some Obj-C in core_location_data_provider_mac.h I was
under the impression that this file had to be a .mm file

On 2010/08/09 12:17:33, joth wrote:
> this could be a .cc file
>

http://codereview.chromium.org/3092015/diff/1/7
File chrome/browser/geolocation/location_arbitrator.cc (right):

http://codereview.chromium.org/3092015/diff/1/7#newcode116
chrome/browser/geolocation/location_arbitrator.cc:116: return
::NewCoreLocationProvider();
On 2010/08/09 12:17:33, joth wrote:
> See http://codereview.chromium.org/3015053/show for the parallel patch for
> windows 7.
> 
> Over there, we have decided to rename NewGpslocationProvider to
> NewSystemLocationProvider, which will make this change obsolete (just
implement
> CoreLocation as the 'system' location provider)
> 

Done.

[joth, 2010-08-10 12:39:48]

Looks good, few small comments then all that remains is the permission prompt.
(Which has an additional impact I realized, noted in data_provider.mm).

If we can't avoid the prompt, the remaining options I can think of are:
- see if we can at least read back the current setting (prompt, always allow,
always deny) to streamline our handling (e.g. if set to always deny, we should
not start any location providers)
- introduce the concept of a gatekeeper provider: the arbitrator will only start
other providers once the gatekeeper has given a callback (unless it completes
with permission denied, in which case it stops all providers)

either of these do have consequences for the user (additional steps, latency
hit) so will need to rope in some UI folks to give thumbs up to it.

Cheers,

http://codereview.chromium.org/3092015/diff/1/6
File chrome/browser/geolocation/core_location_provider_mac.mm (right):

http://codereview.chromium.org/3092015/diff/1/6#newcode4
chrome/browser/geolocation/core_location_provider_mac.mm:4: 
On 2010/08/10 00:44:14, jorgevillatoro wrote:
> Could it?  Since there is some Obj-C in core_location_data_provider_mac.h I
was
> under the impression that this file had to be a .mm file
> 

Ah, that was my other question I forgot to ask: does the obj-c class def and
related bits need to be in that .h? Seems they are implementation details and
could be moved to core_location_data_provider.mm?

http://codereview.chromium.org/3092015/diff/15001/16002
File chrome/browser/geolocation/core_location_data_provider_mac.h (right):

http://codereview.chromium.org/3092015/diff/15001/16002#newcode84
chrome/browser/geolocation/core_location_data_provider_mac.h:84: // Can be
called from anywhere since it only initializes bundle_ once
// Can be called from any thread since it does not require an NSRunLoop. However
it is not threadsafe to receive concurrent calls until after it's first
successful call (to avoid |bundle_| being double initialized)

http://codereview.chromium.org/3092015/diff/15001/16003
File chrome/browser/geolocation/core_location_data_provider_mac.mm (right):

http://codereview.chromium.org/3092015/diff/15001/16003#newcode172
chrome/browser/geolocation/core_location_data_provider_mac.mm:172:
position.error_code = Geoposition::ERROR_CODE_POSITION_UNAVAILABLE;
it would be consistent to map kCLErrorNetwork here too.

http://codereview.chromium.org/3092015/diff/15001/16003#newcode175
chrome/browser/geolocation/core_location_data_provider_mac.mm:175:
position.error_code = Geoposition::ERROR_CODE_PERMISSION_DENIED;
Hmmmm tricky: if this happens to be the first error code that goes back through
to webkit, it will cache the permission block and location will never work for
this page load.
On the other hand, if another location provider gets back first, it will work
fine despite this permission block.
i.e. it's a race, with unpredictable user experience.

Basically comes back to the problem of having two competing permission UIs - if
we can solve that, this should go away.

http://codereview.chromium.org/3092015/diff/15001/16003#newcode177
chrome/browser/geolocation/core_location_data_provider_mac.mm:177: default:
could you list the other codes explicitly ( kCLErrorHeadingFailure,
   kCLErrorRegionMonitoringDenied,
   kCLErrorRegionMonitoringFailure,
   kCLErrorRegionMonitoringSetupDelayed) with a comment saying these are partial
errors we're not interested in, and in the default case put NOTREACHED() <<
"Unknown CoreLocation error: " << [error code];
return;

http://codereview.chromium.org/3092015/diff/15001/16006
File chrome/browser/geolocation/location_arbitrator.cc (right):

http://codereview.chromium.org/3092015/diff/15001/16006#newcode111
chrome/browser/geolocation/location_arbitrator.cc:111: return
::NewGpsLocationProvider();
just to clarify: in the win7 patch I linked, we're renaming
NewGpsLocationProvider to NewSystemLocationProvider, rather than adding it as an
additional provider like you are here.
Might be easier if you copy over the associated changes from that patch, to
reduce merge conflict regardless of who gets to land first

[jorgevillatoro, 2010-08-10 15:18:58]

Thanks for the quick feedback again.  I have a couple of comments as well:

On the subject of the Obj-C class defs in data_provider.h, I agree that they
should probably be moved to data_provider.mm.  Where would you suggest in the
.mm file?  It's essentially laid out top to bottom like this: C++ defs, CL Obj-C
decls, Wrapper Objc-C defs.  Perhaps just right before the wrapper definitions
would be good?

Also, I think core_location_data_provider.h will still need a small amount of
obj-c in it, since it includes scoped_nsobject.h and needs an @class forward
declaration for the wrapper.  This means the core_location_provider.mm file
would have to stay a .mm file, unless there's a way around this.

On permission authorization, I'll keep looking for a way to disable it, but I
think we should move forward assuming that it can't be disabled.  The gatekeeper
concept seems like a good solution to me, I'll check into querying the location
authorization.  It should be possible, although we *may* have to use some
private framework methods.

--Jorge

http://codereview.chromium.org/3092015/diff/15001/16003
File chrome/browser/geolocation/core_location_data_provider_mac.mm (right):

http://codereview.chromium.org/3092015/diff/15001/16003#newcode177
chrome/browser/geolocation/core_location_data_provider_mac.mm:177: default:
On 2010/08/10 12:39:48, joth wrote:
> could you list the other codes explicitly ( kCLErrorHeadingFailure,
>    kCLErrorRegionMonitoringDenied,
>    kCLErrorRegionMonitoringFailure,
>    kCLErrorRegionMonitoringSetupDelayed) with a comment saying these are
partial
> errors we're not interested in, and in the default case put NOTREACHED() <<
> "Unknown CoreLocation error: " << [error code];
> return;
> 

As far as I can tell, only kCLErrorLocationUnknown and kCLErrorDenied are
actually available on OS X.  The CoreLocation.framework headers under
/System/Library and /Developer/SDKs/MacOSX10.6 only have those two, while the
iphone frameworks seem to have all of the other ones you've listed.  I can pull
the definitions from the iPhone headers, but that doesn't seem like it would
make sense.

[joth, 2010-08-10 16:08:05]

On 10 August 2010 16:18, <jorge@tomatocannon.com> wrote:

> Thanks for the quick feedback again.  I have a couple of comments as well:
>
> On the subject of the Obj-C class defs in data_provider.h, I agree that
> they
> should probably be moved to data_provider.mm.  Where would you suggest in
> the
> .mm file?  It's essentially laid out top to bottom like this: C++ defs, CL
> Obj-C
> decls, Wrapper Objc-C defs.  Perhaps just right before the wrapper
> definitions
> would be good?
>
> I'm not very familiar with the obj-C 'rules'... Nicos, any suggestions?


> Also, I think core_location_data_provider.h will still need a small amount
> of
> obj-c in it, since it includes scoped_nsobject.h and needs an @class
> forward
> declaration for the wrapper.  This means the core_location_provider.mmfile
> would have to stay a .mm file, unless there's a way around this.
>
> Oh yes, good point. I'm not bothered by it as it is then, if there's a
reason for it.


> On permission authorization, I'll keep looking for a way to disable it, but
> I
> think we should move forward assuming that it can't be disabled.  The
> gatekeeper
> concept seems like a good solution to me, I'll check into querying the
> location
> authorization.  It should be possible, although we *may* have to use some
> private framework methods.
>
> --Jorge
>
>
>
> http://codereview.chromium.org/3092015/diff/15001/16003
> File chrome/browser/geolocation/core_location_data_provider_mac.mm
> (right):
>
> http://codereview.chromium.org/3092015/diff/15001/16003#newcode177
> chrome/browser/geolocation/core_location_data_provider_mac.mm:177:
> default:
> On 2010/08/10 12:39:48, joth wrote:
>
>> could you list the other codes explicitly ( kCLErrorHeadingFailure,
>>    kCLErrorRegionMonitoringDenied,
>>    kCLErrorRegionMonitoringFailure,
>>    kCLErrorRegionMonitoringSetupDelayed) with a comment saying these
>>
> are partial
>
>> errors we're not interested in, and in the default case put
>>
> NOTREACHED() <<
>
>> "Unknown CoreLocation error: " << [error code];
>> return;
>>
>
>
> As far as I can tell, only kCLErrorLocationUnknown and kCLErrorDenied
> are actually available on OS X.  The CoreLocation.framework headers
> under /System/Library and /Developer/SDKs/MacOSX10.6 only have those
> two, while the iphone frameworks seem to have all of the other ones
> you've listed.  I can pull the definitions from the iPhone headers, but
> that doesn't seem like it would make sense.
>
> Nuts.
Well, OK. Of the errors defined on OSX  we can handle both, so ignore the
other ones I mention but put my NOTREACHED in the default case so we'll find
out (on debug build) if they ever do add more. Make sense?


Cheers

[Nico, 2010-08-10 16:22:41]

On Tue, Aug 10, 2010 at 9:07 AM, Jonathan Dixon <joth@chromium.org> wrote:
>
>
> On 10 August 2010 16:18, <jorge@tomatocannon.com> wrote:
>>
>> Thanks for the quick feedback again.  I have a couple of comments as well:
>>
>> On the subject of the Obj-C class defs in data_provider.h, I agree that
>> they
>> should probably be moved to data_provider.mm.  Where would you suggest in
>> the
>> .mm file?  It's essentially laid out top to bottom like this: C++ defs, CL
>> Obj-C
>> decls, Wrapper Objc-C defs.  Perhaps just right before the wrapper
>> definitions
>> would be good?
>>
> I'm not very familiar with the obj-C 'rules'... Nicos, any suggestions?

I didn't follow the review closely.

If the header that requires objc needs to be included in a cc file, you could do

#if __OBJC__
@class MyClass;
#else
class MyClass;
#endif

for the forward definition.

You can't use scoped_nsobject in that case. Your choices are
* managing that memory manually (in that case, document that it's a
strong pointer in the header and say that you can't use a
scoped_nsobject because you don't have access to objc in all places
the header is included)
* forward declaring scoped_nsobject and turning the pointer into a
scoped_ptr<scoped_nsobject<NSWhateverClass>> (yuck!)
* have a scoped_ptr to a pimpl and have that have a scoped_nsobject

In this case, the first option might be best?

>
>>
>> Also, I think core_location_data_provider.h will still need a small amount
>> of
>> obj-c in it, since it includes scoped_nsobject.h and needs an @class
>> forward
>> declaration for the wrapper.  This means the core_location_provider.mm
>> file
>> would have to stay a .mm file, unless there's a way around this.
>>
> Oh yes, good point. I'm not bothered by it as it is then, if there's a
> reason for it.
>
>>
>> On permission authorization, I'll keep looking for a way to disable it,
>> but I
>> think we should move forward assuming that it can't be disabled.  The
>> gatekeeper
>> concept seems like a good solution to me, I'll check into querying the
>> location
>> authorization.  It should be possible, although we *may* have to use some
>> private framework methods.
>>
>> --Jorge
>>
>>
>> http://codereview.chromium.org/3092015/diff/15001/16003
>> File chrome/browser/geolocation/core_location_data_provider_mac.mm
>> (right):
>>
>> http://codereview.chromium.org/3092015/diff/15001/16003#newcode177
>> chrome/browser/geolocation/core_location_data_provider_mac.mm:177:
>> default:
>> On 2010/08/10 12:39:48, joth wrote:
>>>
>>> could you list the other codes explicitly ( kCLErrorHeadingFailure,
>>>    kCLErrorRegionMonitoringDenied,
>>>    kCLErrorRegionMonitoringFailure,
>>>    kCLErrorRegionMonitoringSetupDelayed) with a comment saying these
>>
>> are partial
>>>
>>> errors we're not interested in, and in the default case put
>>
>> NOTREACHED() <<
>>>
>>> "Unknown CoreLocation error: " << [error code];
>>> return;
>>
>>
>> As far as I can tell, only kCLErrorLocationUnknown and kCLErrorDenied
>> are actually available on OS X.  The CoreLocation.framework headers
>> under /System/Library and /Developer/SDKs/MacOSX10.6 only have those
>> two, while the iphone frameworks seem to have all of the other ones
>> you've listed.  I can pull the definitions from the iPhone headers, but
>> that doesn't seem like it would make sense.
>
> Nuts.
> Well, OK. Of the errors defined on OSX  we can handle both, so ignore the
> other ones I mention but put my NOTREACHED in the default case so we'll find
> out (on debug build) if they ever do add more. Make sense?
>
> Cheers
>

[joth, 2010-08-10 16:39:14]

On 10 August 2010 17:22, Nico Weber <thakis@chromium.org> wrote:

> On Tue, Aug 10, 2010 at 9:07 AM, Jonathan Dixon <joth@chromium.org> wrote:
> >
> >
> > On 10 August 2010 16:18, <jorge@tomatocannon.com> wrote:
> >>
> >> Thanks for the quick feedback again.  I have a couple of comments as
> well:
> >>
> >> On the subject of the Obj-C class defs in data_provider.h, I agree that
> >> they
> >> should probably be moved to data_provider.mm.  Where would you suggest
> in
> >> the
> >> .mm file?  It's essentially laid out top to bottom like this: C++ defs,
> CL
> >> Obj-C
> >> decls, Wrapper Objc-C defs.  Perhaps just right before the wrapper
> >> definitions
> >> would be good?
> >>
> > I'm not very familiar with the obj-C 'rules'... Nicos, any suggestions?
>
> I didn't follow the review closely.
>
> If the header that requires objc needs to be included in a cc file, you
> could do
>
> #if __OBJC__
> @class MyClass;
> #else
> class MyClass;
> #endif
>
> for the forward definition.
>
> You can't use scoped_nsobject in that case. Your choices are
> * managing that memory manually (in that case, document that it's a
> strong pointer in the header and say that you can't use a
> scoped_nsobject because you don't have access to objc in all places
> the header is included)
> * forward declaring scoped_nsobject and turning the pointer into a
> scoped_ptr<scoped_nsobject<NSWhateverClass>> (yuck!)
> * have a scoped_ptr to a pimpl and have that have a scoped_nsobject
>
> In this case, the first option might be best?
>
>
Sorry, I wasn't clear: we can leave the files as .mm rather than convert to
.cc (they're mac files anyway). The question was more for the bits we can
move out of the .h anyway, what order should they appear in their .mm file?


But for the fun of it, another option for your list:
* make the c++ class into an abstract base class (interface) with a static
factory method, and have a derived class defined in the .mm that adds the
scoped_nsobject etc. (like pimpl but with inheritance instead of the extra
pointer indirection)


Cheers

[jorgevillatoro, 2010-08-11 02:28:46]

Now updated with the latest round of comments.

I tried moving the Obj-C class declaration into data_provider.mm, but I quickly
realized that the declaration had to come before it is used (which is in the C++
class).  This means it'd have to be above the C++ class definition inside of
data_provider.mm if we wanted it there, which seems cluttered.  Thoughts on
that?

Also, there appears to be a problem with the error handler. I haven't been able
to actually get it to report any errors.  I'll get back to you once I figure out
what's going on, but my guess is that there's just some small issue with the way
it's implemented in the wrapper.

--jorge

http://codereview.chromium.org/3092015/diff/15001/16002
File chrome/browser/geolocation/core_location_data_provider_mac.h (right):

http://codereview.chromium.org/3092015/diff/15001/16002#newcode84
chrome/browser/geolocation/core_location_data_provider_mac.h:84: // Can be
called from anywhere since it only initializes bundle_ once
On 2010/08/10 12:39:48, joth wrote:
> // Can be called from any thread since it does not require an NSRunLoop.
However
> it is not threadsafe to receive concurrent calls until after it's first
> successful call (to avoid |bundle_| being double initialized)

Done.

http://codereview.chromium.org/3092015/diff/15001/16003
File chrome/browser/geolocation/core_location_data_provider_mac.mm (right):

http://codereview.chromium.org/3092015/diff/15001/16003#newcode177
chrome/browser/geolocation/core_location_data_provider_mac.mm:177: default:
On 2010/08/10 15:18:58, jorgevillatoro wrote:
> On 2010/08/10 12:39:48, joth wrote:
> > could you list the other codes explicitly ( kCLErrorHeadingFailure,
> >    kCLErrorRegionMonitoringDenied,
> >    kCLErrorRegionMonitoringFailure,
> >    kCLErrorRegionMonitoringSetupDelayed) with a comment saying these are
> partial
> > errors we're not interested in, and in the default case put NOTREACHED() <<
> > "Unknown CoreLocation error: " << [error code];
> > return;
> > 
> 
> As far as I can tell, only kCLErrorLocationUnknown and kCLErrorDenied are
> actually available on OS X.  The CoreLocation.framework headers under
> /System/Library and /Developer/SDKs/MacOSX10.6 only have those two, while the
> iphone frameworks seem to have all of the other ones you've listed.  I can
pull
> the definitions from the iPhone headers, but that doesn't seem like it would
> make sense.
> 

Added the NOTREACHED() as you requested

http://codereview.chromium.org/3092015/diff/28002/36006
File chrome/browser/geolocation/gps_location_provider_linux.cc (right):

http://codereview.chromium.org/3092015/diff/28002/36006#newcode124
chrome/browser/geolocation/gps_location_provider_linux.cc:124: #if
defined(OS_LINUX)
From the Win7 patch, is this #if defined necessary since this is a _linux file?

[joth, 2010-08-11 15:52:05]

All looks good, pending the permissions prompt issue and the error handling
problem you mentioned.

Thanks!

On 2010/08/11 02:28:46, jorgevillatoro wrote:
> Now updated with the latest round of comments.
> 
> I tried moving the Obj-C class declaration into data_provider.mm, but I
quickly
> realized that the declaration had to come before it is used (which is in the
C++
> class).  This means it'd have to be above the C++ class definition inside of
> data_provider.mm if we wanted it there, which seems cluttered.  Thoughts on
> that?
> 
Putting the class definitions uptop of the .mm before implementation seems
pretty natural, and seems consistent with the example here:
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Objec...

I'd be tempted to group all the obj C class/type definitions up there (included
all the stuff marked out as from the SDK), then put the implementation below. As
per that example, you might even put the C++ methods at the very bottom of the
file (as the depend on everything that comes before them)

How's this sound?

> Also, there appears to be a problem with the error handler. I haven't been
able
> to actually get it to report any errors.  I'll get back to you once I figure
out
> what's going on, but my guess is that there's just some small issue with the
way
> it's implemented in the wrapper.
> 
> --jorge
> 
> http://codereview.chromium.org/3092015/diff/15001/16002
> File chrome/browser/geolocation/core_location_data_provider_mac.h (right):
> 
> http://codereview.chromium.org/3092015/diff/15001/16002#newcode84
> chrome/browser/geolocation/core_location_data_provider_mac.h:84: // Can be
> called from anywhere since it only initializes bundle_ once
> On 2010/08/10 12:39:48, joth wrote:
> > // Can be called from any thread since it does not require an NSRunLoop.
> However
> > it is not threadsafe to receive concurrent calls until after it's first
> > successful call (to avoid |bundle_| being double initialized)
> 
> Done.
> 
> http://codereview.chromium.org/3092015/diff/15001/16003
> File chrome/browser/geolocation/core_location_data_provider_mac.mm (right):
> 
> http://codereview.chromium.org/3092015/diff/15001/16003#newcode177
> chrome/browser/geolocation/core_location_data_provider_mac.mm:177: default:
> On 2010/08/10 15:18:58, jorgevillatoro wrote:
> > On 2010/08/10 12:39:48, joth wrote:
> > > could you list the other codes explicitly ( kCLErrorHeadingFailure,
> > >    kCLErrorRegionMonitoringDenied,
> > >    kCLErrorRegionMonitoringFailure,
> > >    kCLErrorRegionMonitoringSetupDelayed) with a comment saying these are
> > partial
> > > errors we're not interested in, and in the default case put NOTREACHED()
<<
> > > "Unknown CoreLocation error: " << [error code];
> > > return;
> > > 
> > 
> > As far as I can tell, only kCLErrorLocationUnknown and kCLErrorDenied are
> > actually available on OS X.  The CoreLocation.framework headers under
> > /System/Library and /Developer/SDKs/MacOSX10.6 only have those two, while
the
> > iphone frameworks seem to have all of the other ones you've listed.  I can
> pull
> > the definitions from the iPhone headers, but that doesn't seem like it would
> > make sense.
> > 
> 
> Added the NOTREACHED() as you requested
> 
> http://codereview.chromium.org/3092015/diff/28002/36006
> File chrome/browser/geolocation/gps_location_provider_linux.cc (right):
> 
> http://codereview.chromium.org/3092015/diff/28002/36006#newcode124
> chrome/browser/geolocation/gps_location_provider_linux.cc:124: #if
> defined(OS_LINUX)
> From the Win7 patch, is this #if defined necessary since this is a _linux
file?

yes, this #if  is not needed.
(Sorry, I directed you to the win7 patch forgetting it doesn't yet contain
exactly what I had in my head!)

[jorgevillatoro, 2010-08-13 15:37:50]

Here are the latest changes, this should be in pretty good shape now with the
exception of the UI.

- Rearranged the data_provider files to move Obj-C code out of the .h file
- A setDelegate call was missing in the wrapper, it is in now
- Removed the unnecessary #if defined from gps_provider_linux

I'm out for the weekend, so I'll report back on what I've figured out about
querying for location permissions when I get back.

--Jorge

[jorgevillatoro, 2010-08-17 19:41:19]

Joth,

I've been looking into your question about whether or not we can check the
current state of CLLocationManager's update permissions.  Unfortunately I
haven't found anything useful.

The best possibility was a private method listed by class-dump,
CLLocationManager#locationServicesApproved.  So far I haven't been able to get
this method to return anything other than NO, regardless of what options are
selected in the dialog box.  From some googling I found some references to it in
an old version of the API, and it sounds like it may have been deprecated during
a beta period.

That said, there is almost certainly some way to disable this dialog box, or at
least modify it.  Safari's looks different from the generic one displayed by
applications using CoreLocation.

Here's a shot of the one for Safari:
http://img831.imageshack.us/img831/7527/corelocationsafari.png

And the one for everyone else:
http://img812.imageshack.us/img812/3649/corelocationchrome.png

The main differences seem to be:
- Safari's is application modal while the other is not
- Safari only remembers a user's response for 24 hours
- Safari's permissions are per website rather than for the application as a
whole

Sadly, none of these changes are possible from the public api.  I've taken
class-dump and otx (http://otx.osxninja.com/) to CoreLocation and Safari, but so
far I haven't been able to pull anything useful from them.  Perhaps someone
watching this is or knows someone with experience disassembling and reverse
engineering Mac/Obj-C/Cocoa binaries.  I'll keep looking myself, but at the
moment I'm stumped.

Given this, I think the available options are:

- Hold off on CoreLocation until someone can reverse engineer Safari's method,
or find out how to query CLLocationManager's permission settings
- Use the previously suggested gatekeeper provider concept, where the
CoreLocation provider is started, and the others are only started once the first
successful update has been given by CoreLocation.  This has the downside that
the lag between CLLocationManager#startUpdatingLocation appears to sometimes be
annoyingly long, although I'm not sure it is any longer than Chrome's current
methods.

--Jorge


On 2010/08/13 15:37:50, jorgevillatoro wrote:
> Here are the latest changes, this should be in pretty good shape now with the
> exception of the UI.
> 
> - Rearranged the data_provider files to move Obj-C code out of the .h file
> - A setDelegate call was missing in the wrapper, it is in now
> - Removed the unnecessary #if defined from gps_provider_linux
> 
> I'm out for the weekend, so I'll report back on what I've figured out about
> querying for location permissions when I get back.
> 
> --Jorge

[joth, 2010-08-18 14:12:02]

To add another option (or more like, a strategy to apply to either your
listed options):
we could check-in the current code, but hide it under a command line flag.
This way, we draw a close on the current review cycle, and get some testing
& feedback from any keen users on the canary / dev channels whilst in
parallel working out how to handle the permission UI.

For reference, the windows 7 code is checked in under
--enable-win7-location. we could either add --enable-core-location or
perhaps bundle them together as --enable-system-location if we're fairly
confident both will mature from behind the flag at the same time (I'm not
sure about that right now)

Cheers
Joth


On 17 August 2010 20:41, <jorge@tomatocannon.com> wrote:

> Joth,
>
> I've been looking into your question about whether or not we can check the
> current state of CLLocationManager's update permissions.  Unfortunately I
> haven't found anything useful.
>
> The best possibility was a private method listed by class-dump,
> CLLocationManager#locationServicesApproved.  So far I haven't been able to
> get
> this method to return anything other than NO, regardless of what options
> are
> selected in the dialog box.  From some googling I found some references to
> it in
> an old version of the API, and it sounds like it may have been deprecated
> during
> a beta period.
>
> That said, there is almost certainly some way to disable this dialog box,
> or at
> least modify it.  Safari's looks different from the generic one displayed
> by
> applications using CoreLocation.
>
> Here's a shot of the one for Safari:
> http://img831.imageshack.us/img831/7527/corelocationsafari.png
>
> And the one for everyone else:
> http://img812.imageshack.us/img812/3649/corelocationchrome.png
>
> The main differences seem to be:
> - Safari's is application modal while the other is not
> - Safari only remembers a user's response for 24 hours
> - Safari's permissions are per website rather than for the application as a
> whole
>
> Sadly, none of these changes are possible from the public api.  I've taken
> class-dump and otx (http://otx.osxninja.com/) to CoreLocation and Safari,
> but so
> far I haven't been able to pull anything useful from them.  Perhaps someone
> watching this is or knows someone with experience disassembling and reverse
> engineering Mac/Obj-C/Cocoa binaries.  I'll keep looking myself, but at the
> moment I'm stumped.
>
> Given this, I think the available options are:
>
> - Hold off on CoreLocation until someone can reverse engineer Safari's
> method,
> or find out how to query CLLocationManager's permission settings
> - Use the previously suggested gatekeeper provider concept, where the
> CoreLocation provider is started, and the others are only started once the
> first
> successful update has been given by CoreLocation.  This has the downside
> that
> the lag between CLLocationManager#startUpdatingLocation appears to
> sometimes be
> annoyingly long, although I'm not sure it is any longer than Chrome's
> current
> methods.
>
> --Jorge
>
>
>
> On 2010/08/13 15:37:50, jorgevillatoro wrote:
>
>> Here are the latest changes, this should be in pretty good shape now with
>> the
>> exception of the UI.
>>
>
>  - Rearranged the data_provider files to move Obj-C code out of the .h file
>> - A setDelegate call was missing in the wrapper, it is in now
>> - Removed the unnecessary #if defined from gps_provider_linux
>>
>
>  I'm out for the weekend, so I'll report back on what I've figured out
>> about
>> querying for location permissions when I get back.
>>
>
>  --Jorge
>>
>
>
>
> http://codereview.chromium.org/3092015/show
>

[jorgevillatoro, 2010-08-29 22:55:40]

Sorry for the long period of silence here, I had my attention pulled away for a
bit.

I've added enable-core-location as a command line switch to turn this feature on
as you've suggested, I think that's probably the best way to go about this for
now.  Let me know what I need to do beyond that to close this review and get the
code committed.

--Jorge

On 2010/08/18 14:12:02, joth wrote:
> To add another option (or more like, a strategy to apply to either your
> listed options):
> we could check-in the current code, but hide it under a command line flag.
> This way, we draw a close on the current review cycle, and get some testing
> & feedback from any keen users on the canary / dev channels whilst in
> parallel working out how to handle the permission UI.
> 
> For reference, the windows 7 code is checked in under
> --enable-win7-location. we could either add --enable-core-location or
> perhaps bundle them together as --enable-system-location if we're fairly
> confident both will mature from behind the flag at the same time (I'm not
> sure about that right now)
> 
> Cheers
> Joth
> 
> 
> On 17 August 2010 20:41, <mailto:jorge@tomatocannon.com> wrote:
> 
> > Joth,
> >
> > I've been looking into your question about whether or not we can check the
> > current state of CLLocationManager's update permissions.  Unfortunately I
> > haven't found anything useful.
> >
> > The best possibility was a private method listed by class-dump,
> > CLLocationManager#locationServicesApproved.  So far I haven't been able to
> > get
> > this method to return anything other than NO, regardless of what options
> > are
> > selected in the dialog box.  From some googling I found some references to
> > it in
> > an old version of the API, and it sounds like it may have been deprecated
> > during
> > a beta period.
> >
> > That said, there is almost certainly some way to disable this dialog box,
> > or at
> > least modify it.  Safari's looks different from the generic one displayed
> > by
> > applications using CoreLocation.
> >
> > Here's a shot of the one for Safari:
> > http://img831.imageshack.us/img831/7527/corelocationsafari.png
> >
> > And the one for everyone else:
> > http://img812.imageshack.us/img812/3649/corelocationchrome.png
> >
> > The main differences seem to be:
> > - Safari's is application modal while the other is not
> > - Safari only remembers a user's response for 24 hours
> > - Safari's permissions are per website rather than for the application as a
> > whole
> >
> > Sadly, none of these changes are possible from the public api.  I've taken
> > class-dump and otx (http://otx.osxninja.com/) to CoreLocation and Safari,
> > but so
> > far I haven't been able to pull anything useful from them.  Perhaps someone
> > watching this is or knows someone with experience disassembling and reverse
> > engineering Mac/Obj-C/Cocoa binaries.  I'll keep looking myself, but at the
> > moment I'm stumped.
> >
> > Given this, I think the available options are:
> >
> > - Hold off on CoreLocation until someone can reverse engineer Safari's
> > method,
> > or find out how to query CLLocationManager's permission settings
> > - Use the previously suggested gatekeeper provider concept, where the
> > CoreLocation provider is started, and the others are only started once the
> > first
> > successful update has been given by CoreLocation.  This has the downside
> > that
> > the lag between CLLocationManager#startUpdatingLocation appears to
> > sometimes be
> > annoyingly long, although I'm not sure it is any longer than Chrome's
> > current
> > methods.
> >
> > --Jorge
> >
> >
> >
> > On 2010/08/13 15:37:50, jorgevillatoro wrote:
> >
> >> Here are the latest changes, this should be in pretty good shape now with
> >> the
> >> exception of the UI.
> >>
> >
> >  - Rearranged the data_provider files to move Obj-C code out of the .h file
> >> - A setDelegate call was missing in the wrapper, it is in now
> >> - Removed the unnecessary #if defined from gps_provider_linux
> >>
> >
> >  I'm out for the weekend, so I'll report back on what I've figured out
> >> about
> >> querying for location permissions when I get back.
> >>
> >
> >  --Jorge
> >>
> >
> >
> >
> > http://codereview.chromium.org/3092015/show
> >
>

[allanwoj, 2010-08-31 12:45:17]

Looks good. Apart from a few ordering issues, looks ready to go. You'll probably
need a rebase as well to sort out any conflicts with code that has changed. When
that's all done I can land it on your behalf if you'd like :-)

http://codereview.chromium.org/3092015/diff/48001/49013
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/3092015/diff/48001/49013#newcode1441
chrome/chrome_browser.gypi:1441:
'browser/geolocation/core_location_data_provider_mac.h',
Data provider files should be first.

http://codereview.chromium.org/3092015/diff/48001/49014
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/3092015/diff/48001/49014#newcode469
chrome/common/chrome_switches.cc:469: const char kEnableCoreLocation[]          
 = "enable-core-location";
Should be in alphabetical order.

http://codereview.chromium.org/3092015/diff/48001/49015
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/3092015/diff/48001/49015#newcode151
chrome/common/chrome_switches.h:151: extern const char kEnableCoreLocation[];
Should be in alphabetical order further up the list.

[jorgevillatoro, 2010-09-09 19:01:02]

Allan,

This should tidy up those ordering issues, as well as merge with the current
trunk.  Let me know if you see anything else that should be changed.

Thanks,
--JOrge

http://codereview.chromium.org/3092015/diff/48001/49013
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/3092015/diff/48001/49013#newcode1441
chrome/chrome_browser.gypi:1441:
'browser/geolocation/core_location_data_provider_mac.h',
On 2010/08/31 12:45:18, allanwoj wrote:
> Data provider files should be first.

Done.

http://codereview.chromium.org/3092015/diff/48001/49014
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/3092015/diff/48001/49014#newcode469
chrome/common/chrome_switches.cc:469: const char kEnableCoreLocation[]          
 = "enable-core-location";
On 2010/08/31 12:45:18, allanwoj wrote:
> Should be in alphabetical order.

Done.

http://codereview.chromium.org/3092015/diff/48001/49015
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/3092015/diff/48001/49015#newcode151
chrome/common/chrome_switches.h:151: extern const char kEnableCoreLocation[];
On 2010/08/31 12:45:18, allanwoj wrote:
> Should be in alphabetical order further up the list.

Done.

[allanwoj, 2010-09-13 11:08:35]

Thanks Jorge, I have now landed your patch at
http://src.chromium.org/viewvc/chrome?view=rev&revision=59213
Thanks for the great work!
Allan

On 2010/09/09 19:01:02, jorgevillatoro wrote:
> Allan,
> 
> This should tidy up those ordering issues, as well as merge with the current
> trunk.  Let me know if you see anything else that should be changed.
> 
> Thanks,
> --JOrge
> 
> http://codereview.chromium.org/3092015/diff/48001/49013
> File chrome/chrome_browser.gypi (right):
> 
> http://codereview.chromium.org/3092015/diff/48001/49013#newcode1441
> chrome/chrome_browser.gypi:1441:
> 'browser/geolocation/core_location_data_provider_mac.h',
> On 2010/08/31 12:45:18, allanwoj wrote:
> > Data provider files should be first.
> 
> Done.
> 
> http://codereview.chromium.org/3092015/diff/48001/49014
> File chrome/common/chrome_switches.cc (right):
> 
> http://codereview.chromium.org/3092015/diff/48001/49014#newcode469
> chrome/common/chrome_switches.cc:469: const char kEnableCoreLocation[]        
 
>  = "enable-core-location";
> On 2010/08/31 12:45:18, allanwoj wrote:
> > Should be in alphabetical order.
> 
> Done.
> 
> http://codereview.chromium.org/3092015/diff/48001/49015
> File chrome/common/chrome_switches.h (right):
> 
> http://codereview.chromium.org/3092015/diff/48001/49015#newcode151
> chrome/common/chrome_switches.h:151: extern const char kEnableCoreLocation[];
> On 2010/08/31 12:45:18, allanwoj wrote:
> > Should be in alphabetical order further up the list.
> 
> Done.