Closed Bug 1498967 Opened 6 years ago Closed 6 years ago

Changes to legacy extensions should require a restart

Categories

(Thunderbird :: Add-Ons: Extensions API, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird64+ fixed, thunderbird65 fixed)

RESOLVED FIXED
Thunderbird 65.0
Tracking Status
thunderbird64 + fixed
thunderbird65 --- fixed

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(2 files, 7 obsolete files)

To prevent a whole new bunch of problems, legacy extensions should behave as they did before. That is, require a restart when they are installed/removed/enabled/disabled/updated. The UI should encourage the user to do it. This doesn't actually stop some problems occurring, because the extensions code thinks all extensions can be stopped and started at will. We can "start" and not do anything, or "stop" and keep running, but the UI won't reflect reality unless we change it. If the user restarts the code will have the most accurate view of the way things really are.
Target Milestone: --- → Thunderbird 64.0
Target Milestone: Thunderbird 64.0 → ---
This is part 1 of 2, which updates the AOM interface to reflect what's really going on with legacy extensions.
Assignee: nobody → geoff
Status: NEW → ASSIGNED
This patch changes the new popup notifications to display a restart-required message. Needs bug 1496632 to be fixed first.
Attachment #9017355 - Attachment is obsolete: true
I'd send this to Philipp, but it needs to be reviewed soon.
Attachment #9017372 - Attachment is obsolete: true
Attachment #9018551 - Flags: review?(mkmelin+mozilla)
Attachment #9017360 - Attachment is obsolete: true
Attachment #9018552 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9018551 [details] [diff] [review] 1498967-addon-restart-required-ui-3.diff Review of attachment 9018551 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/aboutAddonsExtra.js @@ +135,5 @@ > > + if (ExtensionSupport.loadedLegacyExtensions.hasAnyState(this._addon.id, true)) { > + let { stringName, undoCommand, version } = getTrueState(this._addon, "gDetailView._addon"); > + > + if (stringName) { perhaps early return if we don't even have a name? @@ +183,2 @@ > > + if (restartButton) { // If one exists, so does the other. one? what does that refer to? @@ +186,5 @@ > undoButton.hidden = true; > } > }; > + > +// Update the UI when things change. JSDoc comment style please @@ +205,5 @@ > + Services.obs.removeObserver(statusChangedObserver, "legacy-addon-status-changed"); > +}); > + > +// The true status of legacy extensions, which AddonManager doesn't know > +// about because it thinks all extensions are restartless. here too, and add @return
Attachment #9018551 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9018552 [details] [diff] [review] 1498967-addon-restart-required-popup-2.diff Review of attachment 9018552 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/modules/ExtensionsUI.jsm @@ +600,5 @@ > }); > break; > } > case "webextension-install-notify": { > + let {addon} = subject.wrappedJSObject; { addon } ::: mail/locales/en-US/chrome/messenger/addons.properties @@ +20,5 @@ > addonPostInstall.okay.key=O > > +# LOCALIZATION NOTE (addonPostInstall.restartRequired.message) > +# %S is the application name > +addonPostInstall.restartRequired.message=%S must be restarted to complete installation. the installation
Attachment #9018552 - Flags: review?(mkmelin+mozilla) → review+
(In reply to Magnus Melin [:mkmelin] from comment #6) > ::: mail/base/content/aboutAddonsExtra.js > @@ +135,5 @@ > > > > + if (ExtensionSupport.loadedLegacyExtensions.hasAnyState(this._addon.id, true)) { > > + let { stringName, undoCommand, version } = getTrueState(this._addon, "gDetailView._addon"); > > + > > + if (stringName) { > > perhaps early return if we don't even have a name? I would but we're not returning after this. There's more code in the function. > @@ +183,2 @@ > > > > + if (restartButton) { // If one exists, so does the other. > > one? what does that refer to? Hmm, I'm not sure that's even true any more.
Attachment #9018551 - Attachment is obsolete: true
Attachment #9019284 - Flags: review+
Attachment #9018552 - Attachment is obsolete: true
Attachment #9019287 - Flags: review+
Keywords: checkin-needed
Comment on attachment 9019284 [details] [diff] [review] 1498967-addon-restart-required-ui-4.diff Applies to both patches.
Attachment #9019284 - Flags: approval-comm-beta+
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/09209cd02343 Display restart message in Add-Ons Manager when changing legacy extensions. r=mkmelin https://hg.mozilla.org/comm-central/rev/8f2aaabd3130 Ask to restart after installing a legacy extension. r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Backout: https://hg.mozilla.org/comm-central/rev/e51fe5c93b88ba50efe167c4883e3394a1c0c4d3 Backed out 2 changesets, 09209cd02343:8f2aaabd3130 (bug 1498967) for MozMill failures. a=backout This seems to interfere with the loading of your MozMill and JS Bridge add-ons. I assume this is the culprit since I can see a green try without this: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&group_state=expanded&selectedJob=207227591&revision=a90f45cf1819b37a158e9b036cff054ffd41857f
Status: RESOLVED → REOPENED
Flags: needinfo?(geoff)
Resolution: FIXED → ---
Oh damn, it's preventing the startup of Lightning. I hadn't thought of that.
Flags: needinfo?(geoff)
So where does that leave us for 64 beta (since this has tracking 64)?
I think I know how to deal with this. We let Lightning run by *not* automatically disabling it at startup (Mozmill changes a pref to do this), I just have to figure out whether it's affected by the pref and let it start. Unfortunately this has a side-effect, but people shouldn't mess with prefs.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5b9663fd92008b7f03a80314594041d529117413 After figuring this out, I now think I can forget about the ADDON_INSTALL startup reason altogether, because I think it implies the rest. Oh well.
Attachment #9019284 - Attachment is obsolete: true
Attachment #9019567 - Flags: review?(mkmelin+mozilla)
(In reply to Geoff Lankow (:darktrojan) from comment #16) > After figuring this out, I now think I can forget about the ADDON_INSTALL > startup reason altogether, because I think it implies the rest. Oh well. Good news: What I thought was incorrect. The startup reason could be ADDON_INSTALL in other situations. Bad news: One of those other situations is when Lightning is installed in a new profile from the distribution folder.
Fix the fresh-install case too. This is ugly but I see no way around it.
Attachment #9019567 - Attachment is obsolete: true
Attachment #9019567 - Flags: review?(mkmelin+mozilla)
Attachment #9019570 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9019570 [details] [diff] [review] 1498967-addon-restart-required-ui-6.diff Review of attachment 9019570 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable to me. r=mkmelin
Attachment #9019570 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/f37d4808a1e0 Ask to restart after installing a legacy extension. r=mkmelin https://hg.mozilla.org/comm-central/rev/66b37e67478f Display restart message in Add-Ons Manager when changing legacy extensions. r=mkmelin
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #9019284 - Flags: approval-comm-beta+
Target Milestone: --- → Thunderbird 65.0
Attachment #9019287 - Flags: approval-comm-beta+
Depends on: 1554127
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: