Georg Fritzsche [:gfritzsche] Assignee
	Description • 11 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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 1 • 11 years ago

This is fine on try now.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 2 • 11 years ago

Updated to changed test directory structure.
https://tbpl.mozilla.org/?tree=Try&rev=8a094871792a

	Jared Wein [:jaws] (please needinfo? me)
	Comment 3 • 11 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 4 • 11 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.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 5 • 11 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 6 • 11 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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 7 • 11 years ago

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

	Jared Wein [:jaws] (please needinfo? me)
	Comment 8 • 11 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 9 • 11 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.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 10 • 11 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 11 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c09eccf1bdb

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/1c09eccf1bdb