Closed
Bug 1102409
Opened 10 years ago
Closed 9 years ago
Dev Edition update nag should be weekly, not daily
Categories
(DevTools :: General, defect, P1)
Tracking
(firefox40 fixed, firefox41 fixed)
RESOLVED
FIXED
Firefox 41
People
(Reporter: canuckistani, Assigned: past)
References
Details
(Whiteboard: [polish-backlog])
Attachments
(1 file, 4 obsolete files)
6.01 KB,
patch
|
Gijs
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Similar to bug 1010999, I'd like to propose we nag people ( eg badge the hamburger icon ) at most once a week. The builds won't change that much. Right now I see daily updates, and so do various others.
Comment 1•9 years ago
|
||
So, what needs to be done for this to happen? It looks like the promptWaitTime is still 168 hours: http://dxr.mozilla.org/mozilla-central/source/browser/branding/aurora/pref/firefox-branding.js#16
Assignee | ||
Comment 2•9 years ago
|
||
We could either make the value of app.update.interval the same as app.update.promptWaitTime, or add code that doesn't show the badge if a week hasn't passed since the observer notification was fired. In the first case we will only be checking for updates weekly, whereas in the latter we will download any updates (but perhaps not apply) silently.
Comment 3•9 years ago
|
||
(In reply to Panos Astithas [:past] from comment #2) > We could either make the value of app.update.interval the same as > app.update.promptWaitTime, or add code that doesn't show the badge if a week > hasn't passed since the observer notification was fired. In the first case > we will only be checking for updates weekly, whereas in the latter we will > download any updates (but perhaps not apply) silently. Before we added the badge, was it similar to the first case (i.e. checking for updates only once a week)? If so, maybe we should just update it to act that way. How much work would it be to handle the latter case? Maybe in this case an update could be applied before prompting if the user happened to restart the browser after one had been downloaded.
Flags: needinfo?(past)
Assignee | ||
Comment 4•9 years ago
|
||
Given that we didn't touch app.update.interval, I believe we used to check often but display the dialog rarely. Doing the same thing with the badge/notification shouldn't be too hard, we just need to keep the state across browser sessions. The code that handles app.update.promptWaitTime should be useful as a starting point.
Flags: needinfo?(past)
Reporter | ||
Comment 5•9 years ago
|
||
That's an excellent expression of what I want: "check often but display the (badge) rarely". Thanks Panos.
I know there's not much time left for devedition-40, but should we attempt something like this? Saw another tweet[1] about the current state being annoying today. [1]: https://twitter.com/chrismuzilla/status/595370847474352129
Flags: needinfo?(jgriffiths)
Reporter | ||
Updated•9 years ago
|
Priority: -- → P1
Reporter | ||
Comment 8•9 years ago
|
||
Proposal - we show the update indicator in the hamburger at most once every 4 days.
Reporter | ||
Comment 9•9 years ago
|
||
Joe - I still would like to see this for June 2 if possible.
Flags: needinfo?(jwalker)
Comment 10•9 years ago
|
||
Agreed. My hunch is that Panos is best placed to do this, what do you think?
Flags: needinfo?(jwalker) → needinfo?(past)
Assignee | ||
Comment 11•9 years ago
|
||
I got it.
Assignee: nobody → past
Status: NEW → ASSIGNED
Flags: needinfo?(past)
Assignee | ||
Comment 12•9 years ago
|
||
This seems to work in my limited testing. I've introduced a new pref app.update.badgeWaitTime with a default value of 4 days that determines how soon the badge will appear after the update is ready. I've been tweaking it to 5 (seconds) from about:config to test it locally. I only postpone the badge on successful downloads, should I also do it on failure?
Assignee | ||
Comment 13•9 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48bce1207761
Reporter | ||
Comment 14•9 years ago
|
||
(In reply to Panos Astithas [:past] from comment #12) > Created attachment 8609476 [details] [diff] [review] > Dev Edition update nag should not be daily > > This seems to work in my limited testing. I've introduced a new pref > app.update.badgeWaitTime with a default value of 4 days that determines how > soon the badge will appear after the update is ready. I've been tweaking it > to 5 (seconds) from about:config to test it locally. > > I only postpone the badge on successful downloads, should I also do it on > failure? Only necessary of we think that happens a lot. My instinct says no, but I have no data.
Assignee | ||
Comment 15•9 years ago
|
||
I agree and I believe if a background update fails, then it will not be retried without user intervention. So the risk to leave users without security and stability fixes is considerable.
Assignee | ||
Updated•9 years ago
|
Attachment #8609476 -
Flags: review?(gijskruitbosch+bugs)
Comment 16•9 years ago
|
||
Comment on attachment 8609476 [details] [diff] [review] Dev Edition update nag should not be daily Review of attachment 8609476 [details] [diff] [review]: ----------------------------------------------------------------- This creates one timer for each window and then only destroys them on application quit. So if I close a window without quitting the application, the timer will leak the window because it keeps a ref.
Attachment #8609476 -
Flags: review?(gijskruitbosch+bugs) → review-
Assignee | ||
Comment 17•9 years ago
|
||
Removed the shutdown observer and changed uninit() to cancel the timer on unload.
Attachment #8609476 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8611145 -
Flags: review?(gijskruitbosch+bugs)
Comment 18•9 years ago
|
||
Comment on attachment 8611145 [details] [diff] [review] Dev Edition update nag should not be daily This patch is identical to the last one.
Attachment #8611145 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Updated•9 years ago
|
Attachment #8611151 -
Flags: review?(gijskruitbosch+bugs)
Comment 20•9 years ago
|
||
Comment on attachment 8611151 [details] [diff] [review] Dev Edition update nag should not be daily Review of attachment 8611151 [details] [diff] [review]: ----------------------------------------------------------------- Almost there. Three notes below. I imagine the next will be r+ -- can you ensure that there's a bug on file for splitting the observer/timer bit into a module of sorts so we don't keep one for every window? ::: browser/base/content/browser.js @@ +2629,5 @@ > + // badgeWaitTime. > + this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > + this.timer.initWithCallback(this, this.badgeWaitTime * 1000, > + this.timer.TYPE_ONE_SHOT); > + return; Please add a comment before the return that explains we're avoiding uninit because we need the timer to stick around. So... what happens if this gets called multiple times? I'm assuming this is possible... we used to uninit immediately after applying the badge / update-status=succeeded. Now we don't, so the observer may get called a second time (maybe a day later when a new update arrives, or something). I'm having trouble tracing the update service's calls to figure out if this is realistic, but perhaps it's wise to guard against it anyway? Presumably, if this.timer is non-null, we don't need to start another timer and can return immediately. Or we can removeObserver in this callback so as to ensure we don't get called again (but that would not deal with having a subsequent notification that isn't successful which should immediately show the "error" state without waiting for a timer). @@ +2652,5 @@ > // update failed to get staged in the background. Therefore we need to keep > // our observer. > return; > } > this.uninit(); It seems like it might now make more sense to just add this call in the STATE_FAILED case, seeing as we're avoiding it everywhere else. Then we can remove the early returns and not rely on whether we let the method run to completion.
Attachment #8611151 -
Flags: review?(gijskruitbosch+bugs) → feedback+
Assignee | ||
Comment 21•9 years ago
|
||
All good points.
Attachment #8611151 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8611183 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 22•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #20) > Almost there. Three notes below. I imagine the next will be r+ -- can you > ensure that there's a bug on file for splitting the observer/timer bit into > a module of sorts so we don't keep one for every window? Filed bug 1168820.
Assignee | ||
Comment 23•9 years ago
|
||
Changes from the last version.
Comment 24•9 years ago
|
||
Comment on attachment 8611185 [details] [diff] [review] Interdiff Review of attachment 8611185 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8611185 -
Flags: review+
Updated•9 years ago
|
Attachment #8611183 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 25•9 years ago
|
||
Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e66224778488
Comment 27•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c9c55b1fb120
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Assignee | ||
Updated•9 years ago
|
Attachment #8611185 -
Attachment is obsolete: true
Assignee | ||
Comment 28•9 years ago
|
||
Comment on attachment 8611183 [details] [diff] [review] Dev Edition update nag should not be daily Approval Request Comment [Feature/regressing bug #]: this is an improvement to an existing feature [User impact if declined]: some users are annoyed by the daily update nagging of the hamburger button badge [Describe test coverage new/current, TreeHerder]: manual testing in nightly all week [Risks and why]: very small risk, change has been verified to work in nightly for quite a while [String/UUID change made/needed]: none
Attachment #8611183 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
status-firefox40:
--- → affected
Comment 29•9 years ago
|
||
Comment on attachment 8611183 [details] [diff] [review] Dev Edition update nag should not be daily Should be safe and if it can make our users happy, I am happy to take it.
Attachment #8611183 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reporter | ||
Updated•9 years ago
|
Whiteboard: [devedition-40] → [polish-backlog]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•