Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[Form Autofill] Collect information on how much time users spent on page with forms (w/wo form autofill)

NEW
Assigned to

Status

()

Toolkit
Form Manager
5 months ago
a day ago

People

(Reporter: steveck, Assigned: steveck, NeedInfo)

Tracking

(Blocks: 2 bugs)

unspecified
x86
Unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 unaffected, firefox55 unaffected, firefox56 affected)

Details

(Whiteboard: [story][form autofill:M4][autofill-metrics][ETA:7/28])

User Story

Product Owner should be able to see how long the user spends filling out forms with Form Autofill enabled and disabled.

MozReview Requests

()

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

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 months ago
Product Owner should be able to see how long does the users spent on the form under form autofill feature enabled/disabled.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

4 months ago
Comment on attachment 8846567 [details]
Bug 1341569 - Part 1: Add the form created time and telemetry probe for form filling duration.

Hi Matt, I'm not sure if there's other way to access the telemetry, but adding a new probe in  Histogram.json directly is one of the solution I know by far. Please let me know if there's other alternative solution for system addon.

BTW we might need basic heuristic for collecting data, otherwise we'll collect nothing since our heuristic could only work for autocomplete property.
Attachment #8846567 - Flags: review?(MattN+bmo) → feedback?(MattN+bmo)
(Assignee)

Comment 4

4 months ago
...Or only collect the data from target website
(Assignee)

Comment 5

4 months ago
Hi Joe, talking about where to collect the data before the feature enabled, do you have any preference? Should we only record the results from 12 target websites, or record data from all the websites that could apply our heuristic checking?
Flags: needinfo?(jcheng)
Comment on attachment 8846567 [details]
Bug 1341569 - Part 1: Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review122724

::: commit-message-0ec22:1
(Diff revision 1)
> +Bug 1341569 - Part 2: Add the from created time and telemetry probe for form filling duration. r?MattN

Did you mean "form" created time?

::: browser/extensions/formautofill/FormAutofillContent.jsm:351
(Diff revision 1)
> +      form.rootElement.addEventListener("submit", (evt) => {
> +        this._addFillFormTimeProbe(formHandler.createdTime);
> +      });

This won't handle "forms" without a <form>. I guess that would be fine for an initial landing but it would be good to refactor/share the formless logic out of pwmgr eventually.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:57
(Diff revision 1)
> +   * The created time of the handler instance.
> +   */
> +  createdTime: null,

In my opinion we should only start the timer when the user first interacts with the form e.g. a `click` or `input` event otherwise we will be basically measuring from page load time which won't be accurate especially when things like session restore are involved.

