Last Comment Bug 793338 - click-to-play: implement multiple plugin doorhanger ui (vulnerability status bits)
: click-to-play: implement multiple plugin doorhanger ui (vulnerability status ...
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 18
Assigned To: David Keeler [:keeler] (use needinfo?)
: Paul Silaghi, QA [:pauly]
Mentors:
Depends on: 754472 788829 797976
Blocks: click-to-play
  Show dependency treegraph
 
Reported: 2012-09-21 16:57 PDT by David Keeler [:keeler] (use needinfo?)
Modified: 2012-11-09 07:01 PST (History)
21 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
patch (21.92 KB, patch)
2012-09-21 17:11 PDT, David Keeler [:keeler] (use needinfo?)
jaws: review-
Details | Diff | Review
patch v2 (23.83 KB, patch)
2012-09-25 14:05 PDT, David Keeler [:keeler] (use needinfo?)
jaws: review+
Details | Diff | Review
patch v3 (23.48 KB, patch)
2012-09-29 19:36 PDT, David Keeler [:keeler] (use needinfo?)
dkeeler: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description David Keeler [:keeler] (use needinfo?) 2012-09-21 16:57:27 PDT
+++ This bug was initially created as a clone of Bug #754472 +++

bug 754472 handled moving to the new UI overall. This bug handles the parts that are specific to click-to-play via blocklisting (i.e. the vulnerability status of the plugins).
Comment 1 David Keeler [:keeler] (use needinfo?) 2012-09-21 17:11:18 PDT
Created attachment 663605 [details] [diff] [review]
patch

Dietrich - this applies on top of the patch in bug 754472.
Comment 2 Dietrich Ayala (:dietrich) 2012-09-21 18:32:30 PDT
Comment on attachment 663605 [details] [diff] [review]
patch

Get initial review from Jared or Margaret. I can do a final pass if needed, but they should be enough.
Comment 3 David Keeler [:keeler] (use needinfo?) 2012-09-24 09:21:45 PDT
Comment on attachment 663605 [details] [diff] [review]
patch

Jared - if you could have another look at this, that'd be great.
I left the exclamation marks in as per the UI design, but if you feel strongly about removing them, I could maybe be convinced.
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-09-24 13:33:08 PDT
Comment on attachment 663605 [details] [diff] [review]
patch

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

I talked with Stephen and I'm OK with the usage of the exclamation points, but it does differ with our Plugin Check tone. For comparison, http://jrswab.files.wordpress.com/2012/09/firefoxpluginupdatepage.png

::: browser/base/content/browser-plugins.js
@@ +437,5 @@
>      for (let pluginName in pluginsList) {
> +      let plugin = pluginsList[pluginName][0];
> +      let warn = false;
> +      let warningtext = "";
> +      let updatelink = Services.urlFormatter.formatURLPref("plugins.update.url");

nit: camelCase these variable names for internal consistency with the rest of this file.

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

Blair had this feedback in bug 772897 comment #4:
If these images are useful to be re-used (shorlander?), they should be moved out of /extensions/ and into something like global/icons

@@ +152,5 @@
> +  background-image: url("chrome://mozapps/skin/extensions/alerticon-info-negative.png");
> +  background-repeat: no-repeat;
> +  width: 16px;
> +  height: 15px;
> +  margin-top: -4px;

Why do we need the negative margin here? Is the image not the correct height or is there something else that can be used here? Also, should the image be height=16px?
Comment 5 David Keeler [:keeler] (use needinfo?) 2012-09-25 14:05:49 PDT
Created attachment 664642 [details] [diff] [review]
patch v2

Fixed the camel case and the negative margins.
With regard to the images (> +  background-image: url("chrome://mozapps/skin/extensions/stripes-info-negative-small.png");), I was just following the style of what was already there - there are a number of such stripe images. If we want to move them all, that sounds like a follow-up bug that we can take care of later.
Comment 6 Jared Wein [:jaws] (please needinfo? me) 2012-09-25 14:42:34 PDT
Comment on attachment 664642 [details] [diff] [review]
patch v2

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

::: toolkit/themes/pinstripe/global/notification.css
@@ +261,5 @@
> +  background-image: url("chrome://mozapps/skin/extensions/alerticon-info-negative.png");
> +  background-repeat: no-repeat;
> +  width: 16px;
> +  height: 15px;
> +  padding-bottom: 4px;

Please change this to margin-bottom before checking in.
Comment 7 David Keeler [:keeler] (use needinfo?) 2012-09-29 19:36:48 PDT
Created attachment 666270 [details] [diff] [review]
patch v3

rebased, etc. patch for landing when the patch in bug 754472 gets r+'d.
(carrying over r+)
Comment 8 Alex Keybl [:akeybl] 2012-10-02 13:56:46 PDT
Comment on attachment 666270 [details] [diff] [review]
patch v3

[Triage Comment]
Pre-approving for Aurora, contingent upon a green build on m-i or m-c.
Comment 9 Jared Wein [:jaws] (please needinfo? me) 2012-10-02 14:05:31 PDT
Landed on m-i. I'll wait for inbound to turn green before landing on m-a.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7949d28f958f
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-10-02 18:45:10 PDT
https://hg.mozilla.org/mozilla-central/rev/7949d28f958f
Comment 11 Jared Wein [:jaws] (please needinfo? me) 2012-10-02 20:33:20 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/a4880b03634f
Comment 12 Dão Gottwald [:dao] 2012-10-03 07:51:09 PDT
Comment on attachment 666270 [details] [diff] [review]
patch v3

>+.center-item-box[warn="true"] {
>+  background-image: url("chrome://mozapps/skin/extensions/stripes-info-negative-small.png");

It's unclear why you're putting this image into mozapps/skin/extensions/. Please file a bug on moving it away from there. Next time around, ask a toolkit peer for review (https://wiki.mozilla.org/Modules/Toolkit).

By the way, browser.xul and browser.css are shared by a variety of different code. Any ids and classes you add ought to be unambiguous; "center-item-box" is way too generic.
Comment 13 Paul Silaghi, QA [:pauly] 2012-10-04 06:18:04 PDT
"Check for updates" option from the doorhanger near the location bar opens a new FF window, unlike the "Check for updates" option from the CTP screen, which opens a new tab.
Comment 14 Paul Silaghi, QA [:pauly] 2012-11-09 07:01:11 PST
The new UI looks ok on all OSes except bug 797976 (comment 13). Verified fixed

Note You need to log in before you can comment on or make changes to this bug.