Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2839063004:
[Autofill] UKM for suggestions polled field. (Closed)
|
Created:
3 years, 8 months ago by jiahuiguo Modified:
3 years, 7 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, asvitkine+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd metrics to instrument which field type the suggestions are polled from, this will allow us identify which suggestion popup (UI) the users are using to autofill the information.
The metric is more useful for card suggestions, since both card holder name and card number could trigger the suggestions, but they have different UIs and information.
BUG=715789
TEST=components:components_unittests
Review-Url: https://codereview.chromium.org/2839063004
Cr-Commit-Position: refs/heads/master@{#474777}
Committed: https://chromium.googlesource.com/chromium/src/+/66e4a1e3d234f8151ef22fcada3e28a82e45fa5d
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address comments. #
Total comments: 6
Patch Set 3 : Change to suggestionShown #
Total comments: 2
Patch Set 4 : Added ukm.xml and resolved conflict #
Total comments: 4
Patch Set 5 : Address comments #
Messages
Total messages: 45 (30 generated)
The CQ bit was checked by jiahuiguo@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...
Description was changed from ========== UKM for suggestions polled field. ========== to ========== Add metrics to instrument which field type the suggestions are polled from, this will allow us identify which suggestion popup (UI) the users are using to autofill the information. The metric is more useful for card suggestions, since both card holder name and card number could trigger the suggestions, but they have different UIs and information. BUG=715789 TEST=components:components_unittests ==========
jiahuiguo@google.com changed reviewers: + csashi@google.com, jsaul@google.com, mathp@chromium.org
Hi Mathieu, Sashi and Jared, PTAL. Thanks.
mathp@chromium.org changed reviewers: + rogerm@chromium.org - mathp@chromium.org
On 2017/04/26 22:13:56, jiahuiguo wrote: > Hi Mathieu, Sashi and Jared, > > PTAL. > > Thanks. Roger is best to review, he is also looking at UKM
lgtm from a style perspective, but definitely going to defer to Roger on best UKM practices https://codereview.chromium.org/2839063004/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_manager.h (right): https://codereview.chromium.org/2839063004/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.h:413: // Returns a list of values from the stored credit cards that match |type| and comment referencing |type| is now incorrect https://codereview.chromium.org/2839063004/diff/1/tools/metrics/ukm/ukm.xml File tools/metrics/ukm/ukm.xml (right): https://codereview.chromium.org/2839063004/diff/1/tools/metrics/ukm/ukm.xml#n... tools/metrics/ukm/ukm.xml:76: Recorded with field type the suggestions are polled from. s/with/which ? https://codereview.chromium.org/2839063004/diff/1/tools/metrics/ukm/ukm.xml#n... tools/metrics/ukm/ukm.xml:76: Recorded with field type the suggestions are polled from. s/Recorded/Records
The CQ bit was checked by jiahuiguo@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2839063004/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_manager.h (right): https://codereview.chromium.org/2839063004/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.h:413: // Returns a list of values from the stored credit cards that match |type| and Update comment? https://codereview.chromium.org/2839063004/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.h (right): https://codereview.chromium.org/2839063004/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_manager.h:406: // |autofill_field.Type()| and the value of |field| and returns the labels of Reorder as "that match the value of |field| and match field type |autofill_field.Type()| to align with order of parameters? https://codereview.chromium.org/2839063004/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_manager.h:415: // the matching credit cards. Same comment. https://codereview.chromium.org/2839063004/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2839063004/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:41: // |UkmEntry| when user polls suggestions. Metrics for field type. Can we not add the metrics to "Suggestions shown" - poll suggestions seems like an internal implementation detail that does not directly map to a comprehensible action - or does it?
The CQ bit was checked by jiahuiguo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jsaul@google.com Link to the patchset: https://codereview.chromium.org/2839063004/#ps40001 (title: "Change to suggestionShown")
The CQ bit was unchecked by jiahuiguo@google.com
The CQ bit was checked by jiahuiguo@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...
Hi Roger, Sashi and Jared, PTAL, I changed the ukm from polledSuggestion action to suggestion shown per Sashi's comments. NOTE: I have some problems with the presubmit check for ukm.xml, will add that later. Thanks, https://codereview.chromium.org/2839063004/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_manager.h (right): https://codereview.chromium.org/2839063004/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.h:413: // Returns a list of values from the stored credit cards that match |type| and On 2017/04/26 22:50:08, Jared Saul wrote: > comment referencing |type| is now incorrect Done. https://codereview.chromium.org/2839063004/diff/1/tools/metrics/ukm/ukm.xml File tools/metrics/ukm/ukm.xml (right): https://codereview.chromium.org/2839063004/diff/1/tools/metrics/ukm/ukm.xml#n... tools/metrics/ukm/ukm.xml:76: Recorded with field type the suggestions are polled from. On 2017/04/26 22:50:08, Jared Saul wrote: > s/with/which ? Done. https://codereview.chromium.org/2839063004/diff/1/tools/metrics/ukm/ukm.xml#n... tools/metrics/ukm/ukm.xml:76: Recorded with field type the suggestions are polled from. On 2017/04/26 22:50:08, Jared Saul wrote: > s/Recorded/Records Done. https://codereview.chromium.org/2839063004/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.h (right): https://codereview.chromium.org/2839063004/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_manager.h:406: // |autofill_field.Type()| and the value of |field| and returns the labels of On 2017/04/27 18:41:17, csashi wrote: > Reorder as > "that match the value of |field| and match field type |autofill_field.Type()| to > align with order of parameters? Moved the ukm from pollsuggestions to suggestionsShown. https://codereview.chromium.org/2839063004/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_manager.h:415: // the matching credit cards. On 2017/04/27 18:41:17, csashi wrote: > Same comment. Acknowledged. https://codereview.chromium.org/2839063004/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2839063004/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:41: // |UkmEntry| when user polls suggestions. Metrics for field type. On 2017/04/27 18:41:17, csashi wrote: > Can we not add the metrics to "Suggestions shown" - poll suggestions seems like > an internal implementation detail that does not directly map to a comprehensible > action - or does it? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm https://codereview.chromium.org/2839063004/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.h (right): https://codereview.chromium.org/2839063004/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_manager.h:415: // the value of |field| and returns the labels of the matching credit cards. nit: "match field type the value of |field|" reads weird. Should the first 'field type' be removed? If so, s/match/matches.
jiahuiguo@google.com changed reviewers: + rkaplow@chromium.org
+rkaplow
Does this need a rebase? (for the try bots) Is there a corresponding log to capture the suggestion UI outcome (i.e., user made a selection, user clicked on autofill settings, user abandoned the UI)? Otherwise, LGTM.
The CQ bit was checked by jiahuiguo@google.com to run a CQ dry run
On 2017/05/24 20:12:03, Roger McFarlane (Chromium) wrote: > Does this need a rebase? (for the try bots) Done, just rebased. > Is there a corresponding log to capture the suggestion UI outcome (i.e., user > made a selection, user clicked on autofill settings, user abandoned the UI)? Sashi already made one: Autofill.SuggestionFilled.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jiahuiguo@google.com changed required reviewers: + rkaplow@chromium.org
Hi Robert, Can you take a look at tools/metrics/ukm/ukm.xml? Thanks. https://codereview.chromium.org/2839063004/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.h (right): https://codereview.chromium.org/2839063004/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_manager.h:415: // the value of |field| and returns the labels of the matching credit cards. On 2017/05/24 18:06:35, Jared Saul wrote: > nit: "match field type the value of |field|" reads weird. Should the first > 'field type' be removed? If so, s/match/matches. Acknowledged. I just reverted this change, thanks for catching this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2839063004/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2839063004/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:1079: MillisecondsSinceFormParsed()); Move this metric to the end to be consistent with other logging? https://codereview.chromium.org/2839063004/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2839063004/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:39: // |UkmEntry| when we show suggestions. and when user edits text field. See |kUkmTextFieldDidChangeEntryName|.
The CQ bit was checked by jiahuiguo@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...
Thanks for the comments. Hi rkaplow@, Can you approve the ukm.xml? https://codereview.chromium.org/2839063004/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2839063004/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:1079: MillisecondsSinceFormParsed()); On 2017/05/25 08:40:33, csashi wrote: > Move this metric to the end to be consistent with other logging? Done. https://codereview.chromium.org/2839063004/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2839063004/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:39: // |UkmEntry| when we show suggestions. On 2017/05/25 08:40:33, csashi wrote: > and when user edits text field. See |kUkmTextFieldDidChangeEntryName|. Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jiahuiguo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rogerm@chromium.org, jsaul@google.com, csashi@google.com Link to the patchset: https://codereview.chromium.org/2839063004/#ps80001 (title: "Address comments")
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": 80001, "attempt_start_ts": 1495743320494790,
"parent_rev": "29b240ae17270e355c2a0d09e0e026be0ee9da40", "commit_rev":
"66e4a1e3d234f8151ef22fcada3e28a82e45fa5d"}
Message was sent while issue was closed.
Description was changed from ========== Add metrics to instrument which field type the suggestions are polled from, this will allow us identify which suggestion popup (UI) the users are using to autofill the information. The metric is more useful for card suggestions, since both card holder name and card number could trigger the suggestions, but they have different UIs and information. BUG=715789 TEST=components:components_unittests ========== to ========== Add metrics to instrument which field type the suggestions are polled from, this will allow us identify which suggestion popup (UI) the users are using to autofill the information. The metric is more useful for card suggestions, since both card holder name and card number could trigger the suggestions, but they have different UIs and information. BUG=715789 TEST=components:components_unittests Review-Url: https://codereview.chromium.org/2839063004 Cr-Commit-Position: refs/heads/master@{#474777} Committed: https://chromium.googlesource.com/chromium/src/+/66e4a1e3d234f8151ef22fcada3e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/66e4a1e3d234f8151ef22fcada3e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
