Closed Bug 1288558 Opened 8 years ago Closed 8 years ago

Merge passwordManagerCommon.js into passwordManager.js

Categories

(Toolkit :: Password Manager, defect, P3)

Product:
Toolkit
Component:
Password Manager
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- verified

People

(Reporter: MattN, Assigned: steveck, Mentored)

References

(
URL
)

Details

(Whiteboard: [passwords:tech-debt] [lang=js])

Attachments

(12 files)

Bug 1288558 - Part 1: Simply move the functions/variables from passwordManagerCommon to passwordManager.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1288558 - Part 2: Replace var with let.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1288558 - Part 3: Replace usage of Components with Cc/Ci/Cu.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1288558 - Part 4: Replace kObserverService and passwordmanager variables with inline Services.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1288558 - Part 5: Merge SignonsStartup/HandleTreeColumnClick into single Startup init function.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1288558 - Part 6: Merge DeleteSelectedItemFromTree into DeleteSignon and polish the spaces around operators.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1288558 - Part 7: Merge DeleteAllFromTree into DeleteAllSignon and polish the spaces around operators.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1288558 - Part 8: Remove unnecessary and unused parameters for SortTree and move parameters to the top.
58 bytes, text/x-review-board-request
Details
Bug 1288558 - Part 9: Create some global variables and Move all the elements to the top.
58 bytes, text/x-review-board-request
Details
Bug 1288558 - Part 10: Remove unnecessary signonsTree parameter passing between functions.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1288558 - Part 11: Add spaces around operaters for the rest of part.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1288558 - Part 12: Switch the function naming _filterPasswords and FilterPasswords.
58 bytes, text/x-review-board-request
MattN
: review+
Details 
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
Please assign it to me.
(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?
Yes.
Assignee: nobody → schung
Status: NEW → ASSIGNED
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 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 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
Attachment #8788410 - Flags: review?(MattN+bmo) → review+
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.
Attachment #8788411 - Flags: review?(MattN+bmo)
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 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
Attachment #8788411 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8788836 [details]
Bug 1288558 - Part 3: Replace usage of Components with Cc/Ci/Cu.

https://reviewboard.mozilla.org/r/77178/#review75608
Attachment #8788836 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8788837 [details]
Bug 1288558 - Part 4: Replace kObserverService and passwordmanager variables with inline Services.

https://reviewboard.mozilla.org/r/77180/#review75610
Attachment #8788837 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8788838 [details]
Bug 1288558 - Part 5: Merge SignonsStartup/HandleTreeColumnClick into single Startup init function.

https://reviewboard.mozilla.org/r/77182/#review75612
Attachment #8788838 - Flags: review?(MattN+bmo) → review+
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
Attachment #8788839 - Flags: review?(MattN+bmo) → review+
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
Attachment #8788840 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8788843 [details]
Bug 1288558 - Part 10: Remove unnecessary signonsTree parameter passing between functions.

https://reviewboard.mozilla.org/r/77192/#review75620
Attachment #8788843 - Flags: review?(MattN+bmo) → review+
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
Attachment #8788844 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8788845 [details]
Bug 1288558 - Part 12: Switch the function naming _filterPasswords and FilterPasswords.

https://reviewboard.mozilla.org/r/77196/#review75626
Attachment #8788845 - Flags: review?(MattN+bmo) → review+
I would be great for QE to verify that the Password Manager (sub)dialog continues to work properly after this change.
Flags: qe-verify+
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.
Attachment #8788841 - Flags: review?(MattN+bmo)
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.
Attachment #8788842 - Flags: review?(MattN+bmo)
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.
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.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: