Closed
Bug 834632
Opened 12 years ago
Closed 12 years ago
Update "testPluginDisableAffectsAboutPlugins.js" so it works with current features of Firefox about:plugins
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(firefox20 unaffected, firefox21 fixed)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox20 | --- | unaffected |
firefox21 | --- | fixed |
People
(Reporter: mario.garbi, Assigned: mario.garbi)
References
Details
(Whiteboard: [mozmill-test-failure] s=130128 u=enhancement c=addons p=1)
Attachments
(2 files, 3 obsolete files)
1.42 KB,
patch
|
whimboo
:
review+
whimboo
:
checkin+
|
Details | Diff | Splinter Review |
4.20 KB,
patch
|
AndreeaMatei
:
review+
whimboo
:
feedback-
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•12 years ago
|
||
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.
Comment 2•12 years ago
|
||
Most likely we should disable the test for now so we do not have tons of failures over the weekend.
Updated•12 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•12 years ago
|
||
It's a skip patch until I come up with a working patch.
Attachment #706329 -
Flags: review?(hskupin)
Attachment #706329 -
Flags: review?(dave.hunt)
Attachment #706329 -
Flags: review?(andreea.matei)
Comment 4•12 years ago
|
||
Comment on attachment 706329 [details] [diff] [review]
patch v0.1 Skip Patch
Review of attachment 706329 [details] [diff] [review]:
-----------------------------------------------------------------
You missed the manifest file.
Attachment #706329 -
Flags: review?(hskupin)
Attachment #706329 -
Flags: review?(dave.hunt)
Attachment #706329 -
Flags: review?(andreea.matei)
Attachment #706329 -
Flags: review-
Assignee | ||
Comment 5•12 years ago
|
||
Skip patch until I come with a working patch.
Attachment #706329 -
Attachment is obsolete: true
Attachment #706340 -
Flags: review?(hskupin)
Attachment #706340 -
Flags: review?(dave.hunt)
Attachment #706340 -
Flags: review?(andreea.matei)
Comment 6•12 years ago
|
||
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)
Attachment #706340 -
Flags: review?(hskupin)
Attachment #706340 -
Flags: review?(dave.hunt)
Attachment #706340 -
Flags: review?(andreea.matei)
Attachment #706340 -
Flags: review+
Attachment #706340 -
Flags: checkin+
Updated•12 years ago
|
status-firefox20:
--- → unaffected
status-firefox21:
--- → disabled
Updated•12 years ago
|
Whiteboard: s=130128 u=enhancement c=addons p=1 → [mozmill-test-failure][mozmill-test-skipped] s=130128 u=enhancement c=addons p=1
Updated•12 years ago
|
Priority: -- → P2
Assignee | ||
Comment 7•12 years ago
|
||
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
Attachment #709048 -
Flags: review?(hskupin)
Attachment #709048 -
Flags: review?(dave.hunt)
Attachment #709048 -
Flags: review?(andreea.matei)
Comment 8•12 years ago
|
||
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
Attachment #709048 -
Flags: review?(hskupin)
Attachment #709048 -
Flags: review?(dave.hunt)
Attachment #709048 -
Flags: review?(andreea.matei)
Attachment #709048 -
Flags: review-
Assignee | ||
Comment 9•12 years ago
|
||
Win XP report :
http://mozmill-crowd.blargon7.com/#/functional/report/1641bc2f6438595b86015bd9010fd080
Linux report :
http://mozmill-crowd.blargon7.com/#/functional/report/1641bc2f6438595b86015bd9010f6ed5
Attachment #709048 -
Attachment is obsolete: true
Attachment #710584 -
Flags: review?(andreea.matei)
Comment 10•12 years ago
|
||
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.
Attachment #710584 -
Flags: review?(andreea.matei) → review-
Assignee | ||
Comment 11•12 years ago
|
||
Fixed as required, if it's all good I will return with reports.
Attachment #710584 -
Attachment is obsolete: true
Attachment #711326 -
Flags: review?(andreea.matei)
Comment 12•12 years ago
|
||
Mario, looks good now, please add reports.
Assignee | ||
Comment 13•12 years ago
|
||
Assignee | ||
Comment 14•12 years ago
|
||
Comment 15•12 years ago
|
||
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)
Attachment #711326 -
Flags: review?(andreea.matei) → review+
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] s=130128 u=enhancement c=addons p=1 → [mozmill-test-failure] s=130128 u=enhancement c=addons p=1
Comment 16•12 years ago
|
||
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.
Attachment #711326 -
Flags: feedback-
Updated•5 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•