Closed
Bug 1120888
Opened 9 years ago
Closed 9 years ago
Telemetry: Record the reasons auto-fill doesn't occur in a histogram
Categories
(Toolkit :: Password Manager, defect, P1)
Toolkit
Password Manager
Tracking
()
People
(Reporter: MattN, Assigned: MattN)
References
Details
User Story
There are multiple reasons why we bail from auto-fill in _fillForm and we log many of them to the Browser Console. Record each reason in it's own bucket and making some more fine-grained will help track improvements to detection heuristics. Examples: * "Password not filled. None of the stored logins match the username already present." may be reduced when our username field heuristics improve. * "Multiple logins for form, so not filling any." may be reduced when we improve our field detection heuristics as we will have less users having 2 logins for a site with different usernames (e.g. one email, one actual username) but with the same password. We should leave room in the histogram to add more reasons in the future.
Attachments
(1 file, 1 obsolete file)
No description provided.
Flags: firefox-backlog+
Assignee | ||
Updated•9 years ago
|
Points: --- → 5
Flags: qe-verify-
Updated•9 years ago
|
Assignee: nobody → MattN+bmo
Priority: -- → P1
Assignee | ||
Comment 1•9 years ago
|
||
I have a WIP patch.
Status: NEW → ASSIGNED
Iteration: --- → 38.3 - 23 Feb
Assignee | ||
Comment 2•9 years ago
|
||
/r/3725 - Bug 1120888 - Record the result of password manager form autofill. r=dolske Pull down this commit: hg pull review -r c625619a7be962ecf642af95239102558aafc277
Attachment #8562534 -
Flags: review?(dolske)
Assignee | ||
Comment 3•9 years ago
|
||
I manually tested all but 1 and 10. 1 would require scripting to remove the password field after DOMFormHasPassword which seems unlikely and 10 shouldn't happen unless we forget to add a didntFillReason in the future. PWMGR_FORM_AUTOFILL_RESULT 38 samples, average = 4.7, sum = 177 0 |######## 2 5% 2 |######################### 6 16% 3 |##################### 5 13% 4 |##################### 5 13% 5 |##################### 5 13% 6 |##################### 5 13% 7 |######################### 6 16% 8 |############# 3 8% 9 |#### 1 3% 10 | 0 0%
Comment 4•9 years ago
|
||
Hot shit, :MattN. I'm eager to see the results.
Updated•9 years ago
|
Attachment #8562534 -
Flags: review?(dolske)
Comment 5•9 years ago
|
||
Comment on attachment 8562534 [details] MozReview Request: bz://1120888/MattN https://reviewboard.mozilla.org/r/3723/#review3147 ::: toolkit/components/passwordmgr/LoginManagerContent.jsm (Diff revision 1) > + const autofillResultHist = Services.telemetry.getHistogramById("PWMGR_FORM_AUTOFILL_RESULT"); Seems like the histogram fetch should just go into recordAutofillResult(). ::: toolkit/components/passwordmgr/LoginManagerContent.jsm (Diff revision 1) > + recordAutofillResult(AUTOFILL_RESULT.NO_PASSWORD_FIELD); Let's add a log() here, just so our telemetry and console output help explain the same things. (There wasn't previously, because pre-DOMFormHasPassword fillForm() was called all the time on passwordless forms, now this basically should't ever happen.) ::: toolkit/components/passwordmgr/LoginManagerContent.jsm (Diff revision 1) > + recordAutofillResult(AUTOFILL_RESULT.NO_SAVED_LOGINS); Maybe worth a comment here that we're not log()ing, because this is the exceptionally common case of a login form without a saved login? Also, we could probably just move this check up to the top of the function... If _fillForm() is called with foundLogins.length == 0, looks like we can just safely bail immediately without doing anything. This would be good to do here, since otherwise the data will be a little skewed -- we'll be reporting NO_PASSWORD_FIELD and PASSWORD_DISABLED_READONLY for pages where the user doesn't even have a login to fill. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm (Diff revision 1) > + recordAutofillResult(AUTOFILL_RESULT.EXISTING_PASSWORD); Maybe add a log() here too or in notifyFoundLogins()... Although now that I'm looking at this, I kinda think we should just kill notifyFoundLogins (in a separate bug). So maybe not bother with extra logging here.
Updated•9 years ago
|
Attachment #8562534 -
Flags: review+
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/f39249695662
Flags: in-testsuite-
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8562534 [details] MozReview Request: bz://1120888/MattN Approval Request Comment [Feature/regressing bug #]: Improvements to password manager in 2015 - Bug 1121127 [User impact if declined]: Possibly less ideal decisions based on lack of data. [Describe test coverage new/current, TreeHerder]: Existing password manager tests pass [Risks and why]: Low risk since it's new code only run during the telemetry gathering phase. [String/UUID change made/needed]: None
Attachment #8562534 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f39249695662
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla38
Updated•9 years ago
|
Attachment #8562534 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 9•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/cd0f43f2bb0e
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8562534 -
Attachment is obsolete: true
Attachment #8619118 -
Flags: review+
Assignee | ||
Comment 11•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•