click-to-play: asynchronously adding different plugins won't update the popup notification

VERIFIED FIXED in Firefox 19

Status

()

Firefox
General
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Trunk
Firefox 20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18 wontfix, firefox19+ verified, firefox20+ verified, firefox-esr1719+ verified)

Details

Attachments

(4 attachments, 1 obsolete attachment)

bug 754472 comment 41:
(In reply to Paul Silaghi [QA] from comment #41)
> Nightly 2012-12-10 Win 7 x64
> 1. Enable CTP
> 2. Open http://wickedgoodgames.com/java2/asteroids.html
> 3. Click the CTP doorhanger
> 
> Actual results:
> Java is not on the list
> http://img24.imageshack.us/img24/181/ctpjava.png
> 
> Any ideas? Works fine with other pages like
> http://mozqa.com/data/firefox/plugins/plugin-test.html

After a bit of investigation, it looks like if a plugin of a different type than those already present is added to the page after the others have been handled, the popup notification won't get updated to include the new type.
Created attachment 691073 [details] [diff] [review]
patch

This should do it. Unfortunately, it also means having a whole bunch of the same type of plugin objects on the same page will result in a lot of wasted work. I'm thinking a cached list of what types are already on the page could help? But maybe that's over-complicating things.
Attachment #691073 - Flags: review?(jaws)
Created attachment 691226 [details]
java is not on the list
Comment on attachment 691073 [details] [diff] [review]
patch

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

::: browser/base/content/test/browser_bug820497.js
@@ +35,5 @@
> +    var notification = PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser);
> +    ok(notification, "should have popup notification");
> +    is(notification.options.centerActions.length, 1, "should be 1 type of plugin in the popup notification");
> +  }
> +  else if (gNumPluginBindingsAttached == 2) {

} else if (...) {

@@ +49,5 @@
> +    gBrowser.removeCurrentTab();
> +    window.focus();
> +    finish();
> +  }
> +  else {

} else {
Attachment #691073 - Flags: review?(jaws) → review+
Created attachment 693176 [details] [diff] [review]
patch rebased

Had to rebase. Carrying over r+.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=24edbe6acce5
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/3aa1bed744bb
Attachment #691073 - Attachment is obsolete: true
Attachment #693176 - Flags: review+

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/3aa1bed744bb
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Verified fixed on Nightly 2012-12-20 Win 7 x64, Ubuntu 12.04 and Mac OS X 10.8.2
Status: RESOLVED → VERIFIED
(Assignee)

Updated

5 years ago
Blocks: 809846
Marking tracking-firefox19? as per bug 809846 comment 32 and 33.
tracking-firefox19: --- → ?
(Assignee)

Updated

5 years ago
tracking-firefox-esr17: --- → ?

Comment 8

5 years ago
Please prepare patches for Aurora/Beta/ESR17 so that we can land before the end of the week.
status-firefox18: --- → wontfix
status-firefox19: --- → affected
status-firefox20: --- → affected
tracking-firefox19: ? → +
tracking-firefox20: --- → +
tracking-firefox-esr17: ? → 19+
Created attachment 706048 [details] [diff] [review]
patch for beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): click-to-play
User impact if declined: users won't be able to use the door-hanger to activate plugins added to the page dynamically
Testing completed (on m-c, etc.): landed in 20
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Attachment #706048 - Flags: review+
Attachment #706048 - Flags: approval-mozilla-beta?
Created attachment 706050 [details] [diff] [review]
patch for esr17

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: needed for click-to-play
User impact if declined: see previous comment
Fix Landed on Version: 20
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #706050 - Flags: review+
Attachment #706050 - Flags: approval-mozilla-esr17?
This is fixed in 20, so no need for an aurora patch. I'm carrying over r+ because the changes are not functional - just to make them apply cleanly to the different branches.
status-firefox20: affected → fixed
status-firefox-esr17: --- → affected
verified FF 20 in comment 6
status-firefox20: fixed → verified
Comment on attachment 706048 [details] [diff] [review]
patch for beta

verified fixed, low risk, approving.
Attachment #706048 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #706050 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
beta: https://hg.mozilla.org/releases/mozilla-beta/rev/5b312c4d8130
esr17: https://hg.mozilla.org/releases/mozilla-esr17/rev/c732d17352f5
status-firefox19: affected → fixed
status-firefox20: verified → fixed
status-firefox-esr17: affected → fixed
Shoot - I didn't mean to change the status-firefox20 flag. Sorry about that, Paul.
Verified fixed FF 19b4 Win7, Ubuntu 12.04, Mac OS X 10.7.5
status-firefox19: fixed → verified
status-firefox20: fixed → verified
Adding verifyme keyword as this will need verification in the next 17esr release.
Keywords: verifyme
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #17)
> Adding verifyme keyword as this will need verification in the next 17esr
> release.

Paul, if you don't want to wait for the release you can verify this with the latest 17esr nightly.
Verified fixed 2013-01-30-03-45-01-mozilla-esr17 Win7, Ubuntu 12.04, Mac OS X 10.8.2
Keywords: verifyme
status-firefox-esr17: fixed → verified

Updated

3 years ago
Blocks: 1145179
You need to log in before you can comment on or make changes to this bug.