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)
Thunderbird
Add-Ons: Extensions API
Tracking
(thunderbird64+ fixed, thunderbird65 fixed)
RESOLVED
FIXED
Thunderbird 65.0
People
(Reporter: darktrojan, Assigned: darktrojan)
References
Details
Attachments
(2 files, 7 obsolete files)
9.37 KB,
patch
|
darktrojan
:
review+
jorgk-bmo
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
20.23 KB,
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 64.0
Updated•6 years ago
|
tracking-thunderbird64:
--- → +
Target Milestone: Thunderbird 64.0 → ---
Assignee | ||
Comment 1•6 years ago
|
||
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
Assignee | ||
Comment 2•6 years ago
|
||
This patch changes the new popup notifications to display a restart-required message. Needs bug 1496632 to be fixed first.
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #9017355 -
Attachment is obsolete: true
Assignee | ||
Comment 4•6 years ago
|
||
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)
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #9017360 -
Attachment is obsolete: true
Attachment #9018552 -
Flags: review?(mkmelin+mozilla)
Comment 6•6 years ago
|
||
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 7•6 years ago
|
||
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+
Assignee | ||
Comment 8•6 years ago
|
||
(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+
Assignee | ||
Comment 9•6 years ago
|
||
Attachment #9018552 -
Attachment is obsolete: true
Attachment #9019287 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 10•6 years ago
|
||
Comment on attachment 9019284 [details] [diff] [review]
1498967-addon-restart-required-ui-4.diff
Applies to both patches.
Attachment #9019284 -
Flags: approval-comm-beta+
Comment 11•6 years ago
|
||
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
Comment 12•6 years ago
|
||
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 → ---
Assignee | ||
Comment 13•6 years ago
|
||
Oh damn, it's preventing the startup of Lightning. I hadn't thought of that.
Flags: needinfo?(geoff)
Comment 14•6 years ago
|
||
So where does that leave us for 64 beta (since this has tracking 64)?
Assignee | ||
Comment 15•6 years ago
|
||
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.
Assignee | ||
Comment 16•6 years ago
|
||
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.
Assignee | ||
Comment 17•6 years ago
|
||
Attachment #9019284 -
Attachment is obsolete: true
Attachment #9019567 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 18•6 years ago
|
||
(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.
Assignee | ||
Comment 19•6 years ago
|
||
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 20•6 years ago
|
||
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+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 21•6 years ago
|
||
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 ago → 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Updated•6 years ago
|
Attachment #9019284 -
Flags: approval-comm-beta+
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 65.0
Updated•6 years ago
|
Attachment #9019287 -
Flags: approval-comm-beta+
Comment 22•6 years ago
|
||
Updated•6 years ago
|
status-thunderbird64:
--- → fixed
status-thunderbird65:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•