Due to recent changes in Firefox about:plugins (Bug 831533) we need to update the test to check the enabled/disabled/blocklisted state of plugins.

Most likely we should disable the test for now so we do not have tons of failures over the weekend.

It's a skip patch until I come up with a working patch.

Comment on attachment 706329 [details] [diff] [review] patch v0.1 Skip Patch Review of attachment 706329 [details] [diff] [review]: ----------------------------------------------------------------- You missed the manifest file.

Skip patch until I come with a working patch.

Comment on attachment 706340 [details] [diff] [review] patch v0.2 Skip Patch Review of attachment 706340 [details] [diff] [review]: ----------------------------------------------------------------- I forgot to check the commit message for this skip patch. :( Please make sure to not simply copy the bug title but what this patch is really about. Thanks. http://hg.mozilla.org/qa/mozmill-tests/rev/e44091c4e75e (default)

Linux report: http://mozmill-crowd.blargon7.com/#/functional/report/0dbaa964aec88a2f5637c68c964a4d0e Mac report: http://mozmill-crowd.blargon7.com/#/functional/report/0dbaa964aec88a2f5637c68c96498e35 Win report: http://mozmill-crowd.blargon7.com/#/functional/report/0dbaa964aec88a2f5637c68c964bfa7d

Comment on attachment 709048 [details] [diff] [review] patch v1.0 Review of attachment 709048 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js @@ +72,5 @@ > // Check that the plugin is disabled > assert.ok(!addonsManager.isAddonEnabled({addon: selectedPlugin}), > pluginName + " has been disabled"); > > + // Check that the plugin state is Disabled from about:plugins "[..] state is Disabled in about:plugins" as you used bellow sounds better. @@ +79,2 @@ > > //Enable the plugin Please add a space after '//' since we're here. @@ +80,5 @@ > //Enable the plugin > tabBrowser.selectedIndex = 1; > addonsManager.enableAddon({addon: selectedPlugin}); > > + // Check that the plugin state is Enabled in about:plugins Here you check the addon is enabled in addons manager, later bellow is for about:plugins. @@ +96,2 @@ > */ > +function pluginStateInAboutPlugins(pluginName) { Please add prefix 'a' for parameters. @@ +102,4 @@ > var nodeCollector = new domUtils.nodeCollector(controller.tabs.activeTab); > var pluginNames = nodeCollector.queryNodes(".plugname").nodes; > + var pluginState = nodeCollector.queryNodes("[label=state]").nodes; > + var pluginIndex = -1; I thinks there is no need for this variable, we could use exists here as before. @@ +110,5 @@ > break; > } > } > > + var exists = pluginState[pluginIndex].parentNode.textContent.contains("Enabled"); This should be moved above in for, using i instead of pluginIndex

Win XP report : http://mozmill-crowd.blargon7.com/#/functional/report/1641bc2f6438595b86015bd9010fd080 Linux report : http://mozmill-crowd.blargon7.com/#/functional/report/1641bc2f6438595b86015bd9010f6ed5

Comment on attachment 710584 [details] [diff] [review] patch v1.1 Review of attachment 710584 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js @@ +58,5 @@ > }); > > var selectedPlugin = addonsManager.getAddons({attribute: "value", > value: plugin.id})[0]; > + var aPluginName = selectedPlugin.getNode().mAddon.name; There's no need for 'a' in this function, we want that for parameters only, the pluginStateInAboutPlugins method is the one with a parameter. @@ -98,5 @@ > tabBrowser.selectedIndex = 0; > controller.open("about:plugins"); > controller.waitForPageLoad(); > > - var exists = false; Why are you removing this? If we don't enter in if, won't the return be undefined? @@ +106,4 @@ > > for (var i = 0; i < pluginNames.length; i++) { > + if (pluginNames[i].textContent === aPluginName) { > + var exists = pluginState[i].parentNode.textContent.contains("Enabled"); No need for var when having exists set to false above as before.

Fixed as required, if it's all good I will return with reports.

Mario, looks good now, please add reports.

Linux report: http://mozmill-crowd.blargon7.com/#/functional/report/d96621f14ec6678200ea5c05ea4d5cbb Windows report: http://mozmill-crowd.blargon7.com/#/functional/report/d96621f14ec6678200ea5c05ea51c648

Mac report: http://mozmill-crowd.blargon7.com/#/functional/report/d96621f14ec6678200ea5c05eae55f81

Comment on attachment 711326 [details] [diff] [review] patch v1.2 Review of attachment 711326 [details] [diff] [review]: ----------------------------------------------------------------- Please use capital letters for commit messages. I have edited this now. Landed on default: http://hg.mozilla.org/qa/mozmill-tests/rev/c597668fba18 (default)

Comment on attachment 711326 [details] [diff] [review] patch v1.2 Review of attachment 711326 [details] [diff] [review]: ----------------------------------------------------------------- Mario, I would like to see that we update the helper method so we 1) rename it, to visualize what boolean value it returns or 2) don't return a boolean but the state itself Please file a follow-up bug so we can get this fixed. At the moment it's not clear what this method returns. Thanks. ::: tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js @@ +103,3 @@ > var nodeCollector = new domUtils.nodeCollector(controller.tabs.activeTab); > var pluginNames = nodeCollector.queryNodes(".plugname").nodes; > + var pluginState = nodeCollector.queryNodes("[label=state]").nodes; nit: This should be named pluginStates.