798237 - Test for bug 797677

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jesse Ruderman]

See https://bugzilla.mozilla.org/show_bug.cgi?id=797677#c5

Do we need to test crazy types like "0" and "9000" and "length", or just some valid and some invalid types?

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: test

It looks like using valid indices or properties that exist on navigator.mimeTypes doesn't actually cause a failure. In the first case, we might get the plugin name wrong, but since it's already an unsupported plugin type, I think this is okay (we never use pluginName in that case). In the second case, this results in a valid js object that doesn't have a property called 'enabledPlugin', so the getPluginInfo doesn't do anything bad with it.
So, the only way we can test this is by using a large, invalid index. Hopefully this is good enough.
Jared - thoughts?

Comment 2

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

Comment on attachment 668511 [details] [diff] [review]
test

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

I'd like to get review from Mihai on the nsIConsoleService usage since he knows more about it than I do.

::: browser/base/content/test/browser_bug797677.js
@@ +1,1 @@
> +var rootDir = getRootDirectory(gTestPath);

Need a license header.

@@ +5,5 @@
> +var gTestBrowser = null;
> +var gErrorListener = null;
> +var gConsoleErrors = 0;
> +var gConsoleService = Cc["@mozilla.org/consoleservice;1"]
> +                      .getService(Ci.nsIConsoleService);

nit: indent this line by two more spaces so the period lines up with the open bracket.

@@ +15,5 @@
> +  gTestBrowser = gBrowser.selectedBrowser;
> +  gTestBrowser.addEventListener("load", pageLoad, true);
> +  gErrorListener = {
> +    observe: function(aMessage) {
> +      if (aMessage.message.indexOf("NS_ERROR") > -1) {

nit: if (aMessage.message.contains("NS_ERROR"))
also, no need for the curly brackets here.

@@ +35,5 @@
> +
> +function pageLoad() {
> +  // The plugin events are async dispatched and can come after the load event
> +  // This just allows the events to fire before we then go on to test the states
> +  executeSoon(runTest);

This seems too fragile. Maybe something like the waitForCondition that is used in other plugin tests?

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 668511 [details] [diff] [review]
test

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

nsIConsoleService usage looks fine, but my worry is that the test could finish() too early. You could wait something like ~2s after page load, but timeouts are not what we want to use.

Comment 4

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

Please request review again after making the changes.

I'm curious to see if you can use waitForCondition here to remove the executeSoon.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Why not use a listener for the plugin event you care about, rather than the load event, and use executeSoon to ensure the browser code's handler runs first?

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: test v2

Comment 7

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

Comment on attachment 669190 [details] [diff] [review]
test v2

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

Thanks! This looks good to me.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=c500fe87be46
Pushed to inbound:  https://hg.mozilla.org/integration/mozilla-inbound/rev/3a14bf605a71

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3a14bf605a71

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/03f06a061206