Closed Bug 1180801 Opened 5 years ago Closed 5 years ago

Replace setDefaultBrowser and isDefaultBrowser with getConfiguration/setConfiguration options

Categories

(Firefox :: Tours, defect)

defect
Not set

Tracking

()

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

People

(Reporter: MattN, Assigned: jaws)

References

Details

Attachments

(1 file, 1 obsolete file)

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: nobody → jaws
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
This fixes bug 1180872 as well.
Attachment #8630684 - Flags: review?(MattN+bmo)
I meant bug 1180782 in the above comment.
Duplicate of this bug: 1180782
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+
Attached patch Patch v1.1Splinter Review
Attachment #8630684 - Attachment is obsolete: true
Attachment #8630758 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/076cfc368b00
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
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 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+
(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)
(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)
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.