Closed Bug 1356462 Opened 7 years ago Closed 7 years ago

about:addons indications for disabled non-MPC extensions

Categories

(Toolkit :: Add-ons Manager, enhancement)

Product:
Toolkit
Component:
Add-ons Manager
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: aswan, Assigned: aswan)

References

Details

Attachments

(6 files)

Nigtly-MPC-disabled-notification.PNG
43.28 KB, image/png
Details
Bug 1356462 Add per-extension notice for non-MPC disabling
59 bytes, text/x-review-board-request
mossop
: review+
Details
screen shot of about:addons entry for a disabled extension
45.51 KB, image/png
Details
screen shot of startup notification
21.65 KB, image/png
Details
Bug 1356462 Show a notification when non-MPC extensions are disabled
59 bytes, text/x-review-board-request
mossop
: review+
Details
2017-04-25_1826.png
150.72 KB, image/png
Details 
As described in bug 1352204, we'll be disabling non-multiprocessCompatible extensions on Nightly.  This bug is meant to cover everything we want to do in about:addons to indicate to users why affected extensions have been disabled.

This includes:

1. A notification to display at the top of the extensions list, analogous to the existing "Some extensions could not be verified".
2. A notification to display next to individual extensions that are disabled, analogous to the existing "(name) could not be verified for use in Nightly and has been disabled"
3. A notification upon first startup when we disable one or more extensions?  (see https://bugzilla.mozilla.org/show_bug.cgi?id=1352204#c16)
There are some open questions here, mostly they boil down to the fact that we'd like to do bug 1352204 as quickly as possible so given that this is going to be short-term and Nightly-only, how simple can we make it.  Anyway here are some specific questions:

1. Can we just re-use the existing "Some extensions could not be verified" notification?  If we want a new one, what do we show if we have both conditions (some extensions disabled due to being unsigned, some extension disabled due to being non-MPC)
2. Clicking on the top-of-page notification for unsigned extensions goes to a list of unsigned extensions.  Is it important that we do that here?  (its doable, its just more work, we also need to define how this interacts with unsigned if we re-use the same notification as suggested in question #1)
3. The per-extension notification for unsigned has a "More information" link that goes to a SUMO page, do we want to do the same here?
4. The notification described in https://bugzilla.mozilla.org/show_bug.cgi?id=1352204#c16 is doable but a non-trivial chunk of work, how important is it?
Flags: needinfo?(mjaritz)
Flags: needinfo?(amckay)
(In reply to Andrew Swan [:aswan] from comment #1)
> 3. The per-extension notification for unsigned has a "More information" link
> that goes to a SUMO page, do we want to do the same here?
Kev might know if and where we would put more information.
Flags: needinfo?(kev)
(In reply to Andrew Swan [:aswan] from comment #1)
> 2. Clicking on the top-of-page notification for unsigned extensions goes to
> a list of unsigned extensions.  Is it important that we do that here?  (its
> doable, its just more work, we also need to define how this interacts with
> unsigned if we re-use the same notification as suggested in question #1)

For Nightly I would just mark disabled extensions in-line with a yellow warning and not have a dedicated page. 
copy maybe: "This extension is potentially impacting performance tests." and if we have a "learn more", link to that.

> 1. Can we just re-use the existing "Some extensions could not be verified"
> notification?  If we want a new one, what do we show if we have both
> conditions (some extensions disabled due to being unsigned, some extension
> disabled due to being non-MPC)

I would re-use the notification plugins and themes use, and use similar text then the notification bar. (see attachment) - this notification should not conflict with the signing notification as it is in a different location.
copy maybe: Due to critical performance testing, we’ve disabled some of your add-ons. They can be re-enabled if needed.

> 4. The notification described in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1352204#c16 is doable but a
> non-trivial chunk of work, how important is it?

It is the only thing that let's users know what we changed, other then them realizing an extension is gone.
And it will help users being less upset about the change as we inform them and let them have control about their experience.
And Kev states it as a requirement in https://bugzilla.mozilla.org/show_bug.cgi?id=1352204#c8
Flags: needinfo?(mjaritz)
(In reply to Markus Jaritz [:designakt prev :maritz] (UX) from comment #3)
> For Nightly I would just mark disabled extensions in-line with a yellow
> warning and not have a dedicated page. 
> copy maybe: "This extension is potentially impacting performance tests." and
> if we have a "learn more", link to that.

Sorry, what is the "that" that we would link to?

> > 1. Can we just re-use the existing "Some extensions could not be verified"
> > notification?  If we want a new one, what do we show if we have both
> > conditions (some extensions disabled due to being unsigned, some extension
> > disabled due to being non-MPC)
> 
> I would re-use the notification plugins and themes use, and use similar text
> then the notification bar. (see attachment) - this notification should not
> conflict with the signing notification as it is in a different location.
> copy maybe: Due to critical performance testing, we’ve disabled some of your
> add-ons. They can be re-enabled if needed.

You mean the "Missing something?" notification?  (there's no attachment...)
(In reply to Andrew Swan [:aswan] from comment #4)
> (In reply to Markus Jaritz [:designakt prev :maritz] (UX) from comment #3)
> > For Nightly I would just mark disabled extensions in-line with a yellow
> > warning and not have a dedicated page. 
> > copy maybe: "This extension is potentially impacting performance tests." and
> > if we have a "learn more", link to that.
> 
> Sorry, what is the "that" that we would link to?

With "that" I wanted to refer to an external page, but so far I think we do not know if we will have one.
Should be defined by Kev. Maybe a SUMO page as you suggested, or some other page. If we think we need more explanation.
https://bugzilla.mozilla.org/show_bug.cgi?id=1356462#c2
 
> > > 1. Can we just re-use the existing "Some extensions could not be verified"
> > > notification?  If we want a new one, what do we show if we have both
> > > conditions (some extensions disabled due to being unsigned, some extension
> > > disabled due to being non-MPC)
> > 
> > I would re-use the notification plugins and themes use, and use similar text
> > then the notification bar. (see attachment) - this notification should not
> > conflict with the signing notification as it is in a different location.
> > copy maybe: Due to critical performance testing, we’ve disabled some of your
> > add-ons. They can be re-enabled if needed.
> 
> You mean the "Missing something?" notification?  (there's no attachment...)
Yes. sorry, now there is an attachment.
There's now three reasons (well probably more) an add-on could be disabled:
* signing
* not MPC=True on nightly
* not a WebExtension in 57+

A message such as shown in the attachment might be confusing if 2 or more of those states occurs at once. Could we keep that message simple and generic and show the details on the listing page?
Flags: needinfo?(amckay)
(In reply to Andy McKay [:andym] from comment #6)
> There's now three reasons (well probably more) an add-on could be disabled:
> * signing
> * not MPC=True on nightly
> * not a WebExtension in 57+
> 
> A message such as shown in the attachment might be confusing if 2 or more of
> those states occurs at once. Could we keep that message simple and generic
> and show the details on the listing page?

As Markus points out in comment 3, the existing signing notification appears in a different place, but we still have the other two cases and I agree with Andy.  And for consistency with the message for plugins, I propose:
Missing something?  Some extensions are no longer supported by (Firefox/Nightly)

As for the per-extension bit, I chatted to andym just now about putting this on wiki.m.o instead of sumo since that will be much easier and this is nightly-only.  Again I vote for keeping the message in about:addons simple, we can explain the connection to performance testing at the linked page.  How about:
This extension has been disabled since it is not multiprocess compatible.

Scott, needinfo to approve or revise the two strings above.
Flags: needinfo?(kev) → needinfo?(sdevaney)
Looks like the wiki page is:
https://wiki.mozilla.org/Add-ons/ShimsNightly
For plugins messaging, I'm okay with: 

"Missing something?  Some extensions are no longer supported by (Firefox/Nightly)"

And for about:addons bit, do we need to mention "multiprocess"? Why not just keep it even more simple and generic: 

"This extension has been disabled since it is not compatible with Firefox."
Flags: needinfo?(sdevaney)
Sorry I think I wasn't clear.  Both of these messages will appear in the "Extensions" view in about:addons.  One is the message at the top of the page if any extensions are disabled, one is next to individual extensions.

The top-of-page message is the "Missing something? ..." message which it sounds like you were okay with, but to clarify, this is about extensions and not plugins.

As for the per-extension message, we already use the message "(name) is incompatible with (Firefox/Nightly)" for version compatibility as opposed to the situation covered here (multiprocess compatibility).  We can re-use that message if you prefer, but that leaves a user with many extensions without an easy way to distinguish which ones are disabled due to multiprocess.  For what its worth, remember that this will only be on Nightly so the word "multiprocess" should be less intimidating to users who will encounter this...
Flags: needinfo?(sdevaney)
Thanks for the clarity, aswan! 

Yes, understood that we're just going with the same messaging convention as used with plug-ins, but with extensions instead. 

For the per-extension message, as you point out it makes sense to distinguish the messaging a bit to clarify why something has been disabled. I'm fine with:

"This extension has been disabled since it is not multiprocess compatible."
Flags: needinfo?(sdevaney)
I've attached work-in-progress patches and screenshots.  After thinking about this a little more, I'm not so sure we need another banner at the top of about:addons.  We have one for plugins since plugins don't actually show up in the list any more so there isn't anything to put a "disabled" notice next to.  And we have one for unsigned extensions that links to a filtered list of disabled extensions, but we previously decided that implementing a corresponding list for non-MPC extensions was unnecessary.  Which I think makes the banner at the top of the page pointless.

Markus, does this seem adequate to you?  If it is, I'll look into tests and get this landed.  (tests for the about:addons bits should be straightforward, but I'm not sure the startup notification will be practical to test)
Flags: needinfo?(mjaritz)
(In reply to Andrew Swan [:aswan] from comment #13)
> Created attachment 8859014 [details]
> screen shot of about:addons entry for a disabled extension

This should be a warning, not an error. (yellow instead of red) and should allow for the extension to be re-enabled. (That is what the startup notification promises.) - And it should state the same reason as the startup notification!


(In reply to Andrew Swan [:aswan] from comment #15)
> I've attached work-in-progress patches and screenshots.  After thinking
> about this a little more, I'm not so sure we need another banner at the top
> of about:addons.  We have one for plugins since plugins don't actually show
> up in the list any more so there isn't anything to put a "disabled" notice
> next to.  And we have one for unsigned extensions that links to a filtered
> list of disabled extensions, but we previously decided that implementing a
> corresponding list for non-MPC extensions was unnecessary.  Which I think
> makes the banner at the top of the page pointless.

The banner on top is there to provide context for people that have not read the startup notification and will not immediately notice one extension in their list changed state, and to remind people that this is important for test reasons.
I do not see why this message needs to be generic... It has a very specific purpose. Out of the 3 messages we do it is the least important, but still plays an important role if we want people to not just re-enable their extensions.

Our messaging currently mixes 2 different reasons for why we disabled:
We disabled extensions because they interfere with some tests we need to run.
We disabled extensions because they are not multi-process compatible.

We should stick with the first reason through all 3 messages. Otherwise we risk confusing people, and they will already have forgotten about the test we need to run when they look at the extension, and therefor have no problem re-enabling it.

I propose:
Startup "Due to critical performance testing, we have disabled some of your add-ons. They can be re-enabled in the Add-ons Manager."
Top     "Due to critical performance testing, we have disabled some of your add-ons. They can be re-enabled if needed."
Detail  "[Name] has been disabled since it is interfering with critical performance testing."
Flags: needinfo?(mjaritz) → needinfo?(sdevaney)
(In reply to Markus Jaritz [:designakt prev :maritz] (UX) from comment #16)
> (In reply to Andrew Swan [:aswan] from comment #13)
> > Created attachment 8859014 [details]
> > screen shot of about:addons entry for a disabled extension
> 
> This should be a warning, not an error. (yellow instead of red) and should
> allow for the extension to be re-enabled. (That is what the startup
> notification promises.) - And it should state the same reason as the startup
> notification!

Re-enabling requires flipping a preference in about:config, this is explained at the "More Information" link.  Given the larger scenario here (this is Nightly, this is important for performance telemetry), I advocate not streamlining this any further.

> (In reply to Andrew Swan [:aswan] from comment #15)
> Our messaging currently mixes 2 different reasons for why we disabled:
> We disabled extensions because they interfere with some tests we need to run.
> We disabled extensions because they are not multi-process compatible.
> 
> We should stick with the first reason through all 3 messages. Otherwise we
> risk confusing people, and they will already have forgotten about the test
> we need to run when they look at the extension, and therefor have no problem
> re-enabling it.

The two are connected and the connection is described at the wiki page that "More Information" links to so I don't think we're requiring people to remember anything...
Given this is a Nightly audience, I'm fine leaving this as-is.
Flags: needinfo?(sdevaney)
(In reply to Andrew Swan [:aswan] from comment #17)
> Re-enabling requires flipping a preference in about:config, this is
> explained at the "More Information" link.  Given the larger scenario here
> (this is Nightly, this is important for performance telemetry), I advocate
> not streamlining this any further.

I didn't know about the pref. Let's do it as you suggested.
Attachment #8859751 - Flags: review?(dtownsend)
Comment on attachment 8859751 [details]
Bug 1356462 Show a notification when non-MPC extensions are disabled

https://reviewboard.mozilla.org/r/131750/#review134626

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1969
(Diff revision 2)
>              }
>  
> +            if (!wasDisabled && newAddon.appDisabled && !ALLOW_NON_MPC &&
> +                !newAddon.multiprocessCompatible) {
> +              AddonManagerPrivate.nonMpcDisabled = true;
> +            }

You probably need to check the other cases for appDisabled. We don't need to set this if the add-on is incompatible or blocklisted for example. Can you add some tests for those cases too please?
Attachment #8859751 - Flags: review?(dtownsend)
Comment on attachment 8859013 [details]
Bug 1356462 Add per-extension notice for non-MPC disabling

https://reviewboard.mozilla.org/r/131034/#review134630

::: toolkit/mozapps/extensions/content/extensions.js:3382
(Diff revision 2)
> +        document.getElementById("detail-error").textContent = gStrings.ext.formatStringFromName(
> +          "details.notification.nonMpcDisabled", [this._addon.name], 1
> +        );
> +        let errorLink = document.getElementById("detail-error-link");
> +        errorLink.value = gStrings.ext.GetStringFromName("details.notification.nonMpcDisabled.link");
> +        errorLink.href = "https://wiki.mozilla.org/Add-ons/ShimsNightly";

This block needs to be further down the list, past the things that the user can't change.

::: toolkit/mozapps/extensions/content/extensions.xml:1285
(Diff revision 2)
> +              this._error.textContent = gStrings.ext.formatStringFromName(
> +                "notification.nonMpcDisabled", [this.mAddon.name], 1
> +              );
> +              this._errorLink.value = gStrings.ext.GetStringFromName("notification.nonMpcDisabled.link");
> +              this._errorLink.href = "https://wiki.mozilla.org/Add-ons/ShimsNightly";
> +              this._errorLink.hidden = false;

Same here.
Attachment #8859013 - Flags: review?(dtownsend)
Comment on attachment 8859751 [details]
Bug 1356462 Show a notification when non-MPC extensions are disabled

https://reviewboard.mozilla.org/r/131750/#review134626

> You probably need to check the other cases for appDisabled. We don't need to set this if the add-on is incompatible or blocklisted for example. Can you add some tests for those cases too please?

Okay, I think the right way to do this is to add an appDisabledReason property rather than duplicating the logic from isUsable.  Unless you have a better idea?
Comment on attachment 8859013 [details]
Bug 1356462 Add per-extension notice for non-MPC disabling

https://reviewboard.mozilla.org/r/131034/#review134630

> This block needs to be further down the list, past the things that the user can't change.

I'm a little confused by the logic for the clause below this one but it looks like the only one that is meant to be shown when the extension is actually disabled -- the remaining clauses are all warnings.  I started trying to add a new field to Addon objects to convey the reason for an extension being disabled but after fussing with different cases for a while I gave up.
That's all to say this just ended up moving down one spot but it appears to be correct to me...
Comment on attachment 8859751 [details]
Bug 1356462 Show a notification when non-MPC extensions are disabled

https://reviewboard.mozilla.org/r/131750/#review134626

> Okay, I think the right way to do this is to add an appDisabledReason property rather than duplicating the logic from isUsable.  Unless you have a better idea?

Ugh, as I said on the first patch, I started out with good intentions to centralize the logic into XPIProvider.jsm and it didn't really work out so here we are...
Comment on attachment 8859013 [details]
Bug 1356462 Add per-extension notice for non-MPC disabling

https://reviewboard.mozilla.org/r/131034/#review135410
Attachment #8859013 - Flags: review?(dtownsend) → review+
Comment on attachment 8859751 [details]
Bug 1356462 Show a notification when non-MPC extensions are disabled

https://reviewboard.mozilla.org/r/131750/#review135412
Attachment #8859751 - Flags: review?(dtownsend) → review+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2519bb971a4e
Add per-extension notice for non-MPC disabling r=mossop
https://hg.mozilla.org/integration/autoland/rev/b6b8098abd34
Show a notification when non-MPC extensions are disabled r=mossop
Attached image 2017-04-25_1826.png — 
Add-ons Manager notification (attached in Comment 5) is not displayed in Extensions Tab on Firefox 55.0a1 (2017-04-25) under Windows 10 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.12.1.   

See attached screenshot.

Any thoughts about this?
Flags: needinfo?(aswan)
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #34)
> Created attachment 8861472 [details]
> 2017-04-25_1826.png
> 
> Add-ons Manager notification (attached in Comment 5) is not displayed in
> Extensions Tab on Firefox 55.0a1 (2017-04-25) under Windows 10 64-bit,
> Ubuntu 14.04 64-bit and Mac OS X 10.12.1.   
> 
> See attached screenshot.
> 
> Any thoughts about this?

We decided not to include it.  See comment 15 through comment 19.
Flags: needinfo?(aswan)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: