Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Description • 12 years ago

Click-to-play needs to treat different plugins separately. For instance, using the notification bar to enable one plugin (e.g. flash) on a site will not enable other plugin types (e.g. java).

	Tetsuharu OHZEKI [:tetsuharu] (UTC+9)
	Comment 1 • 12 years ago

By design, the current implement of PopupNotifications.jsm need to <xul:image> element per each icon. So if we show icons per each plugin, we need to insert <xul:image> element before showing icons. I think that changing the design of showing icons will be hard. Showing multiple actions in one popup may be easier.

From the viewpoint of usability, I seem that showing icons per each plugins is not good. User do not think much difference of each plugins. Many plugins notification confuse user. I think this bug's proposal will too complex.
If we implement this bug's proposal, it is better that to treat different plugins separately should be hidden preference for advanced user.

	Ian Melven :imelven
	Comment 2 • 12 years ago

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #1)
> By design, the current implement of PopupNotifications.jsm need to
> <xul:image> element per each icon. So if we show icons per each plugin, we
> need to insert <xul:image> element before showing icons. I think that
> changing the design of showing icons will be hard. Showing multiple actions
> in one popup may be easier.
> 
> From the viewpoint of usability, I seem that showing icons per each plugins
> is not good. User do not think much difference of each plugins. Many plugins
> notification confuse user. I think this bug's proposal will too complex.
> If we implement this bug's proposal, it is better that to treat different
> plugins separately should be hidden preference for advanced user.

I'm not sure this bug is about showing a different icon per plugin - in my opinion it's more about clicking on say, an instance of Flash Player only activating Flash Player - not also activating a Java applet that happens to be on the same page. From comments in other bugs, there are definitely some users that do think differently of certain plugins. The UI/UX of click to play overall still needs a lot of discussion/thought and I do agree that many notifications will confuse the user.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 3 • 12 years ago

For the doorhanger, we should show plugin specific terminology to let the user know which plugins they will be activating. For instance:

"Would you like to activate Flash and Silverlight on this page?"
"Would you like to activate Flash on this page?"

We can do this for the doorhanger until we have a better way to split up the separate plugin types when being enabled through the doorhanger.

	Tetsuharu OHZEKI [:tetsuharu] (UTC+9)
	Comment 4 • 12 years ago

Is this duplicate of bug 746374?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 5 • 12 years ago

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #4)
> Is this duplicate of bug 746374?

bug 746374 is this bug :)

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 7 • 12 years ago

This is an initial implementation for this that I'd like to get some feedback on (from jaws in particular and maybe joshmoz if he has time, but if anyone else has any comments, feel free to let me know what you think).

	Jared Wein [:jaws] (please needinfo? me)
	Comment 8 • 12 years ago

Comment on attachment 626249 [details] [diff] [review]
initial implementation

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

Just an FYI, you'll need SR for the interface changes.

::: browser/base/content/browser.js
@@ +7065,5 @@
>          pluginsPage = "";
>        }
>      }
>  
> +    tagMimetype = pluginElement.QueryInterface(Components.interfaces.nsIObjectLoadingContent).actualType;

You can replace Components.interfaces here with Ci.

