874587 - Implement a provider based on CoreLocation for OSX

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Comment 1

[Guilherme Gonçalves [:ggp]]

Attachment: Bug 874587 - Add CoreLocation geolocation provider

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 752363 [details] [diff] [review]
Bug 874587 - Add CoreLocation geolocation provider

Review of attachment 752363 [details] [diff] [review]:
-----------------------------------------------------------------

Pretty close.

does 'make package' fail?  Did you have to update any other files?

::: dom/system/Makefile.in
@@ +34,4 @@
>  # On Systems that have build in geolocation providers,
>  # we really do not need these.
>  ifneq (Android,$(OS_TARGET))
> +  ifeq (,$(filter cocoa,$(MOZ_WIDGET_TOOLKIT)))

I don't think indent Makefile.in like this.

also, this is clearer and should work:

ifneq (cocoa,$(MOZ_WIDGET_TOOLKIT))

::: dom/system/mac/CoreLocationLocationProvider.h
@@ +9,5 @@
> +/*
> + * This struct keeps the CoreLocation objects
> + * whose declaration would require an Objective-C++ aware
> + * compiler.
> + *

i do not understand this comment.

@@ +13,5 @@
> + *
> + * We define it in CoreLocationProvider.mm so this header can
> + * be safely included from nsGeolocation.cpp.
> + */
> +struct CoreLocationObjects;

do you need really to forward declare this?

@@ +28,5 @@
> +
> + private:
> +  virtual ~CoreLocationLocationProvider();
> +
> +  struct CoreLocationObjects* mCLObjects;

Shouldn't this be a nsAutoPtr<> or something?

::: dom/system/mac/CoreLocationLocationProvider.mm
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim:set ts=2 sw=2 sts=2 et cindent: */

vim, no.

@@ +36,5 @@
> +}
> +
> +- (void)locationManager:(CLLocationManager*)aManager didFailWithError:(NSError *)aError
> +{
> +  /* TODO notify Geolocation somehow? */

figure this out.  Either create a new bug and reference... or fix it.

@@ +40,5 @@
> +  /* TODO notify Geolocation somehow? */
> +  NSString* message =
> +    [@"Failed to acquire position: " stringByAppendingString: [aError localizedDescription]];
> +
> +  NS_WARNING([message UTF8String]);

Can you log this to the console service instead of NS_WARNING?

@@ +43,5 @@
> +
> +  NS_WARNING([message UTF8String]);
> +}
> +
> +- (void)locationManager:(CLLocationManager*)aManager didUpdateToLocation:(CLLocation *)aNewLocation fromLocation:(CLLocation *)aOldLocation

wrap this line.

@@ +49,5 @@
> +  mProvider->Update(GeoPositionFromCLLocation(aNewLocation));
> +}
> +@end
> +
> +struct CoreLocationObjects {

We're c++.  Make this a class.  Use an Init() to report failure if mLocationManager can't be created.  Same with mLocationDelegate

@@ +57,5 @@
> +    mLocationManager.delegate = mLocationDelegate;
> +  }
> +
> +  LocationDelegate* mLocationDelegate;
> +  CLLocationManager* mLocationManager;

auto ptrs.

@@ +72,5 @@
> +                           aSomewhere.course,
> +                           aSomewhere.speed,
> +                           PR_Now());
> +
> +}

Why do you need this.  It is only called from one location.  It is probably better to move this code into locationManager?

@@ +83,5 @@
> +}
> +
> +CoreLocationLocationProvider::~CoreLocationLocationProvider()
> +{
> +  delete mCLObjects;

auto ptr?

@@ +97,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +CoreLocationLocationProvider::Watch(nsIGeolocationUpdate* aCallback, bool aRequestPrivate)

Does this create a warning about aRequestPrivate being unused?

@@ +121,5 @@
> +CoreLocationLocationProvider::SetHighAccuracy(bool aEnable)
> +{
> +  NS_ENSURE_STATE(mCLObjects);
> +
> +  mCLObjects->mLocationManager.desiredAccuracy = kCLLocationAccuracyBest;

you will want to do something more than this.  SetHighAccuracy can be false.  You will probably want to do what the NetworkLocationProvider does.

Comment 3

[Guilherme Gonçalves [:ggp]]

(In reply to Doug Turner (:dougt) from comment #2)
> Comment on attachment 752363 [details] [diff] [review]
>
> i do not understand this comment.
> 
> do you need really to forward declare this?
> 
> Shouldn't this be a nsAutoPtr<> or something?

We don't want CoreLocationLocationProvider.h to contain any Objective-C, because it needs to be included from nsGeolocation.cpp, which is C++-only. All Objective-C includes and objects should be inside CoreLocationLocationProvider.mm.

I'm keeping the Objective-C objects (CLLocationManager and LocationDelegate) in that struct, which is actually defined in the .mm file. By forward-declaring we get to declare a pointer to it in the header, but sadly we can't use an nsAutoPtr (well, technically we can, but we get a compiler warning for deleting an incomplete type, which can be undefined behavior).

I can rewrite the comment to make that clearer, but I don't see a better solution to the actual problem.

> 
> @@ +57,5 @@
> > +    mLocationManager.delegate = mLocationDelegate;
> > +  }
> > +
> > +  LocationDelegate* mLocationDelegate;
> > +  CLLocationManager* mLocationManager;
> 
> auto ptrs.
> 

These are Objective-C objects, so I don't think we're supposed to be deleting them with auto ptrs.

I'll address the other comments in the next version of the patch. Thanks for the quick review!

Comment 4

[Guilherme Gonçalves [:ggp]]

Attachment: Bug 874587 - Add CoreLocation geolocation provider

Refreshing the patch. Addressed all comments in the previous review, but I'm still trying to get the tests to pass on try -- using CoreLocation causes tests to time out (presumably because of the OSX permission dialog).

Comment 5

[Guilherme Gonçalves [:ggp]]

Try run for #753020: https://tbpl.mozilla.org/?tree=Try&rev=312b61f46024

Seems to have broken browser_geolocation_privatebrowsing_perwindowpb.js, though I can't seem to reproduce locally.

Comment 6

[Reuben Morais [:reuben]]

You can probably override the methods in CLLocationManager and make it return whatever coords you want it to return for tests. I haven't checked if that skips the permission dialog entirely but it should.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 753020 [details] [diff] [review]
Bug 874587 - Add CoreLocation geolocation provider

Review of attachment 753020 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +4574,4 @@
>      MOZ_WIDGET_TOOLKIT=cocoa
>      AC_DEFINE(MOZ_WIDGET_COCOA)
>      LDFLAGS="$LDFLAGS -framework Cocoa -lobjc"
> +    TK_LIBS='-framework CoreLocation -framework QuartzCore -framework Carbon -framework CoreAudio -framework AudioToolbox -framework AudioUnit -framework AddressBook -framework OpenGL'

Lets ask a build peer to review these changes.  They look fine, but we probably want to get their permission.

::: dom/system/mac/CoreLocationLocationProvider.mm
@@ +26,5 @@
> +}
> +
> +- (id)init:(CoreLocationLocationProvider*)aProvider;
> +- (void)locationManager:(CLLocationManager*)aManager didFailWithError:(NSError *)aError;
> +- (void)locationManager:(CLLocationManager*)aManager didUpdateToLocation:(CLLocation *)aNewLocation fromLocation:(CLLocation *)aOldLocation;

holy long lines.

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 753020 [details] [diff] [review]
Bug 874587 - Add CoreLocation geolocation provider

Review of attachment 753020 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the high level change.

smichaud, can you review (or delegate) this for the cocoa parts.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

I have a short week this week.  I'll try to get to this next week.

In addition to doing the review, I'll need to test this code by hand.  Please provide some way to do this.

Comment 10

[Doug Turner (:dougt)]

steven,
load https://dl.dropboxusercontent.com/u/8727858/geo.html
click on allow
see your location

Comment 11

[Guilherme Gonçalves [:ggp]]

Steven, you can manually test the code by simply building the patch on OSX and visiting a page that uses geolocation (for instance, http://html5demos.com/geo). 

After granting permission to share location with the page in the browser's doorhanger, you should also see another permission dialog issued by the system. Granting permission there as well should make the page work as expected.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 753020 [details] [diff] [review]
Bug 874587 - Add CoreLocation geolocation provider

I've started looking at this, though I haven't yet done any testing.

I have a minor style nit, and an observation about some weirdness I found in Apple's docs (which will probably need to be turned into a code comment).

But I've also discovered some other issues that aren't so minor.  I'll deal with these first:

Apple's system provided Location Services can be disabled (in the Security & Privacy system pref panel), though it's on by default.  CLLocationManager has a class method (locationServicesEnabled) to check this setting.  Your patch should probably use it.

And at least on OS X 10.7 and up, any given app must be authorized to access Location Services.  Apple's docs say that the OS asks the user to authorize an app's use of Location Services the first time the app tries to use them (if the app isn't already in the list of authorized apps).  But on 10.7 and up CLLocationManager has a multi-value authorizationStatus class method, which might be useful to not bother the user about apps that have explicitly been denied access.

Here's the minor style nit:

+- (id)init:(CoreLocationLocationProvider*)aProvider;

By convention, the name for this should be:

- (id)initWithProvider:(CoreLocationLocationProvider*)aProvider;

Here's the weirdness in Apple's docs:

Apple's doc on CLLocationManagerDelegate locationManager:didUpdateToLocation:fromLocation: says this method is deprecated, and that locationManager:didUpdateLocations: should be used instead.  But the latter *isn't* documented.

So clearly we should keep using locationManager:didUpdateToLocation:fromLocation:.  But we should probably we should add an XXX comment in the code about the need to change it at some point in the future.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 753020 [details] [diff] [review]
Bug 874587 - Add CoreLocation geolocation provider

I've just tested this patch on OS X 10.7.5 with http://html5demos.com/geo and https://dl.dropboxusercontent.com/u/8727858/geo.html.

I didn't have any problems with the default settings.

And if you don't have any apps in the list of apps that are authorized or not authorized to use Location Services, you're actually prompted to globally enable Location Services if it's not enabled.  You're also prompted to allow or deny an app authorization the first time you try to use it.  But once an app is in the list, it appears to stay there forever (which I find extremely annoying).  And if you've denied it authorization, the test cases appear to wait forever for location information that never arrives.

So it may turn out that we don't need to call +[CLLocationManager locationServicesEnabled].  But we probably *will* need to call +[CLLocationManager authorizationStatus] (on OS X 10.7 and up) to check whether the "current app" has been authorized, and warn the user if it isn't.

Some of the above relies on memory, since I'm not (yet) able to clear the list of explicitly authorized/non-authorized apps.  I'll try to hack out a way to do that.  If I find one I'll comment here.

Comment 14

[Guilherme Gonçalves [:ggp]]

(In reply to Steven Michaud from comment #13)
> Comment on attachment 753020 [details] [diff] [review]
> Bug 874587 - Add CoreLocation geolocation provider
> 
> And if you don't have any apps in the list of apps that are authorized or
> not authorized to use Location Services, you're actually prompted to
> globally enable Location Services if it's not enabled.  You're also prompted
> to allow or deny an app authorization the first time you try to use it.

That's the behavior I get on OX X 10.8 as well.

> And if you've denied it authorization, the test cases
> appear to wait forever for location information that never arrives.

This should be resolved when bug 874996 lands. We will be able to issue a 'permission denied' event in that case.

> But we probably *will* need to call
> +[CLLocationManager authorizationStatus] (on OS X 10.7 and up) to check
> whether the "current app" has been authorized, and warn the user if it isn't.
> 

While I do agree we should warn the user if that happens, I think for the purposes of this patch we're going for what Safari does: just issue a 'permission denied' event if not allowed and let the user figure it out.

And thanks for catching the style nit and documentation issue. Regarding the latter, I had actually experimented with the undocumented method while writing the patch, but it didn't seem to work at all. So I'll add a comment in the next version of the patch as you suggested.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

I've found a way to clear locationd's list of authorized/non-authorized apps.  Very hacky, but it seems to work and to have no side effects:

1) cd /var/folders/zz
2) su
3) find . -name clients.plist
   This file should exist in a directory owned by _locationd.  Delete the file.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

>> And if you've denied it authorization, the test cases appear to
>> wait forever for location information that never arrives.
>
> This should be resolved when bug 874996 lands. We will be able to
> issue a 'permission denied' event in that case.

Sounds good to me.

So it seems this patch won't need to call either +[CLLocationManager
locationServicesEnabled] or +[CLLocationManager authorizationStatus].

