917931 - Cleanup plugin enabledState usage in browser tests

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Georg Fritzsche [:gfritzsche]]

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

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Cleanup nsIPluginTag.enabledState usage in browser tests

This is fine on try now.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: Cleanup nsIPluginTag.enabledState usage in browser tests

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

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 4

[Georg Fritzsche [:gfritzsche]]

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 5

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 6

[Georg Fritzsche [:gfritzsche]]

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

Comment 7

[Georg Fritzsche [:gfritzsche]]

Attachment: 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

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 9

[Georg Fritzsche [:gfritzsche]]

(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 10

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 11

[Georg Fritzsche [:gfritzsche]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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