Closed Bug 1120860 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Password Manager, defect, P1)

Product:
Toolkit
Component:
Password Manager
Type:
defect
Points:
3

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Iteration:
39.3 - 30 Mar
Tracking Flags:
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

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)

Screen Shot of benchmark data
36.60 KB, image/png
Details
MozReview Request: Bug 1120860 - Measure whether an <input type=password> is associated with a <form> in BindToTree. r=smaug
39 bytes, text/x-review-board-request
smaug
: review+
Details 
      No description provided.
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]
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: formless-logins
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
Attached file MozReview Request: bz://1120860/MattN (obsolete) — 
/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 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)
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)
Attachment #8582027 - Flags: review?(bugs) → review+
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
Closed: 9 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?
Attachment #8582027 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

        Attachment #8582027 -
        Attachment is obsolete: true

        Attachment #8619117 -
        Flags: review+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: