Allow UITour to check and set Firefox as the default browser

RESOLVED FIXED in Firefox 40

Status

()

Firefox
Tours
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Dolske, Assigned: jaws)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

Trunk
Firefox 42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40+ fixed, firefox41 fixed, firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Windows 10 makes it harder to set the default browser -- instead of just clicking a button within Firefox, the user must perform a sequence of steps in a native Windows dialog that Firefox has no control over.

One idea to help with this is to have a SUMO page that helps guide the user though this UI with a series of screenshots. It would probably be very helpful for that page to have the ability to trigger opening this new Windows dialog.

While the immediate focus of this is dealing with this new UI on Windows 10, maybe we just want to allow this on other platforms too. (To either bring up the usual Firefox prompt, or to just change it immediately.)

Also, I think by implication this means exposing if Firefox is the default browser or not through UITour, so it knows if this should be offered or not.

See also bug 1170743.

Updated

3 years ago
Priority: -- → P1
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Created attachment 8627930 [details] [diff] [review]
Patch

Hey Geoff and Bobby, I'm trying to use MockObjects.js to mock nsIShellService, but when I run the test the original ShellService implementation is still being used.

I've compared the test in my patch to dom/tests/mochitest/bugs/test_bug61098.html.

Can you take a look at the patch and see if anything is missing or incorrect?
Attachment #8627930 - Flags: feedback?(geoff)
Attachment #8627930 - Flags: feedback?(bobbyholley)
I'm not familiar with MockObjects.js offhand, sorry. I'd need to read code + debug.
I can't see anything obviously wrong, but then again I haven't seen this code in years.
Attachment #8627930 - Flags: feedback?(geoff)
Summary: Allow UITour to trigger setting Firefox as the default browser → Allow UITour to check and set Firefox as the default browser
Comment on attachment 8627930 [details] [diff] [review]
Patch

Dave, do you have any ideas?
Attachment #8627930 - Flags: feedback?(dtownsend)
Created attachment 8628073 [details] [diff] [review]
Patch

Added the ability to check if the browser is already set as the default. I used the "HTTP" check for this, not the combined "HTTP + HTML" check that may also be set (requires using the more advanced Control Panel UI on Windows to achieve the HTML association).
Attachment #8627930 - Attachment is obsolete: true
Attachment #8627930 - Flags: feedback?(dtownsend)
Attachment #8627930 - Flags: feedback?(bobbyholley)
Attachment #8628073 - Flags: review?(dao)
Comment on attachment 8628073 [details] [diff] [review]
Patch

Looks reasonable to me, but I know nothing about UITour stuff, so even if something obvious was wrong here I likely wouldn't notice.

>+Components.utils.import("resource:///modules/UITour.jsm");
>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");

These should already be available. If they weren't, you'd pollute the global scope for subsequent tests with these imports.
Attachment #8628073 - Flags: review?(dao)
Created attachment 8629000 [details] [diff] [review]
Patch v1.1
Attachment #8628073 - Attachment is obsolete: true
Attachment #8629000 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8629000 [details] [diff] [review]
Patch v1.1

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