@@ +7363,5 @@
> +        for (let plugin of cwu.plugins) {
> +          let pluginPermission = Services.perms.testPluginPermission(aBrowser.currentURI, plugin.actualType);
> +          if (pluginPermission != Ci.nsIPermissionManager.DENY_ACTION)
> +            Services.perms.addPlugin(aBrowser.currentURI, plugin.actualType,
> +                                     Ci.nsIPermissionManager.ALLOW_ACTION);

If it's currently DENY_ACTION, how will the user be able to remove DENY_ACTION?

@@ +7395,5 @@
>      let cwu = aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>                              .getInterface(Ci.nsIDOMWindowUtils);
>      for (let plugin of cwu.plugins) {
>        let overlay = doc.getAnonymousElementByAttribute(plugin, "class", "mainBox");
> +      if (overlay) overlay.style.visibility = "hidden";

Place the body of the if-statement on its own line.

::: browser/base/content/test/browser_pluginnotification.js
@@ +6,5 @@
>  var gClickToPlayPluginActualEvents = 0;
>  var gClickToPlayPluginExpectedEvents = 5;
>  
> +function get_test_plugin(name) {
> +  name = (typeof(name) !== 'undefined' ? name : "Test Plug-in");

The outer set of parens here are unnecessary. Were you trying to group the condition?

::: dom/plugins/base/nsPluginTags.cpp
@@ +341,5 @@
> +    Mark(NS_PLUGIN_FLAG_CLICKTOPLAY);
> +  else
> +    UnMark(NS_PLUGIN_FLAG_CLICKTOPLAY);
> +  
> +  mPluginHost->UpdatePluginInfo(nsnull);

Does the propagate the CLICKTOPLAY flag across all plugins of the same type?

::: extensions/cookie/nsPermissionManager.cpp
@@ +368,5 @@
>  }
>  
> +nsresult
> +nsPermissionManager::MimeTypeToPluginType(const char *aMimeType,
> +                                                nsAFlatCString &aPluginType)

nit: please line up the arguments. it looks like nsAFlatCString is aligned with char, but it should be aligned with const.

@@ +372,5 @@
> +                                                nsAFlatCString &aPluginType)
> +{
> +  nsCOMPtr<nsIPluginHost> pluginHostCOM(do_GetService(MOZ_PLUGIN_HOST_CONTRACTID));
> +  nsPluginHost *pluginHost = static_cast<nsPluginHost*>(pluginHostCOM.get());
> +  if (!pluginHost) return NS_ERROR_FAILURE;

It is my understanding that in Gecko, all blocks should have braces and start on their own line.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 9 • 12 years ago

This differentiates plugin permissions by type and vulnerability status. For example, if flash is allowed to always run on a site, java won't automatically activate too. Furthermore, if flash is allowed to always run at one point but is later put on the blocklist, it will not automatically run (until the user clicks "always allow" again).
Currently there's no detailed UI to fiddle with all of this (it just depends on what's present in the page when "always allow" is clicked) - I think that'll be a follow-up for later.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 10 • 12 years ago

Comment on attachment 674037 [details] [diff] [review]
patch

Josh - can you review the changes to the plugin backend code?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 11 • 12 years ago

Comment on attachment 674037 [details] [diff] [review]
patch

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

r- for the .some usage. Everything else seems fine, but I'm not sure what you intended there. Maybe just wanted to use |for (let plugin of cwu.plugins)| ?

::: browser/base/content/browser-plugins.js
@@ +509,5 @@
>      let secondaryActions = [{
>        label: gNavigatorBundle.getString("activatePluginsMessage.always"),
>        accessKey: gNavigatorBundle.getString("activatePluginsMessage.always.accesskey"),
>        callback: function () {
> +        cwu.plugins.some(function(plugin) {

I don't think .some here is what you want. Array.some expects to return a boolean value, and this function only returns undefined.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 12 • 12 years ago

(In reply to Jared Wein [:jaws] from comment #11)
> r- for the .some usage. Everything else seems fine, but I'm not sure what
> you intended there. Maybe just wanted to use |for (let plugin of
> cwu.plugins)| ?

Derp. Not sure what I meant either.
I also added another test case that I thought of.
I'd appreciate another look - thanks!

	Jared Wein [:jaws] (please needinfo? me)
	Comment 13 • 12 years ago

Comment on attachment 676315 [details] [diff] [review]
patch v2

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

::: dom/plugins/base/nsIPluginHost.idl
@@ +81,5 @@
>    void registerPlayPreviewMimeType(in AUTF8String mimeType);
>  
>    void unregisterPlayPreviewMimeType(in AUTF8String mimeType);
> +
> +  ACString getPermissionStringForType(in string mimeType);

The other two functions here take AUTF8Strings, why does this one take a |string|?

::: dom/plugins/base/nsPluginHost.cpp
@@ +1339,5 @@
> +NS_IMETHODIMP
> +nsPluginHost::GetPermissionStringForType(const char *aMimeType, nsACString &aPermissionString)
> +{
> +  aPermissionString.Truncate();
> +  uint32_t blistState;

s/blistState/blocklistState/

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 14 • 12 years ago

I addressed your comments and refactored some things.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 15 • 12 years ago

Comment on attachment 683812 [details] [diff] [review]
patch v3

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

r=me for the /browser changes.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 16 • 12 years ago

Heh - bitrotted myself with another bug. The changes were trivial, so I'm carrying over r+.
Here's the checkin: https://hg.mozilla.org/integration/mozilla-inbound/rev/79b27ec730c2

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/79b27ec730c2

	Paul Silaghi, QA [:pauly]
	Comment 18 • 12 years ago

multiple plugins test page: http://mozqa.com/data/firefox/plugins/plugin-test.html
nightly 20.0a1 (2012-12-03)
2 issues:
1. if plugins.click_to_play=true, java can't be enabled on click. Works fine on a separate page with java only
2. if plugins.click_to_play=true and some plugins are out of date, the CTP UI won't appear for the outdated plugins. Works fine if all plugins are updated. http://img507.imageshack.us/img507/3411/ctpmultipleplugins.png

Are these issues related to this bug (to the fact that are multiple plugin types on a page) ?

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 19 • 12 years ago

I can't seem to reproduce issue 1. Could you be more specific as to what you're seeing? (e.g. does the overlay not go away? Does it go away but then the applet doesn't load?)

For issue 2, I'm pretty sure the UI isn't showing up because the plugins are too small (200x100 isn't enough space to show the whole message). Try with 200x200?

	Paul Silaghi, QA [:pauly]
	Comment 20 • 12 years ago

(In reply to David Keeler from comment #19)
> I can't seem to reproduce issue 1. Could you be more specific as to what
> you're seeing? (e.g. does the overlay not go away? Does it go away but then
> the applet doesn't load?)
Yes, the overlay does not go away. Not working on Win, seems to work on Mac.

> For issue 2, I'm pretty sure the UI isn't showing up because the plugins are
> too small (200x100 isn't enough space to show the whole message). Try with
> 200x200?
You are perfectly right, works fine with 200x200.

	Paul Silaghi, QA [:pauly]
	Comment 21 • 12 years ago

I've also noticed something else. If there are plugins missing and the "missing plugins notification" shows up, the CTP doorhanger near the location bar won't be displayed. Here flash is missing on the 1st position: http://img11.imageshack.us/img11/6981/missingplugins.png

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 22 • 12 years ago

(In reply to Paul Silaghi [QA] from comment #21)
> I've also noticed something else. If there are plugins missing and the
> "missing plugins notification" shows up, the CTP doorhanger near the
> location bar won't be displayed. Here flash is missing on the 1st position:
> http://img11.imageshack.us/img11/6981/missingplugins.png

I filed bug 818118 for this.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 23 • 12 years ago

(In reply to Paul Silaghi [QA] from comment #20)
> (In reply to David Keeler from comment #19)
> > I can't seem to reproduce issue 1. Could you be more specific as to what
> > you're seeing? (e.g. does the overlay not go away? Does it go away but then
> > the applet doesn't load?)
> Yes, the overlay does not go away. Not working on Win, seems to work on Mac.

I think this is a problem with the plugin itself - it seems if it has nothing to do, it doesn't repaint its content area, which makes it look like the overlay doesn't go away (for instance, try minimizing the browser and showing it again). If the plugin actually loads some code and does things, it works for me (even if there are other plugins on the page).

	Paul Silaghi, QA [:pauly]
	Comment 24 • 12 years ago

(In reply to David Keeler from comment #23)
> I think this is a problem with the plugin itself - it seems if it has
> nothing to do, it doesn't repaint its content area, which makes it look like
> the overlay doesn't go away (for instance, try minimizing the browser and
> showing it again). If the plugin actually loads some code and does things,
> it works for me (even if there are other plugins on the page).
Agreed.