Closed
Bug 1120860
Opened 10 years ago
Closed 10 years ago
Telemetry: Whether an <input type=password> is associated with a <form>
Categories
(Toolkit :: Password Manager, defect, P1)
Toolkit
Password Manager
Tracking
()
People
(Reporter: MattN, Assigned: MattN)
References
(Blocks 1 open bug)
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).
Attachments
(2 files, 1 obsolete file)
No description provided.
Updated•10 years ago
|
Flags: firefox-backlog+
Assignee | ||
Comment 1•10 years ago
|
||
May just be a matter of checking `mForm` depending on the destructor ordering.
Points: --- → 3
Flags: qe-verify-
Comment 2•10 years ago
|
||
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?
Comment 3•10 years ago
|
||
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.
Updated•10 years ago
|
Assignee: nobody → ally
Priority: -- → P1
Updated•10 years ago
|
Whiteboard: [cpp]
Updated•10 years ago
|
Whiteboard: [cpp] → [lang=cpp]
Updated•10 years ago
|
Whiteboard: [lang=cpp] → [lang=cpp][wip]
Updated•10 years ago
|
Depends on: formless-logins
Comment 4•10 years ago
|
||
in particular for post DOMFormHasPassword changes.
Updated•10 years ago
|
Assignee: ally → nobody
Whiteboard: [lang=cpp][wip] → [lang=cpp]
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 38.3 - 23 Feb
No longer depends on: formless-logins
Updated•10 years ago
|
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
Updated•10 years ago
|
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
Assignee | ||
Comment 5•10 years ago
|
||
/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)
Assignee | ||
Comment 6•10 years ago
|
||
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•10 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)
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
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•10 years ago
|
Attachment #8582027 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 11•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla39
Assignee | ||
Comment 13•10 years ago
|
||
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?
Updated•10 years ago
|
status-firefox38:
--- → affected
Updated•10 years ago
|
Attachment #8582027 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•10 years ago
|
||
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8582027 -
Attachment is obsolete: true
Attachment #8619117 -
Flags: review+
Assignee | ||
Comment 16•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•