Closed
Bug 1170304
Opened 9 years ago
Closed 9 years ago
Change browser.shell.isSetAsDefaultBrowser to browser.shell.mostRecentDateSetAsDefault
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
People
(Reporter: jaws, Assigned: jaws)
References
(Blocks 2 open bugs)
Details
Attachments
(2 files, 3 obsolete files)
2.30 KB,
patch
|
Details | Diff | Splinter Review | |
2.22 KB,
patch
|
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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+
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8613695 -
Flags: review?(dolske)
Comment 2•9 years ago
|
||
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-
Comment 3•9 years ago
|
||
Do we still need to do this with the new info regarding win10 release not changing the default browser to edge after all?
Assignee | ||
Comment 4•9 years ago
|
||
(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.
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8613695 -
Attachment is obsolete: true
Attachment #8614468 -
Flags: review?(dolske)
Adding a tracking flag for firefox39 and FF40 on this one.
status-firefox39:
--- → affected
status-firefox40:
--- → affected
tracking-firefox39:
--- → +
tracking-firefox40:
--- → +
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)
Comment 8•9 years ago
|
||
(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)
Assignee | ||
Comment 9•9 years ago
|
||
The change is simple enough that I don't think it warrants extra QE attention.
Flags: needinfo?(jaws)
Assignee | ||
Updated•9 years ago
|
Summary: Change browser.shell.isSetAsDefaultBrowser to browser.shell.wasEverSetAsDefaultBrowser → Change browser.shell.isSetAsDefaultBrowser to browser.shell.mostRecentDateSetAsDefault
Comment 10•9 years ago
|
||
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-
Assignee | ||
Comment 11•9 years ago
|
||
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)
Assignee | ||
Comment 12•9 years ago
|
||
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 13•9 years ago
|
||
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+
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8616165 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/3d10908774c5
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/3d10908774c5
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3d10908774c5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Comment 18•9 years ago
|
||
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)
Assignee | ||
Comment 19•9 years ago
|
||
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 20•9 years ago
|
||
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+
Comment 21•9 years ago
|
||
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+
Comment 22•9 years ago
|
||
(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)
Comment 25•9 years ago
|
||
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.
Description
•