Closed Bug 1175293 Opened 5 years ago Closed 5 years ago

Allow UITour to check and set Firefox as the default browser

Categories

(Firefox :: Tours, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox40 + fixed
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: Dolske, Assigned: jaws)

References

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

Details

Attachments

(1 file, 2 obsolete files)

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.
Priority: -- → P1
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
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)
Attached patch Patch (obsolete) — Splinter Review
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)
Attached patch Patch v1.1Splinter Review
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 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+
(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-
https://hg.mozilla.org/mozilla-central/rev/36fd29a59e04
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
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)
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+
Removing qe-verify+ since this fix is already covered by automated testing.
Flags: qe-verify+
Depends on: 1190586
You need to log in before you can comment on or make changes to this bug.