865640 - 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'"

Status: RESOLVED FIXED

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

Description

[Andrei Eftimie]

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

Comment 1

[Andreea Matei [:AndreeaMatei]]

We should get this disabled and investigated.
Mario, can you work on a skip patch please?

Comment 2

[Andrei Eftimie]

Also Java seems affected, updated the summary:

http://mozmill-ci.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde15cfa4c
http://mozmill-ci.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde15d20a9
http://mozmill-ci.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde15c6d76

Comment 3

[mario garbi]

Attachment: Skip patch

Skip patch for Default and failing tests.

Comment 4

[Andreea Matei [:AndreeaMatei]]

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.

Comment 5

[mario garbi]

Attachment: Skip patch v2

Updated skip patch.

Comment 6

[mario garbi]

Attachment: Skip patch

Updated and fixed skip patch

Comment 7

[Andreea Matei [:AndreeaMatei]]

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.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Failed over the place today. Lets get this investigated. It might be a new regression.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Please take care of the product/component and flags when disabling failed tests.

Comment 10

[mario garbi]

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.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks for the hint. So business as usual, please find the regressor and update flags and dependencies.

Comment 12

[mario garbi]

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?

Comment 13

[mario garbi]

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/

Comment 14

[mario garbi]

Reponsible pushlog : http://hg.mozilla.org/integration/mozilla-inbound/rev/450bbfd48532

Comment 15

[u279076]

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?

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 17

[Andrei Eftimie]

Attachment: Patch v1

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

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 19

[Andrei Eftimie]

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

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 23

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 24

[Andreea Matei [:AndreeaMatei]]

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.

Comment 25

[Andreea Matei [:AndreeaMatei]]

This was due to bug 869242 and has now been fixed thanks to Henrik.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The latest patch doesn't apply anymore. Please get it updated to the latest changes which I have made in bug 869050.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 28

[Daniela Petrovici]

I will work on this issue while Andrei is out.

Comment 29

[Daniela Petrovici]

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

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

What does not work? We would need more information about how the given change affects the patch. As of now I cannot say anything.

Comment 31

[Daniela Petrovici]

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

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I want to see the code you are executing here, Daniela. Only that will give me the information I can react on.

Comment 33

[Daniela Petrovici]

Attachment: patch v5

(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

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 35

[Daniela Petrovici]

Attachment: 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 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 747335 [details]
show-all logs

Why an RTF file? I hope it can be seen with text/plain.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So what are the next planned steps you want to do here?

Comment 38

[Daniela Petrovici]

(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

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 40

[Daniela Petrovici]

Attachment: logs with dumps from mozmill - controller.select method

(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

Comment 41

[Daniela Petrovici]

Attachment: patch with dumps

This is where I have added the dumps in mozmill 1.5.21 - select method

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 43

[Daniela Petrovici]

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

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 45

[Daniela Petrovici]

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

Comment 46

[Cosmin Malutan, [:cosmin-malutan]]

Happened today on esr17 with Linux Ubuntu (12.10 x86 & x64) and (12.10 x86 & x64), 

http://mozmill-daily.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d610dfadc
http://mozmill-daily.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d610e163d
http://mozmill-daily.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d610e15f8
http://mozmill-daily.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d610e26d3

Comment 47

[Andreea Matei [:AndreeaMatei]]

We might want to disable this on esr17 as well until we get unblocked here.

http://mozmill-daily.blargon7.com/#/functional/failure?branch=All&platform=All&from=2013-04-25&to=&test=%2FtestAddons%2FtestPluginDisableAffectsAboutPlugins.js&func=testPluginDisableAffectsAboutPlugins.js%3A%3AtestDisableEnablePlugin

Comment 48

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yes, lets get this disabled immediately on esr17, until it has been fixed. The skip patch might apply already?

Comment 49

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: skip_patch_esr17

Skip patch for esr17. With previous patch it fails on esr

Comment 50

[Andreea Matei [:AndreeaMatei]]

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)

Comment 51

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v5.1

I updated the patch. 
Here are the reports:
http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a1a7181b
http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a1a72512
http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a1a95e7b

Comment 52

[Andreea Matei [:AndreeaMatei]]

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?

Comment 53

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v6.0

Hi Andreea, I updated the patch and I bring new reports.

http://mozmill-crowd.blargon7.com/#/functional/report/fb97b6210ae70da1b9ace6744531a654
http://mozmill-crowd.blargon7.com/#/functional/report/fb97b6210ae70da1b9ace67445318d78

http://mozmill-crowd.blargon7.com/#/functional/report/fb97b6210ae70da1b9ace67445316fb2
http://mozmill-crowd.blargon7.com/#/functional/report/fb97b6210ae70da1b9ace67445317583

http://mozmill-crowd.blargon7.com/#/functional/report/fb97b6210ae70da1b9ace6744531848d
http://mozmill-crowd.blargon7.com/#/functional/report/fb97b6210ae70da1b9ace674453161e8

Comment 54

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 798508 [details] [diff] [review]
patch_v5.1

Sorry for this, I added review flags latter for the bad patch.

Comment 55

[Andrei Eftimie]

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.

Comment 56

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v7.0

> 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'
> 
Thanks for catch Andrei , I fixed that to.
Reports
http://mozmill-crowd.blargon7.com/#/functional/report/dc3d4d2f66af31b9e75e291fa0b4ee21
http://mozmill-crowd.blargon7.com/#/functional/report/dc3d4d2f66af31b9e75e291fa0b45202

http://mozmill-crowd.blargon7.com/#/functional/report/dc3d4d2f66af31b9e75e291fa0b3bcc8
http://mozmill-crowd.blargon7.com/#/functional/report/dc3d4d2f66af31b9e75e291fa0b4b728

http://mozmill-crowd.blargon7.com/#/functional/report/dc3d4d2f66af31b9e75e291fa0b94f1f
http://mozmill-crowd.blargon7.com/#/functional/report/dc3d4d2f66af31b9e75e291fa0b9d6de

Comment 57

[Andrei Eftimie]

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

Comment 58

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v7.1

Fixed the nit changes and filled bug 919435, for caching dtd calls.

Comment 59

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 60

[Andrei Eftimie]

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 61

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 62

[Cosmin Malutan, [:cosmin-malutan]]

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

[Andrei Eftimie]

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

Comment 64

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 65

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v7.2

(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

Comment 66

[Andrei Eftimie]

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.

Comment 67

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v7.3

I made the changes.
Reports
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219a49b74
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219a4bfa6
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219c5c35d
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219c4b77f
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219a4bd3e
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219a4d27e

Comment 68

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 69

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v7.4

(In reply to Henrik Skupin (:whimboo) from comment #68)
> I don't see spec.action described here.
Thanks Henrik for catch, I missed that.

Comment 70

[Andreea Matei [:AndreeaMatei]]

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.

Comment 71

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v7.5

Thanks Andreea, fixed the nit.

Comment 72

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 73

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v7.6

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

Comment 74

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 75

[Cosmin Malutan, [:cosmin-malutan]]

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

Comment 76

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 77

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v7.7

(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

Comment 78

[Cosmin Malutan, [:cosmin-malutan]]

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

[Andrei Eftimie]

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

Comment 80

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Andrei Eftimie from comment #79)
> Lets see this pass on CI then look into backporting the changes

Hi Andrei,
I returned with backports for Aurora, the patch applies cleanly and fixes the problem.

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d10361
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d0cc3a
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d10393
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d0d22e
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d0e0fc
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d11311

Comment 81

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 865640-beta.patch

Here is the patch that applies on Beta
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d29526
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d31e75
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d27f55
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d33b37
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d75257
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750d6a72a

Comment 82

[Andrei Eftimie]

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

Comment 83

[Cosmin Malutan, [:cosmin-malutan]]

The beta patch applies cleanly on mozilla-release too:
Here are the reports:
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750e04fb4
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750df9cd4
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750e0223c
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750df75ce
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750e05eaf
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750dfdcb9

Comment 84

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 865640-esr17.patch

This fix does not applies for Esr17 because there we don't have list element for enabling/disabling  elements , but buttons so here is the unskip patch, and reports to see it passes, if it fails on Esr17 we have to file a new bug.

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750f3aad7
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750f41e81
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750f412ba
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750f3a673
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750f43826
http://mozmill-crowd.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a750f3c564

Comment 85

[Andrei Eftimie]

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)