772897 - Implement UI for plugins made click-to-play by the blocklist

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

When we make a plugin click-to-play for whatever reason via the blocklist, we need to have some UI in the addons manager.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: addon manager bits

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: doorhanger bits

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 662306 [details] [diff] [review]
addon manager bits

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

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2784,5 @@
>          var warningLink = document.getElementById("detail-warning-link");
>          warningLink.value = gStrings.ext.GetStringFromName("details.notification.softblocked.link");
>          warningLink.href = this._addon.blocklistURL;
>          warningLink.hidden = false;
> +      } else if (this._addon.blocklistState == Ci.nsIBlocklistService.STATE_OUTDATED || this._addon.blocklistState == Ci.nsIBlocklistService.STATE_VULNERABLE_UPDATE_AVAILABLE) {

From bug 760625 comment 9:
> STATE_NO_UPDATE is intended to convey something a bit
> stronger than STATE_OUTDATED - that is, the plugin is known to be vulnerable
> and there is no update, rather than it's simply old

Given that, I think the wording for STATE_VULNERABLE_UPDATE_AVAILABLE should be different than STATE_OUTDATED. It should probably also be an error notification like STATE_VULNERABLE_NO_UPDATE, rather than just a warning.

@@ +2801,5 @@
> +          "details.notification.vulnerable",
> +          [this._addon.name], 1
> +        );
> +        var errorLink = document.getElementById("detail-error-link");
> +        errorLink.value = gStrings.ext.GetStringFromName("details.notification.blocked.link");

This needs to use a different string (even if the value is the same in en-US), since the context is slightly different.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 662307 [details] [diff] [review]
doorhanger bits

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

This is completely independent from the Add-ons Manager UI bits, so would be good to have it in a separate bug.

And I see this patch depends on the patch in bug 754472 - took me awhile to find that :) Haven't looked at this patch too closely, since I haven't looked at the other bug.

