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)
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)
5.81 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
blocking-fennec1.0: --- → ?
Comment 1•12 years ago
|
||
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?
Assignee | ||
Comment 2•12 years ago
|
||
This is theoretical, just trying to cover all bases. I don't think this should block either.
Updated•12 years ago
|
Assignee: nobody → margaret.leibovic
blocking-fennec1.0: ? → +
Assignee | ||
Comment 3•12 years ago
|
||
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.
Comment 4•12 years ago
|
||
fixed the blocking flag on bug 730318
Comment 5•12 years ago
|
||
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.
Comment 6•12 years ago
|
||
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.
Comment 7•12 years ago
|
||
Well, okay then, we'll fix it all!
Assignee | ||
Comment 9•12 years ago
|
||
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 10•12 years ago
|
||
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+
Assignee | ||
Comment 11•12 years ago
|
||
(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.
Assignee | ||
Comment 12•12 years ago
|
||
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)
Assignee | ||
Comment 13•12 years ago
|
||
Margaret: Here is a link to the interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=606026&action=interdiff&newid=607387&headers=1
Comment 14•12 years ago
|
||
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?
Assignee | ||
Comment 15•12 years ago
|
||
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 16•12 years ago
|
||
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+
Assignee | ||
Comment 17•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=1d77901752ed
Assignee | ||
Comment 18•12 years ago
|
||
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
Assignee | ||
Comment 19•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c120dd831b3f
Target Milestone: --- → Firefox 14
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c120dd831b3f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 21•12 years ago
|
||
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 → ---
Assignee | ||
Comment 22•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/418c8f45151b
Assignee | ||
Comment 23•12 years ago
|
||
Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/29529018c1da
Assignee | ||
Comment 24•12 years ago
|
||
This patch was backed out due to test failures. I'm investigating now. https://hg.mozilla.org/integration/mozilla-inbound/rev/26808dda208c
Assignee | ||
Comment 25•12 years ago
|
||
Landed again, hopefully third times the charm. Try results: https://tbpl.mozilla.org/?tree=Try&rev=889b6bdc70e8 https://hg.mozilla.org/integration/mozilla-inbound/rev/265ae1eb0189
Comment 26•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/265ae1eb0189
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•