1255253 - Remove a mostly-unneeded countLogins() call

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Justin Dolske [:Dolske]]

The code in LoginManagerParent.jsm's sendLoginDataToChild() has a countLogins() call that actually isn't really needed anymore. I think it's mostly a leftover from when this was a hot code path (fixed by using DOMFormHasPassword) and to avoid a master password prompt (fixed by .uiBusy).

The current code basically looks like:

  if (countLogins() == 0)
    return; // no logins to process
  if (uiBusy) {
    // Master password already showing, add a callback to finish operation
    // after it's been entered, to avoid showing a 2nd prompt.
    return;
  }
  login = findLogins();

If countLogins() != 0, removing this code has no effect. If it is 0, there is no effect in the common case of the user either not having a master password or having already entered it.

The only slight difference is that if the user has a master password prompt showing, but is ignoring it and continuing to browse (which is weird because it's a modal prompt), we will add pointless callbacks that will just discover findLogins() have no logins.

That's still zero functional change for the user.

Removing this call allows proceeding in bug 667233 without modifying the countLogins() API or implementing a countSearchLogins() alternative.

Comment 1

[Justin Dolske [:Dolske]]

(Also there's a tiny perf benefit, in not doing the same underlying search twice.)

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

(I didn't actually test this yet :)

Note that I removed the telemetry probe -- it was comparing the countLogins() count (which ignored the formAction) to the findLogins() count (which did not). The telemetry was seeking to understand if password manager would be more useful to users if it stopped taking the form submit URL into account... Currently telemetry shows about 12% of the time using it resulted in 0 logins available to the user (even though countLogins found something), and an additional 5% where fewer logins were available.

We don't have any plans to change this in the near term, so I'm just removing it.

Comment 3

[Matthew N. [:MattN]]

Reminder to land this since bug 667233 depends on it.

Comment 4

[Pulsebot]

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

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f7bfb9a0184a