Paul Rouget [:paul] Reporter
	Description • 12 years ago

STR:
- go to http://imgur.com
- see the popup
- click "never activate plugin for this site"
- reload (ctrl-r)
- popup appears again

It started with yesterday's nightly (20121210).

	Jesper Hansen
	Comment 1 • 12 years ago

It is quite painful to browse imgur now

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

I think this is a dupe of bug 819992.

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

I cannot reproduce on Nightly 20.0a1 (2012-12-10) Win 7 x64

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 4 • 12 years ago

The "do we have an invisible plugin" logic needed a bit of an upgrade. With this patch, it must be the case that a) there be an invisible click-to-play plugin and b) there not be a visible click-to-play plugin, where a "click-to-play plugin" returns true when passed to canActivatePlugin (which is where the permissions check comes in).

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

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

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

::: browser/base/content/browser-plugins.js
@@ +268,4 @@
>        let doc = plugin.ownerDocument;
>        let overlay = doc.getAnonymousElementByAttribute(plugin, "class", "mainBox");
>        if (!overlay)
> +        break;

Why do we want to stop checking if we come across a plugin without an overlay?

@@ +280,5 @@
>                           gPluginHandler.isTooSmall(plugin, overlay));
> +      if (isInvisible) {
> +        haveInvisibleCTPPlugin = true;
> +      }
> +      else {

} else {

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 6 • 12 years ago

Hmm - good point.

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

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

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

::: browser/base/content/browser-plugins.js
@@ +259,5 @@
>  
>      let cwu = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>                              .getInterface(Ci.nsIDOMWindowUtils);
> +    let haveVisibleCTPPlugin = false;
> +    let haveInvisibleCTPPlugin = false;

I think we can just use one variable here, since they are complimentary of each other. We can get rid of the haveInvisibleCTPPlugin variable.

@@ +260,5 @@
>      let cwu = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>                              .getInterface(Ci.nsIDOMWindowUtils);
> +    let haveVisibleCTPPlugin = false;
> +    let haveInvisibleCTPPlugin = false;
> +    for (let plugin of cwu.plugins) {

cwu.plugins is calculated each time that the array is accessed, so it is better to cache the result of the array before the loop. for example, |let plugins = cwu.plugins|

@@ +290,3 @@
>  
>      let notification = PopupNotifications.getNotification("click-to-play-plugins", aBrowser);
> +    if (notification && haveInvisibleCTPPlugin && !haveVisibleCTPPlugin && !this._notificationDisplayedOnce) {

> if (notification && !haveVisibleCTPPlugin && plugins.length && !this._notificationDisplayedOnce) {

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 8 • 12 years ago

Thanks for the review. The second boolean was to catch the situation where there weren't actually any plugins that could be activated (e.g. if all plugins were removed from the page between the time the PluginScripted event was fired and this handler ran, or if the permission were set to "deny always"). I've re-worked it a bit, though, so maybe this makes more sense.

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

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

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

::: browser/base/content/browser-plugins.js
@@ +264,5 @@
> +      return gPluginHandler.canActivatePlugin(objLoadingContent);
> +    });
> +
> +    let haveVisibleCTPPlugin = false;
> +    for (let plugin of plugins) {

This can go use plugins.some() now.

let haveVisibleCTPPlugin = plugins.some(function(plugin) {
  ...
  return !isInvisible;
});

@@ +287,5 @@
>      let notification = PopupNotifications.getNotification("click-to-play-plugins", aBrowser);
> +    // If there aren't any plugins that can be activated (i.e. canActivatePlugin
> +    // returned false), haveVisibleCTPPlugin will still be false.
> +    // So, before opening the popup, check that we actually have any plugins
> +    // to which it applies.

I don't know if this comment actually adds much value, it seems pretty clear to me from reading the code below it.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 10 • 12 years ago

Ok - changed it to use plugins.some and removed the superfluous comment.

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

Comment on attachment 694630 [details] [diff] [review]
patch v4

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

::: browser/base/content/browser-plugins.js
@@ -262,4 @@
>        let doc = plugin.ownerDocument;
>        let overlay = doc.getAnonymousElementByAttribute(plugin, "class", "mainBox");
> -      if (!overlay)
> -        return false;

We can put the |if (!overlay) return false| back and remove the added |!overlay ||| below.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 12 • 12 years ago

Changed the overlay bit back to how it was. Carrying over r+.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=c33a95326c50
Looked good, so here's the checkin: https://hg.mozilla.org/integration/mozilla-inbound/rev/e6f18c0f3a24

	Ed Morley [:emorley]
	Comment 13 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/e6f18c0f3a24

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

I cannot verify this, wasn't able to reproduce the initial problem on nightly 2012-12-10, 2013-01-03.