Closed Bug 1136956 Opened 9 years ago Closed 9 years ago

CoreLocation provider: disable for release+beta build

Categories

(Core :: DOM: Geolocation, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 + fixed
firefox39 + fixed

People

(Reporter: garvan, Assigned: garvan)

References

Details

Attachments

(1 file, 1 obsolete file)

Per the terms of our GLS contract, we can't enable this in Release.

The fix is to ensure the pref to use_corelocation is set to non-release only in
source/browser/app/profile/firefox.js
Reminder: this needs to be uplifted to Aurora
Attachment #8569467 - Flags: review?(cpeterson)
Blocks: 1136976
Comment on attachment 8569467 [details] [diff] [review]
ifndef RELEASE_BUILD wrapper in firefox.js

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

LGTM!

::: browser/app/profile/firefox.js
@@ +1752,5 @@
>  
>  // The request URL of the GeoLocation backend.
>  pref("geo.wifi.uri", "https://location.services.mozilla.com/v1/geolocate?key=%MOZILLA_API_KEY%");
>  
> +#ifndef RELEASE_BUILD

Is your intention to limit CoreLocation to the Nightly and Aurora channels? If you wanted to include the first half of the Beta cycle too, you could use #ifdef EARLY_BETA_OR_EARLIER instead, but RELEASE_BUILD sounds like the right choice.

@@ +1757,3 @@
>  // On Mac, the default geo provider is corelocation.
>  #ifdef XP_MACOSX
>  pref("geo.provider.use_corelocation", true);

We should define the pref in all channels and just changed the value. We don't want to have hidden or missing prefs. Here's an example:

https://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#178
Attachment #8569467 - Flags: review?(cpeterson) → review+
> Is your intention to limit CoreLocation to the Nightly and Aurora channels?
> If you wanted to include the first half of the Beta cycle too, you could use
> #ifdef EARLY_BETA_OR_EARLIER instead, but RELEASE_BUILD sounds like the
> right choice.

I am thinking everything except release build, to get maximum coverage.
 
> We should define the pref in all channels and just changed the value. We
> don't want to have hidden or missing prefs. Here's an example:
> 
> https://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.
> js#178

Ok thanks will do!
(In reply to Garvan Keeley [:garvank] from comment #4)
> > Is your intention to limit CoreLocation to the Nightly and Aurora channels?
> > If you wanted to include the first half of the Beta cycle too, you could use
> > #ifdef EARLY_BETA_OR_EARLIER instead, but RELEASE_BUILD sounds like the
> > right choice.
> 
> I am thinking everything except release build, to get maximum coverage.

Beta is considered a "release" build (i.e. it has the Firefox branding), so

#ifdef RELEASE_BUILD
    Beta and Release channels
#else
    Nightly and Aurora channels
#endif

See comments here:
https://mxr.mozilla.org/mozilla-central/source/configure.in#3551

I don't know of a #define that includes only Nightly, Aurora, and Beta other than EARLY_BETA_OR_EARLIER. I think that is by design so the second half of the Beta cycle is testing the same configuration as the final release.
Oh ok. And after I typed my earlier response I decided that it is too risky to have differences in release and beta (not to mention confusing for QA). 
So, my thought now is to do:
#ifndef RELEASE_BUILD
    Nightly and Aurora channels
#endif
Summary: CoreLocation provider: disable for Release build → CoreLocation provider: disable for release+beta build
Based on review, updated to ensure use_corelocation is always present on mac.
Attachment #8569467 - Attachment is obsolete: true
Attachment #8569913 - Flags: review+
(In reply to Garvan Keeley [:garvank] from comment #6)
> Oh ok. And after I typed my earlier response I decided that it is too risky
> to have differences in release and beta (not to mention confusing for QA). 

That's why we don't have an ifdef to only disable on release but not on beta. :)
I agree that RELEASE_BUILD is right here (we also should only use EARLY_BETA_OR_EARLIER sparingly).
https://hg.mozilla.org/mozilla-central/rev/551548754f83
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8569913 [details] [diff] [review]
set pref in firefox.js for release+beta

Approval Request Comment
[Feature/regressing bug #]: bug 928217
[User impact if declined]: Firefox 38 on OS X will use Apple's CoreLocation geolocation API instead of Google's Location Service (GLS). We want to continue using GLS in the release channels, so this CoreLocation experiment should not advance from Aurora to Beta.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: OS X users might get unexpected geolocation results because we have always been using GLS before.
[String/UUID change made/needed]: None
Attachment #8569913 - Flags: approval-mozilla-aurora?
Depends on: 928217
Attachment #8569913 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: