Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1010263002:
[Autofill] Add FormEvent logging for "will submit form". (Closed)
|
Created:
5 years, 9 months ago by Mathieu Modified:
5 years, 9 months ago CC:
chromium-reviews, estade+watch_chromium.org, browser-components-watch_chromium.org, asvitkine+watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Autofill] Add FormEvent logging for "will submit form".
A previous CL split the AutofillManager::OnFormSubmitted logic into OnWillSubmit and OnFormSubmitted to support AJAX forms. This change splits the logged events similarly and introduces AutofillMetrics::FormEventLogger::OnWillSubmitForm, which is logged anytime a form is about to be submitted. We move FormEventLogger::OnDidSubmitForm back into AutofillManager::OnFormSubmitted, since it more closely matches what happened before.
We will use the extra "will submit" logged events to determine the proportion of submitted forms that are AJAX (don't end up calling OnFormSubmitted).
BUG=460488
TEST=AutofillMetricsTest
Committed: https://crrev.com/6072951da5e97bb2571419005dae5da1f24e06cc
Cr-Commit-Position: refs/heads/master@{#322169}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Changed histograms.xml wording #Patch Set 3 : Changed approach #Patch Set 4 : git cl format #
Total comments: 9
Patch Set 5 : addressed comments #Patch Set 6 : rebase #Patch Set 7 : logged once #
Total comments: 11
Patch Set 8 : nits #Patch Set 9 : split #
Messages
Total messages: 25 (5 generated)
mathp@chromium.org changed reviewers: + estade@chromium.org
Hi, please have a look!
estade@chromium.org changed reviewers: + isherman@chromium.org
mostly lg, +isherman for histograms https://codereview.chromium.org/1010263002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1010263002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:1884: + Tracks whether an Autofillable form was user submitted or not. I don't think the terminology "user submitted" is very clear.
On 2015/03/18 00:45:32, Evan Stade wrote: > mostly lg, +isherman for histograms > > https://codereview.chromium.org/1010263002/diff/1/tools/metrics/histograms/hi... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1010263002/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:1884: + Tracks whether an > Autofillable form was user submitted or not. > I don't think the terminology "user submitted" is very clear. Found an issue, will have to rework this CL, hold off on review, thanks!
Please review and read the new description. The previous way was wrong since |user_submitted| was always false at the point I was logging it. FormEventLogger seems like a good way to do it.
I'd expect to see histograms.xml changes, since you're changing an UMA enum. Could you please make the needed changes, or explain why they're not needed? https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:573: Log(AutofillMetrics::FORM_EVENT_WILL_SUBMIT); Should this event be logged only once, as below? https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:275: FORM_EVENT_WILL_SUBMIT, Why are you adding this event in the middle of the list? I believe that this enumeration backs an enumerated UMA histogram, right? https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics_unittest.cc:232: // Calls both OnWillSubmitForm and OnFormSubmitted. Please add test coverage for OnWillSubmitForm being called, without a subsequent call to OnFormSubmitted. (Or did I miss the place where this is already tested?)
https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:393: credit_card_form_event_logger_->OnDidSubmitForm(); can we keep the names the same? i.e. OnFormSubmitted (or fix the slightly weird discrepancy between AutofillManager::OnFormSubmitted and OnWillSubmitForm)
mathp@chromium.org changed reviewers: + waltercacau@chromium.org
+walter Hi Walter, question below thanks! https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:393: credit_card_form_event_logger_->OnDidSubmitForm(); On 2015/03/20 20:55:03, Evan Stade wrote: > can we keep the names the same? i.e. OnFormSubmitted (or fix the slightly weird > discrepancy between AutofillManager::OnFormSubmitted and OnWillSubmitForm) Sure, renamed OnDidSubmitForm to OnFormSubmitted. https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:573: Log(AutofillMetrics::FORM_EVENT_WILL_SUBMIT); On 2015/03/18 22:42:10, Ilya Sherman wrote: > Should this event be logged only once, as below? +walter I wanted to capture all OnWillSubmit events (possibly AJAX submissions) before the page navigated. I'm not sure why there is logic to avoid logging OnFormSubmitted twice. Perhaps walter can advise? https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:275: FORM_EVENT_WILL_SUBMIT, On 2015/03/18 22:42:10, Ilya Sherman wrote: > Why are you adding this event in the middle of the list? I believe that this > enumeration backs an enumerated UMA histogram, right? Done. Yes of course. https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics_unittest.cc:232: // Calls both OnWillSubmitForm and OnFormSubmitted. On 2015/03/18 22:42:10, Ilya Sherman wrote: > Please add test coverage for OnWillSubmitForm being called, without a subsequent > call to OnFormSubmitted. (Or did I miss the place where this is already > tested?) Done.
https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:573: Log(AutofillMetrics::FORM_EVENT_WILL_SUBMIT); On 2015/03/23 15:24:02, Mathieu Perreault wrote: > On 2015/03/18 22:42:10, Ilya Sherman wrote: > > Should this event be logged only once, as below? > > +walter > > I wanted to capture all OnWillSubmit events (possibly AJAX submissions) before > the page navigated. I'm not sure why there is logic to avoid logging > OnFormSubmitted twice. Perhaps walter can advise? The idea was that we were tracking the basic user flow from: INTERACT -> SHOW SUGGESTION -> SELECT/FILL SUGGESTION -> SUBMIT For that, we wanted it to be counted only once per page load. What is the indent in capturing the FORM_EVENT_WILL_SUBMIT? It may make sense to create also a FORM_EVENT_WILL_SUBMIT_[CASE]_ONCE if we think there are quite a few forms that simply submit with ajax without us being able to detect the user click.
On 2015/03/23 19:06:37, Walter Cacau wrote: > https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... > File components/autofill/core/browser/autofill_metrics.cc (right): > > https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... > components/autofill/core/browser/autofill_metrics.cc:573: > Log(AutofillMetrics::FORM_EVENT_WILL_SUBMIT); > On 2015/03/23 15:24:02, Mathieu Perreault wrote: > > On 2015/03/18 22:42:10, Ilya Sherman wrote: > > > Should this event be logged only once, as below? > > > > +walter > > > > I wanted to capture all OnWillSubmit events (possibly AJAX submissions) before > > the page navigated. I'm not sure why there is logic to avoid logging > > OnFormSubmitted twice. Perhaps walter can advise? > > The idea was that we were tracking the basic user flow from: > > INTERACT -> SHOW SUGGESTION -> SELECT/FILL SUGGESTION -> SUBMIT > For that, we wanted it to be counted only once per page load. > > What is the indent in capturing the FORM_EVENT_WILL_SUBMIT? It may make sense to > create also a FORM_EVENT_WILL_SUBMIT_[CASE]_ONCE if we think there are quite a > few forms that simply submit with ajax without us being able to detect the user > click. The intent is to measure the ratio of (# of forms where only WILL_SUBMIT is logged) / (WILL_SUBMIT + FORM_SUBMITTED_*). Have you seen a case where FORM_SUBMITTED_* is logged twice per page load? IIUC, the form event logger is reset on navigation. I was thinking I wouldn't add such a check for WILL_SUBMIT to capture multiple ajax submissions.
On 2015/03/23 19:17:33, Mathieu Perreault wrote: > On 2015/03/23 19:06:37, Walter Cacau wrote: > > > https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... > > File components/autofill/core/browser/autofill_metrics.cc (right): > > > > > https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... > > components/autofill/core/browser/autofill_metrics.cc:573: > > Log(AutofillMetrics::FORM_EVENT_WILL_SUBMIT); > > On 2015/03/23 15:24:02, Mathieu Perreault wrote: > > > On 2015/03/18 22:42:10, Ilya Sherman wrote: > > > > Should this event be logged only once, as below? > > > > > > +walter > > > > > > I wanted to capture all OnWillSubmit events (possibly AJAX submissions) > before > > > the page navigated. I'm not sure why there is logic to avoid logging > > > OnFormSubmitted twice. Perhaps walter can advise? > > > > The idea was that we were tracking the basic user flow from: > > > > INTERACT -> SHOW SUGGESTION -> SELECT/FILL SUGGESTION -> SUBMIT > > For that, we wanted it to be counted only once per page load. > > > > What is the indent in capturing the FORM_EVENT_WILL_SUBMIT? It may make sense > to > > create also a FORM_EVENT_WILL_SUBMIT_[CASE]_ONCE if we think there are quite a > > few forms that simply submit with ajax without us being able to detect the > user > > click. > > The intent is to measure the ratio of (# of forms where only WILL_SUBMIT is > logged) / (WILL_SUBMIT + FORM_SUBMITTED_*). > > Have you seen a case where FORM_SUBMITTED_* is logged twice per page load? IIUC, > the form event logger is reset on navigation. I was thinking I wouldn't add such > a check for WILL_SUBMIT to capture multiple ajax submissions. I don't remember actually finding a case where it happened, but I didn't look very much to be fair :). I was trying not to rely too much on the way those classes interact as if we ended up logging twice it would make our metrics non reliable for comparing submit rates between the different groups (people with only wallet data, people with no data, ...). As your intent is to compare WILL_SUBMIT with FORM_SUBMITTED_*, I would suggest you to rename it to WILL_SUBMIT_ONCE and add the check of once so at least they are consistent.
On 2015/03/23 19:34:12, Walter Cacau wrote: > On 2015/03/23 19:17:33, Mathieu Perreault wrote: > > On 2015/03/23 19:06:37, Walter Cacau wrote: > > > > > > https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... > > > File components/autofill/core/browser/autofill_metrics.cc (right): > > > > > > > > > https://codereview.chromium.org/1010263002/diff/60001/components/autofill/cor... > > > components/autofill/core/browser/autofill_metrics.cc:573: > > > Log(AutofillMetrics::FORM_EVENT_WILL_SUBMIT); > > > On 2015/03/23 15:24:02, Mathieu Perreault wrote: > > > > On 2015/03/18 22:42:10, Ilya Sherman wrote: > > > > > Should this event be logged only once, as below? > > > > > > > > +walter > > > > > > > > I wanted to capture all OnWillSubmit events (possibly AJAX submissions) > > before > > > > the page navigated. I'm not sure why there is logic to avoid logging > > > > OnFormSubmitted twice. Perhaps walter can advise? > > > > > > The idea was that we were tracking the basic user flow from: > > > > > > INTERACT -> SHOW SUGGESTION -> SELECT/FILL SUGGESTION -> SUBMIT > > > For that, we wanted it to be counted only once per page load. > > > > > > What is the indent in capturing the FORM_EVENT_WILL_SUBMIT? It may make > sense > > to > > > create also a FORM_EVENT_WILL_SUBMIT_[CASE]_ONCE if we think there are quite > a > > > few forms that simply submit with ajax without us being able to detect the > > user > > > click. > > > > The intent is to measure the ratio of (# of forms where only WILL_SUBMIT is > > logged) / (WILL_SUBMIT + FORM_SUBMITTED_*). > > > > Have you seen a case where FORM_SUBMITTED_* is logged twice per page load? > IIUC, > > the form event logger is reset on navigation. I was thinking I wouldn't add > such > > a check for WILL_SUBMIT to capture multiple ajax submissions. > > I don't remember actually finding a case where it happened, but I didn't look > very much to be fair :). > I was trying not to rely too much on the way those classes interact as if we > ended up logging twice it would make our metrics non reliable for comparing > submit rates between the different groups (people with only wallet data, people > with no data, ...). > > As your intent is to compare WILL_SUBMIT with FORM_SUBMITTED_*, I would suggest > you to rename it to WILL_SUBMIT_ONCE and add the check of once so at least they > are consistent. Thanks, Makes sense. I've just done that in the latest patchset.
https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:528: logged_suggestion_filled_was_masked_server_card_(false){}; nit: Please revert the diff on this line. Also, while you're here, please drop the semicolon on this line. https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:611: Log(AutofillMetrics::FORM_EVENT_WILL_SUBMIT_ONCE); Do we want to split up the WILL_SUBMIT_* metrics the same way we do the SUBMITTED_* ones? For some forms, we'll only see the "WILL_SUBMIT" metrics, right? https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:285: // interrupted by JavaScript, the "form submitted" events below will also be nit: "below" -> "above" https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:286: // logged. Triggered once per page load. nit: "once" -> "at most once" https://codereview.chromium.org/1010263002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1010263002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44982: + <int value="15" label="Form is about to be submitted"/> nit: " (once)"
Question as to splitting the metric further, below. Thanks! https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:528: logged_suggestion_filled_was_masked_server_card_(false){}; On 2015/03/23 20:58:16, Ilya Sherman wrote: > nit: Please revert the diff on this line. Also, while you're here, please drop > the semicolon on this line. This is the work of "git cl format" but removing the semi-colon makes it nice again. https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:611: Log(AutofillMetrics::FORM_EVENT_WILL_SUBMIT_ONCE); On 2015/03/23 20:58:16, Ilya Sherman wrote: > Do we want to split up the WILL_SUBMIT_* metrics the same way we do the > SUBMITTED_* ones? For some forms, we'll only see the "WILL_SUBMIT" metrics, > right? Yes some forms will only emit WILL_SUBMIT_ONCE. I don't know, I'm not sure I am after the same analysis as Walter was when he did OnFormSubmitted... I guess I could add it for parity? https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:285: // interrupted by JavaScript, the "form submitted" events below will also be On 2015/03/23 20:58:16, Ilya Sherman wrote: > nit: "below" -> "above" Done. https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:286: // logged. Triggered once per page load. On 2015/03/23 20:58:17, Ilya Sherman wrote: > nit: "once" -> "at most once" Done. https://codereview.chromium.org/1010263002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1010263002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44982: + <int value="15" label="Form is about to be submitted"/> On 2015/03/23 20:58:17, Ilya Sherman wrote: > nit: " (once)" Done.
histograms changes LGTM https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:611: Log(AutofillMetrics::FORM_EVENT_WILL_SUBMIT_ONCE); On 2015/03/23 21:11:28, Mathieu Perreault wrote: > On 2015/03/23 20:58:16, Ilya Sherman wrote: > > Do we want to split up the WILL_SUBMIT_* metrics the same way we do the > > SUBMITTED_* ones? For some forms, we'll only see the "WILL_SUBMIT" metrics, > > right? > > Yes some forms will only emit WILL_SUBMIT_ONCE. I don't know, I'm not sure I am > after the same analysis as Walter was when he did OnFormSubmitted... I guess I > could add it for parity? I defer to Walter and Evan, but it does seem likely that the same data would be useful for AJAX-powered forms as for others.
On 2015/03/23 23:58:17, Ilya Sherman wrote: > histograms changes LGTM > > https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... > File components/autofill/core/browser/autofill_metrics.cc (right): > > https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... > components/autofill/core/browser/autofill_metrics.cc:611: > Log(AutofillMetrics::FORM_EVENT_WILL_SUBMIT_ONCE); > On 2015/03/23 21:11:28, Mathieu Perreault wrote: > > On 2015/03/23 20:58:16, Ilya Sherman wrote: > > > Do we want to split up the WILL_SUBMIT_* metrics the same way we do the > > > SUBMITTED_* ones? For some forms, we'll only see the "WILL_SUBMIT" metrics, > > > right? > > > > Yes some forms will only emit WILL_SUBMIT_ONCE. I don't know, I'm not sure I > am > > after the same analysis as Walter was when he did OnFormSubmitted... I guess I > > could add it for parity? > > I defer to Walter and Evan, but it does seem likely that the same data would be > useful for AJAX-powered forms as for others. Ilya makes a good point. Could you split it for parity? If there is a substantial number of forms where only this will be detected, we want to get that data split.
On 2015/03/24 00:01:07, Walter Cacau wrote: > On 2015/03/23 23:58:17, Ilya Sherman wrote: > > histograms changes LGTM > > > > > https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... > > File components/autofill/core/browser/autofill_metrics.cc (right): > > > > > https://codereview.chromium.org/1010263002/diff/120001/components/autofill/co... > > components/autofill/core/browser/autofill_metrics.cc:611: > > Log(AutofillMetrics::FORM_EVENT_WILL_SUBMIT_ONCE); > > On 2015/03/23 21:11:28, Mathieu Perreault wrote: > > > On 2015/03/23 20:58:16, Ilya Sherman wrote: > > > > Do we want to split up the WILL_SUBMIT_* metrics the same way we do the > > > > SUBMITTED_* ones? For some forms, we'll only see the "WILL_SUBMIT" > metrics, > > > > right? > > > > > > Yes some forms will only emit WILL_SUBMIT_ONCE. I don't know, I'm not sure I > > am > > > after the same analysis as Walter was when he did OnFormSubmitted... I guess > I > > > could add it for parity? > > > > I defer to Walter and Evan, but it does seem likely that the same data would > be > > useful for AJAX-powered forms as for others. > > Ilya makes a good point. Could you split it for parity? > If there is a substantial number of forms where only this will be detected, we > want to get that data split. Hi Walter, have a look at the split version, thanks!
lgtm for metrics thanks for splitting!
The CQ bit was checked by mathp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1010263002/#ps90007 (title: "split")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1010263002/90007
Message was sent while issue was closed.
Committed patchset #9 (id:90007)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6072951da5e97bb2571419005dae5da1f24e06cc Cr-Commit-Position: refs/heads/master@{#322169} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
