798278 - Implement multiple plugin doorhanger UI (Port Bug 797677 and Bug 754472)

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Philip Chee]

From Bug 797677 Comment 0:
> Loading the testcase triggers:

> JavaScript error: chrome://browser/content/browser.js, line 3019: 
> NS_ERROR_FAILURE: Failure
> 
> This corresponds to line 40 of browser-plugins.js:
> 
> 40     let navMimeType = navigator.mimeTypes[tagMimetype];
> 
> This line does the wrong thing if tagMimeType is a number, or any property that 
> exists on navigator.mimeTypes ("__proto__", "watch", "length", etc).

Comment 1

[Philip Chee]

Bug 797677 applies on top of Bug 754472 (click-to-play: implement multiple plugin doorhanger ui)

Comment 2

[Frank Wein [:mcsmurf]]

BTW: Bug 754472 includes string changes, so in case we want to get this in for any future release, we should land those string changes first.

Comment 3

[Frank Wein [:mcsmurf]]

For reference:
Bug 754472 checkin: https://hg.mozilla.org/mozilla-central/rev/644aac25b7ab
Bug 797677 checkin: https://hg.mozilla.org/mozilla-central/rev/ca5f40f6edc6

Comment 4

[Frank Wein [:mcsmurf]]

Attachment: First attempt at patch

Patch does not work yet, I need to find out why.

Comment 5

[Frank Wein [:mcsmurf]]

Actually I see now why it's not working, fixing...

Comment 6

[[e-mail]]

Comment on attachment 688483 [details] [diff] [review]
First attempt at patch

>+              var overlay = doc.getAnonymousElementByAttribute(plugin, "class", "mainBox");
>+              var pluginName = getPluginInfo(plugin).pluginName;
>+              var messageString = this._stringBundle.formatStringFromName("PluginClickToPlay", [pluginName], 1);
>+              var overlayText = doc.getAnonymousElementByAttribute(plugin, "class", "msg msgClickToPlay");
>+              overlayText.textContent = messageString;
>+              if (event.type == "PluginVulnerableUpdatable" ||
>+                  event.type == "PluginVulnerableNoUpdate") {
>+                var vulnerabilityString = this._stringBundle.GetStringFromName(event.type);
>+                var vulnerabilityText = doc.getAnonymousElementByAttribute(plugin, "anonid", "vulnerabilityStatus");
>+                vulnerabilityText.textContent = vulnerabilityString;
>+              }
Wasn't this supposed to be fixed by another bug?

Comment 7

[Frank Wein [:mcsmurf]]

Attachment: Second attempt at patch

Still does not work, but it works better than the first attempt :)
I get these two errors when trying to activate a plugin:
JavaScript strict warning: chrome://communicator/content/bindings/notification.x
ml, line 3195: reference to undefined property action.warn
JavaScript error: chrome://communicator/content/bindings/notification.xml, line
1316: this.getPluginsByName is undefined

I need to check if the first error has to do with the second error, the getPluginsByName callback actually looks to me. But I have to see.

Neil: The code you quoted displays a message in the popup notification that a plugin is out-of-date/vulnerable. And yes, this actually displays this twice then: Once in the popup notification and once in the plugin UI itself.

Comment 8

[[e-mail]]

What happens once you only have one type of plugin left to activate?

UI comparisons:

1. Current Suite UI:
[Generic] Would you like to activate the plugins on this page?
[ Icon  ]                      [Activate Plugins][v]
                               [Always Activate plugins for this site]
                               [Never Activate plugins for this site ]


2. Current Firefox UI (? - attachment might have been mock up)
[Generic] Would you like to activate plugins on this page?
[ Icon  ]
          [Icon] <Name of plugin>     [Activate]
          [Icon] <Name of plugin>     [Activate]
          [Icon] <Name of plugin>     [Activate]
                       [Activate All Plugins][v]
                       [Always Activate plugins for this site]
                       [Never Activate plugins for this site ]

3. Possible UI:
[Generic] Would you like to activate the plugins on this page?
[ Icon  ]                  [Activate All Plugins][v]
                           [Activate <Name of plugin>]
                           [Activate <Name of plugin>]
                           [Activate <Name of plugin>]
                           [Always Activate plugins for this site]
                           [Never Activate plugins for this site ]

4. Possible UI:
[Generic] Would you like to activate the plugins on this page?
[ Icon  ]                      [Activate Plugins][v]
                               [Always Activate plugins for this site]
                               [Never Activate plugins for this site ]
[ Icon  ] Would you like to activate <Name of plugin> on this page?
                                               [Activate]
[ Icon  ] Would you like to activate <Name of plugin> on this page?
                                               [Activate]
[ Icon  ] Would you like to activate <Name of plugin> on this page?
                                               [Activate]

Comment 9

[Frank Wein [:mcsmurf]]

Attachment: Updated tests

This patch updates browser_pluginnotification.js and mostly synces it with the Firefox version. I skipped test18f and test18g from bug 832481 as I was not sure if we want this.
Maybe other tests (especially the browser_pluginplaypreview.js test) also need updating, for now I did this only as it's already quite large and that one is responsible for testing the whole plugin GUI.

Comment 10

[Frank Wein [:mcsmurf]]

Attachment: Patch v1

Almost ready for review, need to check a few minor details. This patch basically copies the Firefox UI.

