Closed Bug 905084 Opened 6 years ago Closed 6 years ago

"Activate this plugin" doesn't work

Categories

(Core :: Plug-ins, defect, P3)

24 Branch
x86_64
Windows 8
defect

Tracking

()

VERIFIED FIXED
mozilla26
Tracking Status
firefox23 --- unaffected
firefox24 + verified
firefox25 + verified
firefox26 + verified

People

(Reporter: mail, Assigned: gfritzsche)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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
Component: Untriaged → Plug-ins
Product: Firefox → Core
This works in 23, so it was probably broken by the doorhanger changes. Tracking due to broken UI
Blocks: 880735
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Priority: -- → P3
It looks like that context menu action just wasn't updated to the gPluginHandler interface changes.
Assignee: nobody → georg.fritzsche
Status: NEW → ASSIGNED
Attachment #790833 - Flags: review?(jaws)
proactively adding verify me on this bug so it is on QA's radar for verification once this lands.
Keywords: verifyme
QA Contact: twalker
Added test coverage for this.
Attachment #790833 - Attachment is obsolete: true
Attachment #790833 - Flags: review?(jaws)
Attachment #790943 - Flags: review?(jaws)
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.
Attachment #790943 - Flags: review?(jaws) → review-
(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.
Flags: needinfo?(jaws)
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?
Flags: needinfo?(jaws)
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.
Attachment #790943 - Attachment is obsolete: true
Attachment #793066 - Flags: review?(jaws)
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.
Attachment #793066 - Flags: review?(jaws) → review+
Right, fixed, thanks for catching my copy'n'paste artifacts :-/

https://hg.mozilla.org/integration/mozilla-inbound/rev/20e064f3e8e0
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.
Depends on: 909342
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?
Attachment #793066 - Attachment is obsolete: true
Attachment #795465 - Flags: review?(jaws)
Attachment #795465 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/d6d84dcbdd8b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
Attachment #795465 - Flags: approval-mozilla-beta?
Attachment #795465 - Flags: approval-mozilla-aurora?
"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.
Attachment #795465 - Flags: approval-mozilla-beta?
Attachment #795465 - Flags: approval-mozilla-beta+
Attachment #795465 - Flags: approval-mozilla-aurora?
Attachment #795465 - Flags: approval-mozilla-aurora+
(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?
Flags: needinfo?(paul.silaghi)
(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)
Status: RESOLVED → VERIFIED
Flags: needinfo?(paul.silaghi)
Keywords: verifyme
QA Contact: twalker
You need to log in before you can comment on or make changes to this bug.