1483087 - Add-on manager UI for legacy extensions needs improvement

Status: RESOLVED FIXED

(MailNews Core :: XUL Replacements, enhancement)

Description

[Geoff Lankow (:darktrojan)]

Currently we modify the AOM to display "This is a legacy add-on, a restart is required to continue. [Restart]" when the user disabled a legacy extension.

If the user clicks to remove the extension, they are told it will be removed when the tab is closed. This is false.

I have a patch to also display the first message in the second situation. On writing it, I realised we should also offer an undo button, and a link to a support article about legacy extensions.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1483087-legacy-extension-ui-1.diff

This support URL is a placeholder. Who do we get to create a page to link to?

Comment 2

[Geoff Lankow (:darktrojan)]

The original AOM strings said (for example):

> Lightning will be disabled after you restart Thunderbird.
> Lightning will be uninstalled after you restart Thunderbird.
> Lightning will be updated after you restart Thunderbird.

Which reminds me, I haven't even considered what happens when an update happens. That could be messy.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: 1483087-legacy-detail-view-1.diff

This adds the same warnings and buttons to the detail view.

There's a major problem with all this: we don't know if an extension is a legacy webextension unless it is enabled, and clicking the "disable" or "remove" button actually disables the extension. We just pretend it doesn't. In a lot of cases this is okay, but if the user doesn't restart immediately and the AOM is reloaded or switches to another view, it stops displaying warnings about restarting.

Comment 4

[Jorg K (CEST = GMT+2)]

(In reply to Geoff Lankow (:darktrojan) from comment #0)
> ..., I realised we should also offer an undo button, and a link to a
> support article about legacy extensions.
(In reply to Geoff Lankow (:darktrojan) from comment #1)
> This support URL is a placeholder. Who do we get to create a page to link to?
I'm not sure why you're asking me. Is this going to be a Wiki page or a SUMO article? Or a page on thunderbird.net? In general, the most appropriate person can write it. You, Philipp, Magnus, me? Sorry, I don't have a better answer.

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: 1483087-legacy-extension-ui-2.diff

I've changed my mind about having a "more information" link. We don't need it.

This patch does five things:
* Adds an undo button to the restart message.
* Adds the restart message to the extension detail view.
* Keeps a list of the webextensions that have started in this session in an accessible place, and uses that to keep the AOM up-to-date.
* Fixes the order of the list, so still-running-but-"disabled" extensions are in the right place.
* Goes back to the old message wording "A will be disabled when you restart B."

Comment 6

[Geoff Lankow (:darktrojan)]

Comment on attachment 9000533 [details] [diff] [review]
1483087-legacy-extension-ui-2.diff

I hate to hit you with yet another big review request Philipp, but this one can go to the back of the queue. At least behind bug 1477956. :)

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9000533 [details] [diff] [review]
1483087-legacy-extension-ui-2.diff

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

Thanks, I gave up and simplified back when I wrote that patch, so good to be more exact.

::: mail/base/content/specialTabs.js
@@ +274,5 @@
> +      _window.gStrings.mailExt = extBundle;
> +      _window.isCorrectlySigned = function() { return true; };
> +
> +      ChromeUtils.import("resource:///modules/extensionSupport.jsm");
> +      _window.ExtensionSupport = ExtensionSupport;

Maybe it makes sense to move most of what you added here into a different function to reduce comlpexity?

@@ +341,5 @@
> +
> +        if (
> +          ExtensionSupport.loadedLegacyExtensions.has(this._addon.id) &&
> +          (this._addon.userDisabled || pending & AddonManager.PENDING_UNINSTALL)
> +        ) {

Did you run eslint on this? I'm not sure, but this seemed different than what I usually see.

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1483087-legacy-extension-ui-3.diff

After looking at this for the first time in a while, I've decided I should move it all to a separate file, like I considered earlier.

Essentially the same code, new place.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9005926 [details] [diff] [review]
1483087-legacy-extension-ui-3.diff

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

::: mail/base/content/aboutAddonsExtra.js
@@ +22,5 @@
> +  hbox.setAttribute("id", "nav-header");
> +  hbox.setAttribute("align", "center");
> +  hbox.setAttribute("pack", "center");
> +
> +  let button1 = document.createElement("toolbarbutton");

I know this is cut/paste, but can we call this back or backButton?

@@ +29,5 @@
> +  button1.setAttribute("command", "cmd_back");
> +  button1.setAttribute("tooltiptext", gStrings.mailExt.GetStringFromName("cmdBackTooltip"));
> +  button1.setAttribute("disabled", "true");
> +
> +  let button2 = document.createElement("toolbarbutton");

forward/forwardButton

@@ +49,5 @@
> +  textbox.setAttribute("placeholder", placeholder);
> +})();
> +
> +window._oldSortElements = window.sortElements;
> +window.sortElements = function(aElements, aSortBy, aAscending) {

You could make this an IIFE if you want to save yourself the extra global variable. Same below.

@@ +56,5 @@
> +  }
> +
> +  let getUIState = function(addon) {
> +    if (addon.pendingOperations == AddonManager.PENDING_DISABLE)
> +      return "pendingDisable";

More a question than a review comment, but my preference is {} even for one line ifs. If you do that, then using else if for the futher cases will save you a few lines. I'm not sure though if mail eventually wants to follow the calendar eslint or define its own, you could ask Magnus for an opinion.

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #9)
> More a question than a review comment, but my preference is {} even for one
> line ifs.
That's standard now in M-C for new code (I was told during a review for an M-C patch last week).

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: 1483087-legacy-extension-ui-4.diff

> You could make this an IIFE if you want to save yourself the extra global
> variable. Same below.

In cases like this where we're still using the original function, I prefer to be able to access it for when something inevitably breaks.

> More a question than a review comment, but my preference is {} even for one
> line ifs. If you do that, then using else if for the futher cases will save
> you a few lines. I'm not sure though if mail eventually wants to follow the
> calendar eslint or define its own, you could ask Magnus for an opinion.

Cut/paste SNAFU.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 9006118 [details] [diff] [review]
1483087-legacy-extension-ui-4.diff

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

::: mail/base/content/aboutAddonsExtra.js
@@ +68,5 @@
> +    if (!addon.isActive &&
> +        (addon.pendingOperations != AddonManager.PENDING_ENABLE &&
> +         addon.pendingOperations != AddonManager.PENDING_INSTALL)) {
> +      return "disabled";
> +  	}

You have a tab here :-( - I'll fix it.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a23b4edca9bf
Improved Add-on Manager UI for legacy extensions. r=philipp