Closed
Bug 1171852
Opened 9 years ago
Closed 9 years ago
Hamburger menu doesn't display the green badge when staging is not possible
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
Tracking
()
RESOLVED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: past, Assigned: past)
Details
Attachments
(1 file, 1 obsolete file)
7.25 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•9 years ago
|
||
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
Assignee | ||
Comment 2•9 years ago
|
||
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?
Assignee | ||
Updated•9 years ago
|
Attachment #8621511 -
Flags: review?(robert.strong.bugs)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → past
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•9 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e3bb1abc801
Comment 4•9 years ago
|
||
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-
Assignee | ||
Comment 5•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Attachment #8622524 -
Flags: review?(robert.strong.bugs)
Comment 6•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
Filed bug 1176501 for investigating the cases in comment 6.
Comment 9•9 years ago
|
||
I had to back this out due to test-failures: https://treeherder.mozilla.org/logviewer.html#?job_id=3536763&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/e67d1044c439
Assignee | ||
Comment 10•9 years ago
|
||
I made a dumb typo that I've just fixed.
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8800b4f3ccbc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in
before you can comment on or make changes to this bug.
Description
•