1145789 - Unify code paths for the password save and change notifications

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[:Paolo Amadini]

This is preliminary work to be able to edit the username inside the password popup notification.

Comment 1

[:Paolo Amadini]

Attachment: The patch

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8580888 [details] [diff] [review]
The patch

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

This refactoring should be covered by existing tests but I would recommend running them in case the change in callback behaviour (where the current storage state is taken into account) breaks one of them unintentionally.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ +783,5 @@
> +   * _showLoginDoorhanger
> +   *
> +   * Displays the PopupNotifications.jsm doorhanger for password save or change.
> +   */
> +  _showLoginDoorhanger(login, type) {

Could you document the arguments (especially `type`), adding another "*" for JSDoc and removing the redundant method name line? I've been trying to cleanup the existing JSDoc comments as I touch them.

I kind of wonder if the method name should be more specific (e.g. indicating these are capture doorhangers, not fill ones) which will be implemented later in a different function I imagine.

@@ +784,5 @@
> +   *
> +   * Displays the PopupNotifications.jsm doorhanger for password save or change.
> +   */
> +  _showLoginDoorhanger(login, type) {
> +    var { browser } = this._getNotifyWindow();

Nit: `let` is the new `var`

@@ +815,5 @@
> +        let logins = foundLogins.filter(l => l.username == login.username);
> +        if (logins.length == 0) {
> +          Services.logins.addLogin(login);
> +        } else if (logins.length == 1) {
> +          this._updateLogin(logins[0], login.password);

So we're not checking `type` here since we're going to allow editing of the captured fields in the doorhanger soon which can affect whether we do an add or update. I think it's fine to have a possible mismatch between the button label and the storage action in the meantime since as you said in-person, there can be dataloss in either direction (before or after the change) if the user modifies storage after the doorhanger appears.

@@ +850,5 @@
> +        persistWhileVisible: true,
> +        passwordNotificationType: type,
> +        eventCallback: function (topic) {
> +          if (topic != "showing") {
> +            return false;

Nit: this function does not always return a value. Feel free to ignore if you're not confident fixing this existing problem.

Comment 3

[:Paolo Amadini]

Attachment: Updated patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2e2108a5029

Comment 4

[:Paolo Amadini]

Fixed the case where we show the popup for HTTP authentication:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5a274c0133d

Comment 5

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/06f1b43f5a7b

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/06f1b43f5a7b