Last Comment Bug 739573 - Try to merge initAcountActionsButton() and updateButtons() in AccountManager.js
: Try to merge initAcountActionsButton() and updateButtons() in AccountManager.js
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 15.0
Assigned To: :aceman
:
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on:
Blocks: 738810
  Show dependency treegraph
 
Reported: 2012-03-27 05:40 PDT by :aceman
Modified: 2012-06-02 11:57 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (8.91 KB, patch)
2012-03-31 08:04 PDT, :aceman
iann_bugzilla: review+
florian: feedback+
Details | Diff | Splinter Review
patch v2 (9.16 KB, patch)
2012-04-10 13:23 PDT, :aceman
iann_bugzilla: review+
Details | Diff | Splinter Review
patch v3 (8.96 KB, patch)
2012-04-24 13:30 PDT, :aceman
mconley: review-
Details | Diff | Splinter Review
patch v4 (8.98 KB, patch)
2012-04-28 05:59 PDT, :aceman
iann_bugzilla: review-
Details | Diff | Splinter Review
patch v5 (9.22 KB, patch)
2012-04-29 08:20 PDT, :aceman
iann_bugzilla: review+
Details | Diff | Splinter Review
patch v6 (9.22 KB, patch)
2012-04-29 08:40 PDT, :aceman
mconley: review+
Details | Diff | Splinter Review
WIP patch of a test (6.98 KB, patch)
2012-05-10 14:03 PDT, :aceman
no flags Details | Diff | Splinter Review
mozmill test (9.72 KB, patch)
2012-05-11 13:27 PDT, :aceman
mconley: review-
Details | Diff | Splinter Review
mozmill test v2 (9.10 KB, patch)
2012-05-16 12:39 PDT, :aceman
mconley: review+
Details | Diff | Splinter Review
mozmill test v3 (7.66 KB, patch)
2012-05-22 14:05 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review
mozmill test v4 (8.56 KB, patch)
2012-05-24 13:41 PDT, :aceman
mconley: review+
Details | Diff | Splinter Review
mozmill test v5 (8.57 KB, patch)
2012-05-29 12:57 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description :aceman 2012-03-27 05:40:29 PDT
1. It looks like those 2 functions have almost the same logic, just set the properties on different objects (Seamonkey buttons, Thunderbird menuitems). That could probably be merged.
2. Besides removing duplicate code (and possibility of divergence) it seems it is also inefficient as TB also calls updateButtons() on each selection of an account in AM, decides the necessary properties just to then bail out of the function without setting anything.
3. the initAcountActionsButton() function has a typo.

Do I understand this correctly? Of course I'd like to merge only the common parts. The TB specific things like "Add IM account" item should be preserved.
Comment 1 :aceman 2012-03-31 08:04:51 PDT
Created attachment 611168 [details] [diff] [review]
patch

UI should be unchanged by this patch, it is just code reorganization.
I have tested this works fine in Thunderbird.

Ian: please check it works on Seamonkey.

Florian: please check the hiding of "Add IM account" item still works as intended.
Comment 2 Florian Quèze [:florian] [:flo] 2012-04-02 07:16:11 PDT
Comment on attachment 611168 [details] [diff] [review]
patch

(In reply to :aceman from comment #1)
> Florian: please check the hiding of "Add IM account" item still works as
> intended.

I haven't really tested this, just looked at the code, but I don't think it breaks the hiding of the "Add IM account" item. If you want to test it, you can use the advanced configuration editor to set mail.chat.enabled to false.
Comment 3 :aceman 2012-04-02 08:01:06 PDT
Thanks, I did that and it worked. I just wanted to be sure I am not missing anything (as I do not use/try IM yet).
Comment 4 Ian Neal 2012-04-08 15:13:05 PDT
Comment on attachment 611168 [details] [diff] [review]
patch

>+function initAccountActionsButtons(menupopup) {
>+  let account = getCurrentAccount();
>+  let tree = document.getElementById("accounttree");

>+  updateItems(tree, account,
You might as well inline tree and account as they're only used the once.
>+    document.getElementById("accountActionsAddMailAccount"),
>+    document.getElementById("accountActionsDropdownSetDefault"),
>+    document.getElementById("accountActionsDropdownRemove"));

>+function updateItems(tree, account, addAccountItem, setDefaultItem, removeItem) {
>+  let canSetDefault = true;
>+  let canDelete = true;
Is it worth having both these set as false to start with and adjusting the logic?

>+function updateBlockedItems(menuItems) {
As these aren't necessarily menuItems, perhaps use aItems as the variable name.

r=me with those answered/addressed.
Comment 5 :aceman 2012-04-10 13:23:28 PDT
Created attachment 613737 [details] [diff] [review]
patch v2

Updated, please see the inverted logic again.
Comment 6 Ian Neal 2012-04-23 15:39:41 PDT
Comment on attachment 613737 [details] [diff] [review]
patch v2

>+    return; // Thunderbird isn't using these
Nit: Comment starts with a capital letter so should end with a full stop.
Comment 7 :aceman 2012-04-24 13:30:07 PDT
Created attachment 618018 [details] [diff] [review]
patch v3

Well, capital T in Thunderbird does not really start a sentence :) But no problem, at least I fixed bitrot. I still bitrot myself with other patches, seems my preventive steps are not enough.
Comment 8 Mike Conley (:mconley) 2012-04-27 08:00:18 PDT
Comment on attachment 618018 [details] [diff] [review]
patch v3

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

aceman:

This looks really good - again, I'm so happy that you're cleaning up our old code.  Keep up the great work!

Just two notes below.  With those fixed, I'll give it another spin / review - and you should also get someone from SeaMonkey to look at this.

-Mike

::: mailnews/base/prefs/content/AccountManager.js
@@ +694,5 @@
> + * since these buttons aren't under the IFRAME.
> + */
> +function updateBlockedItems(aItems) {
> +  // for each loop does not work here.
> +  for (let i = 0; i < aItems.length; i++) {

squib and I have been preferring this looping pattern lately:

for each (let [, item] in Iterator(aItems)) {
  let prefstring = item.getAttribute("prefstring");
  ...
}

@@ +700,4 @@
>      if (!prefstring)
>        continue;
>  
> +    if (Services.prefs.prefIsLocked(prefstring))

The old code seems to check if the preference is locked, is a bool, and is set to "true":

"if (prefBranch.prefIsLocked(prefstring) && prefBranch.getBoolPref(prefstring))"

We should probably carry over that logic, or we might risk some edge-case breakage.
Comment 9 :aceman 2012-04-27 15:24:02 PDT
(In reply to Mike Conley (:mconley) from comment #8)
> Just two notes below.  With those fixed, I'll give it another spin / review
> - and you should also get someone from SeaMonkey to look at this.
Ian from Seamonkey looked at it.

> squib and I have been preferring this looping pattern lately:
> 
> for each (let [, item] in Iterator(aItems)) {
>   let prefstring = item.getAttribute("prefstring");
>   ...
> }
And what is the gain from this besides making the code unreadable for me? :)
Also, be aware I pass menupopup into as aItems and that is some special array that can't be iterated via for each (return 1 more element than there are menu items). So let's hope this Iterator stuff works.

> "if (prefBranch.prefIsLocked(prefstring) &&
> prefBranch.getBoolPref(prefstring))"
> 
> We should probably carry over that logic, or we might risk some edge-case
> breakage.

It does it only in the TB version, not SM. But OK let's make them the same. I do not understand why they would even need be locked (setting to true would be fine) but just carry it over.
Comment 10 :aceman 2012-04-28 05:59:20 PDT
Created attachment 619284 [details] [diff] [review]
patch v4
Comment 11 Ian Neal 2012-04-29 07:52:00 PDT
Comment on attachment 619284 [details] [diff] [review]
patch v4

>+    if (Services.prefs.prefIsLocked(prefstring) &&
>+        Services.prefs.getBoolPref(prefstring))
>+      item.setAttribute("disabled", true);
This would break SeaMonkey users that use this functionality as SM currently requires the pref to be only locked, not locked, a boolean and true.
Potentially there are edge cases from the Thunderbird side where it has only been locked not set to true, though prior to 2009 I believe the requirement was the same as for SM (i.e. locked pref only).
r- :(
Comment 12 :aceman 2012-04-29 08:20:56 PDT
Created attachment 619403 [details] [diff] [review]
patch v5

OK, so let's differentiate TB and SM.
Comment 13 Ian Neal 2012-04-29 08:32:47 PDT
Comment on attachment 619403 [details] [diff] [review]
patch v5


>+/**
>+ * Disable buttons/menu items if their control preference is locked.
>+ * Seamonkey: Not currently handled by WSM or the main loop yet
Nit: SeaMonkey not Seamonkey
>+ * since these buttons aren't under the IFRAME.
>+ *
>+ * @param aItems  the elements to be checked
>+ * @param aMustBeTrue  if true then the value of the pref must be set to true
Nit: "...pref must be boolean and set to true..."
>+ *                     to trigger the disabling (TB requires this, SM not)
Comment 14 :aceman 2012-04-29 08:40:12 PDT
Created attachment 619405 [details] [diff] [review]
patch v6

Ok, thanks.
Comment 15 Mike Conley (:mconley) 2012-04-30 10:28:03 PDT
aceman:

Can I convince you to write some tests for this?

-Mike
Comment 16 :aceman 2012-04-30 12:06:04 PDT
You'd have a hard time at that :) But you can try, depends on how long does it take you to teach tests to me :)
Comment 17 Mike Conley (:mconley) 2012-04-30 14:09:03 PDT
(In reply to :aceman from comment #16)
> You'd have a hard time at that :) But you can try, depends on how long does
> it take you to teach tests to me :)

Our tests can be found under mail/test/mozmill.  You'll want to create the test in the "accounts" subdirectory of that folder.

Your test should programmatically add a variety of account types during its setup, and then select them one by one, checking that the menulist displays the correct items as enabled and disabled.

Sound do-able?

-Mike
Comment 18 :aceman 2012-04-30 14:17:04 PDT
No, I have not yet created any xpcshell nor mozmill test.
Comment 19 Mike Conley (:mconley) 2012-05-02 12:58:41 PDT
Comment on attachment 619405 [details] [diff] [review]
patch v6

So the code looks good, and it appears to work as advertised, but I really would like to see some tests for this stuff.

Can you prepare some tests in another patch on this bug?
Comment 20 :aceman 2012-05-02 13:08:36 PDT
Not yet.
Depends on how much time you have to go through it on IRC :)
Comment 21 Mike Conley (:mconley) 2012-05-09 08:09:03 PDT
aceman:

Any luck on those tests?

-Mike
Comment 22 :aceman 2012-05-09 08:14:32 PDT
We can try to look at them today.
Comment 23 :aceman 2012-05-10 14:03:01 PDT
Created attachment 622905 [details] [diff] [review]
WIP patch of a test
Comment 24 :aceman 2012-05-11 00:54:07 PDT
The WIP test does not need any flags, it was only uploaded so that mconley can try it out and help me with some issues.

I'll upload a more complete version (testing all menu items) soon.
Comment 25 :aceman 2012-05-11 13:27:55 PDT
Created attachment 623274 [details] [diff] [review]
mozmill test

So my first mozmill test comes! Many thanks to mconley.

Mconley, please see if I do not import any unneded modules. I just copied many declarations and some may be useless for this test.
Comment 26 :aceman 2012-05-12 07:20:19 PDT
Mconley, I hardcoded some pref names in the test. Maybe I should fetch them from the attributes of the menutitems so that the test works if they change?
Comment 27 Mike Conley (:mconley) 2012-05-15 14:57:47 PDT
Comment on attachment 623274 [details] [diff] [review]
mozmill test

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

Great work aceman, especially considering this is your first Mozmill test.

Just a few things I noticed. After this, your patch will be in pretty decent shape, and I'll give it a spin.

::: mail/test/mozmill/account/test-account-actions.js
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

This should be MPL2.

@@ +34,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +var MODULE_NAME = "test-archive-actions";

I know you mostly copied most of these from other tests, but MODULE_NAME, RELATIVE_ROOT and MODULE_REQUIRES should be consts.

@@ +40,5 @@
> +var RELATIVE_ROOT = "../shared-modules";
> +var MODULE_REQUIRES = ["folder-display-helpers", "window-helpers",
> +                       "account-manager-helpers"];
> +
> +Components.utils.import("resource:///modules/mailServices.js");

You should be able to use Cu here instead of Components.utils.  It should be available automatically.

@@ +43,5 @@
> +
> +Components.utils.import("resource:///modules/mailServices.js");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +var mozmill = {};

Neither mozmill.js nor controller.js is used, and can be removed.

@@ +48,5 @@
> +Components.utils.import("resource://mozmill/modules/mozmill.js", mozmill);
> +var controller = {};
> +Components.utils.import("resource://mozmill/modules/controller.js", controller);
> +var elib = {};
> +Components.utils.import("resource://mozmill/modules/elementslib.js", elib);

I also don't think elib is required - see below for the instance where you used it.

@@ +50,5 @@
> +Components.utils.import("resource://mozmill/modules/controller.js", controller);
> +var elib = {};
> +Components.utils.import("resource://mozmill/modules/elementslib.js", elib);
> +
> +var accountManager = MailServices.accounts;

Why the alias?

@@ +57,5 @@
> +var nntpAccount;
> +var originalAccountCount;
> +
> +function setupModule(module) {
> +  let wh = collector.getModule("window-helpers");

You're not using wh, fdh, or amh, so these can be shortened to:

collector.getModule("window-helpers").installInto(module);
collector.getModule("folder-display-helpers").installInto(module);
etc

@@ +104,5 @@
> + * @param accountKey  the key of the account to select
> + * @param isSetAsDefaultEnabled  true if the menuitem should be enabled, false otherwise
> + * @param isRemoveEnabled        true if the menuitem should be enabled, false otherwise
> + * @param isAddAccountEnabled    true if the menuitems (Add Mail Account+Add Other Account)
> +                                 should be enabled, false otherwise

Nit, missing a star on this line.

@@ +106,5 @@
> + * @param isRemoveEnabled        true if the menuitem should be enabled, false otherwise
> + * @param isAddAccountEnabled    true if the menuitems (Add Mail Account+Add Other Account)
> +                                 should be enabled, false otherwise
> + */
> +function subtest_check_account_actions(amc, accountKey, isSetAsDefaultEnabled,

Nit - we tend to name function parameters starting with a, like:

aAccountKey, aController, aIsSetAsDefaultEnabled.

See https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Prefixes

@@ +109,5 @@
> + */
> +function subtest_check_account_actions(amc, accountKey, isSetAsDefaultEnabled,
> +                                       isRemoveEnabled, isAddAccountEnabled)
> +{
> +  amc.waitForElement(new elib.Elem(amc.window.document.getElementById("accountActionsButton")));

Controller.waitForElement is deprecated.

Do you really need to wait for the element to appear?  I don't think it's inserted dynamically...in which case, you can probably remove this wait.

@@ +112,5 @@
> +{
> +  amc.waitForElement(new elib.Elem(amc.window.document.getElementById("accountActionsButton")));
> +
> +  let rowIndex = 0; // count which row in the account tree we need to click
> +  let accountTreeNode = amc.window.document.getElementById("account-tree-children");

You should just be able to use amc.e("account-tree-children");

@@ +136,5 @@
> +  amc.click(amc.eid("accountActionsButton"), 5, 5);
> +  wait_for_popup_to_open(amc.e("accountActionsDropdown"));
> +
> +  let actionAddMailAccount = amc.window.document.getElementById("accountActionsAddMailAccount");
> +  assert_equals(actionAddMailAccount != undefined, true);

You can use assert_not_equals here, I believe.

@@ +139,5 @@
> +  let actionAddMailAccount = amc.window.document.getElementById("accountActionsAddMailAccount");
> +  assert_equals(actionAddMailAccount != undefined, true);
> +  assert_equals(!actionAddMailAccount.getAttribute("disabled"), isAddAccountEnabled);
> +
> +  let actionAddOtherAccount = amc.window.document.getElementById("accountActionsAddOtherAccount");

amc.e("accountActionsAddOtherAccount"). Happens a few more times in this function.

@@ +140,5 @@
> +  assert_equals(actionAddMailAccount != undefined, true);
> +  assert_equals(!actionAddMailAccount.getAttribute("disabled"), isAddAccountEnabled);
> +
> +  let actionAddOtherAccount = amc.window.document.getElementById("accountActionsAddOtherAccount");
> +  assert_equals(actionAddOtherAccount != undefined, true);

assert_not_equals. Also use a few times below when checking against undefined.

@@ +215,5 @@
> +  Services.prefs.unlockPref(disableItemPref);
> +  Services.prefs.getDefaultBranch("").deleteBranch(disableItemPref);
> +}
> +
> +function teardownModule(module) {

Please put the teardownModule directly below the setupModule.
Comment 28 :aceman 2012-05-16 12:39:02 PDT
Created attachment 624493 [details] [diff] [review]
mozmill test v2

Thanks, done.
Comment 29 Mike Conley (:mconley) 2012-05-22 13:39:33 PDT
Comment on attachment 624493 [details] [diff] [review]
mozmill test v2

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

Other than the two nits I found, this looks good. With those fixed, r=me.

Great job on your first Mozmill test!

::: mail/test/mozmill/account/test-account-actions.js
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

MPL 2.

@@ +40,5 @@
> +
> +Cu.import("resource:///modules/mailServices.js");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +var imapAccount;

We prefer let over var, and you might as well define them all in a row, like

let imapAccount, nntpAccount, originalAccountCount;
Comment 30 :aceman 2012-05-22 14:05:06 PDT
Created attachment 626173 [details] [diff] [review]
mozmill test v3

Thanks, done!
Comment 32 Mark Banner (:standard8, limited time in Dec) 2012-05-23 03:10:04 PDT
Unfortunately, this seems to have permanent orange on the Linux 32 bit builders, I've therefore backed out just the unit test:

https://hg.mozilla.org/comm-central/rev/7e8025cb9781

https://tbpl.mozilla.org/php/getParsedLog.php?id=11980780&tree=Thunderbird-Trunk

EST-START | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-actions.js | setupModule
Test Failure: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgAccountManager.createIncomingServer]
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-actions.js | test-account-actions.js::setupModule
TEST-START | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-actions.js | test_account_actions
Test Skipped: "setupModule failed."
WARNING | test-account-actions.js::test_account_actions | (SKIP) setupModule failed.
TEST-START | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-actions.js | teardownModule
Test Failure: Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIMsgAccountManager.removeAccount]
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-actions.js | test-account-actions.js::teardownModule


(I would have disabled it on Linux, but I don't think we can skip setupModule with skip-if)
Comment 33 :aceman 2012-05-23 03:42:10 PDT
Did all other platforms work?

I find it strange it fails in nsIMsgAccountManager.createIncomingServer().
Comment 34 :aceman 2012-05-23 06:29:01 PDT
I developed the test on 32bit Linux and it worked. Any ideas?
Comment 35 :aceman 2012-05-24 13:41:28 PDT
Created attachment 626948 [details] [diff] [review]
mozmill test v4

Fixed test. Actually the test-archive-options.js test was not cleaning up after itself and the 2 tests collided (created an account with the same server name).
The problem did not appear if running each test standalone.
Comment 36 Mike Conley (:mconley) 2012-05-29 12:55:26 PDT
Comment on attachment 626948 [details] [diff] [review]
mozmill test v4

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

Just one nit. With that fixed, I think this is good to go (again).

(I ran the tests locally, and this looked fine.)

::: mail/test/mozmill/account/test-account-actions.js
@@ +17,5 @@
> +  collector.getModule("window-helpers").installInto(module);
> +  collector.getModule("folder-display-helpers").installInto(module);
> +  collector.getModule("account-manager-helpers").installInto(module);
> +
> +  // There may already be some accounts existent.

Probably better: "There may be pre-existing accounts from other tests."
Comment 37 :aceman 2012-05-29 12:57:54 PDT
Created attachment 628073 [details] [diff] [review]
mozmill test v5

Thanks!
Comment 38 Mike Conley (:mconley) 2012-06-02 11:57:41 PDT
Mozmill test checked in: https://hg.mozilla.org/comm-central/rev/9e2dc0559734

Note You need to log in before you can comment on or make changes to this bug.