I tested the patch. I seem following things:
-If the number of plugins require playing is only 1, it is good for a doorhanger does not show a center button.
-Should removing close button from xul popup-notification be separated from this bug?

Created attachment 656196[details][diff][review]
patch v2
Saneyuki - thanks for the feedback. I'm not the designer, so I'll defer to what Stephen says regarding the close button and having only one plugin type.
Implemented for all three platforms now.

Comment on attachment 656196[details][diff][review]
patch v2
Jared, this isn't quite ready for a formal review since a lot of the ui elements aren't finalized yet, but if you could have a look at the code changes, that'd be great.

Comment on attachment 656196[details][diff][review]
patch v2
Adding a request for feedback from Margaret since she worked on the initial doorhanger implementation (IIUC). I might not be able to get to this until next week.

Comment on attachment 656196[details][diff][review]
patch v2
Sorry for the delayed response, I've been traveling. I actually won't be back at work until Thursday, so I will likely be slow to respond until then.
Instead of extending the PopupNotifications for this one case, I think that it would be better to just override the binding, like we do for other unique doorhangers that require more UI. You should look at what we did for "geolocation-notification" and "addon-progress-notification". This should let you avoid changes to PopupNotifications and the generic bindings, and you should be able to put the logic for these center buttons in with your new binding.

Once this gets r+ and ui-review+ lands, we'll be pushing out test blocklists targeting FF18. Assigning Paul as the qacontact given the fact that he'll be helping us verify. We'll also be uplifting to FF17 on Aurora around the same time.

Created attachment 663604[details][diff][review]
patch v4
Thanks for the reviews, Jared and Margaret.
Regarding creating the child elements the aboutPermissions way - I couldn't get that to work. Given that the way I'm doing it is the same as in popupNotifications.jsm, maybe we can fix both in a follow-up.
Also, I would use :last-of-type, but the patch for bug 793338 uses padbottom as well, and in ways last-of-type wouldn't work.
Dietrich - I'd appreciate a review for XBL best practices and whatnot.

Comment on attachment 664689[details][diff][review]
patch v5
>+ var centerAction = notification.options.centerActions[0];
>+
>+ ok(centerAction.message == "Second Test", "Test 21c, found center action for the Second Test plugin");
>+ // fake having clicked on the "Activate" button for the Test plugin
>+ centerAction.callback();
>+ notification.options.centerActions.splice(notification.options.centerActions.indexOf(centerAction), 1);
Did you want to change this instance as well?
>diff --git a/toolkit/content/PopupNotifications.jsm b/toolkit/content/PopupNotifications.jsm
>--- a/toolkit/content/PopupNotifications.jsm
>+++ b/toolkit/content/PopupNotifications.jsm
>@@ -466,16 +466,21 @@ PopupNotifications.prototype = {
> if (bo.height == 0 && bo.width == 0)
> anchorElement = selectedTab; // hidden
> } else {
> anchorElement = selectedTab; // null
> }
>
> this._currentAnchorElement = anchorElement;
>
>+ let doc = this.window.document;
>+ let arrow = doc.getAnonymousElementByAttribute(this.panel, "anonid", "arrow");
>+ // On OS X and Linux we need a different panel arrow color for
>+ // click-to-play plugins, so copy the popupid and use css.
>+ arrow.setAttribute("popupid", this.panel.firstChild.getAttribute("popupid"));
This code shouldn't be calling getAnonymousElementByAttribute as it relies on the internal structure of the panel. You look like you're using it only to style the arrow, but you could do that with a descendant/child selector.
All the changes you made to the toolkit/*css files look like they don't belong there. They should be in browser/themes somewhere, as they apply to a notification that is specific to the browser implementation.
Did you mean to add a pluginBlocked-64.png for linux as well?
>diff --git a/toolkit/themes/pinstripe/global/popup.css b/toolkit/themes/pinstripe/global/popup.css
>--- a/toolkit/themes/pinstripe/global/popup.css
>+++ b/toolkit/themes/pinstripe/global/popup.css
>@@ -80,16 +80,20 @@ panel[type="arrow"][side="right"] {
>+.panel-arrow[side="top"][popupid="click-to-play-plugins"] {
>+ list-style-image: url("chrome://global/skin/arrow/panelarrow-light-vertical.png");
>+}
>+
You also need to support this for side="bottom" when the panel opens upwards, for instance when the window is near the bottom of the screen.

Created attachment 665124[details][diff][review]
patch v6
Thanks for the review, again. I'm pretty sure I addressed everything you mentioned (of course, that's what I said last time...)
With respect to pluginBlocked-64.png for linux, I'm waiting on shorlander for that. In the meantime, it doesn't actually matter because from what I understand, linux will just pick up the windows one if it doesn't have one of its own.

The code and test changes look ok. I'd want someone else to look at the style changes, especially those that relate to popup-notification-menubutton as I don't understand what the ramifications of those changes are.

(In reply to Jared Wein [:jaws] from comment #24)
> I'm seeing a mismatch of the icon on Windows. Is there plans to get a blue
> icon added to the panel for cases where there isn't a vulnerable/out-of-date
> plugin? See http://screencast.com/t/68HQMswW for a screenshot on Windows.
I'm punting on that, for now. At the moment, unless users turn on plugins.click_to_play, they'll only ever see this ui if we've blocklisted a plugin, in which case we show the correct icon. When click-to-play (non-blocklist style) becomes a full feature of its own, it'll need a non-security icon.
> > +.click-to-play-plugins-notification-separator {
> > + border-left: 1px solid hsla(211,79%,6%,.1);
>
> Should this be -moz-border-start?
This is actually for a zero-width element, but I suppose that's best.
> > +.center-item-button {
> > + min-width: 0;
>
> What does this achieve?
Buttons seem to have a min-width set, so without this, the center item buttons are too wide.
> ::: browser/themes/pinstripe/browser.css
> @@ +3533,5 @@
> > + padding-top: 16px;
> > + -moz-padding-end: 12px;
> > + -moz-padding-start: 20px;
> > + border-bottom-left-radius: 5px; /* oh dear... */
> > + border-top-left-radius: 5px; /* what do? */
>
> I'm not sure what these comments are for, but this looks like it is missing
> RTL support.
>
> If you need RTL, you can do:
> > .click-to-play-plugins-notification-icon-box:-moz-locale-dir(ltr) {
> > }
> > .click-to-play-plugins-notification-icon-box:-moz-locale-dir(rtl) {
> > }
Thanks - that's exactly why I left those comments in there and then promptly forgot about them :)

