Closed Bug 1341569 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Form Manager, defect)

x86
Unspecified
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: steveck, Assigned: steveck)

References

(Blocks 2 open bugs)

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.

Attachments

(2 files, 3 obsolete files)

Product Owner should be able to see how long does the users spent on the form under form autofill feature enabled/disabled.
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler 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)
...Or only collect the data from target website
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 - Add the form created time in handler 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)
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.
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler 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.
Assignee: nobody → schung
Whiteboard: [story] [form autofill][autofill-metrics] → [story][form autofill:M2][autofill-metrics]
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
Attachment #8846566 - Flags: review?(MattN+bmo)
Attachment #8846567 - Flags: review?(MattN+bmo)
Attachment #8846566 - Attachment is obsolete: true
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler 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.
Whiteboard: [story][form autofill:M2][autofill-metrics] → [story][form autofill:M3][autofill-metrics]
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler 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)
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler 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?
(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.
Flags: needinfo?(jcheng)
User Story: (updated)
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.
Attachment #8864474 - Attachment is obsolete: true
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler 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 - Add the form created time in handler 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.
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler 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 - Add the form created time in handler 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"
(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)
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 - Add the form created time in handler 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)
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.
Flags: needinfo?(schung)
Whiteboard: [story][form autofill:M4][autofill-metrics] → [story][form autofill:M3][autofill-metrics]
(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)
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.
Whiteboard: [story][form autofill:M3][autofill-metrics] → [story][form autofill:M4][autofill-metrics]
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler 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)
Whiteboard: [story][form autofill:M4][autofill-metrics] → [story][form autofill:M4][autofill-metrics][ETA:7/28]
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler 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 on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler 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.
Blocks: 1382143
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler and telemetry probe for form filling duration.

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