::: browser/base/content/urlbarBindings.xml
@@ +7,5 @@
>  
>  <!DOCTYPE bindings [
>  <!ENTITY % notificationDTD SYSTEM "chrome://global/locale/notification.dtd">
>  %notificationDTD;
> +<!ENTITY % pluginsDTD SYSTEM "chrome://mozapps/locale/plugins/plugins.dtd">

Can't re-use strings like this (different context, which matters a lot in some locales). You'll need to add new strings to browser/locales/en-US/chrome/browser/browser.properties, and handle it like the other notifications.

::: toolkit/themes/pinstripe/global/notification.css
@@ +224,5 @@
>    padding-bottom: 12px;
>  }
>  
> +.center-item-box[warn="true"] {
> +  background-image: url("chrome://mozapps/skin/extensions/stripes-info-negative-small.png");

If these images are useful to be re-used (shorlander?), they should be moved out of /extensions/ and into something like global/icons

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 662307 [details] [diff] [review]
doorhanger bits

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

Blair already gave good feedback, and I just noticed too small things. Nice work so far, thanks for working on this!

::: browser/base/content/browser-plugins.js
@@ +414,5 @@
>      }
>  
>      let centerActions = [];
>      for (let pluginName in pluginsList) {
> +      let onePlugin = pluginsList[pluginName][0];

Maybe rename variable to 'plugin'?

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +133,4 @@
>  PluginVulnerableUpdatable=This plugin is vulnerable and should be updated.
>  PluginVulnerableNoUpdate=This plugin has security vulnerabilities.
> +vulnerableUpdatablePluginWarning=Outdated Version!
> +vulnerableNoUpdatePluginWarning=Vulnerable Plugin!

I think we can drop the exclamation marks.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks for the reviews, Blair and Jared. I'm waiting on Shorlander for some feedback, and then I'll put up the revised patches (and I'll probably spin off the notification patch into its own bug).

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v2

Thanks for the review. I've addressed your comments, so I'm asking for another one.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 664122 [details] [diff] [review]
patch v2

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

r+ with the following string change:

::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
@@ +27,5 @@
>  notification.outdated=An important update is available for %1$S.
>  notification.outdated.link=Update Now
> +#LOCALIZATION NOTE (notification.vulnerableUpdatable) %1$S is the add-on name
> +notification.vulnerableUpdatable=%1$S is known to be vulnerable. Please update it.
> +notification.vulnerableUpdatable.link=More Information

This doesn't quite feel right to me - "Please update it" is a call to action, but the link is separate. Taking inspiration from the outdated notification:

notification.vulnerableUpdatable=%1$S is known to be vulnerable and should be updated.
notification.vulnerableUpdatable.link=Update Now

(Same for the details variant of those strings.)

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v3

Fixed strings. Carrying over r+.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=48f092dd21d7
Thanks, Blair!

Comment 10

[Ian Melven :imelven]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f341fc58f595

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f341fc58f595

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 664599 [details] [diff] [review]
patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): blocklist/click-to-play plugins (bug 760625)
User impact if declined: users won't see the vulnerability status of click-to-play-blocklisted plugins in about:addons
Testing completed (on m-c, etc.): tested on m-c, been on m-c for about a week
Risk to taking this patch (and alternatives if risky): low (this is really just some small UI changes)
String or UUID changes made by this patch: added strings: 

+notification.vulnerableUpdatable=%1$S is known to be vulnerable and should be updated.
+notification.vulnerableUpdatable.link=Update Now

+notification.vulnerableNoUpdate=%1$S is known to be vulnerable. Use with caution.
+notification.vulnerableNoUpdate.link=More Information

+details.notification.vulnerableUpdatable=%1$S is known to be vulnerable and should be updated.
+details.notification.vulnerableUpdatable.link=Update Now

+details.notification.vulnerableNoUpdate=%1$S is known to be vulnerable. Use with caution.
+details.notification.vulnerableNoUpdate.link=More Information

Comment 13

[Alex Keybl [:akeybl]]

(In reply to David Keeler from comment #12)
> User impact if declined: users won't see the vulnerability status of
> click-to-play-blocklisted plugins in about:addons

If we don't take this in FF17, wouldn't the about:addons experience match that of a soft/hard block?

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Alex Keybl [:akeybl] from comment #13)
> (In reply to David Keeler from comment #12)
> > User impact if declined: users won't see the vulnerability status of
> > click-to-play-blocklisted plugins in about:addons
> 
> If we don't take this in FF17, wouldn't the about:addons experience match
> that of a soft/hard block?

Unfortunately, no. The UI depends on the exact value of the plugin's blocklistState. If it isn't one of STATE_OUTDATED, STATE_SOFTBLOCKED, or STATE_BLOCKED, it'll just look like a normal plugin.

Comment 15

[Alex Keybl [:akeybl]]

(In reply to David Keeler from comment #14)
> Unfortunately, no. The UI depends on the exact value of the plugin's
> blocklistState. If it isn't one of STATE_OUTDATED, STATE_SOFTBLOCKED, or
> STATE_BLOCKED, it'll just look like a normal plugin.

Can you propose a workaround that would not have l10n string impact, given the fact that this wasn't included in what we originally intended to uplift?

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Alex Keybl [:akeybl] from comment #15)
> Can you propose a workaround that would not have l10n string impact, given
> the fact that this wasn't included in what we originally intended to uplift?

For Fx17 we could just show the existing outdated warning instead (which would also mean minimal code change, fwiw).

Comment 17

[Alex Keybl [:akeybl]]

(In reply to Blair McBride (:Unfocused) from comment #16)
> (In reply to Alex Keybl [:akeybl] from comment #15)
> > Can you propose a workaround that would not have l10n string impact, given
> > the fact that this wasn't included in what we originally intended to uplift?
> 
> For Fx17 we could just show the existing outdated warning instead (which
> would also mean minimal code change, fwiw).

Let's go with this if David agrees.

Comment 18

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: aurora workaround

Something like this?

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 667704 [details] [diff] [review]
aurora workaround

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

Yep. Would really like the test ported too though.

Nit: Wrap the lines after the || ?

Comment 20

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: aurora workaround v2

Ported the tests, added line breaks.

Comment 21

[Paul Silaghi, QA [:pauly]]

FF 18.0a1 (2012-10-03)
Clicking on "Update now" in Add-ons Manager right next to:
Shockwave Flash 11.1.102.63 => links to https://addons.mozilla.org/en-US/firefox/blocked/p94

Silverlight 4.0.60531.0 => links to https://addons.mozilla.org/en-US/firefox/blocked/p147 (We're sorry, but we can't find what you're looking for)

Wouldn't be better that "Update now" to link to the plugins check page ?

Comment 22

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 664599 [details] [diff] [review]
patch v3

This isn't the patch we're considering for aurora, so removing the flag.

Comment 23

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Paul Silaghi [QA] from comment #21)
> FF 18.0a1 (2012-10-03)
> Clicking on "Update now" in Add-ons Manager right next to:
> Shockwave Flash 11.1.102.63 => links to
> https://addons.mozilla.org/en-US/firefox/blocked/p94
> 
> Silverlight 4.0.60531.0 => links to
> https://addons.mozilla.org/en-US/firefox/blocked/p147 (We're sorry, but we
> can't find what you're looking for)

Looks like there aren't pages up for the new staged blocks.

> Wouldn't be better that "Update now" to link to the plugins check page ?

I'm on the fence about this one. On the one hand, that's where all other "Update now" links go. On the other, using the block url allows us to give really specific information about what this new kind of block is and how the user can update their plugin. Blair, what do you think?

Comment 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to David Keeler from comment #23)
> Looks like there aren't pages up for the new staged blocks.

AFAIK, that's normal for staged blocks.


> > Wouldn't be better that "Update now" to link to the plugins check page ?
> 
> I'm on the fence about this one. On the one hand, that's where all other
> "Update now" links go. On the other, using the block url allows us to give
> really specific information about what this new kind of block is and how the
> user can update their plugin. Blair, what do you think?

Yea, I'm on the fence too. The blocked pages do link to the same update link that the Plugin Checker page links to - so IMO its not a problem, more of a question of which messaging is betterer. Filed bug 798176 for a decision on that, but IMO it's something that can be sorted out at any time.

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 667741 [details] [diff] [review]
aurora workaround v2

thanks for working around the l10n changes post-string freeze, makes this possible to uplift. please land prior to this coming monday, oct 8th which is merge day.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7a30519f6f10

Comment 27

[Paul Silaghi, QA [:pauly]]

Addons Manager FF 17b5:
"An important update is available for Java 7u7"

Addons Manager FF 18, 19:
"Java 7u7 is known is known to be vulnerable and should be updated"

Why not the same message ?

Comment 28

[Robert Kaiser]

(In reply to Paul Silaghi [QA] from comment #27)
> Why not the same message ?

AFAIK because the L10n freeze didn't allow us to get the changed string into 17.

Comment 29

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Robert Kaiser (:[e-mail]) from comment #28)
> (In reply to Paul Silaghi [QA] from comment #27)
> > Why not the same message ?
> 
> AFAIK because the L10n freeze didn't allow us to get the changed string into
> 17.

Yes. See comment 15.

Comment 30

[Paul Silaghi, QA [:pauly]]

Verified fixed on FF 17b5 based on comment 29.

Comment 31

[Paul Silaghi, QA [:pauly]]

Verified fixed on FF 18b3.