Closed Bug 1170304 Opened 4 years ago Closed 4 years ago

Change browser.shell.isSetAsDefaultBrowser to browser.shell.mostRecentDateSetAsDefault

Categories

(Firefox :: General, defect)

defect
Not set
Points:
1

Tracking

()

RESOLVED FIXED
Firefox 41
Iteration:
41.2 - Jun 8
Tracking Status
firefox39 + fixed
firefox40 + fixed
firefox41 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 3 obsolete files)

The code introduced by bug 1169754 sets the isSetAsDefaultBrowser pref during each start up. If the browser is no longer set as default, the valuable information in this preference is lost.

We should change this preference to `browser.shell.wasEverSetAsDefaultBrowser`, where it can store if the browser was ever set to be default.

This is particularly valuable if someone is using Firefox on Windows 8.1 as their default browser, then updates their OS to Windows 10 and during OS upgrade the default browser gets changed to Edge.
Flags: qe-verify-
Flags: firefox-backlog+
Attached patch Patch (obsolete) — Splinter Review
Attachment #8613695 - Flags: review?(dolske)
Comment on attachment 8613695 [details] [diff] [review]
Patch

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

I don't this it's useful to have permanent persistence like this -- consider someone who set Firefox as their default 10 years ago but then switched to Opera(!!!1!).

Maybe store the timestamp of the last time we checked and we were still the default? That would enable questions like "were we recently the user's default browser".
Attachment #8613695 - Flags: review?(dolske) → review-
Do we still need to do this with the new info regarding win10 release not changing the default browser to edge after all?
(In reply to :Gijs Kruitbosch from comment #3)
> Do we still need to do this with the new info regarding win10 release not
> changing the default browser to edge after all?

I don't think we can assume that the default browser will not be switched to Edge in the default case for users. I can imagine a UI where the default upgrade options use Edge but users have the ability to do an "advanced upgrade" which lets them keep prior associations.

Further, we should still do this because it may be useful information as the default browser UI is still much more clumsy and we may want to give more help to users who had Firefox as their default browser recently but not anymore after an upgrade.
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8613695 - Attachment is obsolete: true
Attachment #8614468 - Flags: review?(dolske)
Adding a tracking flag for firefox39 and FF40 on this one.
While reviewing this bug, I noticed that qe-verify is set to "-". Jared, is there a reason to set this to not be verified by QE?

Florin, could you please comment whether this needs to be QE verified or not?
Flags: needinfo?(jaws)
Flags: needinfo?(florin.mezei)
(In reply to Ritu Kothari (:ritu) from comment #7)
> Florin, could you please comment whether this needs to be QE verified or not?

I think it depends really on how much risk there is involved with this change. If we consider it as something risky then we should treat it with higher priority and verify it; if it's a low impact change then I think we can treat it as a low priority fix and not do any verification (we still have plenty of other fixes pending verification and a rather short period of time left to do it). I'll let Jared comment on it.
Flags: needinfo?(florin.mezei)
The change is simple enough that I don't think it warrants extra QE attention.
Flags: needinfo?(jaws)
Summary: Change browser.shell.isSetAsDefaultBrowser to browser.shell.wasEverSetAsDefaultBrowser → Change browser.shell.isSetAsDefaultBrowser to browser.shell.mostRecentDateSetAsDefault
Comment on attachment 8614468 [details] [diff] [review]
Patch v2

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

::: browser/components/nsBrowserGlue.js
@@ +1118,5 @@
> +        previousValue = new Date(previousValue);
> +        let newValue = Date.now();
> +        if (!previousValue || newValue > previousValue) {
> +          Services.prefs.setCharPref("browser.shell.wasEverSetAsDefaultBrowser", newValue.toUTCString());
> +        }

The pref should just store the numerical Date.now() value. That avoids all kinds of time-conversion problems.

Any particular thinking behind why your comparing with the existing value? For an accurate clock, it won't matter. For a clock being changed, it fixes some cases but can make others worse. Just overwrite the existing value?
Attachment #8614468 - Flags: review?(dolske) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
I had to use a string here instead of an int because when I was calling setIntPref with Date.now(), about:config showed it as a negative number. I'm not sure where the overflow is coming from, I can look deeper in to it though.
Attachment #8614468 - Attachment is obsolete: true
Attachment #8616165 - Flags: review?(dolske)
It's overflowing because nsPrefBranch::SetIntPref uses a signed 32-bit integer, and 1,433,532,563,540 is far more than 2,147,483,647 (max signed 32bit int).
Comment on attachment 8616165 [details] [diff] [review]
Patch v3

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

How about just dividing by 1000 to store as whole seconds? We don't really need millisecond-level precision, and 31-bit time is goot for another 23 years.
Attachment #8616165 - Flags: review?(dolske) → review+
Attachment #8616165 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/3d10908774c5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
jaws, dolske: can you request uplift for this if you think it is low-risk enough to go to 39 in late beta?
Flags: needinfo?(jaws)
Flags: needinfo?(dolske)
Approval Request Comment
[Feature/regressing bug #]: add local data caching of when browser was most recently set as default browser to help with retaining users post upgrade to windows 10 where setting the default browser is more difficult
[User impact if declined]: less information available for bug 1172854
[Describe test coverage new/current, TreeHerder]: manual testing and available on nightly for a few days
[Risks and why]: none expected
[String/UUID change made/needed]: none
Flags: needinfo?(jaws)
Flags: needinfo?(dolske)
Attachment #8622016 - Flags: approval-mozilla-beta?
Attachment #8622016 - Flags: approval-mozilla-aurora?
Comment on attachment 8622016 [details] [diff] [review]
Patch for Aurora40 and Beta39

Approved for uplift to aurora and beta.
Attachment #8622016 - Flags: approval-mozilla-beta?
Attachment #8622016 - Flags: approval-mozilla-beta+
Attachment #8622016 - Flags: approval-mozilla-aurora?
Attachment #8622016 - Flags: approval-mozilla-aurora+
We should verify this on windows 10 for beta - I know it checked out on Nightly but I'd like to make sure for 39 release that updates work ok.
Blocks: windows-10
Flags: qe-verify- → qe-verify+
(In reply to Liz Henry (:lizzard) from comment #21)
> We should verify this on windows 10 for beta - I know it checked out on
> Nightly but I'd like to make sure for 39 release that updates work ok.

What scenario(s) should we verify exactly here? Are we talking about verifying that Beta updates correctly on Windows 10?
Flags: needinfo?(lhenry)
Florin - sorry, I missed the discussion in comments 8 and 9. Actually I think we're ok here and dolske was right!
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(lhenry)
You need to log in before you can comment on or make changes to this bug.