::: browser/components/uitour/UITour.jsm
@@ +721,5 @@
> +
> +      case "setDefaultBrowser": {
> +        let shell = Components.classes["@mozilla.org/browser/shell-service;1"]
> +                              .getService(Components.interfaces.nsIShellService);
> +        shell.setDefaultBrowser(true, true);

Please use (true, false). (true, true) will bring the UAC dialog.

Comment 9

3 years ago
Comment on attachment 8629000 [details] [diff] [review]
Patch v1.1

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

LGTM with the existing comment from Masatoshi-san addressed, plus the following notes:

::: browser/components/uitour/test/browser_UITour_defaultBrowser.js
@@ +44,5 @@
> +
> +let tests = [
> +
> +  /* This test is disabled since the MockObjectRegisterer
> +     is not actually replacing the original ShellService.

Followup bug for this? I expect this has to do with the shell service first being created before the mock is registered, but I'm not sure. UITour is probably hard to xpcshell-test (where you could avoid that), though.

@@ +60,5 @@
> +    let shell = Components.classes["@mozilla.org/browser/shell-service;1"]
> +                          .getService(Components.interfaces.nsIShellService);
> +    let isDefault = shell.isDefaultBrowser(false);
> +    gContentAPI.isDefaultBrowser(function(data) {
> +      is(data.value, isDefault, "gContentAPI.isDefaultBrowser should match shellService.isDefaultBrowser");

Meh. We have no APIs to undo setting as default browser, but this test suffers from confirmation bias (ie I could implement the API by always returning true/false/whatever it is right now on our test infra - likely 'false' because the path will be different every time). Not much we can do about that though.
Attachment #8629000 - Flags: review?(gijskruitbosch+bugs) → review+
Depends on: 1180714
(In reply to :Gijs Kruitbosch from comment #9)
> @@ +60,5 @@
> > +    let shell = Components.classes["@mozilla.org/browser/shell-service;1"]
> > +                          .getService(Components.interfaces.nsIShellService);
> > +    let isDefault = shell.isDefaultBrowser(false);
> > +    gContentAPI.isDefaultBrowser(function(data) {
> > +      is(data.value, isDefault, "gContentAPI.isDefaultBrowser should match shellService.isDefaultBrowser");
> 
> Meh. We have no APIs to undo setting as default browser, but this test
> suffers from confirmation bias (ie I could implement the API by always
> returning true/false/whatever it is right now on our test infra - likely
> 'false' because the path will be different every time). Not much we can do
> about that though.

Ideally the MockShellService could be used to test both values of true and false here, so that we see calling the gCotentAPI.isDefaultBrowser would return the value that the MockShellService is returning.
Comment on attachment 8629000 [details] [diff] [review]
Patch v1.1

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

::: browser/components/uitour/UITour-lib.js
@@ +290,5 @@
>  	};
>  
> +	Mozilla.UITour.setDefaultBrowser = function() {
> +		_sendEvent('setDefaultBrowser');
> +	}

Nit: missing semicolon

@@ +296,5 @@
> +	Mozilla.UITour.isDefaultBrowser = function(callback) {
> +		_sendEvent('isDefaultBrowser', {
> +			callbackID: _waitForCallback(callback),
> +		});
> +	}

Ditto

::: browser/components/uitour/UITour.jsm
@@ +727,5 @@
> +      }
> +
> +      case "isDefaultBrowser": {
> +        let shell = Components.classes["@mozilla.org/browser/shell-service;1"]
> +                              .getService(Components.interfaces.nsIShellService);

We usually put the shell service usage in a try…catch as I don't think it's implemented for some OSs (though I don't know if that's only non-Linux unix-based OSs). We should probably do the same here. We could probably use getShellService from utilityOverlay.js and do a null check. I'll file a follow-up.
Comment on attachment 8629000 [details] [diff] [review]
Patch v1.1

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

Thinking about this more. It should have been implemented using the existing getConfiguration and setConfiguration APIs to reduce the exposed API to maintain.
Attachment #8629000 - Flags: review-
Blocks: 1180904
https://hg.mozilla.org/mozilla-central/rev/36fd29a59e04
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Flags: qe-verify+
Comment on attachment 8629000 [details] [diff] [review]
Patch v1.1

Approval Request Comment
[Feature/regressing bug #]: UITour for windows 10 default browser
[User impact if declined]: UITour won't be able to set the default browser
[Describe test coverage new/current, TreeHerder]: some automated and manual testing
[Risks and why]: none expected, adds capabilities to UITour
[String/UUID change made/needed]: none

See follow-up bug 1180801 which is also requested for uplifts.
Attachment #8629000 - Flags: approval-mozilla-beta?
Attachment #8629000 - Flags: approval-mozilla-aurora?
NI jaws - is this still on track to hit Firefox 40 (release)?
Flags: needinfo?(jaws)
(In reply to Cory Price [:ckprice] from comment #16)
> NI jaws - is this still on track to hit Firefox 40 (release)?

Yes, see beta-approval and aurora-approval request above in comment #15.
Flags: needinfo?(jaws)
Depends on: 1181770
Comment on attachment 8629000 [details] [diff] [review]
Patch v1.1

This didn't bounce when hitting m-c but we're not going to get any testing there or from our Aurora or Beta pre-release audiences. The use case is pretty significant for Windows 10 so I'd like to have this land on Beta while we're in early beta so that we can take this in 40. 

I would like to get the webdev team to test and ensure that their requirements are met with beta3 if possible.

Beta+ Aurora+
Attachment #8629000 - Flags: approval-mozilla-beta?
Attachment #8629000 - Flags: approval-mozilla-beta+
Attachment #8629000 - Flags: approval-mozilla-aurora?
Attachment #8629000 - Flags: approval-mozilla-aurora+
status-firefox40: --- → affected
tracking-firefox40: --- → +
https://hg.mozilla.org/releases/mozilla-aurora/rev/4ace51e5f532
status-firefox41: affected → fixed
Flags: in-testsuite+
Removing qe-verify+ since this fix is already covered by automated testing.
Flags: qe-verify+
(Reporter)

Updated

3 years ago
Depends on: 1190586
You need to log in before you can comment on or make changes to this bug.