1124891 - Telemetry: Record the number of saved credentials for HTTP Auth sites in a profile

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Comment 1

[Chris Karlof [:ckarlof]]

Paulo, can you take this one?

Comment 2

[:Paolo Amadini]

Attachment: The patch

Comment 3

[Matthew N. [:MattN]]

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

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

r=me with the changes.

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +248,5 @@
>  
>      let logins = this.getAllLogins({});
>  
> +    clearAndGetHistogram("PWMGR_NUM_HTTPAUTH_PASSWORDS").add(
> +      logins.filter(login => login.httpRealm).length

I think we should use the existing API that knows how to filter and doesn't need MP which means we can also move this above the isLoggedIn check:
  this.countLogins("", null, "")

I think your approach also fails for realms that are falsy in JS e.g. "" (not sure if that's valid) so using the existing logic seems better.

Comment 4

[:Paolo Amadini]

Attachment: Updated patch

(In reply to Matthew N. [:MattN] from comment #3)
> I think we should use the existing API that knows how to filter and doesn't
> need MP which means we can also move this above the isLoggedIn check:
>   this.countLogins("", null, "")

Good idea, moved up.

> I think your approach also fails for realms that are falsy in JS e.g. ""
> (not sure if that's valid) so using the existing logic seems better.

That's not valid, and the API enforces this when logins are added. The difference between empty string and null applies only to formSubmitURL (un)fortunately.

Comment 5

[:Paolo Amadini]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2730498d7d77

Comment 7

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Setting as qe-verify- as this has a test in "toolkit/components/passwordmgr/test/unit/test_telemetry.js". Please set back qe-verify+ if you consider that additional manual testing is also needed here.