Closed Bug 865640 Opened 7 years ago Closed 6 years ago

3rd party plugin failures: "Shockwave Flash is disabled - 'true' should equal 'false'" and "Java Plug-in 1.7.0_21 has been disabled - got 'false'"

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

defect

Tracking

(firefox23 wontfix, firefox24 fixed, firefox25 fixed, firefox26 fixed, firefox27 fixed, firefox-esr17 fixed, firefox-esr24 fixed)

RESOLVED FIXED
Tracking Status
firefox23 --- wontfix
firefox24 --- fixed
firefox25 --- fixed
firefox26 --- fixed
firefox27 --- fixed
firefox-esr17 --- fixed
firefox-esr24 --- fixed

People

(Reporter: andrei, Assigned: cosmin-malutan)

References

()

Details

(Keywords: regression, Whiteboard: [mozmill-test-failure][mozmill-test-skipped][sprint2013-31])

Attachments

(8 files, 13 obsolete files)

4.32 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
26.29 KB, text/plain
Details
1.35 KB, text/plain
Details
2.62 KB, patch
Details | Diff | Splinter Review
1.50 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
11.23 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
11.33 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
1.46 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
Lots of failures on Nightly, OSX and Linux:

In /testAddons/testPluginDisableAffectsAboutPlugins.js :
http://mozmill-ci.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde158dd0e

and in /restartTests/testAddons_pluginDisabledAfterRestart/test1.js :
http://mozmill-ci.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde15aacbe
We should get this disabled and investigated.
Mario, can you work on a skip patch please?
Summary: Failure "Shockwave Flash is disabled - 'true' should equal 'false'" → 3rd party plugin failures: "Shockwave Flash is disabled - 'true' should equal 'false'" and "Java Plug-in 1.7.0_21 has been disabled - got 'false'"
Attached patch Skip patch (obsolete) — Splinter Review
Skip patch for Default and failing tests.
Attachment #741815 - Flags: review?(andreea.matei)
Comment on attachment 741815 [details] [diff] [review]
Skip patch

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

Please update the manifest files and also skip teardown() if there is one.
Attachment #741815 - Flags: review?(andreea.matei) → review-
Attached patch Skip patch v2 (obsolete) — Splinter Review
Updated skip patch.
Attachment #741815 - Attachment is obsolete: true
Attachment #741831 - Flags: review?(andreea.matei)
Attached patch Skip patchSplinter Review
Updated and fixed skip patch
Attachment #741831 - Attachment is obsolete: true
Attachment #741831 - Flags: review?(andreea.matei)
Attachment #741834 - Flags: review?(andreea.matei)
Comment on attachment 741834 [details] [diff] [review]
Skip patch

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

I have updated a small nit, landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/e0d36670ef28 (default)

Thanks Mario.
Attachment #741834 - Flags: review?(andreea.matei) → review+
Failed over the place today. Lets get this investigated. It might be a new regression.
Priority: -- → P1
Please take care of the product/component and flags when disabling failed tests.
Component: Mozmill → Mozmill Tests
Product: Testing → Mozilla QA
Whiteboard: [mozmill-test-failure][mozmill-test-skipped]
For the latest version of Nightly we have a "menu-list" instead of a "Disable/Enable" button, and this is not covered by our tests.
Assignee: nobody → mario.garbi
Thanks for the hint. So business as usual, please find the regressor and update flags and dependencies.
Priority: P1 → P2
I've created a patch that is in internal review at the moment that will handle this new menuList. Should I create a new bug for it or upload it here?
The change took place on 24.04.2013, following is the inbound tinderbox build that first had this new menuList:

