1288558 - Merge passwordManagerCommon.js into passwordManager.js

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P3)

Description

[Matthew N. [:MattN]]

After bug 1288557 removes passwordManagerExceptions.xul I think it makes sense to merge passwordManagerCommon.js[1] into passwordManager.js[2] since they will both only have one consumer (passwordManager.xul) and it's confusing that passwordManager.js relies on globals defined in passwordManagerCommon.js.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/content/passwordManagerCommon.js
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/content/passwordManager.js

Comment 1

[pushpankark]

Please assign it to me.

Comment 2

[Matthew N. [:MattN]]

(In reply to pushpankark from comment #1)
> Please assign it to me.

Bug 1288557 needs to be completed first, do you want to take that one?

Comment 3

[pushpankark]

Yes.

Comment 4

[Steve Chung [:steveck]]

Attachment: Bug 1288558 - Part 1: Simply move the functions/variables from passwordManagerCommon to passwordManager.

Review commit: https://reviewboard.mozilla.org/r/76914/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76914/

Comment 5

[Steve Chung [:steveck]]

Attachment: Bug 1288558 - Part 2: Replace var with let.

Review commit: https://reviewboard.mozilla.org/r/76916/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76916/

Comment 6

[Steve Chung [:steveck]]

Hi Matt, I split the patch into 2 commits. The first part simply moved the codes from passwordManagerCommon.js to passwordManager.js. The second part I removed the unnecessary functions/parameters, and some dead code that I think it won't be triggered. In the meantime I also found a bug that existed in central:

1) Open an empty list
2) Type something on the search bar, and remove all the words
3) See the remove button, it should be disabled => But it's enabled and all the items in contextmenu are enabled as well.

Should I file a bug for this, or we already created this issue?

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8788411 [details]
Bug 1288558 - Part 2: Replace var with let.

https://reviewboard.mozilla.org/r/76916/#review75114

::: toolkit/components/passwordmgr/content/passwordManager.js
(Diff revision 1)
> -  if (!ascending)
> +  if (!ascending) {
>      table.reverse();
> -
> -  // restore the selection
> -  var selectedRow = -1;
> -  if (selectedNumber>=0 && updateSelection) {

I can't see when the "updateSelection" will be set, so I remove all the logic related to updateSelection here

::: toolkit/components/passwordmgr/content/passwordManager.js:576
(Diff revision 1)
>      seln.getRangeAt(i, min, max);
>      signonsTreeView._lastSelectedRanges.push({ min: min.value, max: max.value });
>    }
>  }
>  
> -function _filterPasswords() {
> +function FilterPasswords() {

I switch the function name here because it would be more clear to export the capitalize function name in markup instead of being private.

::: toolkit/components/passwordmgr/content/passwordManager.js:641
(Diff revision 1)
> -  for (let menuItem of menupopup.querySelectorAll("menuitem")) {
> -    menuItems.set(menuItem.id, menuItem);
> -  }
>  
>    if (!singleSelection) {
> -    for (let menuItem of menuItems.values()) {
> +    e.preventDefault();

It seems more appropriate if we don't display the contextmenu instead of showing the menu with all items disabled. But I'm fine if we want to keep the original behavior.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8788410 [details]
Bug 1288558 - Part 1: Simply move the functions/variables from passwordManagerCommon to passwordManager.

https://reviewboard.mozilla.org/r/76914/#review75256

Thanks

::: toolkit/components/passwordmgr/content/passwordManager.js:65
(Diff revision 1)
> +  passwordmanager = Components.classes["@mozilla.org/login-manager;1"]
> +                        .getService(Components.interfaces.nsILoginManager);
> +
> +  // be prepared to reload the display if anything changes
> +  kObserverService = Components.classes["@mozilla.org/observer-service;1"]
> +                               .getService(Components.interfaces.nsIObserverService);

I was going to note that incorrect alignment on line 66 but I think it's easier to just switch to use Services.logins and Services.obs instead of getService

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8788411 [details]
Bug 1288558 - Part 2: Replace var with let.

https://reviewboard.mozilla.org/r/76916/#review75260

In the future please separate each refactoring in its own commit. For example, `var` to `let` should be its own commit. Btw. while converting to `let`, there should be no remaining `var` in the file. Even globals should use `let` or `const` instead of `var` in our style.

::: toolkit/components/passwordmgr/content/passwordManager.js:6
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /*** =================== SAVED SIGNONS CODE =================== ***/
> -var { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
> +var { classes: Cc, interfaces: Ci, utils: Cu } = Components;

Please leave Cr as our team prefers to just define all 4 so they are ready to use meaning you don't need to check if they are defined.

::: toolkit/components/passwordmgr/content/passwordManager.js:33
(Diff revision 1)
> +
>  // password-manager lists
>  var signons = [];
>  var deletedSignons = [];
>  
> -var signonsTree;
> +var DOM = {};

`DOM` stands for Document Object Model so this doesn't seem like a good name for a map of IDs to elements. I also don't see this pattern used very much in our code and I think the indirection just adds unnecesary complexity in this case where the caching getElementByID calls isn't very important. Could you revert the `DOM.` changes for now?

Since this is hard to review and mixing function and style changes, I would suggest making a series of commits like so:
1) Merge init functions
2) Replace usage of `Components` with Cc/Ci/Cu, etc.
3) Replace usage of `var` in the whole file
4) Move `lastSignonSort*` to `const`s at the top of the file
5) Replace `kObserverService` and `passwordmanager` variables with inline `Services.(obs|logins)`
6) Fix occurences of no spaces around operators (e.g. `let j=0; j<table.length; j++` should be `let j = 0; j < table.length; j++`)
7) Remove `updateSelection` code
8) Rename `filterPasswords` to `FilterPasswords` to match other functions 
9) New bug: Change contextmenu behaviour on disabled items

