Closed Bug 734323 Opened 12 years ago Closed 12 years ago

Invisible plugins added to the DOM after the "load" event will not prompt a doorhanger to appear

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(blocking-fennec1.0 +)

RESOLVED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- +

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(1 file, 2 obsolete files)

The logic for determining if a doorhanger should appear is handled during the "load" event. Any invisible plugins added to the DOM after the load event will not trigger the doorhanger.

One way to solve this is to add two booleans to the ?Tab? to keep track of the state of the doorhanger: a boolean stating if the doorhanger has been shown for this page, and another boolean stating if the "load" event has been processed.

If we get a "PluginClickToPlay" event and the "load" event has been processed while the doorhanger hasn't been shown for this page, then we should show the doorhanger.
blocking-fennec1.0: --- → ?
I think this is pretty edge-case-y, so I don't think it needs to block. Jared, have you seen this problem in the wild at all, or is this just theoretical?
This is theoretical, just trying to cover all bases. I don't think this should block either.
Assignee: nobody → margaret.leibovic
blocking-fennec1.0: ? → +
Brad: This bug depends on bug 730318 which was marked as blocking-fennec1.0-. Are you sure you want this bug to block 1.0? I hope to land bug 730318 soon, but blocking flag difference between the two bugs is odd.
fixed the blocking flag on bug 730318
Brad, to clarify, this issue doesn't exist until bug 730318 lands, so if we don't want to make bug 730318 a blocker, this doesn't actually need to block.
bug 730318 should legitimately block on its own merits. Having the click-to-play be hokey has caused me to enable flash always on my GN. That really isn't an option on pre-Honeycomb devices.
Well, okay then, we'll fix it all!
Yoink!
Assignee: margaret.leibovic → jwein
Status: NEW → ASSIGNED
Blocks: 735833
This patch implements the necessary behavior to invisible plugins that are added to the DOM after the 'load' event.
Attachment #606026 - Flags: review?(margaret.leibovic)
Comment on attachment 606026 [details] [diff] [review]
Patch for bug (apply on top of patch for bug 730318)

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+    // Track state of click-to-play plugin notifications.
>+    this.clickToPlayPluginDoorhangerShown = false;
>+    this.loadEventProcessed = false;

I think you should declare these in Tab(), like we used to do for |this._pluginCount| and |this._pluginOverlayShowing|, since that's where we initialize similar boolean flags.