ftp://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-linux/1366820510/
Looks like the Mozmill tests might be getting hung up on click-to-play? If so maybe we should consider running Mozmill tests with click-to-play disabled by default?
(In reply to mario garbi from comment #14)
> Reponsible pushlog :
> http://hg.mozilla.org/integration/mozilla-inbound/rev/450bbfd48532

Mario, please always add the regressing bug to the dependency list so we have the connection to it. Otherwise good work in nailing it down.

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #15)
> Looks like the Mozmill tests might be getting hung up on click-to-play? If
> so maybe we should consider running Mozmill tests with click-to-play
> disabled by default?

No, that's not related here. But we should indeed test how we handle click to play. Not sure if we have done this yet.
Attached patch Patch v1 (obsolete) — Splinter Review
Picked up where Mario left this, since he's on PTO this week and this was almost finished.

For 'plugins' we now use a menuList item instead of buttons.
Also unskipped affected tests.

Testruns:
OSX: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1c9fe6d
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1cdb066
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1cf627a
Attachment #743032 - Flags: review?(andreea.matei)
Status: NEW → ASSIGNED
Assignee: mario.garbi → andrei.eftimie
Comment on attachment 743032 [details] [diff] [review]
Patch v1

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

::: lib/addons.js
@@ +363,5 @@
> +      var alwaysActivateLabel = utils.getEntity(this.dtds, "cmd.alwaysActivate.label");
> +      this._controller.select(menu, null, alwaysActivateLabel);
> +      assert.waitFor(function() {
> +        return menu.node.label === alwaysActivateLabel;
> +      }, "The plugin has been enabled");

Why do we have to waitFor here? This should not be necessary.

@@ +504,5 @@
> +   *
> +   * @param {object} aSpec
> +   *        Information on which add-on to operate on
> +   *        Elements: addon  - Add-on element
> +   *                  button - Button (disable, enable, preferences, remove)

Why button? I believe Mario wanted it to rename to menu.

@@ +513,5 @@
> +  getAddonMenu : function AddonsManager_getAddonButton(aSpec) {
> +    var spec = aSpec || { };
> +    var addon = spec.addon;
> +    var menu = spec.menu;
> +    assert.ok(menu, arguments.callee.name + ": Menu has been specified.");

nit: please add a blank line before.

@@ +1153,5 @@
>        case "listView_element":
>          nodeCollector.queryAnonymousNode(subtype, value);
>          break;
> +      case "listView_stateMenu":
> +        nodeCollector.queryAnonymousNode("anonid", "tristate-menulist");

This is a kinda bad naming of the anonid. Is there no better property set which indicates the status in its name? If not we might wanna fix it in Firefox itself.
Attachment #743032 - Flags: review?(andreea.matei) → review-
(In reply to Henrik Skupin (:whimboo) from comment #18)
> Comment on attachment 743032 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 743032 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/addons.js
> @@ +363,5 @@
> > +      var alwaysActivateLabel = utils.getEntity(this.dtds, "cmd.alwaysActivate.label");
> > +      this._controller.select(menu, null, alwaysActivateLabel);
> > +      assert.waitFor(function() {
> > +        return menu.node.label === alwaysActivateLabel;
> > +      }, "The plugin has been enabled");
> 
> Why do we have to waitFor here? This should not be necessary.

The test would fail right after changing the state of the addon where we assert that it has been enabled / disabled.
Seems that code would execute before the plugin's state would actually be changed.

Waiting for the menuList to have the correct value fixes the issue.

> @@ +1153,5 @@
> >        case "listView_element":
> >          nodeCollector.queryAnonymousNode(subtype, value);
> >          break;
> > +      case "listView_stateMenu":
> > +        nodeCollector.queryAnonymousNode("anonid", "tristate-menulist");
> 
> This is a kinda bad naming of the anonid. Is there no better property set
> which indicates the status in its name? If not we might wanna fix it in
> Firefox itself.

Nothing seems to really stand out.

This is all that we have at DOM level:
> anonid="tristate-menulist"
> class="addon-control"
> tooltiptext="Change when this add-on runs"
> sizetopopup="pref"
> label="Always Activate"

All (old/standard) buttons have the "addon-control" class + each has a custom class like 'preferences', 'enable', 'disable', 'remove'
(In reply to Andrei Eftimie from comment #19)
> > Why do we have to waitFor here? This should not be necessary.
> 
> The test would fail right after changing the state of the addon where we
> assert that it has been enabled / disabled.
> Seems that code would execute before the plugin's state would actually be
> changed.
> 
> Waiting for the menuList to have the correct value fixes the issue.

I would consider it a bug in the select() method of the controller object. Means it should wait itself until the right option has been set. If this is not the case, lets file a bug and add a comment here why we have to wait for the target state.

> > > +      case "listView_stateMenu":
> > > +        nodeCollector.queryAnonymousNode("anonid", "tristate-menulist");
> > 
> > This is a kinda bad naming of the anonid. Is there no better property set
> > which indicates the status in its name? If not we might wanna fix it in
> > Firefox itself.
> 
> Nothing seems to really stand out.
> 
> This is all that we have at DOM level:
> > anonid="tristate-menulist"
> > class="addon-control"
> > tooltiptext="Change when this add-on runs"
> > sizetopopup="pref"
> > label="Always Activate"
> 
> All (old/standard) buttons have the "addon-control" class + each has a
> custom class like 'preferences', 'enable', 'disable', 'remove'

Blair, the anonid of the new plugin state menulist is kinda awkward. Was it set by design? Why hasn't it gotten a more meaningful id? Is it something you would be ok to get changed?
Flags: needinfo?(bmcbride)
(In reply to Henrik Skupin (:whimboo) from comment #20)
> Blair, the anonid of the new plugin state menulist is kinda awkward. Was it
> set by design? Why hasn't it gotten a more meaningful id? Is it something
> you would be ok to get changed?

No real reason - we were uninspired. If you can come up with one, I'd be happy for it to be changed.
Flags: needinfo?(bmcbride)
(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #21)
> No real reason - we were uninspired. If you can come up with one, I'd be
> happy for it to be changed.

Why can't it simply be state-menulist? Beside that we could also update the class to 'addon-control state' to be in sync with the other buttons (enable, disable, ...). So we could grab the element via the class.
Yep, both of those suggestions sound reasonable - happy to r+ a patch that did that.

(Just so you're aware, the UI for this may be changing sometime in the future - the current usage of a menulist was a bit of a compromise to get this shipped quickly. No ETA on when that may happen though.)
It started to happen on release, with the new merge. This was not reproducing before on beta.
http://mozmill-ci.blargon7.com/#/functional/report/452ec32f8deec0960aea87aa06f1649d

I'll check the skip patch or provide a new one for it.
This was due to bug 869242 and has now been fixed thanks to Henrik.
The latest patch doesn't apply anymore. Please get it updated to the latest changes which I have made in bug 869050.
Who can take this while Andrei is absent? I would like to see the test re-enabled before the next merge (default -> aurora) on Monday.
I will work on this issue while Andrei is out.
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] → [mozmill-test-failure][mozmill-test-skipped][sprint2013-31]
I have modified the patch, but it does not work properly anymore on MAC OS (tested on 10.6.8, 10.7.5 and 10.8.3) since Nightly 23.0a1 from 7th of May.

The problem is this changeset: http://hg.mozilla.org/mozilla-central/rev/dbe30ba57be1
What does not work? We would need more information about how the given change affects the patch. As of now I cannot say anything.
(In reply to Henrik Skupin (:whimboo) from comment #30)
> What does not work? We would need more information about how the given
> change affects the patch. As of now I cannot say anything.

Selecting an option from the menu does not work. For instance the line below that appears in Andrei's patch (disableAddon method)
this._controller.select(menu, null, neverActivateLabel);

NOTE: This happens only on MAC, but not on Linux.
I want to see the code you are executing here, Daniela. Only that will give me the information I can react on.
Attached patch patch v5 (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #32)
> I want to see the code you are executing here, Daniela. Only that will give
> me the information I can react on.
I have attached the patch containing the code I am executing.

It works properly on Linux and Windows but does not work on MAC OS (any version). Reports for the failure that appears on MAC are:
http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145e2c9562
http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145e2ca8e6
Well, the first thing you should always do when investigating issues is to run mozmill with the --show-all option. That will show you what's wrong.
Attached file show-all logs
(In reply to Henrik Skupin (:whimboo) from comment #34)
> Well, the first thing you should always do when investigating issues is to
> run mozmill with the --show-all option. That will show you what's wrong.

Please see the attached log file with the result of using --show-all option.

Based on details from comment #31, I think that the cause is that selecting an option from this menulist does not work on MAC.

The result of controller.select is pass, but the option is not really selected in the UI.

Since this happens only on MAC and not Linux/Windows and the issue does not reproduce manually, I think that this is an issue with select in mozmill 1.5.21 for this menulist.
Comment on attachment 747335 [details]
show-all logs

Why an RTF file? I hope it can be seen with text/plain.
Attachment #747335 - Attachment mime type: application/rtf → text/plain
So what are the next planned steps you want to do here?
(In reply to Henrik Skupin (:whimboo) from comment #37)
> So what are the next planned steps you want to do here?
We can make a workaround for now in case we do not want another release for mozmill 1.5. For mozmill 2.0 this issue will be fixed in bug 860662
Why do you think that this is a problem in Mozmill proper? Have you yet checked if select() gets the correct values to be able to successfully select the appropriate entry?
(In reply to Henrik Skupin (:whimboo) from comment #39)
> Why do you think that this is a problem in Mozmill proper? Have you yet
> checked if select() gets the correct values to be able to successfully
> select the appropriate entry?

I have checked that the values were correctly sent to the select method by adding dumps to the mozmill 1.5.21 code. I will attach the patch containing the dumps in the select method in a moment.

The dumps show that although we find the option to which to change to correctly, we are not changing it in this line: https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L867
Attached patch patch with dumpsSplinter Review
This is where I have added the dumps in mozmill 1.5.21 - select method
Just seen... why don't we make use of value but option?

> Option: Never Activate
> Value: undefined

That would be way more appropriate given that we usually should not work with elements by using a label or localized content.
(In reply to Henrik Skupin (:whimboo) from comment #42)
> Just seen... why don't we make use of value but option?

There is no value in the Javascript properties and all we have at DOM level was added in comment #19. Using index would also not be correct, because sometimes we have three options in the control: Ask to Activate, Activate and Never activate and sometime Ask to Activate does not appear there (for instance when the addon is considered vulnerable and we are asked to activate it). So, I think that using the option/label was the only way.
Ok, so the problem here is that the focus inside the select() method is not set to the menulist properly. Instead it remains on the window. When the method presses the VK_DOWN key you can see that the selection of the current plugin is moving. 

If it also happens in Mozmill 2 please file a bug with a simplified testcase.
(In reply to Henrik Skupin (:whimboo) from comment #44)
> Ok, so the problem here is that the focus inside the select() method is not
> set to the menulist properly. Instead it remains on the window. When the
> method presses the VK_DOWN key you can see that the selection of the current
> plugin is moving. 
> 
> If it also happens in Mozmill 2 please file a bug with a simplified testcase.
I have logged bug 871441 for this issue and attached the minimized testcase to it.
Depends on: 871441
Assignee: andrei.eftimie → dpetrovici
Yes, lets get this disabled immediately on esr17, until it has been fixed. The skip patch might apply already?
Attachment #743032 - Attachment is obsolete: true
Attached patch skip_patch_esr17Splinter Review
Skip patch for esr17. With previous patch it fails on esr
Comment on attachment 780232 [details] [diff] [review]
skip_patch_esr17

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

I have added a blank line between the end of test and skipping lines.

http://hg.mozilla.org/qa/mozmill-tests/rev/b259541f9320 (esr17)
Attachment #780232 - Flags: review+
Comment on attachment 798508 [details] [diff] [review]
patch_v5.1

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

Please update the commit message under our format.

::: lib/addons.js
@@ +366,5 @@
> +      var menu = this.getAddonMenu(spec);
> +      var alwaysActivateLabel = utils.getEntity(this.dtds, "cmd.alwaysActivate.label");
> +      this._controller.select(menu, null, alwaysActivateLabel);
> +
> +      // Bug 846277 - select() method does not wait for specified element to be selected

It does now, I fixed that and the bug is closed.

@@ +370,5 @@
> +      // Bug 846277 - select() method does not wait for specified element to be selected
> +      assert.waitFor(function() {
> +        return menu.node.label === alwaysActivateLabel;
> +      }, "The plugin has been enabled");
> +    } else {

This should be on a new line.

@@ +397,5 @@
> +      this._controller.select(menu, null, neverActivateLabel);
> +      assert.waitFor(function() {
> +        return menu.node.label === neverActivateLabel;
> +      }, "The plugin has been disabled");
> +    } else {

Same as above.

::: tests/functional/restartTests/testAddons_pluginDisabledAfterRestart/test1.js
@@ +72,2 @@
>  teardownModule.__force_skip__ = "Bug 865640 - Shockwave Flash and Java Plug-in are" +
>                               " disabled - 'true' should equal 'false'";

Why aren't we removing this skip as well?
Attachment #798508 - Flags: review?(andreea.matei) → review-
Attachment #798508 - Flags: review?(andrei.eftimie)
Attachment #798508 - Flags: review?(andreea.matei)
Attachment #798508 - Flags: review-
Attachment #800090 - Flags: review?(andrei.eftimie)
Attachment #800090 - Flags: review?(andreea.matei)
Attachment #800090 - Flags: review?
Comment on attachment 798508 [details] [diff] [review]
patch_v5.1

Sorry for this, I added review flags latter for the bad patch.
Attachment #798508 - Flags: review?(andrei.eftimie)
Attachment #798508 - Flags: review?(andreea.matei)
Attachment #798508 - Flags: review-
Comment on attachment 800090 [details] [diff] [review]
patch v6.0

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

This fails in testPluginDisableAffectsAboutPlugins.js on localised builds (tested with de and zh-TW):
> QuickTime Plug-in 7.7.1 state is Enabled in about:plugins - got 'false'

::: lib/addons.js
@@ +395,5 @@
> +    else {
> +      spec.button = "disable";
> +      var button = this.getAddonButton(spec);
> +      this._controller.click(button);
> +    }

Since both the enableAddon and the disableAddon share most of their code it would be great to abstract them to be more DRY.
Possibly a new private method that both should call.
Attachment #800090 - Flags: review?(andrei.eftimie)
Attachment #800090 - Flags: review?(andreea.matei)
Attachment #800090 - Flags: review-
Comment on attachment 805325 [details] [diff] [review]
patch v7.0

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

This looks good, only a few issues and nits to fix and we can land this.
Also works fine with mozmill-2.0.

::: lib/addons.js
@@ +31,5 @@
>    "local",
>    "remote"
>  ];
>  
> +// Ids for getting labels dtds

These are not ID's

@@ +607,5 @@
> +    // Handle 'plugins' panel separately from the others due to different UI
> +    if (addonCategory === "plugin") {
> +      spec.menu = "state";
> +      var menu = this.getAddonMenu(spec);
> +      var stateLabel = utils.getEntity(this.dtds, ADDON_STATE_LABEL[aState]);

We shouldn't need to evaluate the dtd each time we change the state of an addon.

I see we have a few of them in the this library.
Please file a followup bug to cache these calls.
(Probably evaluate them once we we initialise the object, and save them in the objects' namespace)
Check if other libraries might want this too.

::: tests/functional/testAddons/manifest.ini
@@ -1,3 @@
>  [testManagerKeyboardShortcut.js]
>  [testPluginDisableAffectsAboutPlugins.js]
> -disabled = Bug 865640 - Shockwave Flash and Java Plug-in are disabled -'true' should equal 'false'

This file doesn't apply cleanly anymore.

::: tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js
@@ +13,4 @@
>  
>  const BASE_URL = collector.addHttpResource("../../../data/");
>  const TEST_DATA = BASE_URL + "layout/mozilla.html";
> +// We have to take the plugin state from chrome, so it will work on localized builds

I don't think we need this comment, it is pretty self-explanatory

@@ +13,5 @@
>  
>  const BASE_URL = collector.addHttpResource("../../../data/");
>  const TEST_DATA = BASE_URL + "layout/mozilla.html";
> +// We have to take the plugin state from chrome, so it will work on localized builds
> +const PLUGIN_ENABLED  = utils.getProperty("chrome://global/locale/plugins.properties",

nit: you have 2 spaces before the assignment operand
Attachment #805325 - Flags: review?(andrei.eftimie)
Attachment #805325 - Flags: review?(andreea.matei)
Attachment #805325 - Flags: review-
Attached patch patch v7.1 (obsolete) — Splinter Review
Fixed the nit changes and filled bug 919435, for caching dtd calls.
Attachment #805325 - Attachment is obsolete: true
Attachment #808462 - Flags: review?
Attachment #808462 - Flags: review?(andrei.eftimie)
Attachment #808462 - Flags: review?(andreea.matei)
Attachment #808462 - Flags: review?
Comment on attachment 808462 [details] [diff] [review]
patch v7.1

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

::: lib/addons.js
@@ +20,5 @@
>  const TIMEOUT_REMOTE = 30000;
>  
> +const ADDON_STATE_LABEL = {
> +  enabled : "cmd.alwaysActivate.label",
> +  disabled : "cmd.neverActivate.label"

I wouldn't put those into the global scope but in the ui class which makes use of it. As used here it can cause problems in the future if the same DTD was used on different places but then has been separated.

@@ +486,5 @@
> +   *
> +   * @returns Add-on button
> +   * @type {ElemBase}
> +   */
> +  getAddonMenu : function AddonsManager_getAddonButton(aSpec) {

The function name differs from the internal name. Also it's unclear for me when seeing this method what it does. Does it return the menu or an element from the menu?

@@ +600,5 @@
> +   *        State of addon to be set, it can be "enabled" or "disabled"
> +   */
> +  setAddonState : function AddonsManager_setAddonState(aSpec, aState) {
> +    var spec = aSpec || { };
> +    var addonCategory = this.selectedCategory.getNode("name").value.split("/")[3];

What is this doing? I would love to see a comment here. But please explain first.

@@ +602,5 @@
> +  setAddonState : function AddonsManager_setAddonState(aSpec, aState) {
> +    var spec = aSpec || { };
> +    var addonCategory = this.selectedCategory.getNode("name").value.split("/")[3];
> +
> +    // Handle 'plugins' panel separately from the others due to different UI

nit: remove ´panel´

@@ +607,5 @@
> +    if (addonCategory === "plugin") {
> +      spec.menu = "state";
> +      var menu = this.getAddonMenu(spec);
> +      var stateLabel = utils.getEntity(this.dtds, ADDON_STATE_LABEL[aState]);
> +      this._controller.select(menu, null, stateLabel);

If you want to cover that, you will also take the 'always ask' state into account.

@@ +610,5 @@
> +      var stateLabel = utils.getEntity(this.dtds, ADDON_STATE_LABEL[aState]);
> +      this._controller.select(menu, null, stateLabel);
> +    }
> +    else {
> +      spec.button = aState;

I wonder if we cannot do ´spec.state´ for both types.
Attachment #808462 - Flags: review?(andrei.eftimie)
Attachment #808462 - Flags: review?(andreea.matei)
Attachment #808462 - Flags: review-
Comment on attachment 808462 [details] [diff] [review]
patch v7.1

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

::: lib/addons.js
@@ +20,5 @@
>  const TIMEOUT_REMOTE = 30000;
>  
> +const ADDON_STATE_LABEL = {
> +  enabled : "cmd.alwaysActivate.label",
> +  disabled : "cmd.neverActivate.label"

You will need to amend these names to to enable / disable

@@ +361,5 @@
>     *        Information on which add-on to operate on
>     *        Elements: addon - Add-on element
>     */
>    enableAddon : function AddonsManager_enableAddon(aSpec) {
> +    this.setAddonState(aSpec, "enabled");

This should be "enable"

@@ +372,5 @@
>     *        Information on which add-on to operate on
>     *        Elements: addon - Add-on element
>     */
>    disableAddon : function AddonsManager_disableAddon(aSpec) {
> +    this.setAddonState(aSpec, "disabled");

And "disable"
Comment on attachment 808462 [details] [diff] [review]
patch v7.1

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

::: lib/addons.js
@@ +361,5 @@
>     *        Information on which add-on to operate on
>     *        Elements: addon - Add-on element
>     */
>    enableAddon : function AddonsManager_enableAddon(aSpec) {
> +    this.setAddonState(aSpec, "enabled");

I also thought that first, but here we are not referring to an action but a state. And the state is 'enabled' or 'disabled'. So that is correct.
(In reply to Henrik Skupin (:whimboo) from comment #59)
> @@ +600,5 @@
> > +   *        State of addon to be set, it can be "enabled" or "disabled"
> > +   */
> > +  setAddonState : function AddonsManager_setAddonState(aSpec, aState) {
> > +    var spec = aSpec || { };
> > +    var addonCategory = this.selectedCategory.getNode("name").value.split("/")[3];
> 
> What is this doing? I would love to see a comment here. But please explain
> first.
I get the current category(extensions/plugins) element under the Add-ons Manager tab, and from his value property (ex addons://list/plugin) I get the last string, so I will know how to treat the addon as plugin or extension.
Comment on attachment 808462 [details] [diff] [review]
patch v7.1

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

::: lib/addons.js
@@ +361,5 @@
>     *        Information on which add-on to operate on
>     *        Elements: addon - Add-on element
>     */
>    enableAddon : function AddonsManager_enableAddon(aSpec) {
> +    this.setAddonState(aSpec, "enabled");

Semantically yes, referring to state is correct. But we pass this string on, see next comment.

@@ +610,5 @@
> +      var stateLabel = utils.getEntity(this.dtds, ADDON_STATE_LABEL[aState]);
> +      this._controller.select(menu, null, stateLabel);
> +    }
> +    else {
> +      spec.button = aState;

The "state" string is passed here, and we construct the final name (eg. "listView_enabledButton") using it.
This fails as we expect "listView_enableButton")
(In reply to Cosmin Malutan from comment #62)
> I get the current category(extensions/plugins) element under the Add-ons
> Manager tab, and from his value property (ex addons://list/plugin) I get the
> last string, so I will know how to treat the addon as plugin or extension.

We have getCategoryId() for that purpose. There is no need to re-invent the wheel.
Attached patch patch v7.2 (obsolete) — Splinter Review
(In reply to Andrei Eftimie from comment #63)
> The "state" string is passed here, and we construct the final name (eg.
> "listView_enabledButton") using it.
> This fails as we expect "listView_enableButton")
I updated the patch and for extensions I set the state to "enabled"/"disabled" by clicking on "enable"/"disable" button.


Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219239044
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee69721922dd48
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee69721922ce68
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219233033
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219237da5
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee69721922f18a
Attachment #808462 - Attachment is obsolete: true
Attachment #809125 - Flags: review?(andrei.eftimie)
Attachment #809125 - Flags: review?(andreea.matei)
Comment on attachment 809125 [details] [diff] [review]
patch v7.2

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

It seems we still have to figure out some things.

::: lib/addons.js
@@ +49,5 @@
>  function AddonsManager(aController) {
>    this._controller = aController;
>    this._tabBrowser = new tabs.tabBrowser(this._controller);
> +  this._addonStateLabel = {
> +    askToActivate : "cmd.askToActivate.label",

I don't see this label used anywhere.

@@ +493,5 @@
> +   */
> +  getAddonMenu : function AddonsManager_getAddonMenu(aSpec) {
> +    var spec = aSpec || { };
> +    var addon = spec.addon;
> +    var menu = spec.menu;

These 2 lines seem redundant.
I don't see any advantage to declare them as local variables when we can use them directly as `spec.addon` and `spec.menu` in the code below.

@@ +614,5 @@
> +    else {
> +      if (aSpec.state === "enabled")
> +        aSpec.button = "enable";
> +      else if (aSpec.state === "disabled")
> +        aSpec.button = "disable";

I would really like for us to handle this differently.
If we keep passing the name around, lets not change it halfway through the stack.
Attachment #809125 - Flags: review?(andrei.eftimie)
Attachment #809125 - Flags: review?(andreea.matei)
Attachment #809125 - Flags: review-
Comment on attachment 811002 [details] [diff] [review]
patch_v7.3

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

::: lib/addons.js
@@ +595,5 @@
> +   * Set addon enabled/disabled state
> +   *
> +   * @param {object} aSpec
> +   *        Information on which add-on to operate on
> +   *        Elements: addon - Add-on element

I don't see spec.action described here.
Attached patch patch v7.4 (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #68)
> I don't see spec.action described here.
Thanks Henrik for catch, I missed that.
Attachment #811002 - Attachment is obsolete: true
Attachment #811002 - Flags: review?(andrei.eftimie)
Attachment #811002 - Flags: review?(andreea.matei)
Attachment #811019 - Flags: review?(andrei.eftimie)
Attachment #811019 - Flags: review?(andreea.matei)
Comment on attachment 811019 [details] [diff] [review]
patch v7.4

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

Just one more thing.

::: tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js
@@ +13,5 @@
>  
>  const BASE_URL = collector.addHttpResource("../../../data/");
>  const TEST_DATA = BASE_URL + "layout/mozilla.html";
> +const PLUGIN_ENABLED = utils.getProperty("chrome://global/locale/plugins.properties",
> +                                         "state_enabled");

This should be separated from the other 2 consts that make a single block. 
But besides that, I'm not sure this should even be a constant, we're using it just once and we've taken properties before when we need it, as a variable.
Attachment #811019 - Flags: review?(andrei.eftimie)
Attachment #811019 - Flags: review?(andreea.matei)
Attachment #811019 - Flags: review-
Attached patch patch v7.5 (obsolete) — Splinter Review
Thanks Andreea, fixed the nit.
Attachment #811019 - Attachment is obsolete: true
Attachment #811106 - Flags: review?(andrei.eftimie)
Attachment #811106 - Flags: review?(andreea.matei)
Comment on attachment 811106 [details] [diff] [review]
patch v7.5

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

::: lib/addons.js
@@ +484,5 @@
> +   *
> +   * @param {object} aSpec
> +   *        Information on which add-on to operate on
> +   *        Elements: addon - Add-on element
> +   *                  menu  - Menu (disable, enable, preferences, remove)

Sorry but what is this menu then? The one I see has 'always active', 'never activate' and 'ask to activate'.

@@ +595,5 @@
> +   * Set addon enabled/disabled state
> +   *
> +   * @param {object} aSpec
> +   *        Information on which add-on to operate on
> +   *        Elements: addon - Add-on element

Still no action property described here.
Attachment #811106 - Flags: review?(andrei.eftimie)
Attachment #811106 - Flags: review?(andreea.matei)
Attachment #811106 - Flags: review-
Attached patch patch v7.6 (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #72)
> > +   *                  menu  - Menu (disable, enable, preferences, remove)
> 
> Sorry but what is this menu then? The one I see has 'always active', 'never
> activate' and 'ask to activate'.
Here I refer to menu element I retrieve for the specific addon, and is "state"(-menu) or "details-state"(-menu).
Attachment #811106 - Attachment is obsolete: true
Attachment #811907 - Flags: review?(andrei.eftimie)
Attachment #811907 - Flags: review?(andreea.matei)
Comment on attachment 811907 [details] [diff] [review]
patch v7.6

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

::: lib/addons.js
@@ +597,5 @@
> +   * @param {object} aSpec
> +   *        Information on which add-on to operate on
> +   *        Elements: addon  - Add-on element
> +   *                  action - Action to be taken on addons state,
> +   *                           can be "enable" or "disable"

What about 'ask to activate' for the state of plugins? I don't see it listed here.
(In reply to Henrik Skupin (:whimboo) from comment #74)
> Comment on attachment 811907 [details] [diff] [review]
> patch v7.6
> 
> Review of attachment 811907 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/addons.js
> @@ +597,5 @@
> > +   * @param {object} aSpec
> > +   *        Information on which add-on to operate on
> > +   *        Elements: addon  - Add-on element
> > +   *                  action - Action to be taken on addons state,
> > +   *                           can be "enable" or "disable"
> 
> What about 'ask to activate' for the state of plugins? I don't see it listed
> here.
We had that in a previous patches, but as Andrei said on comment 66 we never use that state, so do you want to have it in our library, just in case ?
(In reply to Cosmin Malutan from comment #75)
> > What about 'ask to activate' for the state of plugins? I don't see it listed
> > here.
> We had that in a previous patches, but as Andrei said on comment 66 we never
> use that state, so do you want to have it in our library, just in case ?

It's a library and even if we don't make use of it at the moment, it has to be listed and supported. Not sure why we shouldn't add those possible values.
Attached patch patch_v7.7Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #74)
> > +   *                  action - Action to be taken on addons state,
> > +   *                           can be "enable" or "disable"
> 
> What about 'ask to activate' for the state of plugins? I don't see it listed
> here.
I added 'ask to activate' as a state.
After a closer look I've seen that the Addon has only one menu ('state') in both list-view and detail-view, so I removed that parameter. 

Test-run reports:
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a7505d45b8
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a7505cb0d9
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a7505ca883
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a7505ca477
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a7505ca536
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a7505c9838
Attachment #811907 - Attachment is obsolete: true
Attachment #811907 - Flags: review?(andrei.eftimie)
Attachment #811907 - Flags: review?(andreea.matei)
Attachment #812522 - Flags: review?(andrei.eftimie)
Attachment #812522 - Flags: review?(andreea.matei)
(In reply to Cosmin Malutan from comment #77)
> After a closer look I've seen that the Addon has only one menu ('state') in
> both list-view and detail-view, so I removed that parameter. 
I'm talking about getAddonMenu method here.
Comment on attachment 812522 [details] [diff] [review]
patch_v7.7

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

I think we're done here.

Lets get this landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/66f5c1b35fb0 (default)

Lets see this pass on CI then look into backporting the changes
Attachment #812522 - Flags: review?(andrei.eftimie)
Attachment #812522 - Flags: review?(andreea.matei)
Attachment #812522 - Flags: review+
Attachment #812522 - Flags: checkin+
Comment on attachment 813494 [details] [diff] [review]
865640-beta.patch

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

Landed backports:
http://hg.mozilla.org/qa/mozmill-tests/rev/48c1c89c42b5 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/f5eebbf1fec5 (mozilla-beta)

I've noticed that in the beta patch you also took out `this.scrollAddonIntoView(aSpec);`
That was introduced in bug 780957 and is no longer needed except ESR17

Please file a bug to get rid of that method and all references to it.
We should find it only on mozilla-beta, mozilla-release, mozilla-esr24
We should leave it on mozilla-esr17
Attachment #813494 - Flags: review?(andrei.eftimie)
Attachment #813494 - Flags: review?(andreea.matei)
Attachment #813494 - Flags: review+
Attachment #813494 - Flags: checkin+
Comment on attachment 813743 [details] [diff] [review]
865640-esr17.patch

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

Beta patch transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/be10f94118ec (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/ddc932cf00df (mozilla-esr24)

And ESR17 patch landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/ed3ad62e5169 (mozilla-esr17)
Attachment #813743 - Flags: review?(andrei.eftimie)
Attachment #813743 - Flags: review?(andreea.matei)
Attachment #813743 - Flags: review+
Attachment #813743 - Flags: checkin+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.