User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release) Build ID: 20130812173056 Steps to reproduce: 1. Enable plugins.click_to_play 2. Set plugin to "Ask to Activate" 3. Right click on a blocked plugin and select "Activate this plugin" Actual results: Nothing Expected results: The plugin should activate

This works in 23, so it was probably broken by the doorhanger changes. Tracking due to broken UI

It looks like that context menu action just wasn't updated to the gPluginHandler interface changes.

proactively adding verify me on this bug so it is on QA's radar for verification once this lands.

Added test coverage for this.

Comment on attachment 790943 [details] [diff] [review] Fix "Activate this plugin" context menu action Review of attachment 790943 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/browser_CTP_context_menu.js @@ +10,5 @@ > + > +// This listens for the next opened tab and checks it is of the right url. > +// opencallback is called when the new tab is fully loaded > +// closecallback is called when the tab is closed > +function TabOpenListener(url, opencallback, closecallback) { This TabOpenListener code looks unused and dead. Can this just be deleted? @@ +96,5 @@ > +// Due to layout being async, "PluginBindAttached" may trigger later. > +// This wraps a function to force a layout flush, thus triggering it, > +// and schedules the function execution so they're definitely executed > +// afterwards. > +function runAfterPluginBindingAttached(func) { This function should be moved to head.js since it is duplicated in multiple test files. @@ +108,5 @@ > + executeSoon(func); > + }; > +} > + > +// Test that the click-to-play doorhanger still works when navigating to data URLs I don't see any mention of data URLs for this specific test.

(In reply to Jared Wein [:jaws] from comment #6) > @@ +96,5 @@ > > +// Due to layout being async, "PluginBindAttached" may trigger later. > > +// This wraps a function to force a layout flush, thus triggering it, > > +// and schedules the function execution so they're definitely executed > > +// afterwards. > > +function runAfterPluginBindingAttached(func) { > > This function should be moved to head.js since it is duplicated in multiple > test files. We ran into this previously in bug 896965: (from bug 896965, comment #57) > > ::: browser/base/content/test/browser_CTP_data_urls.js > > @@ +94,5 @@ > > > gNextTest = nextTest; > > > gTestBrowser.contentWindow.location = url; > > > } > > > > > > +function runAfterPluginBindingAttached(func, doc) { > > > > can this be put in some sort of head.js? > > I was thinking about that, but AFAICT this function couldn't depend on > gTestBrowser and would always have to receive the doc or browser explicitly. > See also e.g. prepareTest() being duplicated. > > Any better suggestions or do you think i can go with this? For the rest, two bad cases of copy'n'paste left-overs. Will fix.

needinfo? for comment 7 - i don't see how i could move runAfterPluginBindingAttached() to head.js without the problem mentioned above.

One way would be to move runAfterPluginBindingAttached, prepareTest to a plugin_helpers.inc.js file and then #include the file in the various tests. I am not sure if this would be worth the future overhead of trying to understand line numbers and pulling in the full test context when debugging. What are your thoughts?

I'm not even sure if test files can #include something. Don't worry about this issue.

Ok. I'd personally also like to avoid more #includes, it's quite a stopper for quickly checking into test-failures.

Addressed the other two review issues.

Comment on attachment 793066 [details] [diff] [review] Fix "Activate this plugin" context menu action - v2 Review of attachment 793066 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/browser_CTP_context_menu.js @@ +98,5 @@ > + waitForCondition(() => objLoadingContent.activated, test3, "Waited too long for plugin to activate"); > +} > + > +function test3() { > + clearAllPluginPermissions(); clearAllPluginPermissions() is already called in the first line of finishTest. It's also called by the anonymous function passed to registerCleanupFunction. It seems that it should only remain in the anonymous function.

Right, fixed, thanks for catching my copy'n'paste artifacts :-/ https://hg.mozilla.org/integration/mozilla-inbound/rev/20e064f3e8e0

The newly added test has been failing off and on since it was added <https://tbpl.mozilla.org/php/getParsedLog.php?id=26792799&tree=Mozilla-Inbound>, so this was backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/53853b86f19b

Weird, this is apparently failing only on Linux. test_contextmenu.html is disabled on Linux for bug 513558, but those failures don't seem related. One other failure type i've seen on TBPL: https://tbpl.mozilla.org/php/getParsedLog.php?id=26794601&tree=Mozilla-Inbound TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | uncaught exception - TypeError: gContextMenu is null at chrome://browser/content/browser.xul:1 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | Test 2, The click-to-play notification should not be dismissed TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | uncaught exception - TypeError: PopupNotifications.panel.firstChild is null at chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js:94 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | Test timed out TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | Found a tab after previous test timed out: http://127.0.0.1:8888/browser/browser/base/content/test/plugin_test.html

... the others being of the kind of comment 15: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | uncaught exception - TypeError: PopupNotifications.panel.firstChild is null at chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js:94 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | Test timed out TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | Found a tab after previous test timed out: http://127.0.0.1:8888/browser/browser/base/content/test/plugin_test.html

Just a quick reminder that we will be entering the fourth week of betas for Fx24 starting upcoming week with Beta's going to build Monday and Thursday morning PT and this will be the last opportunity for any low risk fixes. Given this is a Fx24 regression we should try to get it in by then so that we atleast have a week's bake time on beta.

I'm looking into the Linux failures, Monday is currently looking unrealistic though unless i'd land with the Linux test disabled for now.

We want to land with the test disabled on Linux for now. Try run: https://tbpl.mozilla.org/?tree=Try&rev=f415ebf69d00

The only change here is disabling the test on Linux as i won't be able to look into those failures soon enough. Bug 909342 is the follow-up bug for that. jaws, is that ok with you?

Comment on attachment 795465 [details] [diff] [review] Fix "Activate this plugin" context menu action - v3, disabled on Linux [Approval Request Comment] Bug caused by (feature/regressing bug #): Plugin doorhanger redesign (bug 880735 et al). User impact if declined: "Activate this plugin" context action doesn't work for blocked & CTP plugins. Testing completed (on m-c, etc.): On m-c, awaiting verification. Risk to taking this patch (and alternatives if risky): Low-risk - the menu entry is already broken, this patch just fixes one line to make it work again. String or IDL/UUID changes made by this patch: None.

"Activate this plugin" works fine in nightly 26.0a1 (2013-08-27) on Win 7, Ubuntu 12.04 and Mac OS X 10.7.5

Comment on attachment 795465 [details] [diff] [review] Fix "Activate this plugin" context menu action - v3, disabled on Linux Approving on branches given the nightly verification and as we want this regression to be fixed for Fx24. Requesting QA verification on Beta once it lands there.

https://hg.mozilla.org/releases/mozilla-aurora/rev/eddf5b70bd74 https://hg.mozilla.org/releases/mozilla-beta/rev/8d8061c0f2e5

(In reply to bhavana bajaj [:bajaj] from comment #26) > Requesting QA verification on Beta once it lands there. Paul, can you check for this when the build is out?

(In reply to Paul Silaghi [QA] from comment #25) > "Activate this plugin" works fine in nightly 26.0a1 (2013-08-27) on Win 7, > Ubuntu 12.04 and Mac OS X 10.7.5 Verified fixed FF 24b7, 25.0a2 (2013-08-30)