Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2575263002:
[Autofill] Add a new metric for upstream. (Closed)
|
Created:
4 years ago by Shanfeng Modified:
4 years ago CC:
chromium-reviews, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Autofill] Add a new metric for upstream.
This can meature usage rate more accurately.
Count the number of submissions with autofill suggestion shown.
BUG=674678
TEST=AutofillMetricsTest
Committed: https://crrev.com/0132f2f2759b9e62b94c8846712c5b77e02baab7
Cr-Commit-Position: refs/heads/master@{#439569}
Patch Set 1 #Patch Set 2 : Added the xml file #Patch Set 3 : adding some tests #Patch Set 4 : Added all the unit tests. #
Total comments: 11
Patch Set 5 : Resolve comments. #
Total comments: 8
Patch Set 6 : Resolve comments #
Messages
Total messages: 34 (20 generated)
Description was changed from ========== Add a new metrix for upstream. This can meature usage rate more accurately. BUG= ========== to ========== Add a new metrix for upstream. This can meature usage rate more accurately. https://docs.google.com/document/d/1jTgXl-za-7ZZRDGO1yjxWywLvJSSyibJy9se0CT5c... BUG= ==========
Description was changed from ========== Add a new metrix for upstream. This can meature usage rate more accurately. https://docs.google.com/document/d/1jTgXl-za-7ZZRDGO1yjxWywLvJSSyibJy9se0CT5c... BUG= ========== to ========== Add a new metrix for upstream. Count the number of submissions with auto-fill suggestion shown. This can meature usage rate more accurately. https://docs.google.com/document/d/1jTgXl-za-7ZZRDGO1yjxWywLvJSSyibJy9se0CT5c... BUG= ==========
Description was changed from ========== Add a new metrix for upstream. Count the number of submissions with auto-fill suggestion shown. This can meature usage rate more accurately. https://docs.google.com/document/d/1jTgXl-za-7ZZRDGO1yjxWywLvJSSyibJy9se0CT5c... BUG= ========== to ========== Add a new metrix for upstream. Count the number of submissions with auto-fill suggestion shown. This can measure the autofill usage rate more accurately. https://docs.google.com/document/d/1jTgXl-za-7ZZRDGO1yjxWywLvJSSyibJy9se0CT5c... BUG= ==========
Description was changed from ========== Add a new metrix for upstream. Count the number of submissions with auto-fill suggestion shown. This can measure the autofill usage rate more accurately. https://docs.google.com/document/d/1jTgXl-za-7ZZRDGO1yjxWywLvJSSyibJy9se0CT5c... BUG= ========== to ========== Add a new metrix for upstream. Count the number of submissions with auto-fill suggestion shown. This can measure the autofill usage rate more accurately. https://docs.google.com/document/d/1jTgXl-za-7ZZRDGO1yjxWywLvJSSyibJy9se0CT5c... BUG=674678 ==========
szhangcs@google.com changed reviewers: + waltercacau@chromium.org
szhangcs@google.com changed reviewers: + mathp@chromium.org
jsaul@google.com changed reviewers: + jsaul@google.com
https://codereview.chromium.org/2575263002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2575263002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:369: FORM_EVENT_SUGGESTION_SHOWN_SUBMITTED_ONCE, FORM_EVENT_SUGGESTION_SHOWN_SUBMITTED_ONCE shouldn't go in this section, because the other four events are mutually exclusive, whereas your new metric will always happen if suggestions are shown and the form is submitted, regardless of what the user picked. https://codereview.chromium.org/2575263002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:383: FORM_EVENT_SUGGESTION_SHOWN_WILL_SUBMIT_ONCE, ditto https://codereview.chromium.org/2575263002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2575263002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:76575: + <int value="9" label="Submitted with suggestion shwon (once)"/> Can these numbers actually change without affecting past data? https://codereview.chromium.org/2575263002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:76588: + <int value="16" label="About to be Submitted with suggestion shwon (once)"/> s/Submitted/submitted to be consistent https://codereview.chromium.org/2575263002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:76588: + <int value="16" label="About to be Submitted with suggestion shwon (once)"/> s/shwon/shown
The CQ bit was checked by szhangcs@google.com
The CQ bit was unchecked by szhangcs@google.com
https://codereview.chromium.org/2575263002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2575263002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:369: FORM_EVENT_SUGGESTION_SHOWN_SUBMITTED_ONCE, On 2016/12/15 21:30:26, jsaul wrote: > FORM_EVENT_SUGGESTION_SHOWN_SUBMITTED_ONCE shouldn't go in this section, because > the other four events are mutually exclusive, whereas your new metric will > always happen if suggestions are shown and the form is submitted, regardless of > what the user picked. Done. https://codereview.chromium.org/2575263002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:383: FORM_EVENT_SUGGESTION_SHOWN_WILL_SUBMIT_ONCE, On 2016/12/15 21:30:26, jsaul wrote: > ditto Done. https://codereview.chromium.org/2575263002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2575263002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:76575: + <int value="9" label="Submitted with suggestion shwon (once)"/> On 2016/12/15 21:30:26, jsaul wrote: > Can these numbers actually change without affecting past data? One way to do this is to put the new data at the bottom of this list. But I don't think changing more lines is a big deal here. https://codereview.chromium.org/2575263002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:76588: + <int value="16" label="About to be Submitted with suggestion shwon (once)"/> On 2016/12/15 21:30:26, jsaul wrote: > s/shwon/shown Done. https://codereview.chromium.org/2575263002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:76588: + <int value="16" label="About to be Submitted with suggestion shwon (once)"/> On 2016/12/15 21:30:26, jsaul wrote: > s/Submitted/submitted > to be consistent Done.
https://codereview.chromium.org/2575263002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2575263002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:76575: + <int value="9" label="Submitted with suggestion shwon (once)"/> On 2016/12/15 21:59:03, szhangcs wrote: > On 2016/12/15 21:30:26, jsaul wrote: > > Can these numbers actually change without affecting past data? > > One way to do this is to put the new data at the bottom of this list. But I > don't think changing more lines is a big deal here. I don't mean changing more lines is a problem. I mean that these histograms have 'value="#"' numbers and I want to be sure that if you change the number of a histogram, previous histogram data won't get confused. Like "I used to be histogram 11, but now I'm histogram 12, so if you ask for histogram 11's data you're going to get two different histograms on the graph" or something like that. https://codereview.chromium.org/2575263002/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2575263002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:366: FORM_EVENT_SUGGESTION_SHOWN_SUBMITTED_ONCE, Looks like this is a duplicate now; remove? https://codereview.chromium.org/2575263002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:379: // A dropdown with suggestions was shown and a form was submitted after that nit: s/that/that. (for consistency; all the others end in a period) https://codereview.chromium.org/2575263002/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/2575263002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_metrics_unittest.cc:2812: AutofillMetrics::FORM_EVENT_LOCAL_SUGGESTION_WILL_SUBMIT_ONCE, 0); Nice fix!
https://codereview.chromium.org/2575263002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2575263002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:76575: - <int value="9" label="Submitted with no suggestion filled (once)"/> do not change the ids, IIRC they are actually what gets stored and it will mess up the UMA graphs treat it like proto tag/field ids
On 2016/12/15 22:21:00, jsaul wrote: > https://codereview.chromium.org/2575263002/diff/60001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2575263002/diff/60001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:76575: + <int value="9" > label="Submitted with suggestion shwon (once)"/> > On 2016/12/15 21:59:03, szhangcs wrote: > > On 2016/12/15 21:30:26, jsaul wrote: > > > Can these numbers actually change without affecting past data? > > > > One way to do this is to put the new data at the bottom of this list. But I > > don't think changing more lines is a big deal here. > > I don't mean changing more lines is a problem. I mean that these histograms > have 'value="#"' numbers and I want to be sure that if you change the number of > a histogram, previous histogram data won't get confused. Like "I used to be > histogram 11, but now I'm histogram 12, so if you ask for histogram 11's data > you're going to get two different histograms on the graph" or something like > that. > > https://codereview.chromium.org/2575263002/diff/80001/components/autofill/cor... > File components/autofill/core/browser/autofill_metrics.h (right): > > https://codereview.chromium.org/2575263002/diff/80001/components/autofill/cor... > components/autofill/core/browser/autofill_metrics.h:366: > FORM_EVENT_SUGGESTION_SHOWN_SUBMITTED_ONCE, > Looks like this is a duplicate now; remove? > > https://codereview.chromium.org/2575263002/diff/80001/components/autofill/cor... > components/autofill/core/browser/autofill_metrics.h:379: // A dropdown with > suggestions was shown and a form was submitted after that > nit: s/that/that. > (for consistency; all the others end in a period) > > https://codereview.chromium.org/2575263002/diff/80001/components/autofill/cor... > File components/autofill/core/browser/autofill_metrics_unittest.cc (right): > > https://codereview.chromium.org/2575263002/diff/80001/components/autofill/cor... > components/autofill/core/browser/autofill_metrics_unittest.cc:2812: > AutofillMetrics::FORM_EVENT_LOCAL_SUGGESTION_WILL_SUBMIT_ONCE, 0); > Nice fix! Oh. That's reasonable. I added the new field to the last of this list.
Thanks for review! https://codereview.chromium.org/2575263002/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2575263002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:366: FORM_EVENT_SUGGESTION_SHOWN_SUBMITTED_ONCE, On 2016/12/15 22:20:59, jsaul wrote: > Looks like this is a duplicate now; remove? Done. https://codereview.chromium.org/2575263002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:379: // A dropdown with suggestions was shown and a form was submitted after that On 2016/12/15 22:20:59, jsaul wrote: > nit: s/that/that. > (for consistency; all the others end in a period) Done. https://codereview.chromium.org/2575263002/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/2575263002/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_metrics_unittest.cc:2812: AutofillMetrics::FORM_EVENT_LOCAL_SUGGESTION_WILL_SUBMIT_ONCE, 0); On 2016/12/15 22:20:59, jsaul wrote: > Nice fix! Thanks.
Thanks! https://codereview.chromium.org/2575263002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2575263002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:76575: - <int value="9" label="Submitted with no suggestion filled (once)"/> On 2016/12/15 23:04:23, Walter Cacau wrote: > do not change the ids, IIRC they are actually what gets stored and it will mess > up the UMA graphs > > treat it like proto tag/field ids Done.
Description was changed from ========== Add a new metrix for upstream. Count the number of submissions with auto-fill suggestion shown. This can measure the autofill usage rate more accurately. https://docs.google.com/document/d/1jTgXl-za-7ZZRDGO1yjxWywLvJSSyibJy9se0CT5c... BUG=674678 ========== to ========== [Autofill] Add a new metric for upstream. This can meature usage rate more accurately. Count the number of submissions with autofill suggestion shown. BUG=674678 ==========
Description was changed from ========== [Autofill] Add a new metric for upstream. This can meature usage rate more accurately. Count the number of submissions with autofill suggestion shown. BUG=674678 ========== to ========== [Autofill] Add a new metric for upstream. This can meature usage rate more accurately. Count the number of submissions with autofill suggestion shown. BUG=674678 TEST=AutofillMetricsTest ==========
lgtm
lgtm
lgtm, thanks Shanfeng!
The CQ bit was checked by szhangcs@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
szhangcs@google.com changed reviewers: + rkaplow@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
The CQ bit was checked by szhangcs@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1482177849776110,
"parent_rev": "b20fc306fceca7e57e99b2b10a5cf918d85a2c77", "commit_rev":
"1b0cd747cfc2daad3b69a806378fb7e8c8b90952"}
Message was sent while issue was closed.
Description was changed from ========== [Autofill] Add a new metric for upstream. This can meature usage rate more accurately. Count the number of submissions with autofill suggestion shown. BUG=674678 TEST=AutofillMetricsTest ========== to ========== [Autofill] Add a new metric for upstream. This can meature usage rate more accurately. Count the number of submissions with autofill suggestion shown. BUG=674678 TEST=AutofillMetricsTest Review-Url: https://codereview.chromium.org/2575263002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Autofill] Add a new metric for upstream. This can meature usage rate more accurately. Count the number of submissions with autofill suggestion shown. BUG=674678 TEST=AutofillMetricsTest Review-Url: https://codereview.chromium.org/2575263002 ========== to ========== [Autofill] Add a new metric for upstream. This can meature usage rate more accurately. Count the number of submissions with autofill suggestion shown. BUG=674678 TEST=AutofillMetricsTest Committed: https://crrev.com/0132f2f2759b9e62b94c8846712c5b77e02baab7 Cr-Commit-Position: refs/heads/master@{#439569} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0132f2f2759b9e62b94c8846712c5b77e02baab7 Cr-Commit-Position: refs/heads/master@{#439569} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