Comment on attachment 666127[details][diff][review]
patch v7
Review of attachment 666127[details][diff][review]:
-----------------------------------------------------------------
I have a few concerns about the button style rules, but it's mostly just a concern about making our styles more intuitive to read. I tested this out on OSX and tried to look for gotchas, but it seems good to me. Some of the padding values seem a little odd, but I trust that shorlander has verified that the pixels are where they should be :)
Jared, can you test out the focus styles on Windows to make sure those look okay?
::: browser/base/content/urlbarBindings.xml
@@ +1426,5 @@
> + xbl:inherits="src=itemicon"/>
> + <xul:description class="center-item-label"
> + xbl:inherits="xbl:text=itemtext"/>
> + <xul:spacer flex="1"/>
> + <xul:button class="popup-notification-menubutton center-item-button"
Drive-by on the XBL: This class name doesn't really make sense because this isn't a menubutton. If we want to apply the same styles, I think it would make more sense to make common popup-notification-button styles, then we can do special things for type="menu-button" where necessary.
I know that we want to land this bug ASAP, so I don't think this is important enough to block the bug, but it could be a good follow-up to file.
::: browser/themes/pinstripe/browser.css
@@ +3534,5 @@
> + list-style-image: url("chrome://global/skin/arrow/panelarrow-light-vertical.png");
> +}
> +
> +.click-to-play-plugins-notification-content {
> + margin: -16px;
Why do we need this negative margin? (Same question applies to the same rule in the other CSS files.)
::: toolkit/themes/pinstripe/global/notification.css
@@ -126,5 @@
>
> -.popup-notification-menubutton:not([type="menu-button"]) {
> - padding: 2px 9px;
> -}
> -
I guess we don't have any .popup-notification-menubutton buttons that aren't type="menu-button" now that we always have a "Not now" secondary action. This was probably added before that happened.

Thanks for the review, Margaret.
I agree about the button classes/css issues - I'll file a follow-up.
The negative margin is because the panel that contains the popup content has a padding of 16, whereas the design calls for no padding there. Rather than change that and every other popup, I opted for this popup to use a negative margin to basically override that. I imagine there's a way to do this with child/descendant selectors and whatnot, but maybe this could be another follow-up. (for reference, that css is here: http://dxr.lanedo.com/mozilla-central/toolkit/themes/pinstripe/global/popup.css.html#l69 )

(In reply to David Keeler from comment #29)
> The negative margin is because the panel that contains the popup content has
> a padding of 16, whereas the design calls for no padding there. Rather than
> change that and every other popup, I opted for this popup to use a negative
> margin to basically override that. I imagine there's a way to do this with
> child/descendant selectors and whatnot, but maybe this could be another
> follow-up. (for reference, that css is here:
> http://dxr.lanedo.com/mozilla-central/toolkit/themes/pinstripe/global/popup.
> css.html#l69 )
Cool, thanks for the clarification. I just wanted to make sure we weren't covering up some other bizarre issue.

(In reply to Jared Wein [:jaws] from comment #31)
> I'm not seeing any focus issues on Windows. I think this still needs r+ from
> Neil though.
Neil (In reply to Neil Deakin from comment #23)
> The code and test changes look ok. I'd want someone else to look at the
> style changes, especially those that relate to popup-notification-menubutton
> as I don't understand what the ramifications of those changes are.
I think Neil as good as r+'d it. Plus, I've been told he's away for the next two weeks. I think we've had enough eyes on this and it's ready to land (here's a try run from last week, by the way: https://tbpl.mozilla.org/?tree=Try&rev=715fdc15679c )
Jared - do you feel comfortable landing this?