>         // If the overlay is too small, hide the overlay and act like this
>         // is a hidden plugin object
>-        if (PluginHelper.isTooSmall(plugin, overlay)) {
>-          overlay.style.visibility = "hidden";
>+        if (!overlay || PluginHelper.isTooSmall(plugin, overlay)) {
>+          if (overlay)
>+            overlay.style.visibility = "hidden";
>+          if (this.loadEventProcessed && !this.clickToPlayPluginDoorhangerShown)
>+            PluginHelper.showDoorHanger(this);

Thinking really edge-case-y here, if another invisible plugin is added even later, we won't show another doorhanger. That being said, I don't think we want to be showing multiple doorhangers, since that's annoying, so maybe this is the best we can do. What do you think?

>@@ -1914,16 +1920,20 @@ Tab.prototype = {
>     let uri = browser.currentURI.spec;
>     let documentURI = "";
>     let contentType = "";
>     if (browser.contentDocument) {
>       documentURI = browser.contentDocument.documentURIObject.spec;
>       contentType = browser.contentDocument.contentType;
>     }
> 
>+    // Reset state of click-to-play plugin notifications.
>+    this.clickToPlayPluginDoorhangerShown = false;
>+    this.loadEventProcessed = false;

Doing this on location change, are we going to run into any of those issues we were having before with the bfcache? I haven't thought this through, and I was just wondering if you have :)

r=me, with comments addressed/pondered.
Attachment #606026 - Flags: review?(margaret.leibovic) → review+
(In reply to Margaret Leibovic [:margaret] from comment #10)
> Comment on attachment 606026 [details] [diff] [review]
> Patch for bug (apply on top of patch for bug 730318)
> 
> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> 
> >+    // Track state of click-to-play plugin notifications.
> >+    this.clickToPlayPluginDoorhangerShown = false;
> >+    this.loadEventProcessed = false;
> 
> I think you should declare these in Tab(), like we used to do for
> |this._pluginCount| and |this._pluginOverlayShowing|, since that's where we
> initialize similar boolean flags.

Oh, that's a better place than where I was adding these lines. I'll move them.

> >         // If the overlay is too small, hide the overlay and act like this
> >         // is a hidden plugin object
> >-        if (PluginHelper.isTooSmall(plugin, overlay)) {
> >-          overlay.style.visibility = "hidden";
> >+        if (!overlay || PluginHelper.isTooSmall(plugin, overlay)) {
> >+          if (overlay)
> >+            overlay.style.visibility = "hidden";
> >+          if (this.loadEventProcessed && !this.clickToPlayPluginDoorhangerShown)
> >+            PluginHelper.showDoorHanger(this);
> 
> Thinking really edge-case-y here, if another invisible plugin is added even
> later, we won't show another doorhanger. That being said, I don't think we
> want to be showing multiple doorhangers, since that's annoying, so maybe
> this is the best we can do. What do you think?

The doorhanger is already visible. If the user has denied activation, we won't show another doorhanger which is OK. This did make me realize that I'm not handling the case where the user allows activation. In that scenario, we should auto-activate upon receiving this event. I'll fix the patch to handle that case.

> >@@ -1914,16 +1920,20 @@ Tab.prototype = {
> >     let uri = browser.currentURI.spec;
> >     let documentURI = "";
> >     let contentType = "";
> >     if (browser.contentDocument) {
> >       documentURI = browser.contentDocument.documentURIObject.spec;
> >       contentType = browser.contentDocument.contentType;
> >     }
> > 
> >+    // Reset state of click-to-play plugin notifications.
> >+    this.clickToPlayPluginDoorhangerShown = false;
> >+    this.loadEventProcessed = false;
> 
> Doing this on location change, are we going to run into any of those issues
> we were having before with the bfcache? I haven't thought this through, and
> I was just wondering if you have :)

We shouldn't run in to issues with the bfcache as I understand it. More testing on this would be good though.
This is mainly the previous r+'d patch, but with a change to auto-activate newly added plugins if plugins have already been activated for the page while the page is loaded.
Attachment #606026 - Attachment is obsolete: true
Attachment #607387 - Flags: review?(margaret.leibovic)
Comment on attachment 607387 [details] [diff] [review]
Patch for bug v2 (apply on top of patch for bug 730318)

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>       case "PluginClickToPlay": {
>         let plugin = aEvent.target;
>         let overlay = plugin.ownerDocument.getAnonymousElementByAttribute(plugin, "class", "mainBox");
>-        if (!overlay)
>+
>+        if (this.clickToPlayPluginsActivated) {
>+          PluginHelper.playPlugin(plugin);
>           return;
>+        }

Nit: Put this above the overlay variable declaration, since that isn't used until below.

>         // Add click to play listener to the overlay
>         overlay.addEventListener("click", function(e) {
>           if (e) {
>             if (!e.isTrusted)
>               return;
>             e.preventDefault();
>           }
>+          let tab = BrowserApp.getTabForWindow(e.target.ownerDocument.defaultView.top);
>+          tab.clickToPlayPluginsActivated = true;
>           PluginHelper.playAllPlugins(e.target.ownerDocument.defaultView);

Factor out e.target.ownerDocument.defaultView? Also, why are you setting clickToPlayPluginsActivated on defaultView.top, but only passing defaultView to playAllPlugins?
Nice catch with the wrong parameter being passed to playAllPlugins. Thank you for noticing that :)
Attachment #607387 - Attachment is obsolete: true
Attachment #607820 - Flags: review?(margaret.leibovic)
Attachment #607387 - Flags: review?(margaret.leibovic)
Comment on attachment 607820 [details] [diff] [review]
Patch for bug v3 (apply on top of patch for bug 730318)

(In reply to Jared Wein [:jaws] from comment #15)

> Nice catch with the wrong parameter being passed to playAllPlugins. Thank
> you for noticing that :)

Haha, that's what review is for! :)
Attachment #607820 - Flags: review?(margaret.leibovic) → review+
And a much greener try run after I fixed a minor issue with the patch that this patch depends on:
https://tbpl.mozilla.org/?tree=Try&rev=1f7fe227e41d
https://hg.mozilla.org/mozilla-central/rev/c120dd831b3f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Backed out on inbound since the patch for bug 730318 caused bustage on b2g builds.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d85fe5da4eca
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This patch was backed out due to test failures. I'm investigating now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/26808dda208c
https://hg.mozilla.org/mozilla-central/rev/265ae1eb0189
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.