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)

36 Branch
x86
macOS
defect

Tracking

(firefox40 fixed, firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: canuckistani, Assigned: past)

References

Details

(Whiteboard: [polish-backlog])

Attachments

(1 file, 4 obsolete files)

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.
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
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.
(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)
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)
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)
Agreed
Flags: needinfo?(jgriffiths)
Whiteboard: [devedition-40]
Priority: -- → P1
Proposal - we show the update indicator in the hamburger at most once every 4 days.
Joe - I still would like to see this for June 2 if possible.
Flags: needinfo?(jwalker)
Agreed.
My hunch is that Panos is best placed to do this, what do you think?
Flags: needinfo?(jwalker) → needinfo?(past)
I got it.
Assignee: nobody → past
Status: NEW → ASSIGNED
Flags: needinfo?(past)
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?
(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.
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.
Attachment #8609476 - Flags: review?(gijskruitbosch+bugs)
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-
Removed the shutdown observer and changed uninit() to cancel the timer on unload.
Attachment #8609476 - Attachment is obsolete: true
Attachment #8611145 - Flags: review?(gijskruitbosch+bugs)
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)
Woops!
Attachment #8611145 - Attachment is obsolete: true
Attachment #8611151 - Flags: review?(gijskruitbosch+bugs)
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+
All good points.
Attachment #8611151 - Attachment is obsolete: true
Attachment #8611183 - Flags: review?(gijskruitbosch+bugs)
(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.
Attached patch Interdiff (obsolete) — Splinter Review
Changes from the last version.
Comment on attachment 8611185 [details] [diff] [review]
Interdiff

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

Thanks!
Attachment #8611185 - Flags: review+
Attachment #8611183 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/c9c55b1fb120
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Depends on: 1170240
Attachment #8611185 - Attachment is obsolete: true
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?
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+
Whiteboard: [devedition-40] → [polish-backlog]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.