Hamburger menu doesn't display the green badge when staging is not possible

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: past, Assigned: past)

Tracking

Trunk
Firefox 41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

From bug 1170240 comment 5:

"[The code] doesn't appear to handle the case where staging is not possible or staging is disabled via pref. Try setting app.update.staging.enabled to false to disable it and you will get the same result as when staging is not possible."

I tried it and I don't see the badge in that case, although the about dialog displays the button to restart and update. I'm trying to figure out which notification is fired in that case.
There isn't an observer notification currently though I would be fine with adding one. The call to UpdatePrompt showUpdateDownloaded is what this should tie into.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#4570
Created attachment 8621511 [details] [diff] [review]
Display the update badge on the hamburger menu when staging is not possible

This patch seems to work for me: with staging disabled I get an update badge if I (manually) trigger showUpdateDownloaded(). Is this what you had in mind Rob?
Attachment #8621511 - Flags: review?(robert.strong.bugs)
Assignee: nobody → past
Status: NEW → ASSIGNED
Comment on attachment 8621511 [details] [diff] [review]
Display the update badge on the hamburger menu when staging is not possible

To simplify things I think you can remove the following since staging also calls showUpdateDownloaded
Services.obs.addObserver(this, "update-staged", false);

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#3425

and move the following to before the call to this._getAltUpdateWindow()

if (background && getPref("getBoolPref", PREF_APP_UPDATE_SILENT, false))
  return;
}
Services.obs.notifyObservers(null, "update-downloaded", null);

if (this._getAltUpdateWindow())
  return;


Is there also a notification when the user has set the pref to "Check for updates, but let me choose whether to install them"? Does the notification also handle errors with the check or the download?
Attachment #8621511 - Flags: review?(robert.strong.bugs) → review-
Created attachment 8622524 [details] [diff] [review]
Display the update badge on the hamburger menu when staging is not possible v2

I couldn't remove the update-staged observer, because we need it for the failure case (update.state == "failed"), where execution doesn't enter showUpdateDownloaded. I managed to simplify the code a little more however with your suggestions and fix a few minor errors that I spotted in the process.

About the "Check for updates, but let me choose whether to install them" option, I don't see any notifications firing when the user toggles it, just setting a couple of prefs:

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/advanced.js#732

It sets app.update.enabled = true, app.update.auto = false.
Attachment #8621511 - Attachment is obsolete: true
Attachment #8622524 - Flags: review?(robert.strong.bugs)
Comment on attachment 8622524 [details] [diff] [review]
Display the update badge on the hamburger menu when staging is not possible v2

This is an improvement so I'm ok with this but I am fairly certain this won't handle all error cases it should handle (non staging download failures for starters). I won't have time in the for at least several weeks to investigate so let's go with this and file a followup.
Attachment #8622524 - Flags: review?(robert.strong.bugs) → review+
Filed bug 1176501 for investigating the cases in comment 6.
I made a dumb typo that I've just fixed.
https://hg.mozilla.org/mozilla-central/rev/8800b4f3ccbc
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.