Originally, the tests for bug 810082 (opening the popup notification for invisible click-to-play plugins) tested a few situations where we would expect a popup and a few where we wouldn't expect a popup. However, after bug 819992 (making bug 810082 less annoying by only opening the popup once per session), the tests only have the popup show once (because, well, we only have the one session, so that's what it does). I think we need to re-include testing for multiple scenarios where the popup should open (and of course include a test that ensures it only opens once per session).

This patch applies on top of the one in bug 820303 (they conflict with each other and that one's more important, so I intend for that one to land first).

Comment on attachment 693506 [details] [diff] [review] patch Review of attachment 693506 [details] [diff] [review]: ----------------------------------------------------------------- This removes the implicit test that the next page that gets visited will not show the doorhanger even though it has only hidden plugins. It also can be confusing to anyone looking at the test and wondering why the browser behaves differently (if they don't notice the private variable being reset). Let's do this: Implement nsIObserver for gPluginHandler. The tests can then observe() a test-only topic which triggers the _notificationDisplayedOnce to get reset. Keep this test file and keep two scripted popup accesses, where the first expects the popup and the second one expects no popup. Use registerCleanupFunction to call the observe() with the topic (to reset the _notificationDisplayedOnce state upon exiting the test). Move the deny scenario to a new test file (and repeat the same registerCleanupFunction usage).

(In reply to Jared Wein [:jaws] from comment #2) > Comment on attachment 693506 [details] [diff] [review] > patch > > Review of attachment 693506 [details] [diff] [review]: > ----------------------------------------------------------------- > > This removes the implicit test that the next page that gets visited will not > show the doorhanger even though it has only hidden plugins. If I understand what you're saying, that's why I added testOnlyShowPopupOncePart1, etc. > It also can be > confusing to anyone looking at the test and wondering why the browser > behaves differently (if they don't notice the private variable being reset). > > Let's do this: > Implement nsIObserver for gPluginHandler. > The tests can then observe() a test-only topic which triggers the > _notificationDisplayedOnce to get reset. > Keep this test file and keep two scripted popup accesses, where the first > expects the popup and the second one expects no popup. > Use registerCleanupFunction to call the observe() with the topic (to reset > the _notificationDisplayedOnce state upon exiting the test). > Move the deny scenario to a new test file (and repeat the same > registerCleanupFunction usage). I think this might be too complicated for what we need to do. How about adding a public function to gPluginHandler like resetNotificationDisplayed() that we call from the tests. That way, anyone reading the test will see that the code that tracks whether or not the notification has been displayed purposefully gets reset (except for when we explicitly test that, in testOnlyShowPopupOnce...).

(In reply to David Keeler from comment #3) > How about > adding a public function to gPluginHandler like resetNotificationDisplayed() > that we call from the tests. That way, anyone reading the test will see that > the code that tracks whether or not the notification has been displayed > purposefully gets reset (except for when we explicitly test that, in > testOnlyShowPopupOnce...). OK, I'm fine with that.

After the changes in bug 880735 we don't automatically show the doorhanger anymore; only clicks on the overlay or the notification icon trigger it. Is this addressing other scenarios as well or can we close this?

Yep, we can close this. (For future reference, bug 880735 already removed the test: https://hg.mozilla.org/mozilla-central/diff/3da4f4ddc833/browser/base/content/test/browser_CTPScriptPlugin.js )