Telemetry: Whether an <input type=password> is associated with a <form>

RESOLVED FIXED in Firefox 38

Status

()

Toolkit
Password Manager
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

(Blocks: 1 bug)

unspecified
mozilla39
Points:
3
Bug Flags:
firefox-backlog +
in-testsuite -
qe-verify -

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed)

Details

User Story

* It's useful to know how common it is for <input type=password> to appear outside of a <form> to understand the priority of supporting logins outside of <form>.

We can probably measure this in ~HTMLInputElement(). Even better if the probe only reports on <input> with non-empty values (i.e. ones that were actually used).

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)

Updated

3 years ago
Flags: firefox-backlog+
May just be a matter of checking `mForm` depending on the destructor ordering.
Points: --- → 3
Flags: qe-verify-
If this ends up being complex to measure, we should just drop it. I think there's already a pretty solid case for wanting to make this work anyway.

I suppose that's worth thinking about -- if we want it anyway, we'd need DOMFormHasPassword (or an equivalent) to fire. And at that point it's easy to record |input.form == null| into telemetry.

But assuming we implement this anyway, I'm not sure the telemetry data is useful beyond proving that we actually needed it?
If we ever want to give some special treatment to sites that use forms, it would be nice to know this. I agree it's lower priority ATM, but if it's easy, let's do it.
Assignee: nobody → ally
Priority: -- → P1
Whiteboard: [cpp]
Whiteboard: [cpp] → [lang=cpp]
Whiteboard: [lang=cpp] → [lang=cpp][wip]
Depends on: 1119035
in particular for post DOMFormHasPassword changes.
Assignee: ally → nobody
Whiteboard: [lang=cpp][wip] → [lang=cpp]
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 38.3 - 23 Feb
No longer depends on: 1119035

Updated

3 years ago
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar

Updated

3 years ago
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
Created attachment 8582027 [details]
MozReview Request: bz://1120860/MattN

/r/5903 - Bug 1120860 - Measure whether an <input type=password> is associated with a <form> in BindToTree. r=smaug

Pull down this commit:

hg pull review -r 266256300312886d1e04e3eb61f761029942e536
Attachment #8582027 - Flags: review?(bugs)
I thought this may be trivial without introducing any state variables using the destructor but since this is a virtual destructor and mForm is defined a parent class, the timing doesn't work out. Also accumulating in UnbindFromTree would lead to double-counting elements in forms as it gets called twice for them but only once for non-form inputs. For these reasons I implemented the accumulation in BindToTree which I think it good enough.

Comment 7

3 years ago
Comment on attachment 8582027 [details]
MozReview Request: bz://1120860/MattN

Feature wise this might be ok, but I have no idea about the performance characteristics of Telemetry::Accumulate.
So please ask around about the performance and re-ask review.
Attachment #8582027 - Flags: review?(bugs)
Vladan, do you know how fast Telemetry::Accumulate is? Is it fine for hot paths? I see that it bails if `!TelemetryImpl::CanRecord()`. Do you know if that means it would bail quickly if Telemetry is off (e.g. by default on Release)?
Flags: needinfo?(vdjeric)
Created attachment 8582103 [details]
Screen Shot of benchmark data

I confirmed that Services.telemetry.canRecord == false in a new Release build so this shouldn't affect performance for release users who didn't opt-in to Telemetry.
Flags: needinfo?(vdjeric)
Comment on attachment 8582027 [details]
MozReview Request: bz://1120860/MattN

/r/5903 - Bug 1120860 - Measure whether an <input type=password> is associated with a <form> in BindToTree. r=smaug

Pull down this commit:

hg pull review -r 238718dac209eee9506f954c1607179d83a7eb35
Attachment #8582027 - Flags: review?(bugs)

Updated

3 years ago
Attachment #8582027 - Flags: review?(bugs) → review+

Updated

3 years ago
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/1ac5202c2aff
Flags: in-testsuite-
Keywords: checkin-needed
Whiteboard: [lang=cpp] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1ac5202c2aff
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla39
Comment on attachment 8582027 [details]
MozReview Request: bz://1120860/MattN

Approval Request Comment
[Feature/regressing bug #]: Affects the prioritization of bug 1119035
[User impact if declined]: Affects the prioritization of bug 1119035
[Describe test coverage new/current, TreeHerder]: No test. Simple 3 lines for telemetry (a very common pattern)
[Risks and why]: Low risk since problems would have shown up quite quickly in automated tests and by Nightly users and there haven't been any issues.
[String/UUID change made/needed]: None
Attachment #8582027 - Flags: approval-mozilla-aurora?
status-firefox38: --- → affected
Attachment #8582027 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8582027 [details]
MozReview Request: bz://1120860/MattN
Attachment #8582027 - Attachment is obsolete: true
Attachment #8619117 - Flags: review+
Created attachment 8619117 [details]
MozReview Request: Bug 1120860 - Measure whether an <input type=password> is associated with a <form> in BindToTree. r=smaug
You need to log in before you can comment on or make changes to this bug.