928217 - Enable support for corelocation geolocation

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Doug Turner (:dougt)]

With Bug 874587, we have a CoreLocation geolocation provider.  We disabled it because CoreLocation requires WiFi to be enabled.  This follow patch checks to see if WiFi is enabled.  If it isn't we fallback on GLS.  This has some drawbacks -- such as if the user decides to toggle WiFi in the middle of doing requests or something -- but I suspect that isn't a case that matters very much in practice.

Comment 1

[Doug Turner (:dougt)]

Attachment: bug_xxxxx_enable_cocoa_location_provider

jdm, please review the overall change.

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 818819 [details]
bug_xxxxx_enable_cocoa_location_provider

smichaud, can you review the cocoa bits and let me know if I screwed something up.  It has been a while since I hacked on mac stuff.  Specifically tell me if I am messed up the ownership of things.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 818819 [details]
bug_xxxxx_enable_cocoa_location_provider

This looks fine to me, though I haven't tested it.  You've got the memory management exactly right.

I have a couple of what are basically style nits.  But both of my suggested changes, while technically "better", are inconsistent with the style of the module you're working in.  So it's probably best to leave your patch the way it is.

First, the following class exists in widget/cocoa/nsCocoaUtils.h and a number of other places:

class nsAutoreleasePool {
public:
  nsAutoreleasePool()
  {
    mLocalPool = [[NSAutoreleasePool alloc] init];
  }
  ~nsAutoreleasePool()
  {
    [mLocalPool release];
  }
private:
  NSAutoreleasePool *mLocalPool;
};

At some point we should go through the entire tree and use this class to replace all low-level manipulation of autorelease pools (such as your patch contains).  But that's work for another day.

Second, the CoreWLAN framework is always available on OS X 10.6 and up, so we no longer need to check for its presence (since we only run on OS X 10.6 and up).  There are a number of other places (in addition to your patch) where we make this check.  At some point we should go through and remove them ... but probably not in this patch.

Comment 4

[Doug Turner (:dougt)]

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

smichaud, I probably added most of those CoreWLAN imports.  Do you have a bug to fix those up?  I probably can do this before you get to it.

Comment 5

[Carsten Book [:Tomcat]]

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

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Don't feel obliged to make us use the nsAutoreleasePool class throughout the tree :-)

As for not importing the CoreWLAN framework, that's a much more reasonable-sized project.  Please go ahead if you have time.  And no, there's not a bug for it (as far as I know).

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Looks like this caused bug 929686.  But I'm not sure how.

I can take care of fixing this, Doug, if you want me to.  I'll probably do it at bug 929686.

Comment 8

[Doug Turner (:dougt)]

Thanks Steven.  I do not have 10.9.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

I've temporarily disabled CoreLocations services on OS X 10.9 and up to deal with bug 929686.  Apple's removed a lot of stuff from its CoreWLAN header files on 10.9 without ever bothering to deprecate them, including some of the stuff your patch uses.  So we'll need to find alternatives (if they're available).  And for that we'll need to have some kind of test of geolocation -- one that at least exercises your patch.

Please concoct one and attach it to this bug.

Comment 10

[Doug Turner (:dougt)]

Steven,

I was using CoreWLAN to determine of CoreLocation was available.  Clearly when I say it this way, it is obvious we were doing something wrong.

I am going to remove IsCoreLocationAvailable() and flip the geo.provider.use_corelocation pref to default false for now.

As for tests, Juan in QA has promised me for a number of years that it's being tested manually.

Comment 11

[Doug Turner (:dougt)]

Attachment: 0001-Bug-928217-Remove-IsCoreLocationAvailable-method-bec.patch

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8392014 [details] [diff] [review]
0001-Bug-928217-Remove-IsCoreLocationAvailable-method-bec.patch

> I was using CoreWLAN to determine of CoreLocation was available.
> Clearly when I say it this way, it is obvious we were doing
> something wrong.

It's not so clear to me :-) Please explain in more detail.  Many APIs
include some kind of test of whether or not they're supported and
working.

> As for tests, Juan in QA has promised me for a number of years that
> it's being tested manually.

The point of the manual testcase is so that whoever rewrites this
patch for OS X 10.9 and up has some way to test his/her work.

> I am going to remove IsCoreLocationAvailable() and flip the
> geo.provider.use_corelocation pref to default false for now.

Do you really want to do this on all versions of OS X?  And what will
happen on OS X 10.9 if someone manually turns
geo.provider.use_corelocation on?  Once again, we need a manual
testcase, referenced here, to test this.

But I don't have any problem with *temporarily* turning CoreLocation
off by default on the Mac.

Comment 13

[Doug Turner (:dougt)]

> Do you really want to do this on all versions of OS X?  

Yes

> And what will happen on OS X 10.9 if someone manually turns geo.provider.use_corelocation on? 

It will not work.  Which is fine

Comment 14

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47c2471c73fb

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/47c2471c73fb

Comment 16

[u279076]

Doug, is there any help from QA required here or is this pretty well covered by existing tests?

Comment 17

[Doug Turner (:dougt)]

no