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 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 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 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.

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).

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

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.)

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

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

(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?

(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.

(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?

(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).

(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.

Something like this?

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 || ?

Ported the tests, added line breaks.

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 on attachment 664599 [details] [diff] [review] patch v3 This isn't the patch we're considering for aurora, so removing the flag.

(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?

(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 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.

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 ?

(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.

(In reply to Robert Kaiser (:kairo@mozilla.com) 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.

Verified fixed on FF 17b5 based on comment 29.

Verified fixed on FF 18b3.