Closed
Bug 1180801
Opened 9 years ago
Closed 9 years ago
Replace setDefaultBrowser and isDefaultBrowser with getConfiguration/setConfiguration options
Categories
(Firefox :: Tours, defect)
Firefox
Tours
Tracking
()
RESOLVED
FIXED
Firefox 42
People
(Reporter: MattN, Assigned: jaws)
References
Details
Attachments
(1 file, 1 obsolete file)
6.44 KB,
patch
|
jaws
:
review+
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
We shouldn't add custom APIs to UITour when we have general APIs for getting/setting values in the browser. Reusing existing patterns reduces the complexity of UITour.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
This fixes bug 1180872 as well.
Attachment #8630684 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 2•9 years ago
|
||
I meant bug 1180782 in the above comment.
Reporter | ||
Comment 4•9 years ago
|
||
Comment on attachment 8630684 [details] [diff] [review] Patch Review of attachment 8630684 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. ::: browser/components/uitour/UITour.jsm @@ +1719,5 @@ > case "availableTargets": > this.getAvailableTargets(aMessageManager, aWindow, aCallbackID); > break; > + case "defaultBrowser": > + let isDefault = false; I still think the webpage will want to distinguish between Firefox not being the default and Firefox not knowing whether it's the default. If we default to false, the page may nag the user to make it their default when it may already be the case. I would use null/undefined for that. @@ +1726,5 @@ > + if (shell) { > + isDefault = shell.isDefaultBrowser(false); > + } > + } catch (e) {} > + this.sendPageCallback(messageManager, data.callbackID, { value: isDefault }); s/value/defaultBrowser/ Sorry to be a PITA but could you add this as an `isDefaultBrowser` property for `getConfiguration("appinfo");` instead? Since these calls are async and cross-process I think that will be nicer for mozilla.org since I know they already fetch appInfo in some cases. The setConfiguration part is fine as-is.
Attachment #8630684 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8630684 -
Attachment is obsolete: true
Attachment #8630758 -
Flags: review+
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/076cfc368b00
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8630758 [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 bug 1175293 which is also requested for uplifts.
Attachment #8630758 -
Flags: approval-mozilla-beta?
Attachment #8630758 -
Flags: approval-mozilla-aurora?
Comment 9•9 years ago
|
||
Comment on attachment 8630758 [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 #8630758 -
Flags: approval-mozilla-beta?
Attachment #8630758 -
Flags: approval-mozilla-beta+
Attachment #8630758 -
Flags: approval-mozilla-aurora?
Attachment #8630758 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
Comment 10•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/39fdfdfbf830
Flags: in-testsuite+
Comment 12•9 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #4) > I still think the webpage will want to distinguish between Firefox not being > the default and Firefox not knowing whether it's the default. If we default > to false, the page may nag the user to make it their default when it may > already be the case. I would use null/undefined for that. Just saw this comment and took a peek at the code. So we can assume that in some cases `defaultBrowser` may actually be `null` and not a Boolean value? Just making sure we account for these things in the web content side, as this will be useful for us to know - thanks. Out of interest, under what circumstances would the try/catch likely fail here?
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Alex Gibson [:agibson] from comment #12) > (In reply to Matthew N. [:MattN] from comment #4) > > I still think the webpage will want to distinguish between Firefox not being > > the default and Firefox not knowing whether it's the default. If we default > > to false, the page may nag the user to make it their default when it may > > already be the case. I would use null/undefined for that. > > Just saw this comment and took a peek at the code. So we can assume that in > some cases `defaultBrowser` may actually be `null` and not a Boolean value? > Just making sure we account for these things in the web content side, as > this will be useful for us to know - thanks. > > Out of interest, under what circumstances would the try/catch likely fail > here? Getting the value of the default browser shouldn't fail, but if it were to fail I would presume that it may be due to the user account not having permission to read that data. I've seen setting the default browser "fail" on my local machine when disk access was very slow and the Windows 10 Settings app was not responding.
Flags: needinfo?(MattN+bmo)
Reporter | ||
Comment 14•9 years ago
|
||
The main case I know about is some Linux environments that we don't know how to read/set the default browser in.
You need to log in
before you can comment on or make changes to this bug.
Description
•