The default bug view has changed. See this FAQ.

click-to-play: implement multiple plugin doorhanger ui (vulnerability status bits)

VERIFIED FIXED in Firefox 17

Status

()

Firefox
General
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Trunk
Firefox 18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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

Updated

5 years ago
No longer blocks: 772897
No longer depends on: 746374
Created attachment 663605 [details] [diff] [review]
patch

Dietrich - this applies on top of the patch in bug 754472.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #663605 - Flags: review?(dietrich)
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.
Attachment #663605 - Flags: review?(dietrich)
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.
Attachment #663605 - Flags: review?(jaws)
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?
Attachment #663605 - Flags: review?(jaws) → review-
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.
Attachment #663605 - Attachment is obsolete: true
Attachment #664642 - Flags: review?(jaws)
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.
Attachment #664642 - Flags: review?(jaws) → review+
QA Contact: paul.silaghi
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+)
Attachment #664642 - Attachment is obsolete: true
Attachment #666270 - Flags: review+
Attachment #666270 - Attachment is patch: true

Comment 8

5 years ago
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.
Attachment #666270 - Flags: approval-mozilla-aurora+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [land with the patch in bug 793338]
(Assignee)

Updated

5 years ago
Whiteboard: [land with the patch in bug 793338] → [land with the patch in bug 754472]
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
Keywords: checkin-needed
Whiteboard: [land with the patch in bug 754472]
https://hg.mozilla.org/mozilla-central/rev/7949d28f958f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
https://hg.mozilla.org/releases/mozilla-aurora/rev/a4880b03634f
status-firefox17: --- → fixed
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.

Updated

5 years ago
Component: Plug-ins → General
Product: Core → Firefox
QA Contact: paul.silaghi
Target Milestone: mozilla18 → ---

Updated

5 years ago
QA Contact: paul.silaghi
Target Milestone: --- → Firefox 18
"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.
(Assignee)

Updated

5 years ago
Depends on: 797976
Keywords: verifyme
The new UI looks ok on all OSes except bug 797976 (comment 13). Verified fixed
Status: RESOLVED → VERIFIED
status-firefox17: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.