Closed Bug 1120860 Opened 5 years ago Closed 5 years ago
Telemetry: Whether an <input type=password> is associated with a <form>
* 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).
No description provided.
May just be a matter of checking `mForm` depending on the destructor ordering.
Points: --- → 3
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.
in particular for post DOMFormHasPassword changes.
Assignee: ally → nobody
Whiteboard: [lang=cpp][wip] → [lang=cpp]
5 years ago
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 38.3 - 23 Feb
No longer depends on: formless-logins
/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
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 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.
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)?
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.
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
5 years ago
Whiteboard: [lang=cpp] → [fixed-in-fx-team]
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?
Attachment #8582027 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.