::: toolkit/components/telemetry/Histograms.json:11065
(Diff revision 1)
>      "kind": "categorical",
>      "bug_numbers": [1307689],
>      "description": "Records a value each time a tab that is showing the loading throbber is interrupted. Desktop only.",
>      "labels": ["stop", "back", "forward", "historyNavigation", "reload", "tabClosed", "newURI"]
> +  },
> +  "FORM_FILLING_REQUIRED_TIME_S": {

I think integer seconds isn't granular enough. I suspect we will only move the needle by less than 10 seconds so with the rounding of seconds it may be noisy to see a difference.

::: toolkit/components/telemetry/Histograms.json:11066
(Diff revision 1)
>      "bug_numbers": [1307689],
>      "description": "Records a value each time a tab that is showing the loading throbber is interrupted. Desktop only.",
>      "labels": ["stop", "back", "forward", "historyNavigation", "reload", "tabClosed", "newURI"]
> +  },
> +  "FORM_FILLING_REQUIRED_TIME_S": {
> +    "alert_emails": ["schung@mozilla.com"],

Nit: I think you can add our public list too: autofill@…

::: toolkit/components/telemetry/Histograms.json:11067
(Diff revision 1)
>      "description": "Records a value each time a tab that is showing the loading throbber is interrupted. Desktop only.",
>      "labels": ["stop", "back", "forward", "historyNavigation", "reload", "tabClosed", "newURI"]
> +  },
> +  "FORM_FILLING_REQUIRED_TIME_S": {
> +    "alert_emails": ["schung@mozilla.com"],
> +    "expires_in_version": "60",

IIRC I think we want this probe to be opt-out on the release channel.

::: toolkit/components/telemetry/Histograms.json:11068
(Diff revision 1)
> +    "kind": "linear",
> +    "high": 600,
> +    "n_buckets": 20,

I think https://telemetry.mozilla.org/histogram-simulator/#low=0&high=600&n_buckets=22&kind=linear&generate=uniform would be better to have round numbers for the buckets.
Comment on attachment 8846566 [details]
Bug 1341569 - Part 1: Make the formfill executable for metrics when exerimental pref is off,

https://reviewboard.mozilla.org/r/119616/#review122722

::: browser/extensions/formautofill/FormAutofillParent.jsm:54
(Diff revision 1)
>  this.log = null;
>  FormAutofillUtils.defineLazyLogGetter(this, this.EXPORTED_SYMBOLS[0]);
>  
>  const PROFILE_JSON_FILE_NAME = "autofill-profiles.json";
>  const ENABLED_PREF = "browser.formautofill.enabled";
> +const EXPERIMENT_PREF="browser.formautofill.experimental";

Nit: spaces around operators

::: browser/extensions/formautofill/bootstrap.js:43
(Diff revision 1)
>  function startup() {
>    // Besides this pref, we'll need dom.forms.autocomplete.experimental enabled
>    // as well to make sure form autocomplete works correctly.
> -  if (!Services.prefs.getBoolPref("browser.formautofill.experimental")) {
> +  if (Services.prefs.getBoolPref("browser.formautofill.experimental")) {

I'm not sure it's a good idea to put this telemetry in F.A.P. Our extension doesn't even get built on beta/release (see browser/extensions/moz.build) so this won't actually get us data. I mostly worry about having to "ship" our system add-on without proper QA just to get the baseline numbers. I would feel better if the telemetry was more self-contained.

Have you considered putting this telemetry in Satchel instead?
Attachment #8846566 - Flags: review?(MattN+bmo)
Attachment #8846567 - Flags: feedback?(MattN+bmo)
(Assignee)

Comment 8

4 months ago
mozreview-review-reply
Comment on attachment 8846566 [details]
Bug 1341569 - Part 1: Make the formfill executable for metrics when exerimental pref is off,

https://reviewboard.mozilla.org/r/119616/#review122722

> I'm not sure it's a good idea to put this telemetry in F.A.P. Our extension doesn't even get built on beta/release (see browser/extensions/moz.build) so this won't actually get us data. I mostly worry about having to "ship" our system add-on without proper QA just to get the baseline numbers. I would feel better if the telemetry was more self-contained.
> 
> Have you considered putting this telemetry in Satchel instead?

If we put telemetry in Satchel, that means we could only record the data from target websites.  There's no heuristic logic in Satchel to know whether the website is suitable for profile filling. I'm fine with that if Joe wants to narrow down the focus to limited websites.
(Assignee)

Comment 9

4 months ago
mozreview-review
Comment on attachment 8846567 [details]
Bug 1341569 - Part 1: Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review123280

::: browser/extensions/formautofill/FormAutofillContent.jsm:351
(Diff revision 1)
> +      form.rootElement.addEventListener("submit", (evt) => {
> +        this._addFillFormTimeProbe(formHandler.createdTime);
> +      });

Thanks for pointing out this issue, I'll create a bug for it since we'll have lots of post-submitting tasks in Q2

::: browser/extensions/formautofill/FormAutofillHandler.jsm:57
(Diff revision 1)
> +   * The created time of the handler instance.
> +   */
> +  createdTime: null,

Yeah I think that would be more accurate.

Updated

4 months ago
Assignee: nobody → schung
Whiteboard: [story] [form autofill][autofill-metrics] → [story][form autofill:M2][autofill-metrics]
(Assignee)

Comment 10

4 months ago
Hi Joe, except for the question in comment 5, I have more questions for you:
- In your imagination, would you expect that the result is displayed like these histograms[1] that each histogram will represent the submitted time while feature disabled and enabled?
- In autofill enabled case, should we record the time only when feature is enabled and users actually apply the stored profile?

[1] https://mzl.la/2mvvRgl : Dashboard about the ADDON_CONTENT_POLICY_SHIM_BLOCKING_LOADING_MS
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8846566 - Flags: review?(MattN+bmo)
(Assignee)

Updated

4 months ago
Attachment #8846567 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8846566 - Attachment is obsolete: true
(Assignee)

Comment 14

4 months ago
mozreview-review
Comment on attachment 8846567 [details]
Bug 1341569 - Part 1: Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review129880

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js
(Diff revision 3)
>    handleEvent(evt) {
>      if (!evt.isTrusted) {
>        return;
>      }
>  
> -    if (!Services.prefs.getBoolPref("browser.formautofill.enabled")) {

I removed this part in order to trigger identifyAutofillFields and create cache. I think it's easier to maintain the form status even when feature is disabled, and it won't affect the search result since it should fallback to history at startSearch.

Updated

3 months ago
Whiteboard: [story][form autofill:M2][autofill-metrics] → [story][form autofill:M3][autofill-metrics]
Comment on attachment 8846567 [details]
Bug 1341569 - Part 1: Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review135088

::: browser/extensions/formautofill/FormAutofillContent.jsm:413
(Diff revision 3)
> +      form.rootElement.addEventListener("submit", (evt) => {
> +        this._addFillFormTimeProbe(formHandler.inputStartedTime);
> +      });

Change this to use the form submit observer from your other bug.

::: browser/extensions/formautofill/FormAutofillContent.jsm:442
(Diff revision 3)
> +    let histogram = Services.telemetry.getHistogramById("FORM_FILLING_REQUIRED_TIME_MS");
> +    histogram.add(Date.now() - startedTime);
> +  },

This is a great place to use https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryStopwatch.jsm

::: browser/extensions/formautofill/FormAutofillHandler.jsm:88
(Diff revision 3)
>          // A field with the same identifier already exists.
>          log.debug("Not collecting a field matching another with the same info:", info);
>          continue;
>        }
>  
> +      element.addEventListener("change", () => {

I think this should be "input" since "change" only fires when you blur the field IIRC so we wouldn't be measuring time spent in the first field. Also, it would be great for memory and CPU usage to use event bubbling and only have a listener on the whole document for each handler and it gets removed for that handler by looking up the event.target's handler.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:88
(Diff revision 3)
>          // A field with the same identifier already exists.
>          log.debug("Not collecting a field matching another with the same info:", info);
>          continue;
>        }
>  
> +      element.addEventListener("change", () => {

For any event listener from untrusted pages we should be returning early if `event.isTrusted` is false.

::: toolkit/components/telemetry/Histograms.json:11333
(Diff revision 3)
> +    "kind": "exponential",
> +    "high": 300000,
> +    "n_buckets": 22,
> +    "bug_numbers": [1341569],
> +    "releaseChannelCollection": "opt-out",
> +    "description": "Collect information on how much time users spent on page with forms"

Nit: "Collect information on " isn't necessary.

"Milliseconds between focusing an autofill-eligible field and submitting its form"
Attachment #8846567 - Flags: review?(MattN+bmo)
(Assignee)

Comment 16

3 months ago
mozreview-review
Comment on attachment 8846567 [details]
Bug 1341569 - Part 1: Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review135128

::: browser/extensions/formautofill/FormAutofillContent.jsm:442
(Diff revision 3)
> +    let histogram = Services.telemetry.getHistogramById("FORM_FILLING_REQUIRED_TIME_MS");
> +    histogram.add(Date.now() - startedTime);
> +  },

I knew we had this API for time recording, but my main concern is that user might fill multiple forms at the same time. We could only store one of the record if that happened(or we skip this time recording since it should be a rare case). WDYT?
The API already handles this with the aObj argument: https://dxr.mozilla.org/mozilla-central/rev/8b854986038cf3f3f240697e27ef48ea65914c13/toolkit/components/telemetry/TelemetryStopwatch.jsm#124-128
(Assignee)

Updated

3 months ago
Duplicate of this bug: 990201
(Assignee)

Comment 19

3 months ago
(In reply to Steve Chung [:steveck] from comment #10)
> Hi Joe, except for the question in comment 5, I have more questions for you:
> - In your imagination, would you expect that the result is displayed like
> these histograms[1] that each histogram will represent the submitted time
> while feature disabled and enabled?

After some discussion with Joe, he is satisfied with the basic dashboards that brought by main pings.

> - In autofill enabled case, should we record the time only when feature is
> enabled and users actually apply the stored profile?

He would like to know the form submitting time even the feature is disabled, that means we'll need to identify forms and listen to earlysubmit event all the time. We might need to beware of this since the overhead wont be reduced even user disable the feature.
(Assignee)

Updated

3 months ago
Flags: needinfo?(jcheng)
User Story: (updated)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

3 months ago
mozreview-review
Comment on attachment 8864474 [details]
Bug 1341569 - Part 2: Add properties about change state to understand when the autofill changed.

https://reviewboard.mozilla.org/r/136156/#review139240

::: browser/extensions/formautofill/FormAutofillHandler.jsm:97
(Diff revision 1)
>          section: info.section,
>          addressType: info.addressType,
>          contactType: info.contactType,
>          fieldName: info.fieldName,
>          elementWeakRef: Cu.getWeakReference(element),
> +        autofillState: null,

I add this state because checking if form(and input) is changed is not that straightforward. I tried to add input event listener after setUserInput to listen to the input change event, but in handleEvent side there's no way to classify whether the event is from setUserInput or user manual input. So I added this autofillState in details for handling the post-autofill state and I guess highlight might need this as well.
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8864474 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 25

3 months ago
mozreview-review
Comment on attachment 8846567 [details]
Bug 1341569 - Part 1: Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review139456

::: browser/extensions/formautofill/FormAutofillHandler.jsm:66
(Diff revision 6)
>    filledProfileGUID: null,
>  
>    /**
> +   * Boolean to represent if the form has been edited manually.
> +   */
> +  hasNonProfileAutofillValue: false,

This might not a necessay property and I just want to save some time for checking the state in getFormFillingState. Do you think we could keep it but make it looks more like a private property, or remove it and check every input's value and autofillState everytime?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:97
(Diff revision 6)
>          section: info.section,
>          addressType: info.addressType,
>          contactType: info.contactType,
>          fieldName: info.fieldName,
>          elementWeakRef: Cu.getWeakReference(element),
> +        autofillState: null,

I add this state because checking if form(and input) is changed is not that straightforward. I tried to add input event listener after setUserInput to listen to the input change event, but in handleEvent side there's no way to classify whether the event is from setUserInput or user manual input. So I added this autofillState in details for handling the post-autofill state and I guess highlight might need this as well.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:256
(Diff revision 6)
> +   * will be in used in the end.
> +   */
> +  startFillFormTimer() {
> +    HISTTOGRAM_KEYS.forEach((key) => {
> +      if (TelemetryStopwatch.timeElapsedKeyed(FORM_FILLING_MS_HIST, key, this.form) == -1) {
> +        TelemetryStopwatch.startKeyed(FORM_FILLING_MS_HIST, key, this.form);

I'm not sure if it's kind of abuse the TelemetryStopwatch because I started 3 timers at the same time because we could not confirm which timer will be applied in the end.
Comment on attachment 8846567 [details]
Bug 1341569 - Part 1: Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review139456

> I add this state because checking if form(and input) is changed is not that straightforward. I tried to add input event listener after setUserInput to listen to the input change event, but in handleEvent side there's no way to classify whether the event is from setUserInput or user manual input. So I added this autofillState in details for handling the post-autofill state and I guess highlight might need this as well.

Why do we need to distinguish whether the `input` event is from the user or not? Is that only for pwmgr autofill? For regular form autofill I think that would still signifiy a user started filling a form.

> I'm not sure if it's kind of abuse the TelemetryStopwatch because I started 3 timers at the same time because we could not confirm which timer will be applied in the end.

Hmm… yeah, while discussing the measurement with Georg I realized that this would be a problem with using TelemetryStopwatch. Let me think about it.
(Assignee)

Comment 27

3 months ago
mozreview-review-reply
Comment on attachment 8846567 [details]
Bug 1341569 - Part 1: Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review139456

> Why do we need to distinguish whether the `input` event is from the user or not? Is that only for pwmgr autofill? For regular form autofill I think that would still signifiy a user started filling a form.

Because we want to distinguish the cases that form is autofilled without changes or autofilled with additional user input. We apply setUserInput API for autofilling the from, and it'll also trigger the input event. Ideally we should only mark the form is edited if handler receive the event from real user input and ignore the event from setUserInput.
Comment on attachment 8846567 [details]
Bug 1341569 - Part 1: Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review139456

> Because we want to distinguish the cases that form is autofilled without changes or autofilled with additional user input. We apply setUserInput API for autofilling the from, and it'll also trigger the input event. Ideally we should only mark the form is edited if handler receive the event from real user input and ignore the event from setUserInput.

OK, I guess we had different ideas in mind and apologies for not looking at the patch before discussing. In my mind the "input" event with once:true should be used to know that a user started filling (not sure if it works for <select>) but we only need to use the "change" event (which happens on blur for <input>) to know that a user made a correction. I think we should coordinate with Ray for this since we should use the same logic as removing the highlight. How do we want to handle a user changing the the value and back to the autofilled one without blurring e.g. "John" => Joh" (with backspace) => "John"
(Assignee)

Comment 29

2 months ago
(In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are blocking you) from comment #28)
> Comment on attachment 8846567 [details]
> Bug 1341569 - Add the form created time and telemetry probe for form filling
> duration.
> 
> https://reviewboard.mozilla.org/r/119618/#review139456
> 
> > Because we want to distinguish the cases that form is autofilled without changes or autofilled with additional user input. We apply setUserInput API for autofilling the from, and it'll also trigger the input event. Ideally we should only mark the form is edited if handler receive the event from real user input and ignore the event from setUserInput.
> 
> OK, I guess we had different ideas in mind and apologies for not looking at
> the patch before discussing. In my mind the "input" event with once:true
> should be used to know that a user started filling (not sure if it works for
> <select>) but we only need to use the "change" event (which happens on blur
> for <input>) to know that a user made a correction. I think we should
> coordinate with Ray for this since we should use the same logic as removing
> the highlight. How do we want to handle a user changing the the value and
> back to the autofilled one without blurring e.g. "John" => Joh" (with
> backspace) => "John"

About the highlighting, I confirmed with Ray that the example you mentioned should remove the highlight even filled data is resumed, and it's also the result that Chrome behaves.

I think the highlighting behavior and actual contact(profile) modification might be decoupled(like Chrome), but we didn't reach consensus about the timing for showing update doorhanger, and I assume that this metrics should align with the doorhanger. Ideally the form visually changes(whether highlights are existed or not) and the storage changes should be the same, but there are some corner cases that would make us confused:
- 1) Edit the fields and revert the changes before submitting the form(like MattN's example)
- 2) Data in storage is changed(via sync or editing profile at the same time) while filling a form and submit

Hi Joe, IIRC Juwei would prefer "Not" to display the doorhanger in both cases, but she could compromise if the doorhanger is aligned with the visual changes. MattN prefers that this metrics should simply considering the data changes just like password manager did. So there are two questions for this metrics:
- ) How to define the "form changes" for this metrics?
- ) Should we decouple the definition of "changes" in update doorhanger and metrics?

Hi MattN, please correct me if I misunderstood your saying on IRC(and any additional insight is welcome)
Flags: needinfo?(jcheng)
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 30

2 months ago
Based on meeting notes, we'll decouple the metrics and update doorhanger since the doorhanger will don't show up in several situations, even the submitted content is different from the address in storage.
In the meeting today we decided that a change means that the value in the field is different than the one we would store (e.g. after stripping leading and trailing whitespace) for the field and we will only consider fields we autofilled when looking for changes.

For this time probe we will have 9 buckets like so (I just made up prefixes now):
address.manual
address.autofill
address.modified
creditCard.manual
creditCard.autofill
creditCard.modified
addressAndCreditCard.manual
addressAndCreditCard.autofill
addressAndCreditCard.modified

I used addressAndCreditCard in case instead of "both" in case we support a third type of autofill in the future.

For tracking the number of times forms were submitted in the three cases I guess we need to reopen bug 990201 since it will only need 6 buckets instead of 9 because we don't need to handle addressAndCreditCard together since we have individual fields to consider for each type.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8846567 [details]
Bug 1341569 - Part 1: Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review141908

Clearing review flag for now based on the meeting discussion. I'm fine with this bug only handling the address fields for now but it may make sense to consider both during implementation to avoid incorrect data in the short term by counting credit card fields as address fields.

If it no longer makes sense to use TelemetryStopwatch then it's fine not to… I didn't anticipate the complexity when I suggested it.

Thanks
Attachment #8846567 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

2 months ago
Whiteboard: [story][form autofill:M3][autofill-metrics] → [story][form autofill:M4][autofill-metrics]
This is supposed to land in Fx55 to get a baseline so moving back to M3.

Steve, your patch will also need to change the moz.build file to start building the system add-on in non-Nightly builds otherwise it won't actually get run in beta/release.
status-firefox54: --- → unaffected
status-firefox55: --- → affected
Flags: needinfo?(schung)
Whiteboard: [story][form autofill:M4][autofill-metrics] → [story][form autofill:M3][autofill-metrics]
(Assignee)

Comment 36

a month ago
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #35)
> This is supposed to land in Fx55 to get a baseline so moving back to M3.
> 
> Steve, your patch will also need to change the moz.build file to start
> building the system add-on in non-Nightly builds otherwise it won't actually
> get run in beta/release.

But this patch wont work for the non-Nightly builds since the flags are supposed to be off, unless we enabled the feature for non-Nightly or adding probe in form toolkit instead of in formautofill system addon. The former will bring performance penalty even the feature is disabled, and the later will add extra code in central only for FX55 but with imprecise measurement(because we could not know which form need to measure/when to start measure without formautofill functions).

I talked with Joe about this before, and he agreed that we only record the number only when feature is enabled. We could still get the baseline in FX56 like first time filling form and some other cases. I don't see much benefit if we get the number in FX55.
Flags: needinfo?(schung)
(Assignee)

Updated

a month ago
Flags: needinfo?(MattN+bmo)
I've double checked with Joe that we don't land this on Fx55 because it's impossible to get a precise result without enabling the entire feature as Steve mentioned.

Updated

a month ago
status-firefox55: affected → unaffected
status-firefox56: --- → affected
Whiteboard: [story][form autofill:M3][autofill-metrics] → [story][form autofill:M4][autofill-metrics]
Comment on attachment 8846567 [details]
Bug 1341569 - Part 1: Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review158342

::: browser/extensions/formautofill/FormAutofillHandler.jsm:185
(Diff revision 7)
> +      case "input":
> +        if (!event.isTrusted) {
> +          return;
> +        }
> +

Please add an early return after this which checks `FormAutofillUtils.isFieldEligibleForAutofill(element)` so we don't do extra work or count non-autfill fields
Flags: needinfo?(MattN+bmo)
Comment on attachment 8874345 [details]
Bug 1341569 - Part 2: Send form started time and set probes in FormAutofillParent.

https://reviewboard.mozilla.org/r/145702/#review162334

::: browser/extensions/formautofill/FormAutofillContent.jsm:328
(Diff revision 1)
>    _onFormSubmit(profile, handler) {
>      Services.cpmm.sendAsyncMessage("FormAutofill:OnFormSubmit", profile);
> -    // TODO: Add message listener from parent for setting probes
> +    Services.cpmm.addMessageListener("FormAutofill:FillingType", function getFillingType(result) {
> +      Services.cpmm.removeMessageListener("FormAutofill:FillingType", getFillingType);
> +      handler.setFillFormTimeProbe(result.data);
> +    });

Rather than messaging in both directions, why not send the timestamp with OnFormSubmit and record the telemetry in the parent process?
Attachment #8874345 - Flags: review?(MattN+bmo)
(Assignee)

Updated

8 days ago
Whiteboard: [story][form autofill:M4][autofill-metrics] → [story][form autofill:M4][autofill-metrics][ETA:7/28]
Comment on attachment 8846567 [details]
Bug 1341569 - Part 1: Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review162326

::: browser/extensions/formautofill/FormAutofillHandler.jsm:62
(Diff revision 7)
>     * String of the filled profile's guid.
>     */
>    filledProfileGUID: null,
>  
>    /**
> +   * The time about when does user start to fill in data.

Time in milliseconds since epoch when a user started filling in the form.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:64
(Diff revision 7)
>    filledProfileGUID: null,
>  
>    /**
> +   * The time about when does user start to fill in data.
> +   */
> +  inputStartedTime:null,

Nit: missing space after : which should be caught by eslint

::: browser/extensions/formautofill/FormAutofillHandler.jsm:64
(Diff revision 7)
>    filledProfileGUID: null,
>  
>    /**
> +   * The time about when does user start to fill in data.
> +   */
> +  inputStartedTime:null,

Maybe `timeStartedFillingMS`?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:71
(Diff revision 7)
>      this.fieldDetails = fieldDetails ? fieldDetails : [];
> +    this.form.rootElement.addEventListener("input", this);
>      log.debug("Collected details on", this.fieldDetails.length, "fields");

I'm not sure it's a great place to have the event listener inside `collectFormFields` as it seems a bit unexpected there. What about beside the `formHandler.collectFormFields();` line instead?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:72
(Diff revision 7)
>     * Set fieldDetails from the form about fields that can be autofilled.
>     */
>    collectFormFields() {
>      let fieldDetails = FormAutofillHeuristics.getFormInfo(this.form);
>      this.fieldDetails = fieldDetails ? fieldDetails : [];
> +    this.form.rootElement.addEventListener("input", this);

Please get Sean to help you do a talos comparison to ensure this doesn't regress anything.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:166
(Diff revision 7)
>    },
> +  /**

Nit: add a new line

::: browser/extensions/formautofill/FormAutofillHandler.jsm:169
(Diff revision 7)
> +   *
> +   * @returns {Object|null}
> +   *          Return fieldDetail if fieldDetail's element ref could match the target.
> +   *          (or return null if the element could not match any fieldDetail).
> +   */
> +  getDetailByElement(element) {

Please document the @param and the assumption that `this.fieldDetails` was already populated

::: browser/extensions/formautofill/FormAutofillHandler.jsm:174
(Diff revision 7)
> +   *
> +   * @returns {Object|null}
> +   *          Return fieldDetail if fieldDetail's element ref could match the target.
> +   *          (or return null if the element could not match any fieldDetail).
> +   */
> +  getDetailByElement(element) {

Maybe `getFieldDetailsForElement`?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:184
(Diff revision 7)
> +    }
> +    return null;
> +  },
> +
> +  handleEvent(event) {
> +    switch(event.type) {

Nit: I think you're supposed to have a space after `switch` but I always forget

::: browser/extensions/formautofill/FormAutofillHandler.jsm:203
(Diff revision 7)
> +  setFillFormTimeProbe(formType, fillingType) {
> +    // There will have 3 type of form(address/creditcard/address+creditcard)
> +    // and 3 filling type.

Replace this comment with @param documentation

::: toolkit/components/telemetry/Histograms.json:13314
(Diff revision 7)
> +  "FORM_FILLING_REQUIRED_TIME_MS": {
> +    "record_in_processes": ["main"],

With e10s `setFillFormTimeProbe` should run in the content process so I guess this doesn't work with it enabled.

::: toolkit/components/telemetry/Histograms.json:13323
(Diff revision 7)
> +    "kind": "exponential",
> +    "high": 300000,
> +    "n_buckets": 22,
> +    "keyed": true,
> +    "bug_numbers": [1341569],
> +    "description": "Milliseconds between focusing an autofill-eligible field and submitting its form"

"focusing" is incorrect here. Maybe say "Milliseconds between starting to fill an autofill-eligible form field and submitting the form"
Attachment #8846567 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 43

2 days ago
mozreview-review-reply
Comment on attachment 8846567 [details]
Bug 1341569 - Part 1: Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review162326

> I'm not sure it's a great place to have the event listener inside `collectFormFields` as it seems a bit unexpected there. What about beside the `formHandler.collectFormFields();` line instead?

I still keep this in `collectFormFields` because it's the only entry for listener and most of logic is still based on current handler instead of FormAutofillContent, which means we'll need to query the handler again by received event target in handleEvent.
(Assignee)

Updated

2 days ago
Blocks: 1382143
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
You need to log in before you can comment on or make changes to this bug.