Comment 11

[Frank Wein [:mcsmurf]]

Attachment: Patch v2

So, this patch ports the multiple plugin doorhanger UI (Bug 754472) and also ports Bug 820497 (need to display the CTP UI more often to fix pages which asynchronously add plugin objects; that's the removal of the clickToPlayPromptShown field/attribute).
The patch is quite large, but I don't see any way to avoid that. You probably also want to review the test patch attached to this bug, at least you might want to use it for the automated tests. I basically ported all test changed from the last few month to the SeaMonkey version of the test.
To your question in Comment 8: The patch creates a GUI that looks and behaves like the Firefox one.

To the patch itself: The so-called centerActions are the single items (here: plugin types) inside the new plugin doorhanger notification UI. 
The activateSinglePlugin method was changed so that it will remove the plugin doorhanger UI (and, if other plugins are left, will add the UI again) when a plugin has been activated.
The pluginNeedsActivationExceptThese method is a helper method that's used by various methods to determinate if there are still plugins on the page that can be activated.
The getPluginsByName method is a helper method to get all object/plugins with a certain name on a page. That method is used to activate all plugins of a certain type with the new doorhanger UI (e.g. all objects that use the Adobe Flash plugin).
The makeCenterActions method creates all center actions for the new plugin doorhanger UI, it iterates over all plugins that can be activated (vulnerable plugins, click-to-play plugins) and adds the "Check for updates..." link to the center item when a plugin is vulnerable and can be updated.
The PluginBindingAttached event handler was modified so that with the new UI it will display a bit more information when an object/plugin is click-to-play; when it uses a vulnerable plugin it displays a warning string so that the user knows about that.
The pageshow event handler uses now the new pluginNeedsActivationExceptThese helper method, nothing else should have changed.
About the theme CSS changed: I mostly copied those from Firefox, it looks fine with the Classic and Modern theme. I must admit that I cannot explain why every CSS rule in there is needed, but it looks fine on Windows.
All plugin CTP UI now pass (with the ported test changed from the other patch).
Feel free to ask any questions!

Comment 12

[Frank Wein [:mcsmurf]]

> All plugin CTP UI now pass (with the ported test changed from the other patch).

Make that: "All plugin CTP UI tests now pass (with the ported test changed from the other patch)."

Comment 13

[[e-mail]]

Comment on attachment 750470 [details] [diff] [review]
Patch v2

Haven't looked at the CSS yet.

>diff --git a/suite/browser/urlbarBindings.xml b/suite/browser/urlbarBindings.xml
>--- a/suite/browser/urlbarBindings.xml
>+++ b/suite/browser/urlbarBindings.xml
>-
> </bindings>
Nit: unnecessary change

>+            var pluginNeedsActivation = this.pluginNeedsActivationExceptThese([aPlugin]);
>+            this.removePluginClickToPlayPrompt();
>+
>+            if (pluginNeedsActivation)
>+              this.showPluginClickToPlayPrompt();
Why do you need to remove it and then show it again?

>+            var pluginNeedsActivation = this.contentWindowUtils
>+                                            .plugins.some(function(aPlugin) {
>+              var objLoadingContent = aPlugin.QueryInterface(Components.interfaces.nsIObjectLoadingContent);
>+              return (this.canActivatePlugin(objLoadingContent) &&
>+                      aExceptThese.indexOf(aPlugin) < 0);
>+            }.bind(this));
return this.contentWindowUtils.plugins.some(function(aPlugin) {
   ...
}, this);

>+              if (event.type == "PluginVulnerableUpdatable" ||
>+                  event.type == "PluginVulnerableNoUpdate") {
This is the wrong check; we don't have any code that maps the pluginFallbackType to a string yet. (Also you could put this code in the previous case block.)

Comment 14

[Frank Wein [:mcsmurf]]

Attachment: Patch v3

I've now ported the getBindingType helper function from Bug 800018 over to notification.xml to get the correct string for the in-content CTP UI when a plugin is marked as vulnerable. I'm not sure if this isn't a bit of overkill, but this function might be useful in the future, so maybe it's worth it. The function is not called that often anyway.
I need to call this.removePluginClickToPlayPrompt(); and after that this.showPluginClickToPlayPrompt(); to remove the already activated plugin from the centerActions list inside the notification. There's no way to remove a single centerAction from a notification (though I think that could be added in case this would be a performance critical code).

Comment 15

[[e-mail]]

Comment on attachment 750708 [details] [diff] [review]
Patch v3

>             case nsIObjectLoadingContent.PLUGIN_VULNERABLE_UPDATABLE:
>               var updateLink = doc.getAnonymousElementByAttribute(plugin, "class", "checkForUpdatesLink");
>               this.addLinkClickCallback(updateLink, this.openURLPref, "plugins.update.url");
>               // fall through
>             case nsIObjectLoadingContent.PLUGIN_VULNERABLE_NO_UPDATE:
>+              var overlay = doc.getAnonymousElementByAttribute(plugin, "class", "mainBox");
>+              var eventType = this.getBindingType(plugin);
>+              var pluginName = this.getPluginInfo(plugin).pluginName;
>+              var messageString = this._stringBundle.formatStringFromName("PluginClickToActivate", [pluginName], 1);
>+              var overlayText = doc.getAnonymousElementByAttribute(plugin, "class", "msg msgClickToPlay");
>+              overlayText.textContent = messageString;
>+              if (eventType == "PluginVulnerableUpdatable" ||
>+                  eventType == "PluginVulnerableNoUpdate") {
>+                var vulnerabilityString = this._stringBundle.GetStringFromName(eventType);
>+                var vulnerabilityText = doc.getAnonymousElementByAttribute(plugin, "anonid", "vulnerabilityStatus");
>+                vulnerabilityText.textContent = vulnerabilityString;
>+              }
Nit: eventType will always be one of those two strings at this point.

Comment 16

[[e-mail]]

(In reply to Frank Wein from comment #14)
> I need to call this.removePluginClickToPlayPrompt(); and after that
> this.showPluginClickToPlayPrompt(); to remove the already activated plugin
> from the centerActions list inside the notification. There's no way to
> remove a single centerAction from a notification (though I think that could
> be added in case this would be a performance critical code).

Unfortunately this breaks the notificationbar version (browser.doorhanger.enabled = false).

Comment 17

[[e-mail]]

(In reply to [e-mail] from comment #16)
> (In reply to Frank Wein from comment #14)
> > I need to call this.removePluginClickToPlayPrompt(); and after that
> > this.showPluginClickToPlayPrompt(); to remove the already activated plugin
> > from the centerActions list inside the notification. There's no way to
> > remove a single centerAction from a notification (though I think that could
> > be added in case this would be a performance critical code).
> 
> Unfortunately this breaks the notificationbar version
> (browser.doorhanger.enabled = false).

Actually, activating all plugins doesn't work either; that needs to be fixed first...

Comment 18

[[e-mail]]

Attachment: Modern screenshot

Oops.

Comment 19

[[e-mail]]

Attachment: Modern screenshot

Geolocation for comparison.

Comment 20

[[e-mail]]

(In reply to [e-mail] from comment #17)
> Actually, activating all plugins doesn't work either

So, there are two differences between the new centre actions doorhanger and the notification bar:
1. The doorhanger must be updated each time a plugin is activated. This is done by hiding and showing it. However this is incorrect behaviour for the notificationbar.
2. The doorhanger must be updated each time a plugin is added to the document. This is done by showing it. However this is also incorrect behaviour for the notificationbar.

The removal of the doorhanger in case 1. appears to be unnecessary, therefore this should be changed to show the doorhanger if there are still plugins to activate, otherwise hide it. This reduces case 1. to case 2. which is readily solved by adding an already showing check to the existing notification, along the lines of the geolocation check (for example).

Comment 21

[Frank Wein [:mcsmurf]]

Attachment: Patch v4

Ok, now this is better. The border in modern theme is visible and the small triangle at the top also looks correct now. I removed the negative margin in the CSS file and reduced the padding a bit in the bottom <hbox> (the one with Activate All Plugins). The Modern version now matches the Default Theme version regarding position more or less. So far only tested on Windows, need to check regarding positioning on other platforms (does the Modern theme have any platform-specific rules like other padding/margin/etc.?)
I've also modified the code to get the notification bar version working. It works as expected, except one thing maybe: When you have two plugin object on a page, close the notification bar and then press the "Play" button on one plugin object, the notification bar opens again. So maybe the notification bar should keep its state per page load?

Comment 22

[Frank Wein [:mcsmurf]]

Attachment: Screenshot with Default and Modern theme doorhanger

Here I've positioned the plugin doorhanger UI of the Default and the Modern theme in two layers in GIMP and added some transparency. I've used the top side and the left side to position the Modern version. So here you can see how far the positioning and size of elements in the two themes are still different. It's still a bit different, maybe need to change the padding a bit.

Comment 23

[Frank Wein [:mcsmurf]]

Regarding the Close button: Hrm, I forgot that one. I've noticed Firefox has no close button either, I filed Bug 875724 on that. There was some discussion on removing the close button on all doorhanger UIs.

Comment 24

[Philip Chee]

> There was some discussion on removing the close button on all doorhanger UIs.
And before that there was a bug that added a close button to all doorhangers.

Comment 25

[[e-mail]]

(In reply to Frank Wein from comment #21)
> Ok, now this is better. The border in modern theme is visible and the small
> triangle at the top also looks correct now.
Good!

> (does the Modern theme
> have any platform-specific rules like other padding/margin/etc.?)
No, it's the same on all platforms.

> I've also modified the code to get the notification bar version working. It
> works as expected, except one thing maybe: When you have two plugin object
> on a page, close the notification bar and then press the "Play" button on
> one plugin object, the notification bar opens again. So maybe the
> notification bar should keep its state per page load?
Good catch, I hadn't spotted that one; you might have to keep the this.clickToPlayPromptShown variable after all.

Comment 26

[[e-mail]]

Comment on attachment 754601 [details] [diff] [review]
Patch v4

Is there an easy way to test vulnerable plugins?

>+              return (this.canActivatePlugin(objLoadingContent) &&
>+                      aExceptThese.indexOf(aPlugin) < 0);
Nit: outer set of ()s is unnecessary.

>+      <method name="makeCenterActions">
Nit: only makes sense for popup notifications, so can be moved to nearer the caller (e.g. line 2540).

>+              if (eventType == "PluginVulnerableUpdatable" ||
>+                  eventType == "PluginVulnerableNoUpdate") {
Nit: eventType has to be one of these for us to get here in the first place.

>+         <xul:image class="popup-notification-icon"
>+                     xbl:inherits="popupid,src=icon"/>
Nit: alignment incorrect

>+        if (item != null) {
Nit: if (item)

>+  background: hsla(0,0%,100%,.4);
>+  -moz-border-end: 1px solid hsla(0,0%,100%,.2);
Nit: space after comma (all over)
Nit: I'd prefer rgba colours

>+  background-image: url("chrome://browser/skin/click-to-play-warning-stripes.png");
SeaMonkey does not have a chrome://browser package, nor does it have a PNG of that name anywhere in its skins.

>+.center-item-warning > .text-link {
>+  color: #3d8cd7;
[Odd choice of colour?]

>+  min-width: 0;
Nit: I prefer 0px

>+  background: hsla(0,0%,100%,.4);
>+  -moz-border-end: 1px solid hsla(0,0%,100%,.2);
I only have 16-bit colour at the moment so I still need to try this out and see whether the colours end up feeling Modern or not.

Comment 27

[[e-mail]]

Comment on attachment 754601 [details] [diff] [review]
Patch v4

[Minusing for this and previous review comments.]

>diff --git a/suite/themes/modern/navigator/navigator.css b/suite/themes/modern/navigator/navigator.css
>--- a/suite/themes/modern/navigator/navigator.css
>+++ b/suite/themes/modern/navigator/navigator.css
>@@ -565,16 +565,120 @@ toolbar[mode="icons"] #search-button > .
> }
> 
> #plugins-notification-icon {
>   list-style-image: url("chrome://mozapps/skin/plugins/pluginGeneric-16.png");
>   width: 16px;
>   height: 16px;
> }
> 
>+.click-to-play-plugins-notification-icon-box {
>+  background: hsla(0,0%,100%,.4);
Nit: background-color:
I think this should probably be #DDE3EB;

>+  -moz-border-end: 1px solid hsla(0,0%,100%,.2);
>+  padding-top: 16px;
>+  -moz-padding-end: 16px;
>+  -moz-padding-start: 24px;
>+}
>+
>+.click-to-play-plugins-notification-icon-box:-moz-locale-dir(ltr) {
>+  border-bottom-left-radius: 4px;
>+  border-top-left-radius: 4px;
>+}
>+
>+.click-to-play-plugins-notification-icon-box:-moz-locale-dir(rtl) {
>+  border-bottom-right-radius: 4px;
>+  border-top-right-radius: 4px;
Border radius makes no sense in the Modern theme doorhanger.

>+  background-color: hsla(211,79%,6%,.05);
Not sure about this one, maybe #BDC7D6 will work.

Comment 28

[Frank Wein [:mcsmurf]]

Neil: I want to add the click-to-play-warning-stripes.png file to the modern theme, that file is used as background image in the doorhanger UI when a vulnerable plugin will be used. What place should I put this file in? I did not find a good place for this file so far, directly under chrome://navigator/skin/ maybe?
Another question on the colors: For the color conversation from hsla to rgb, did you use an image program to get the new rgb colors? Just wondering how you handled the transparency in the hsla colors.

Comment 29

[[e-mail]]

(In reply to Frank Wein from comment #28)
> Neil: I want to add the click-to-play-warning-stripes.png file to the modern
Not just Modern; it's not in toolkit so Classic doesn't have it either.

> theme, that file is used as background image in the doorhanger UI when a
> vulnerable plugin will be used. What place should I put this file in? I did
> not find a good place for this file so far, directly under
> chrome://navigator/skin/ maybe?
What do these stripes look like? Compare chrome://mozapps/skin/extensions/stripes-error.png or alternatively maybe you can use a repeating linear gradient.

> Another question on the colors: For the color conversation from hsla to rgb,
> did you use an image program to get the new rgb colors? Just wondering how
> you handled the transparency in the hsla colors.
For the Modern theme the colours I suggested were supposed to be solid. (And I forgot to consider the border colours.)

Comment 30

[Philip Chee]

> Another question on the colors: For the color conversation from hsla to rgb, did you
> use an image program to get the new rgb colors? Just wondering how you handled the
> transparency in the hsla colors.
The A in HSLA is the Alpha channel.

Comment 31

[Frank Wein [:mcsmurf]]

Attachment: Patch v5

Neil: This patch should now include fixes for all the review comments
- I added back the clickToPlayPromptShown field, it will display the notification bar exactly once per page load (if required)
- I converted the new CSS rules to RGBA colors in classic theme
- I used RGB colors in modern theme (I used GIMP to get the color values via a screenshot from the hsla colors)
- Removed the special link color from the theme, it looks ok to me without that special link color (that's used when a vulnerable, updatable plugin is discovered)
- Added the stripes png file to modern and classic theme; using the other stripes png file and downscaling it looked a bit strange; using CSS repeated linear gradients is not possible afaik as I would need to combine two linear gradients (left to right for the stripes and top to bottom for the color fading)
- Moved makeCenterActions to a better place
- Removed the "if (eventType == "PluginVulnerableUpdatable" || [...]" code

Comment 32

[[e-mail]]

(In reply to Frank Wein from comment #31)
> - I used RGB colors in modern theme (I used GIMP to get the color values via
> a screenshot from the hsla colors)
Actually I had deliberately used the nearest existing Modern colours I could find.

Comment 33

[Frank Wein [:mcsmurf]]

Attachment: Patch v6

This patch uses the suggested (already existing) background colors from Modern. I've adjusted the border colors using the HSV color system in GIMP. I used the background color from the element inside the border and raised the V value (=brightness) by 4. This way the border color looks like how it should look like :)

Comment 34

[[e-mail]]

I happened to notice a JavaScript error on line 2161 of notification.xml, I assume this was because a plugin crashed but our code is out of date, and the change is not relevant here?

Comment 35

[[e-mail]]

Ah, that's fallout from bug 648675, filed by none other than our own KaiRo...

Comment 36

[Frank Wein [:mcsmurf]]

Yeah, that's going to be fixed in Bug 870290. The patch there conflicts with the patch here (regarding patch apply), so I wanted to fix this bug here first.

Comment 37

[[e-mail]]

(In reply to Frank Wein from comment #36)
> Yeah, that's going to be fixed in Bug 870290. The patch there conflicts with
> the patch here (regarding patch apply), so I wanted to fix this bug here
> first.

Ideally that patch would land on branches, while this one can't because it has string changes. The JS error was in the bottomLink code which doesn't rely on this patch being applied, so is it just a case of rebasing the patches to apply the other way around?

Comment 38

[Frank Wein [:mcsmurf]]

Actually I wanted to land the patch from this bug here on -aurora branch later, is this not possible? But yeah, I can look up what's up with that JS error.

Comment 39

[[e-mail]]

(In reply to Frank Wein from comment #38)
> Actually I wanted to land the patch from this bug here on -aurora branch
> later, is this not possible? But yeah, I can look up what's up with that JS
> error.

-aurora maybe with late-l10n, but not beta (if we get that far...)

The patch in 870290 removes the line with the JS error.

Comment 40

[[e-mail]]

Comment on attachment 758471 [details] [diff] [review]
Patch v6

Only tried this with doorhangers so far, and haven't checked the Modern changes yet.

>+            return {mimetype: mimetype,
>+                    pluginsPage: pluginsPage,
>+                    pluginName: pluginName };
Nit: The space before the } is obviously inconsistent with the {.
I couldn't really find anything suitable to compare with.
The nearest was aboutSessionRestore.js which starts a new line after the { i.e.
return {
  mimetype: mimetype,
  pluginsPage: pluginsPage,
  pluginName: pluginName
};
But just putting a space after the { (and aligning the other lines) would be OK.

>+            // Only show notification bar once per page load, make sure to not add it again on new plugin objects
>+            // or when playing a CTP plugin object
>+            if (this.clickToPlayPromptShown)
>+              return;
>+
>             this.clickToPlayPromptShown = true;
>-
Nit: no need to delete this line.
Once per page load isn't actually correct, in particular I believe Vimeo works by having a plugin that displays the poster image which creates a second plugin that plays the actual video, so it's necessary to remember if the notification was hidden by plugin activation, in case a new plugin needs to be activated.

>+              var objLoadingContent = aPlugin.QueryInterface(Components.interfaces.nsIObjectLoadingContent);
>+              return this.canActivatePlugin(objLoadingContent) &&
>+                     aExceptThese.indexOf(aPlugin) < 0;
Nit: test for exceptions first, it's quicker.
[We should really make canActivatePlugin accept a plugin element and make it do the QI.]

>+      <method name="getPluginsByName">
>+        <parameter name="aDOMWindowUtils"/>
>+        <parameter name="aName"/>
>+        <body>
>+          <![CDATA[
>+            /* Gets all plugins currently in the page of the given name */
>+            var plugins = [];
>+            for (let plugin of aDOMWindowUtils.plugins) {
>+              var objLoadingContent = plugin.QueryInterface(Components.interfaces.nsIObjectLoadingContent);
>+              if (this.canActivatePlugin(objLoadingContent)) {
>+                var pluginName = this.getPluginInfo(plugin).pluginName;
>+                if (aName == pluginName) {
>+                  plugins.push(objLoadingContent);
>+                }
>+              }
>+            }
>+
>+            return plugins;
Originally I wondered why this didn't use Array.filter.
Then I wondered why it didn't use this.contentWindowUtils.
Then I wondered why it even existed at all, since there is only one call i.e.
callback: (function(aName) {
  for (let plugin of aDOMWindowUtils.plugins) {
    var objLoadingContent = plugin.QueryInterface(Components.interfaces.nsIObjectLoadingContent);
    if (this.canActivatePlugin(objLoadingContent) &&
        this.getPluginInfo(plugin).pluginName == aName)
      objLoadingContent.playPlugin();
etc.

>             case nsIObjectLoadingContent.PLUGIN_VULNERABLE_NO_UPDATE:
>+              var overlay = doc.getAnonymousElementByAttribute(plugin, "class", "mainBox");
Not used?

>+              var pluginName = this.getPluginInfo(plugin).pluginName;
>+              var messageString = this._stringBundle.formatStringFromName("PluginClickToActivate", [pluginName], 1);
>+              var overlayText = doc.getAnonymousElementByAttribute(plugin, "class", "msg msgClickToPlay");
>+              overlayText.textContent = messageString;
This probably belongs in setupPluginClickToPlay so that all CTP plugins get the right string.

>+              let updateLink = Services.urlFormatter.formatURLPref("plugins.update.url");
No point fetching the url unless the plugin is updatable.

>+vulnerablePluginsMessage=Some plugins have been deactivated for your safety.
Not used, but maybe it's supposed to be?

Comment 41

[[e-mail]]

Attachment: canActivatePlugin untested example changes

Comment 42

[[e-mail]]

Here's a link that demonstrates dynamically created plugins:

http://support.brightcove.com/en/video-cloud/docs/dynamically-loading-player-using-javascript

1. Click Add Player. The notificationbar appears.
2. Click Activate Adobe Flash. The notificationbar hides.
3. Click Remove Player.
4. Click Add Player. The notificationbar should appear again.

Comment 43

[[e-mail]]

Comment on attachment 758471 [details] [diff] [review]
Patch v6

>diff --git a/suite/themes/modern/navigator/navigator.css b/suite/themes/modern/navigator/navigator.css

>+.click-to-play-plugins-notification-icon-box {
>+  background: #DDE3EB;
Nit: background-color:

>+  -moz-border-end: 1px solid #E6ECF5;
Don't bother with all of these little borders, it doesn't really add anything.

>+  padding-top: 16px;
>+  -moz-padding-end: 16px;
>+  -moz-padding-start: 24px;
Now what's confusing you here is that the popup-notification-icon has a 10px end margin, so ideally you would remove that on the click-to-play notification and then you can be consistent with your padding (either padding: 16px or padding: 24px, I don't mind).

>+.click-to-play-plugins-notification-separator {
>+  -moz-border-start: 1px solid #B3BCC6;
>+  border-top: 1px solid #B3BCC6;
Except this border of course, but change it to #B1BBC5 please.

>+.click-to-play-plugins-notification-description-box {
>+  border-bottom: 1px solid #D2D9E1;
>+  -moz-border-start: 1px solid #D2D9E1;
>+  padding-top: 12px;
>+  -moz-padding-end: 11px;
>+   padding-bottom: 9px;
>+  -moz-padding-start: 10px;
Again, lose the borders, and stick with one padding value please.

>+.click-to-play-plugins-notification-center-box {
>+  border-top: 1px solid #C6D1E0;
>+  border-bottom: 1px solid #C6D1E0;
>+  -moz-border-start: 1px solid #C6D1E0;
>+  background-color: #BDC7D6;
>+}
>+
>+.click-to-play-plugins-notification-button-container {
>+  border-top: 1px solid #D2D9E1;
>+  -moz-border-start: 1px solid #D2D9E1;
>+  margin: 0px;
Again, lose the borders, and also the margin looks useless too.

>+.center-item-box[showseparator="true"] {
>+  border-top: 1px solid #ABB4C2;
Let's go with #B1BBC5 again please.

Comment 44

[[e-mail]]

Comment on attachment 758471 [details] [diff] [review]
Patch v6

XBL: I haven't tried these, it's really hard to use DOM Insepctor on a popup notification...

>+    <content align="center">
Probably don't need the align.

>+      <xul:vbox flex="1" class="center-item-box"
>+                xbl:inherits="warn,showseparator,padbottom">
>+        <xul:hbox flex="1" align="center">

>+        <xul:hbox flex="1" align="center" class="center-item-warning">
Not entirely sure that these flexes are necessary.

>+          <xul:spacer flex="1"/>
Don't need this at the end of a box.

>+    <content align="start" class="click-to-play-plugins-notification-content">
Again, the align seems unnecessary.

>+      <xul:hbox flex="1">
This whole box seems unnecessary.

>+        <xul:vbox class="click-to-play-plugins-notification-icon-box" flex="1">
>+         <xul:image class="popup-notification-icon"
>+                    xbl:inherits="popupid,src=icon"/>
Nit: incorrect indentation. Also the flex may be unnecessary.

>+          <xul:spacer flex="1"/>
Unnecessary spacer.

>+        <xul:vbox flex="1" class="popup-notification-main-box"
Unnecessary flex?

Comment 45

[Philip Chee]

Bah, it looks like Bug 880735 is going to bit rot most of our CTP code. :P

Bug 880735 - Reimplement the plugin doorhanger to deal with the new CtP spec

Comment 46

[Frank Wein [:mcsmurf]]

Hm, I knew that bug existed, but I did not know the API changes will be that big. The Target Milestone is Gecko 24, so looks like they want to land it within this cycle. I guess I'll try to modify my code here in time. Or at least land all string changes from this bug in this cycle.

Comment 47

[Frank Wein [:mcsmurf]]

(In reply to [e-mail] from comment #44)
> Comment on attachment 758471 [details] [diff] [review]
> Patch v6
> 
> XBL: I haven't tried these, it's really hard to use DOM Insepctor on a popup
> notification...

Yeah, I wondered if there is a way to keep the popup notification open. For Firefox I found tricks like:
"- Doorhanger Bonus: to temporarily show arrowpanel popups so they stay visible while you style them:
#identity-popup {
display: -moz-box !important;
-moz-margin-start: 20px !important;}"

but I could not get this CSS trick working with our popup notifications.

Comment 48

[[e-mail]]

(In reply to Frank Wein from comment #46)
> Or at least land all string changes from this bug in this cycle.

Yes please.

Comment 49

[Frank Wein [:mcsmurf]]

I pushed the string changes to comm-central: https://hg.mozilla.org/comm-central/rev/29ef791b915f

Comment 50

[Frank Wein [:mcsmurf]]

Attachment: Updated tests

I had to update the tests to include the fixes from Bug 882858 (new showing notification for doorhangers, have to ignore that in some tests) and Bug 882339 (calling the callback code async now as blocklist state is now cached and gets reset during testrun)

Comment 51

[Frank Wein [:mcsmurf]]

Comment on attachment 768886 [details] [diff] [review]
Updated tests

Forgot mentioning Bug 880675: It removed the .blocklisted attribute from plugin tags again, so I did that in the tests, too.

Comment 52

[Frank Wein [:mcsmurf]]

Attachment: Patch v7

Ok, this is it, (almost) all issues should be fixed now :):
- Fixed align of the { ... }
- Modified the showPluginClickToPlayPrompt function of the notification bar version to cover all cases:
  When there is already an existing plugin notification bar, don't display a second, third, ... one (for example when there are multiple plugins on a page)
  Only display a plugin notification bar when a new plugin has been added (I introduced the aTriggeredByNewPlugin parameter for that); note that the Vimeo example does no longer seem to work though (they fixed their player looks like); this fixes the "notification bar appearing (again) when clicking on the CTP UI" issue
  I used this test page to test that (and the page in Comment 24):
data:text/html,<script>function addElement() {var newdiv = document.createElement('embed');newdiv.setAttribute('type', 'audio/mid'); document.getElementById('test').appendChild(newdiv);}</script><embed type="application/x-java-applet"/><embed type="application/x-shockwave-flash"/><div id="test"><a href="javascript:;" onclick="addElement();">add new plugin</a></div>

- Removed the obsolete clickToPlayPromptShown property, it's no longer needed to due to the changes to showPluginClickToPlayPrompt
- Integrated getPluginsByName into the makeCenterActions callback
- Moved the code that sets the overlay string for the CTP UI to setupPluginClickToPlay (good catch :)
- vulnerablePluginsMessage string now gets used
- Only fetch updateLink when a vulnerable plugin is updatable
- Removed the little borders in the Modern CSS
- Removed a few "flex" and "align" properties, except this one (iirc):
">+      <xul:hbox flex="1">
This whole box seems unnecessary."

Now if I remember correctly, the background (color) on the left side of the doorhanger UI ended right below the icon and the default background color was visible. So I kept the hbox
- Can you explain again the thing with
">+  padding-top: 16px;
>+  -moz-padding-end: 16px;
>+  -moz-padding-start: 24px;"?

I must admit I'm not sure what changes I should apply there.

- The changes from Bug 880735 did not matter at all for our code, they only added some new functions to the core interfaces.
- All (mostly doorhanger-only) tests pass

Comment 53

[[e-mail]]

Comment on attachment 770078 [details] [diff] [review]
Patch v7

Some nits:

There are still several uses of document.getAnonymousElementByAttribute which could perhaps be switched to getPluginUI instead. You're adding at least one more that could be easily switched.

The pageshow event needs to stop checking the click_to_play pref because that doesn't affect vulnerable plugins. It also needs to force the prompt to be shown, otherwise clicking back to a page with plugins won't prompt.

I got worried when the theming didn't work but then I realised that patch doesn't handle binaries and my build had simply forgotten to update any CSS...

Comment 54

[[e-mail]]

(In reply to Frank Wein from comment #52)
> Can you explain again the thing with
> >+  padding-top: 16px;
> >+  -moz-padding-end: 16px;
> >+  -moz-padding-start: 24px;
The icon itself has some default margin from the beginning of that section in navigator.css, which is why you are giving it different padding here to make it approximately centred. If you override the click-to-play plugin icon's margin as well as its size then you can use equal padding here.

Comment 55

[[e-mail]]

Comment on attachment 770078 [details] [diff] [review]
Patch v7

>+            var haveVulnerablePlugin = this.contentWindowUtils.plugins.some(function(plugin) {
>+              return (this.canActivatePlugin(plugin) &&
>+                      (plugin.pluginFallbackType ==
>+                       Components.interfaces.nsIObjectLoadingContent.PLUGIN_VULNERABLE_UPDATABLE ||
>+                       plugin.pluginFallbackType ==
>+                       Components.interfaces.nsIObjectLoadingContent.PLUGIN_VULNERABLE_NO_UPDATE));
>+            }, this);
>+            if (haveVulnerablePlugin) {
>+              messageString = this._stringBundle.GetStringFromName("vulnerablePluginsMessage");
>+            }
Nit: should be written something like this:
var messageString = this._stringBundle.GetStringFromName(haveVulnerablePlugin ? "vulnerablePluginsMessage" : "activatepluginsMessage.title");
(shame about the string naming)

Comment 56

[Frank Wein [:mcsmurf]]

Attachment: Patch for checkin

I restored some padding that I accidentally deleted from patch v6 to patch v7.
The suggestion from Comment 54 was fixed.
Fixes nits from Comment 53.

Callek: I want checkin approval for this patch :)

Comment 57

[Frank Wein [:mcsmurf]]

Comment on attachment 780481 [details] [diff] [review]
Patch for checkin

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: No nice click-to-play GUI
Testing completed (on m-c, etc.): I've tested the patch for many weeks on mozilla-central locally, unfortunately (approval) checkin to mozilla-central is still pending; but time is running out rather soon as the next merge is coming up
Risk to taking this patch (and alternatives if risky): Rather low, although there are a lot of changed. The patch has been tested quite much and I also fixed/added automated tests (see the other patch) to make sure everything is working as expected
String changes made by this patch: -

Comment 58

[Philip Chee]

Comment on attachment 780481 [details] [diff] [review]
Patch for checkin

a=me comm-aurora

Comment 59

[Frank Wein [:mcsmurf]]

Comment on attachment 780481 [details] [diff] [review]
Patch for checkin

Landed the patch after asking ewong if the tree looks somewhat green (it does, Linux is currently red due to some mozilla-central issue that will be fixed today or tomorrow): https://hg.mozilla.org/comm-central/rev/e36c030950bc

Comment 60

[Frank Wein [:mcsmurf]]

Comment on attachment 768886 [details] [diff] [review]
Updated tests

Landed the test fixes as well: https://hg.mozilla.org/comm-central/rev/28c68bdee3e6

Comment 61

[Frank Wein [:mcsmurf]]

Also landed the patches on aurora:
https://hg.mozilla.org/releases/comm-aurora/rev/9eedab7adf4a
https://hg.mozilla.org/releases/comm-aurora/rev/ac21aa1c8876

All fixed! Thanks everyone involved :)

Comment 62

[Stefan [:stefanh]]

> All fixed!

You forgot suite/themes/classic/mac/navigator/navigator.css... Reopen and fix here?

Comment 63

[Frank Wein [:mcsmurf]]

Doh!

Comment 64

[Stefan [:stefanh]]

I see that Firefox now have done this:
http://hg.mozilla.org/mozilla-central/rev/1da2051be457
http://hg.mozilla.org/mozilla-central/rev/c456780d7d1f

So the style rules might be gone from themes/osx,win,linux. I haven't looked at it in detail, so I don't know if all rules are there, but it's probably safe to just copy the style rules from the above changesets.

Comment 65

[Frank Wein [:mcsmurf]]

Unfortunately this is for the "new new plugin doorhanger UI", we've implemented the "old new plugin doorhanger UI" (for now) :). The rules we need are at http://mxr.mozilla.org/comm-beta/search?string=click-to-play-plugins-notification-content (at least for now, due to next uplift this will change I think). The CSS rules are quite different for some elements when looking at the linux/osx/windows css files. I'll probably stick to the mac version and someone (you?) has to tell me if this looks good and fits with the rest of the theme.

Comment 66

[Stefan [:stefanh]]

Yeah, just ask me for r? A quick look at the win/osx files indicates that colors are the same.

Comment 67

[Frank Wein [:mcsmurf]]

Forgot to hg add a test file, pushed a fix to trunk and aurora:
https://hg.mozilla.org/comm-central/rev/1f4e05dd5399
https://hg.mozilla.org/releases/comm-aurora/rev/8d7c23490db0

Comment 68

[Frank Wein [:mcsmurf]]

Attachment: OS X theme patch

This one ports over the required OS X theme CSS changed from the Firefox theme. The UI styling seems to look better compared to just porting Linux/Windows CSS over to the Mac part of theme. I also fixed packaging on the stripes png file, it's the same for all platforms.

Comment 69

[Stefan [:stefanh]]

Comment on attachment 785889 [details] [diff] [review]
OS X theme patch

Looks good, thanks - just one nit:

diff --git a/suite/themes/classic/mac/navigator/navigator.css b/suite/themes/classic/mac/navigator/navigator.css
--- a/suite/themes/classic/mac/navigator/navigator.css
+++ b/suite/themes/classic/mac/navigator/navigator.css
@@ -335,6 +335,121 @@ toolbar[mode="text"] > #window-controls 
   height: 16px;
 }
 
+
+.click-to-play-plugins-notification-content {
+  margin: -16px;
+  border-radius: 5px;
+}

You can get rid of the extra empty line at the top.

Comment 70

[Frank Wein [:mcsmurf]]

Comment on attachment 785889 [details] [diff] [review]
OS X theme patch

[Approval Request Comment]
Regression caused by (bug #): Bug 798278 (this bug)
User impact if declined: Broken looking plugin doorhanger UI on Mac OS X
Testing completed (on m-c, etc.): Works fine on comm-central
Risk to taking this patch (and alternatives if risky): Low risk
String changes made by this patch: -

Comment 71

[Stefan [:stefanh]]

Comment on attachment 785889 [details] [diff] [review]
OS X theme patch

jftr, this has landed on c-c as http://hg.mozilla.org/comm-central/rev/d35d65667c2f

Comment 72

[Frank Wein [:mcsmurf]]

Pushed to aurora: https://hg.mozilla.org/releases/comm-aurora/rev/e4ee0c086864

Comment 73

[Frank Wein [:mcsmurf]]

Comment on attachment 785889 [details] [diff] [review]
OS X theme patch

Pushed: https://hg.mozilla.org/releases/comm-beta/rev/c329dddc0afb

Comment 74

[Frank Wein [:mcsmurf]]

All fixed!
Adjusting status-fixed flag so that the bug will appear in the correct bug queries.