Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Description • 12 years ago

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).

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 1 • 12 years ago

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).

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

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).

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 3 • 12 years ago

(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...).

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

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

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

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?

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 6 • 11 years ago

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 )