::: browser/extensions/formautofill/FormAutofillContent.jsm:337
(Diff revision 9)
> +   * @param {int} timeStartedFillingMS Time of form filling started.
>     */
> -  _onFormSubmit(profile, domWin) {
> +  _onFormSubmit(profile, domWin, timeStartedFillingMS) {
>      let mm = this._messageManagerFromWindow(domWin);
>      mm.sendAsyncMessage("FormAutofill:OnFormSubmit", profile);
> +    // TODO: Send handler.timeStartedFillingMS to parent for recording time
>    },
>  

Please fold this file's changes into part 2 to avoid the temporary TODO

::: browser/extensions/formautofill/FormAutofillHandler.jsm:384
(Diff revision 9)
>  
>      return profile;
>    },
> +
> +  /**
> +   * Find the fieldDetail by HTML element(assume all details were collected in collectFormFields).

Nit: space before "("

::: browser/extensions/formautofill/FormAutofillParent.jsm:327
(Diff revision 9)
> +  /**
> +   * Set the probes for the filling time with specific filling type and form type.
> +   *
> +   * @private

Changes to this file and Histograms.json should also be in part 2.

::: browser/extensions/formautofill/FormAutofillParent.jsm:332
(Diff revision 9)
> +   *         3 type of form(address/creditcard/address-creditcard).
> +   * @param  {string} fillingType
> +   *         3 filling type(manual/autofill/autofill-update).

Nit: put spaces before "(" (2x)
Attachment #8846567 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8874345 [details]
Bug 1341569 - Part 2: Send form started time and set probes in FormAutofillParent.

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

::: browser/extensions/formautofill/FormAutofillParent.jsm:279
(Diff revision 3)
> -    let {address} = data;
> +    let {profile, timeStartedFillingMS} = data;
> +    let {address} = profile;

Nit: I believe there is syntax to do this in one line. I believe Ray used it in another bug.

::: browser/extensions/formautofill/FormAutofillParent.jsm:284
(Diff revision 3)
> -    let {address} = data;
> +    let {profile, timeStartedFillingMS} = data;
> +    let {address} = profile;
>  
>      if (address.guid) {
>        if (!this.profileStorage.addresses.mergeIfPossible(address.guid, address.record)) {
> +        this._setFillFormTimeProbe("address", "autofill-update", timeStartedFillingMS);

Can you rename this method to `_recordFormFillingTime`
Attachment #8874345 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler and telemetry probe for form filling duration.

Hi Benjamin, it's another metrics about the form autofilling time. Similar to bug 990200 but this one is focused on the duration of the form filling. I apply the keyed histogram because it'll have 9 different categories. Please let me know if the description in the json is not clear to you, thanks!
Attachment #8846567 - Flags: review?(benjamin)
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler and telemetry probe for form filling duration.

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

data-r=me
Attachment #8846567 - Flags: review?(benjamin) → review+
Attachment #8874345 - Attachment is obsolete: true
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler and telemetry probe for form filling duration.

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

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

Comparison in: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=402ad3c884023a2bacac46de03ff9d2641df4631&newProject=try&newRevision=8a31d78717469a2dc1393bba4cf050c3ed125651&framework=1&showOnlyImportant=0
(In reply to Steve Chung [:steveck] from comment #51)
> Comment on attachment 8846567 [details]
> Bug 1341569 - Add the form created time in handler and telemetry probe for
> form filling duration.
> 
> https://reviewboard.mozilla.org/r/119618/#review162326
> 
> > Please get Sean to help you do a talos comparison to ensure this doesn't regress anything.
> 
> Comparison in:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=402ad3c884023a2bacac46de03ff9d26
> 41df4631&newProject=try&newRevision=8a31d78717469a2dc1393bba4cf050c3ed125651&
> framework=1&showOnlyImportant=0

Hmm… from looking at the graphs, only the "quantum_pageload_amazon opt e10s stylo" looks concerning after checking "Show only important changes". That's a newer test that I'm not familiar with. There is a description at https://wiki.mozilla.org/Buildbot/Talos/Tests#tp6

I retriggered them some more to to see if they are just noisy or suffer from fluctuations due to day of week or time of day. It seems like we need to investigate that issue. If you think it's a false positive you could talk to jmaher.
(Will ni Joel after his PTO)

Hi Joel, do you think the comparison result in comment 51 is just a false positive, or it really is a regression? This patch will definitely bring some side effect since it added another event listener in the call path, but I'm not sure whether it's severe or not in the result since most of the items are low impact.
Flags: needinfo?(jmaher)
there are 3 issues which I see in the comparison:
* linux64-stylo-seq sessionreestore
* win10 tp5 responsiveness
* linux64 tresize

these are marked as medium severity in the compare due to the large volume of noise- overall they have increased.  It is impossible to continue adding features and code without adjusting performance- it is ideal to understand why it is increasing :)

The tresize seems to be a less important test case.  sessionrestore is important, but this is restricted to linux64-stylo-seq.  For win10 tp5 responsiveness, the regression is ~5%, this is probably the one to worry most about.
Flags: needinfo?(jmaher)
Hi Matt,

Maybe we can just start record when focused in without adding another listener, therefore it shouldn't affect the performance like the original patch(but it might not as accurate as the old one). wdyt?
Flags: needinfo?(MattN+bmo)
What if we only add the event listener inside the "focusin" handler and after the eligibility check: https://dxr.mozilla.org/mozilla-central/rev/b95b1638db48fc3d450b95b98da6bcd2f9326d2f/browser/extensions/formautofill/content/FormAutofillFrameScript.js#49

I would recommend rebasing the patch on current m-c (possibly on top of the faster findLabelElements patch) to see if it still causes problems since there have been some changes.

(In reply to Steve Chung [:steveck] from comment #55)
> Maybe we can just start record when focused in without adding another
> listener, therefore it shouldn't affect the performance like the original
> patch(but it might not as accurate as the old one). wdyt?

I think I'd only be fine with that if we can detect a user-initiated focus and not record if the field is auto-focused. I'm not sure if that can be detected properly though as event.isTrusted may be true for @autofocus or .focus() maybe? If we start the timer when a field is auto-focused then we'll count a lot of time before the user starts typing which I think will shadow any improvement autofill brings.
Flags: needinfo?(MattN+bmo)
Flags: needinfo?(jcheng) → needinfo?(chsiang)
Seems like only linux64-stylo-sequential have some alert. Retrigger the test again to confirm if that's regression or false alert.
After 15 runs for linux64-stylo-sequential, the warning seems disappeared.
Flags: needinfo?(chsiang)
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s fb78fc3f270f -d 75ad7acb73f6: rebasing 416999:fb78fc3f270f "Bug 1341569 - Add the form created time in handler and telemetry probe for form filling duration. r=benjamin+7044,MattN" (tip)
merging browser/extensions/formautofill/FormAutofillContent.jsm
merging toolkit/components/telemetry/Histograms.json
warning: conflicts while merging toolkit/components/telemetry/Histograms.json! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Patch rebased.
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71d4452dd938
Add the form created time in handler and telemetry probe for form filling duration. r=benjamin+7044,MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/71d4452dd938
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1395727
Can we please back this out immediately.

Adding any new input or change event listener surely is going to regress Speedometer badly.
Backed out at smaug's request for regressing speedometer-misc-Angular2-TypeScript-TodoMVC-CompletingAllItems-sync.

https://hg.mozilla.org/integration/mozilla-inbound/rev/23668b78e645d68f3240d4554516572ab57c6319
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla57 → ---
Based on a quick skim over what the patch here did, it was accessing event.target from the input event handler in JS, which causes xray wrappers to be created for the element which are extremely expensive.  Also this kind of event handler would run too frequently, for example when the value of a form control is changed by web page script through setting the value property.  Also it seems that the intention behind the patch was to only do this for text controls, whereas the input event handler here would handle all events from all HTMLInputElement control types (checkboxes, file controls, numeric controls, etc.)
Ok, so it seems not possible to have a listener right here. Hi Matt, instead of setting the start time at collectFields, would it be better to start the recording at the startSearch?
Flags: needinfo?(MattN+bmo)
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler and telemetry probe for form filling duration.

Requesting Luke's feedback for the new measuring as well. I think this changes should bring much less impact than previous one but it could still record the form filling time properly in any filling cases(manual/autofill/update).
Attachment #8846567 - Flags: feedback?(lchang)
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler and telemetry probe for form filling duration.

The latest patch will only start recording the time when Autofill ProfileAutoCompleteSearch registered properly, which means it must have profiles stored. So we could not record the duration for first time use(or people who clear the storage without disabling the feature). We'll get less data for manual filling, but we might not have much choices considering the performance.
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #70)
> Based on a quick skim over what the patch here did, it was accessing
> event.target from the input event handler in JS, which causes xray wrappers
> to be created for the element which are extremely expensive.

We could have instead added the event listener to each relevant form element in order to avoid the filtering inside the event handler but that seemed less clean and would require more (focused) event listeners.

> Also this kind
> of event handler would run too frequently, for example when the value of a
> form control is changed by web page script through setting the value
> property.

Well, it only runs once if the input event was for an autofill-eligible field since we call `removeEventListener` in that case. The point of this code is to know when a user starts editing a form (indicated by an "input' event) to record a start timestamp. We don't care about edits to autofill-related fields after the first edit which is why we remove the listener after the first relevant event.

>  Also it seems that the intention behind the patch was to only do
> this for text controls, whereas the input event handler here would handle
> all events from all HTMLInputElement control types (checkboxes, file
> controls, numeric controls, etc.)

This is related to my first reply above where we chose to add the event listener to the form container element and filter out irrelevant fields rather than add a listener to each relevant field separately and have to remove all of them after the first event is received. Only adding the listener to eligible fields may be more performant on Speedometer but more listeners could cause other slowdowns.
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler and telemetry probe for form filling duration.

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

::: browser/extensions/formautofill/FormAutofillHandler.jsm:134
(Diff revisions 13 - 14)
>      this.fieldDetails = fieldDetails ? fieldDetails : [];
> -    this.form.rootElement.addEventListener("input", this);
>      log.debug("Collected details on", this.fieldDetails.length, "fields");

Can we try only adding a `once` listener on the eligible fields and removing it from all others when the field starts to get filled?

::: browser/extensions/formautofill/FormAutofillContent.jsm:111
(Diff revision 14)
> +    // Record filling start time since user triggered searching
> +    if (!handler.timeStartedFillingMS) {
> +      handler.timeStartedFillingMS = Date.now();
> +    }

This isn't going to record for users with autofill turned off either…

::: toolkit/components/telemetry/Histograms.json:13592
(Diff revision 14)
> +    "kind": "exponential",
> +    "high": 300000,
> +    "n_buckets": 22,
> +    "keyed": true,
> +    "bug_numbers": [1341569],
> +    "description": "Milliseconds between starting to fill an autofill-eligible form field and submitting the form"

You should indicate what the key is
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler and telemetry probe for form filling duration.

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

> This isn't going to record for users with autofill turned off either…

But I guess we weren't going to record when autofill was turned off anyways so this matches what you said about affecting the case where storage is empty. I'm just concerned that we're going to get too little data for the autofill not used case since we're already excluding the autofill off case.
Flags: needinfo?(MattN+bmo)
Patch updated. I still add event listener but it's only for the inputs that would be autofilled instead of for form or document in the previous patch. I also triggered the talos test for the new patch here: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=2ee4ca6b5a76a036ca80a1decddccab72f16f700&newProject=try&newRevision=fa57a48e56043cde18e36be29fc96b7f6b1419a1&framework=1&showOnlyImportant=0

Hi Ehsan, please let me know if you still discover any possible regression from the patch or talos result, thanks!
Flags: needinfo?(ehsan)
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler and telemetry probe for form filling duration.

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

> Can we try only adding a `once` listener on the eligible fields and removing it from all others when the field starts to get filled?

2 reasons that I don't apply addEventListener once here:
- Fitler out the non-trusted event
- We'll still need to remove other input's event listener
(In reply to Steve Chung [:steveck] from comment #79)
> Patch updated. I still add event listener but it's only for the inputs that
> would be autofilled instead of for form or document in the previous patch. I
> also triggered the talos test for the new patch here:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=2ee4ca6b5a76a036ca80a1decddccab7
> 2f16f700&newProject=try&newRevision=fa57a48e56043cde18e36be29fc96b7f6b1419a1&
> framework=1&showOnlyImportant=0

Speedometer doesn't run on Talos.

> Hi Ehsan, please let me know if you still discover any possible regression
> from the patch or talos result, thanks!

Sorry, I don't have time to look into this right now, you should profile this yourself before landing.  :-)

You can find the latest version of the Speedometer benchmark here: https://mozilla.github.io/arewefastyet-speedometer/2.0/

Please use the Gecko Profiler to profile this.  On Linux and Mac OSX, you can set the profile interval to 0.1ms, set the buffer size to something large like 450MB and profile running the first 150 or so subtests of the benchmark with your patch applied on an optimized build in a new profile.  The desired end state is for the code that you are adding to not appear in the captured profile.  Please post a link to the captured profile on the bug showing the results before landing.  Thank you!
Flags: needinfo?(ehsan)
Here is the profile result for the first 150+ test : https://perfht.ml/2eHV6Wg It seems like formautofill will not be triggered in the Speedometer by input event any more. I realize the biggest problem in the previous patch is I should avoid adding event listener if there's no valid form for autofill. It will cause the document receives input event repeatedly in the Speedometer test, even the test page doesn't contains any form for autofill. You'll still find some formautofill related calls in the profile, but that's necessity for checking whether the web page contains form for autofill.
Flags: needinfo?(ehsan)
Blocks: 1397636
Thanks, it looks like what you see in the profile is all _doIdentifyFormAutofillFileds() calls.  We are still waiting for bug 1375568 to fix those, but that seems unrelated to this patch.
Flags: needinfo?(ehsan)
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler and telemetry probe for form filling duration.

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

::: toolkit/components/telemetry/Histograms.json:13641
(Diff revision 16)
> +  "FORM_FILLING_REQUIRED_TIME_MS": {
> +    "record_in_processes": ["main"],
> +    "alert_emails": ["autofill@lists.mozilla.org", "jcheng@mozilla.com", "chsiang@mozilla.com"],

Please keep all the formautofill probes together in the histogram file
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time in handler and telemetry probe for form filling duration.

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

To be clear, this is r=me assuming the speedometer score didn't get worse.
I triggered speedometer test locally and both score are roughly the same (42runs/minute ± 2%). Sometimes the number will lower than base but sometimes higher, so I assume there's no(or very little) regression introduced by this patch.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1fe86d8df2cd
Add the form created time in handler and telemetry probe for form filling duration. r=benjamin+7044,MattN
Keywords: checkin-needed
Backed out for eslint failure at browser/extensions/formautofill/FormAutofillHandler.jsm:666: Missing trailing comma:

https://hg.mozilla.org/integration/autoland/rev/6bbc9cb736d0256d94ad13c593e14f885539bb10

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1fe86d8df2cd7172c66b0506bf0b0e789e86cf01&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=129541819&repo=autoland
> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/extensions/formautofill/FormAutofillHandler.jsm:666:4 | Missing trailing comma. (comma-dangle)
Flags: needinfo?(schung)
Lint error fixed
Flags: needinfo?(schung)
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/38e3ac4eee42
Add the form created time in handler and telemetry probe for form filling duration. r=benjamin+7044,MattN
Keywords: checkin-needed
Hi Matt, this patch is for the beta uplift since there's too many conflicts if we want to uplift the original patch to beta.
Attachment #8906500 - Flags: review?(MattN+bmo)
https://hg.mozilla.org/mozilla-central/rev/38e3ac4eee42
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8906500 [details] [diff] [review]
beta_uplift.patch

Review of attachment 8906500 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks
Attachment #8906500 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8906500 [details] [diff] [review]
beta_uplift.patch

Approval Request Comment
[Feature/Bug causing the regression]:
Feature.

[User impact if declined]:
We won't have users' metrics data for further improvements.

[Is this code covered by automated tests?]:
No. It's metrics only.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
N/A

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
It affects Form Autofill system add-on only.

[String changes made/needed]:
N/A
Attachment #8906500 - Flags: approval-mozilla-beta?
Comment on attachment 8906500 [details] [diff] [review]
beta_uplift.patch

Taking this for beta 12 since we are planning to ship Form Autofill in 56. 
If there are any problems (for example, perf issues) we will need to back this patch out.
Attachment #8906500 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Luke Chang [:lchang] from comment #96)
> [Needs manual test from QE? If yes, steps to reproduce]: 
> No.
Marking this bug as qe-verify- per Luke's assessment on manual testing needs.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.