Closed
Bug 1175293
Opened 9 years ago
Closed 9 years ago
Allow UITour to check and set Firefox as the default browser
Categories
(Firefox :: Tours, defect, P1)
Firefox
Tours
Tracking
()
RESOLVED
FIXED
Firefox 42
People
(Reporter: Dolske, Assigned: jaws)
References
(Depends on 2 open bugs, Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
5.49 KB,
patch
|
Gijs
:
review+
MattN
:
review-
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•9 years ago
|
Priority: -- → P1
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
I'm not familiar with MockObjects.js offhand, sorry. I'd need to read code + debug.
Comment 3•9 years ago
|
||
I can't see anything obviously wrong, but then again I haven't seen this code in years.
Updated•9 years ago
|
Attachment #8627930 -
Flags: feedback?(geoff)
Assignee | ||
Updated•9 years ago
|
Summary: Allow UITour to trigger setting Firefox as the default browser → Allow UITour to check and set Firefox as the default browser
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8627930 [details] [diff] [review] Patch Dave, do you have any ideas?
Attachment #8627930 -
Flags: feedback?(dtownsend)
Assignee | ||
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8628073 -
Attachment is obsolete: true
Attachment #8629000 -
Flags: review?(gijskruitbosch+bugs)
Comment 8•9 years ago
|
||
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•9 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+
Assignee | ||
Comment 10•9 years ago
|
||
(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 12•9 years ago
|
||
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 13•9 years ago
|
||
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: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Updated•9 years ago
|
Flags: qe-verify+
Assignee | ||
Comment 15•9 years ago
|
||
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?
Comment 16•9 years ago
|
||
NI jaws - is this still on track to hit Firefox 40 (release)?
Flags: needinfo?(jaws)
Assignee | ||
Comment 17•9 years ago
|
||
(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 18•9 years ago
|
||
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+
Updated•9 years ago
|
status-firefox40:
--- → affected
tracking-firefox40:
--- → +
Comment 19•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/4ace51e5f532
Flags: in-testsuite+
Comment 21•9 years ago
|
||
Removing qe-verify+ since this fix is already covered by automated testing.
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•