Policy: Don't check if Firefox is the default browser on startup

VERIFIED FIXED in Firefox 60

Status

()

defect
P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

Trunk
Firefox 60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 verified)

Details

Attachments

(1 attachment)

Don't check if Firefox is the default browser on startup
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
So there were three ways to implement this:

- Calling ShellService.shouldCheckDefaultBrowser = false on startup
- Making the getter call Services.policies.isAllowed
- Locking the pref

I chose the locking the pref method because about:preferences already correctly reflects this by graying out the checkbox. The other two methods would give the users the impression of being able to change the setting but it would never take effect, so we would need to do extra work to fix that.

Comment 3

a year ago
mozreview-review
Comment on attachment 8943990 [details]
Bug 1429141 - Policy: Don't check if Firefox is the default browser on startup.

https://reviewboard.mozilla.org/r/214316/#review220186

::: browser/components/enterprisepolicies/tests/browser/browser_policy_default_browser_check.js:6
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +Cu.import("resource:///modules/ShellService.jsm");

I've seen other people complain that this adds global variable warnings. YMMV; if necessary use:

```js
const {ShellService} = ...;
```
to fix.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_default_browser_check.js:16
(Diff revision 1)
> +  // testing profile. Let's start with true for a sanity check.
> +
> +  ShellService.shouldCheckDefaultBrowser = true;
> +  is(ShellService.shouldCheckDefaultBrowser, true, "Sanity check");
> +
> +  await setupPolicyEngineWithJson("config_dont_check_default_browser.json");

Given the long list of desired policies, I'm tempted to suggest that we want a helper that just takes a JS object instead of a separate file, otherwise the number of files required here will just keep ballooning.

Don't need to fix that here, but might be worth doing in a followup "soon"-ish so that we nip it in the bud before we have 50 of these files with 2 lines of relevant JSON and 3 lines of curly braces ;-)
Attachment #8943990 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 4

a year ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c67fb9e69640
Policy: Don't check if Firefox is the default browser on startup. r=Gijs
(In reply to :Gijs (lower availability until Jan 27) from comment #3)
> Comment on attachment 8943990 [details]
> Bug 1429141 - Policy: Don't check if Firefox is the default browser on
> startup.
> 
> https://reviewboard.mozilla.org/r/214316/#review220186
> 
> :::
> browser/components/enterprisepolicies/tests/browser/
> browser_policy_default_browser_check.js:6
> (Diff revision 1)
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +"use strict";
> > +Cu.import("resource:///modules/ShellService.jsm");
> 
> I've seen other people complain that this adds global variable warnings.
> YMMV; if necessary use:
> 
> ```js
> const {ShellService} = ...;
> ```
> to fix.

Done 

> 
> Don't need to fix that here, but might be worth doing in a followup
> "soon"-ish so that we nip it in the bud before we have 50 of these files
> with 2 lines of relevant JSON and 3 lines of curly braces ;-)

Good idea. I'll do that in a follow-up soon. I'll just make the test code write a temp file with a given JSON from the test and set the browser.policies.alternatePath to that file

Comment 6

a year ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87defcccf49a
Fix test when it runs twice in a row, e.g. on test-verify. r=me

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c67fb9e69640
https://hg.mozilla.org/mozilla-central/rev/87defcccf49a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
We have tested this on latest Nightly-60.0a1 [build that have this patch] and it does not check if Nighty is the default browser when this policy is in use. 
 
Test cases and runs are here: https://testrail.stage.mozaws.net/index.php?/plans/view/7734

The following operating systems are used for testing:
Windows 7 x86, Windows 10 x64, OSX 10.12 and Ubuntu 16.04 64bits.
Status: RESOLVED → VERIFIED
We retested this on beta builds[FX60] with ADM and JSON policy formats and it is verified as fixed.

Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760
You need to log in before you can comment on or make changes to this bug.