Could you please split this commit up something like the above? I think only #1 and #7 are required for this bug but the others would be nice though could be in a separate bug (or bugs).

(In reply to Steve Chung [:steveck] from comment #6)
> In the meantime I also found a bug that existed in central:
> 
> 1) Open an empty list
> 2) Type something on the search bar, and remove all the words
> 3) See the remove button, it should be disabled => But it's enabled and all
> the items in contextmenu are enabled as well.
> 
> Should I file a bug for this, or we already created this issue?

I don't see one on file so it would be great to do so.

Comment 10

[Steve Chung [:steveck]]

Attachment: Bug 1288558 - Part 3: Replace usage of Components with Cc/Ci/Cu.

Review commit: https://reviewboard.mozilla.org/r/77178/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77178/

Comment 11

[Steve Chung [:steveck]]

Attachment: Bug 1288558 - Part 4: Replace kObserverService and passwordmanager variables with inline Services.

Review commit: https://reviewboard.mozilla.org/r/77180/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77180/

Comment 12

[Steve Chung [:steveck]]

Attachment: Bug 1288558 - Part 5: Merge SignonsStartup/HandleTreeColumnClick into single Startup init function.

Review commit: https://reviewboard.mozilla.org/r/77182/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77182/

Comment 13

[Steve Chung [:steveck]]

Attachment: Bug 1288558 - Part 6: Merge DeleteSelectedItemFromTree into DeleteSignon and polish the spaces around operators.

Review commit: https://reviewboard.mozilla.org/r/77184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77184/

Comment 14

[Steve Chung [:steveck]]

Attachment: Bug 1288558 - Part 7: Merge DeleteAllFromTree into DeleteAllSignon and polish the spaces around operators.

Review commit: https://reviewboard.mozilla.org/r/77186/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77186/

Comment 15

[Steve Chung [:steveck]]

Attachment: Bug 1288558 - Part 8: Remove unnecessary and unused parameters for SortTree and move parameters to the top.

Review commit: https://reviewboard.mozilla.org/r/77188/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77188/

Comment 16

[Steve Chung [:steveck]]

Attachment: Bug 1288558 - Part 9: Create some global variables and Move all the elements to the top.

Review commit: https://reviewboard.mozilla.org/r/77190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77190/

Comment 17

[Steve Chung [:steveck]]

Attachment: Bug 1288558 - Part 10: Remove unnecessary signonsTree parameter passing between functions.

Review commit: https://reviewboard.mozilla.org/r/77192/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77192/

Comment 18

[Steve Chung [:steveck]]

Attachment: Bug 1288558 - Part 11: Add spaces around operaters for the rest of part.

Review commit: https://reviewboard.mozilla.org/r/77194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77194/

Comment 19

[Steve Chung [:steveck]]

Attachment: Bug 1288558 - Part 12: Switch the function naming _filterPasswords and FilterPasswords.

Review commit: https://reviewboard.mozilla.org/r/77196/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77196/

Comment 20

[Steve Chung [:steveck]]

Comment on attachment 8788410 [details]
Bug 1288558 - Part 1: Simply move the functions/variables from passwordManagerCommon to passwordManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76914/diff/1-2/

Comment 21

[Steve Chung [:steveck]]

Comment on attachment 8788411 [details]
Bug 1288558 - Part 2: Replace var with let.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76916/diff/1-2/

Comment 22

[Steve Chung [:steveck]]

Comment on attachment 8788411 [details]
Bug 1288558 - Part 2: Replace var with let.

https://reviewboard.mozilla.org/r/76916/#review75260

> `DOM` stands for Document Object Model so this doesn't seem like a good name for a map of IDs to elements. I also don't see this pattern used very much in our code and I think the indirection just adds unnecesary complexity in this case where the caching getElementByID calls isn't very important. Could you revert the `DOM.` changes for now?

I revert the DOM object for caching all the elements in the new commits, but I still cache some frequent used elements to global variable since we already cached some and we could also apply to other elements and unify them

Comment 23

[Matthew N. [:MattN]]

Comment on attachment 8788411 [details]
Bug 1288558 - Part 2: Replace var with let.

https://reviewboard.mozilla.org/r/76916/#review75604

::: toolkit/components/passwordmgr/content/passwordManager.js:6
(Diff revision 2)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /*** =================== SAVED SIGNONS CODE =================== ***/
> -var { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
> +let { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;

Nit: we normally use `const` for this

Comment 24

[Matthew N. [:MattN]]

Comment on attachment 8788836 [details]
Bug 1288558 - Part 3: Replace usage of Components with Cc/Ci/Cu.

https://reviewboard.mozilla.org/r/77178/#review75608

Comment 25

[Matthew N. [:MattN]]

Comment on attachment 8788837 [details]
Bug 1288558 - Part 4: Replace kObserverService and passwordmanager variables with inline Services.

https://reviewboard.mozilla.org/r/77180/#review75610

Comment 26

[Matthew N. [:MattN]]

Comment on attachment 8788838 [details]
Bug 1288558 - Part 5: Merge SignonsStartup/HandleTreeColumnClick into single Startup init function.

https://reviewboard.mozilla.org/r/77182/#review75612

Comment 27

[Matthew N. [:MattN]]

Comment on attachment 8788839 [details]
Bug 1288558 - Part 6: Merge DeleteSelectedItemFromTree into DeleteSignon and polish the spaces around operators.

https://reviewboard.mozilla.org/r/77184/#review75616

Comment 28

[Matthew N. [:MattN]]

Comment on attachment 8788840 [details]
Bug 1288558 - Part 7: Merge DeleteAllFromTree into DeleteAllSignon and polish the spaces around operators.

https://reviewboard.mozilla.org/r/77186/#review75618

Comment 29

[Matthew N. [:MattN]]

Comment on attachment 8788843 [details]
Bug 1288558 - Part 10: Remove unnecessary signonsTree parameter passing between functions.

https://reviewboard.mozilla.org/r/77192/#review75620

Comment 30

[Matthew N. [:MattN]]

Comment on attachment 8788844 [details]
Bug 1288558 - Part 11: Add spaces around operaters for the rest of part.

https://reviewboard.mozilla.org/r/77194/#review75622

Comment 31

[Matthew N. [:MattN]]

Comment on attachment 8788845 [details]
Bug 1288558 - Part 12: Switch the function naming _filterPasswords and FilterPasswords.

https://reviewboard.mozilla.org/r/77196/#review75626

Comment 32

[Matthew N. [:MattN]]

I would be great for QE to verify that the Password Manager (sub)dialog continues to work properly after this change.

Comment 33

[Matthew N. [:MattN]]

Comment on attachment 8788841 [details]
Bug 1288558 - Part 8: Remove unnecessary and unused parameters for SortTree and move parameters to the top.

https://reviewboard.mozilla.org/r/77188/#review75682

Please leave the dead code to be fixed in a folow-up but I'm fine with cleaning up the other arguments to the function.

::: toolkit/components/passwordmgr/content/passwordManager.js:219
(Diff revision 1)
>    // remember which item was selected so we can restore it after the sort
>    let selections = GetTreeSelections(tree);
>    let selectedNumber = selections.length ? table[selections[0]].number : -1;

These three lines will become unused after the removal.

I actually think this dead code should be enabled by removing the `updateSelection` check as it seems to be implementing expected UX (at least on OS X and Windows, not sure about Linux) so please file a follow-up bug for this since it will need an automated browser-chrome test.

Comment 34

[Matthew N. [:MattN]]

Comment on attachment 8788842 [details]
Bug 1288558 - Part 9: Create some global variables and Move all the elements to the top.

https://reviewboard.mozilla.org/r/77190/#review75690

::: toolkit/components/passwordmgr/content/passwordManager.js:26
(Diff revision 1)
> +let filter;
> +let togglePasswords;
> +let signonsIntro;
> +let removeSignon;
> +let removeAllSignons;

For the 4 names that start with verbs (filter, togglePasswords, removeSignon, and removeAllSignons), I think these variable names can be confusing. Please include a suffix to indicate that it refers to an element and the type of the element. (e.g. filterField, togglePasswordsButton, removeButton, removeAllButton)

::: toolkit/components/passwordmgr/content/passwordManager.js:69
(Diff revision 1)
> +  signonsTree = document.getElementById("signonsTree");
> +  kSignonBundle = document.getElementById("signonBundle");
> +  filter = document.getElementById("filter");
> +  togglePasswords = document.getElementById("togglePasswords");
> +  signonsIntro = document.getElementById("signonsIntro");
> +  removeSignon = document.getElementById("removeSignon");
> +  removeAllSignons = document.getElementById("removeAllSignons");

I don't think we need a new function for this. Just add the new ones below where signonsTree and kSignonBundle were initialized before.

Thanks for all the cleanup! Only a few comments above.

Comment 35

[Matthew N. [:MattN]]

Comment on attachment 8788841 [details]
Bug 1288558 - Part 8: Remove unnecessary and unused parameters for SortTree and move parameters to the top.

https://reviewboard.mozilla.org/r/77188/#review75682

> These three lines will become unused after the removal.
> 
> I actually think this dead code should be enabled by removing the `updateSelection` check as it seems to be implementing expected UX (at least on OS X and Windows, not sure about Linux) so please file a follow-up bug for this since it will need an automated browser-chrome test.

Since you're OOO I've fixed this and the other issues. I filed bug 1301248 for the sort selection issue.

Comment 36

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/ad1252b88e46
https://hg.mozilla.org/integration/fx-team/rev/6f18e82e0ccd
https://hg.mozilla.org/integration/fx-team/rev/5db2cc28a8ff
https://hg.mozilla.org/integration/fx-team/rev/62a6840937fa
https://hg.mozilla.org/integration/fx-team/rev/03d181077402
https://hg.mozilla.org/integration/fx-team/rev/c4d5f6a4663d
https://hg.mozilla.org/integration/fx-team/rev/4374419eef58
https://hg.mozilla.org/integration/fx-team/rev/f2d7fbd590f3
https://hg.mozilla.org/integration/fx-team/rev/cc1837731206
https://hg.mozilla.org/integration/fx-team/rev/827f28a6ff64
https://hg.mozilla.org/integration/fx-team/rev/3430dfafe8fd
https://hg.mozilla.org/integration/fx-team/rev/0f6c121124c0
https://hg.mozilla.org/integration/fx-team/rev/9295128924d1

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ad1252b88e46
https://hg.mozilla.org/mozilla-central/rev/6f18e82e0ccd
https://hg.mozilla.org/mozilla-central/rev/5db2cc28a8ff
https://hg.mozilla.org/mozilla-central/rev/62a6840937fa
https://hg.mozilla.org/mozilla-central/rev/03d181077402
https://hg.mozilla.org/mozilla-central/rev/c4d5f6a4663d
https://hg.mozilla.org/mozilla-central/rev/4374419eef58
https://hg.mozilla.org/mozilla-central/rev/f2d7fbd590f3
https://hg.mozilla.org/mozilla-central/rev/cc1837731206
https://hg.mozilla.org/mozilla-central/rev/827f28a6ff64
https://hg.mozilla.org/mozilla-central/rev/3430dfafe8fd
https://hg.mozilla.org/mozilla-central/rev/0f6c121124c0
https://hg.mozilla.org/mozilla-central/rev/9295128924d1

Comment 38

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I ran some test cases for the Password Manager feature and compared the results with an older build. No new issues were found. I verified using Fx 51.0b2 (20161121093909) on Windows 10 x64, Mac OS X 10.11 and Ubuntu 14.04 LTS.