Closed Bug 928217 Opened 7 years ago Closed 6 years ago

Enable support for corelocation geolocation

Categories

(Core :: DOM: Geolocation, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: dougt, Assigned: dougt)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
jdm, please review the overall change.
Attachment #818819 - Flags: review?(josh)
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.
Attachment #818819 - Flags: review?(smichaud)
Attachment #818819 - Flags: review?(josh) → review+
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.
Attachment #818819 - Flags: review?(smichaud) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/a98a45e180f6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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).
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.
Thanks Steven.  I do not have 10.9.
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.
Flags: needinfo?(doug.turner)
Status: RESOLVED → REOPENED
Flags: needinfo?(dougt)
Resolution: FIXED → ---
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.
Attachment #818819 - Attachment is obsolete: true
Attachment #8392014 - Flags: review?(smichaud)
Summary: Enable corelocation geolocation support on mac → Enable support for corelocation geolocation
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.
Attachment #8392014 - Flags: review?(smichaud) → review+
> 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
https://hg.mozilla.org/mozilla-central/rev/47c2471c73fb
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla27 → mozilla31
Doug, is there any help from QA required here or is this pretty well covered by existing tests?
Flags: needinfo?(dougt)
no
Flags: needinfo?(dougt)
QA Whiteboard: [qa-]
See Also: → 1121265
Depends on: 1124522
You need to log in before you can comment on or make changes to this bug.