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

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

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.

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

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

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

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?

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.

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]

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.

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

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!

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

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

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

(In reply to neil@parkwaycc.co.uk 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...

Oops.

Geolocation for comparison.

(In reply to neil@parkwaycc.co.uk 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).

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?

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.

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.

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

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

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.

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

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

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

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

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

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?

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

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.

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

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.

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

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

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

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.

(In reply to neil@parkwaycc.co.uk 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.

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

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

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

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

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

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 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 on attachment 780481 [details] [diff] [review] Patch for checkin a=me comm-aurora

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 on attachment 768886 [details] [diff] [review] Updated tests Landed the test fixes as well: https://hg.mozilla.org/comm-central/rev/28c68bdee3e6

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

> All fixed! You forgot suite/themes/classic/mac/navigator/navigator.css... Reopen and fix here?

Doh!

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.

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.

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

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

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

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

Comment on attachment 785889 [details] [diff] [review] OS X theme patch Pushed: https://hg.mozilla.org/releases/comm-beta/rev/c329dddc0afb

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