Cleanup plugin enabledState usage in browser tests

RESOLVED FIXED in mozilla27

Status

()

Core
Plug-ins
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gfritzsche, Assigned: gfritzsche)

Tracking

Trunk
mozilla27
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
We should fixup the browser tests, similar to the fixups in bug 899080, utilizing |setTestPluginEnabledState()| added by bug 790483.

This should fix some stale/non-default |enabledState| and removes the need to explicitly reset it.

https://tbpl.mozilla.org/?tree=Try&rev=82155246a7a8
(Assignee)

Comment 1

4 years ago
Created attachment 807150 [details] [diff] [review]
Cleanup nsIPluginTag.enabledState usage in browser tests

This is fine on try now.
Attachment #807150 - Flags: review?(jaws)
(Assignee)

Comment 2

4 years ago
Created attachment 808625 [details] [diff] [review]
Cleanup nsIPluginTag.enabledState usage in browser tests

Updated to changed test directory structure.
https://tbpl.mozilla.org/?tree=Try&rev=8a094871792a
Attachment #807150 - Attachment is obsolete: true
Attachment #807150 - Flags: review?(jaws)
Attachment #808625 - Flags: review?(jaws)
Comment on attachment 808625 [details] [diff] [review]
Cleanup nsIPluginTag.enabledState usage in browser tests

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

What happens if a test does the following:

function test() {
  setPluginTestEnabledState(CLICK_TO_PLAY);
  setPluginTestEnabledState(ENABLED);
}

Where the original state was DISABLED? What does the final state become?

Won't this introduce two new "registerCleanupFunction" instances. I don't know if the order of execution is guaranteed there, but I would assume that it is a FIFO order, meaning that the described order above would result in a plugin with CLICK_TO_PLAY being the final state when it should have been DISABLED.
(Assignee)

Comment 4

4 years ago
The cleanup functions are handled in LIFO order:
http://hg.mozilla.org/mozilla-central/annotate/f97307cb4c95/testing/mochitest/tests/SimpleTest/SimpleTest.js#l729
http://hg.mozilla.org/mozilla-central/diff/e13bb42d2872/testing/mochitest/tests/SimpleTest/SimpleTest.js

This seems like the desired behaviour, Ted agreed on that.
Comment on attachment 808625 [details] [diff] [review]
Cleanup nsIPluginTag.enabledState usage in browser tests

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

Ok cool, these changes all look great.
Attachment #808625 - Flags: review?(jaws) → review+
(Assignee)

Comment 6

4 years ago
Given the failures on bug 790483 i'm running this for all mochitests on try again:
https://tbpl.mozilla.org/?tree=Try&rev=2f0265623ff4
(Assignee)

Comment 7

4 years ago
Created attachment 809856 [details] [diff] [review]
Cleanup nsIPluginTag.enabledState usage in browser tests

The try-run above turned up that test_contextmenu.html is a mochitest-plain and we only have setTestPluginEnabledState() for browser tests in browser/base/content/test/general so far.

The previous only consumer of head_plain.js was moved to a mochitest-chrome in [1], making waitForCondition() unused, hence i stripped it.
I'm keeping it here for a shared setTestPluginEnabledState() for mochitest-plain.
Does that sound reasonable?

The only changes compared to the last patch version are in:
head_plain.js
test_contextmenu.html

New try run is green:
https://tbpl.mozilla.org/?tree=Try&rev=daf0bedfb8f3

[1] http://hg.mozilla.org/mozilla-central/rev/450bbfd48532
Attachment #808625 - Attachment is obsolete: true
Attachment #809856 - Flags: review?(jaws)
Comment on attachment 809856 [details] [diff] [review]
Cleanup nsIPluginTag.enabledState usage in browser tests

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

(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> I'm keeping it here for a shared setTestPluginEnabledState() for
> mochitest-plain.

I'm not sure what this means. setTestPluginEnabledState() in head_plain.js looks to be unused.
(Assignee)

Comment 9

4 years ago
(In reply to Jared Wein [:jaws] from comment #8)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> > I'm keeping it here for a shared setTestPluginEnabledState() for
> > mochitest-plain.
> 
> I'm not sure what this means. setTestPluginEnabledState() in head_plain.js
> looks to be unused.

It is used in test_contextmenu.html.
Comment on attachment 809856 [details] [diff] [review]
Cleanup nsIPluginTag.enabledState usage in browser tests

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

I guess I can't trust Bugzilla's built-in "diff" feature much, since it didn't have any mention of the setTestPluginEnabledState() function in test_contextmenu.html.

r=me
Attachment #809856 - Flags: review?(jaws) → review+
(Assignee)

Comment 11

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c09eccf1bdb
https://hg.mozilla.org/mozilla-central/rev/1c09eccf1bdb
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.