But a decent fix for bug 874996 might need to call +[CLLocationManager
authorizationStatus] to distinguish between different kinds of
failure.

We can (I hope) do better than Safari :-)

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Oops.  What I said in comment #15 only works on OS X 10.7.X.  On 10.8.X the locationd clients.plist file is in a different location.  Note that deleting this file may be somewhat risky, since it has other information than just the authorized/non-authorized apps.  But turning location services off and on and restarting your computer restores the file (and that information).

1) sudo rm /var/db/locationd/clients.plist
2) Turn Location Services off and on in the Security & Privacy pref panel.
3) Restart your computer.

Comment 18

[Reuben Morais [:reuben]]

FWIW, locationManager:didUpdateLocations: is documented in the iOS docs: https://developer.apple.com/library/ios/#documentation/CoreLocation/Reference/CLLocationManagerDelegate_Protocol/CLLocationManagerDelegate/CLLocationManagerDelegate.html#//apple_ref/occ/intfm/CLLocationManagerDelegate/locationManager:didUpdateLocations:

Comment 19

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 753020 [details] [diff] [review]
Bug 874587 - Add CoreLocation geolocation provider

Since we've now resolved all but the following minor issues, I'll go ahead and r+ your patch, on the understanding that you'll fix them in a future revision.

(Quoting from comment #12)

Here's the minor style nit:

+- (id)init:(CoreLocationLocationProvider*)aProvider;

By convention, the name for this should be:

- (id)initWithProvider:(CoreLocationLocationProvider*)aProvider;

Here's the weirdness in Apple's docs:

Apple's doc on CLLocationManagerDelegate locationManager:didUpdateToLocation:fromLocation: says this method is deprecated, and that locationManager:didUpdateLocations: should be used instead.  But the latter *isn't* documented.

So clearly we should keep using locationManager:didUpdateToLocation:fromLocation:.  But we should probably we should add an XXX comment in the code about the need to change it at some point in the future.

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 753020 [details] [diff] [review]
Bug 874587 - Add CoreLocation geolocation provider

Review of attachment 753020 [details] [diff] [review]:
-----------------------------------------------------------------

Build foo is mostly good. Thanks for asking a build peer for review.

::: dom/system/mac/Makefile.in
@@ +19,5 @@
> +
> +include $(topsrcdir)/config/config.mk
> +include $(topsrcdir)/ipc/chromium/chromium-config.mk
> +
> +CMMSRCS += CoreLocationLocationProvider.mm

CMMSRCS has been moved to moz.build. This will likely still work for the time being, but it should be defined in the moz.build file.

Comment 21

[Guilherme Gonçalves [:ggp]]

Attachment: Bug 874587 - Add CoreLocation geolocation provider r=dougt,gps,smichaud

Updating the patch after addressing review comments.

Comment 22

[Guilherme Gonçalves [:ggp]]

Attachment: Bug 874587 - Add CoreLocation geolocation provider r=dougt,gps,smichaud

Updating with the PERMISSION DENIED events after bug 874996 landed. Try run for this:  https://tbpl.mozilla.org/?tree=Try&rev=1bc5de7dfcf5 .

Comment 23

[Guilherme Gonçalves [:ggp]]

Attachment: Bug 874587 - Add CoreLocation geolocation provider r=dougt,gps,smichaud

Updating due to conflicts with mozilla-central. Carrying over r+ from previous version.
https://tbpl.mozilla.org/?tree=Try&rev=c147bd591db9

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a1c75f4aed

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b2a1c75f4aed

Comment 26

[Gregory Szorc [:gps]]

dom/system/mac/CoreLocationLocationProvider.mm references NSThreadUtils.h but the actual file is nsThreadUtils.h

This case sensitivity issue is causing a build failure for Stefan on m-c on OS X :/

Comment 27

[Stefan Mirea [:smirea]]

I have made a special case sensitive drive on OS X because when building boot 2 gecko it is recommended that you do so and therefore I started getting build errors now due to that file not being found

Comment 28

[Guilherme Gonçalves [:ggp]]

Attachment: Bug 874587 - Remove unnecessary and misspelled header

Stefan, can you try this? The header doesn't really seem to be necessary
at all (it's an artifact from a previous version of the patch), so removing
the include should fix your build issues.

Comment 29

[Stefan Mirea [:smirea]]

i actually edited CoreLocationLocationProvider.mm to include the correctly named file and it worked so I'm guessing deleting it would also solve the problem.

Comment 30

[David Zbarsky (:dzbarsky)]

Comment on attachment 770494 [details] [diff] [review]
Bug 874587 - Remove unnecessary and misspelled header

Review of attachment 770494 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing this.

Comment 31

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cba9fca0d4db

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cba9fca0d4db

Comment 33

[SC]

Hi, sorry to bother, but I was trying to build the firefox source on OS X 10.8.4 for the first time, and it fails for me on CoreLocationProvider.mm:

/Users/shajith/code/ff/mozilla-central/dom/system/mac/CoreLocationLocationProvider.mm:15:10: fatal error: 'Corelocation/CLError.h' file not found
#include <Corelocation/CLError.h>
         ^
1 error generated.

In the directory  /Users/shajith/code/ff/mozilla-central/obj-ff-dbg/dom/system/mac
The following command failed to execute properly:
/opt/local/bin/ccache clang++ -o CoreLocationLocationProvider.o -c -fvisibility=hidden -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -DOS_POSIX=1 -DOS_MACOSX=1 -I/Users/shajith/code/ff/mozilla-central/ipc/chromium/src -I/Users/shajith/code/ff/mozilla-central/ipc/glue -I../../../ipc/ipdl/_ipdlheaders -I/Users/shajith/code/ff/mozilla-central/dom/src/geolocation -I/Users/shajith/code/ff/mozilla-central/dom/system/mac -I. -I../../../dist/include -I/Users/shajith/code/ff/mozilla-central/obj-ff-dbg/dist/include/nspr -I/Users/shajith/code/ff/mozilla-central/obj-ff-dbg/dist/include/nss -fPIC -Qunused-arguments -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -DNO_X11 -pipe -DDEBUG -D_DEBUG -DTRACING -g -fno-omit-frame-pointer -Qunused-arguments -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MP -MF .deps/CoreLocationLocationProvider.o.pp -fobjc-exceptions /Users/shajith/code/ff/mozilla-central/dom/system/mac/CoreLocationLocationProvider.mm
make[7]: *** [CoreLocationLocationProvider.o] Error 1
make[6]: *** [libs] Error 2
make[5]: *** [system_libs] Error 2
make[5]: *** Waiting for unfinished jobs....

I followed the provided instructions[1] to build, but I am doing this for the first time, and it looks like I'm missing some basic configuration. Any pointers would be appreciated, thank you.

1: https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions/Mac_OS_X_Prerequisites

Comment 34

[Steven Michaud [:smichaud] (Retired)]

This is another case-sensitivity/misspelling error.  In these four lines, "Corelocation" should be "CoreLocation":

https://hg.mozilla.org/mozilla-central/annotate/04d8c309fe72/dom/system/mac/CoreLocationLocationProvider.mm#l15

Comment 35

[Guilherme Gonçalves [:ggp]]

Attachment: Bug 874587 - Fix more case-sensitivity issues.

Steven is probably right. SC, can you please try this patch and report
any other issues you encounter?

Comment 36

[SC]

Yup, that patch did help. The build worked once I applied those changes, no further issues showed up for me. Thanks!

Comment 37

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6960174c45d0

Comment 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6960174c45d0