Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2776223002:
Adds UKM for autofill attributes in form_structure. (Closed)
|
Created:
3 years, 9 months ago by csashi Modified:
3 years, 8 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, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, rkaplow Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds UKM for autofill attributes in form_structure.
BUG=706646
Review-Url: https://codereview.chromium.org/2776223002
Cr-Commit-Position: refs/heads/master@{#461005}
Committed: https://chromium.googlesource.com/chromium/src/+/c6d68221043cb769554616d3658fd99b299024a2
Patch Set 1 #Patch Set 2 : Adds UKM for autofill attributes in form_structure. #Patch Set 3 : Adds UKM for autofill attributes in form_structure. #
Total comments: 5
Patch Set 4 : Uses vector of pairs instead of map for metrics. #
Total comments: 2
Patch Set 5 : Fixes typo: Changes flag name from kAutofillUkmLogging -> AutofillUkmLogging #Patch Set 6 : Fixes typo: Changes flag name from kAutofillUkmLogging -> AutofillUkmLogging #Patch Set 7 : Updates tests to account for multiple metrics for same source_url(). #Patch Set 8 : Updates tests to account for multiple metrics for same source_url(). #
Total comments: 5
Patch Set 9 : Makes FORM_PARSED with and without hints as exclusive metrics. #Patch Set 10 : Makes FORM_PARSED with and without hints as exclusive metrics. #Patch Set 11 : Resolves merge conflicts with FORM_CONTAINS_UPI_VPA_HINT. #Patch Set 12 : Resolves merge conflicts with FORM_CONTAINS_UPI_VPA_HINT. #Messages
Total messages: 80 (52 generated)
csashi@google.com changed reviewers: + mathp@chromium.org
Hi Mathieu, Please take a look. I will add additional unit-tests if you are OK with the change. Thanks, -sashi.
The CQ bit was checked by csashi@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 ========== Adds UKM for autofill attributes in form_structure. BUG= ========== to ========== Adds UKM for autofill attributes in form_structure. BUG= ==========
csashi@google.com changed reviewers: + sebsg@chromium.org
Hi Sebastien, Can you also take a look? Thanks, -sashi.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csashi@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: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by csashi@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...
https://codereview.chromium.org/2776223002/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2776223002/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1929: pending_upload_request_url_); FormStructure::source_url is what you need, so can be internal? https://codereview.chromium.org/2776223002/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/2776223002/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_metrics_unittest.cc:4412: TEST_F(AutofillMetricsTest, RecordDeveloperEnagementMetric) { *engagement https://codereview.chromium.org/2776223002/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_metrics_unittest.cc:4412: TEST_F(AutofillMetricsTest, RecordDeveloperEnagementMetric) { This is not a sufficient test. You should also add a test alongside where we test for DeveloperEngagement _UMA_ metric (Line 1436)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi, I modified the interface to take a vector of metrics instead of a map to align with the message definition which allows for the metric name to be repeated. Please take a look. Thanks, -sashi. https://codereview.chromium.org/2776223002/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2776223002/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1929: pending_upload_request_url_); On 2017/03/27 18:58:55, Mathieu wrote: > FormStructure::source_url is what you need, so can be internal? Done. https://codereview.chromium.org/2776223002/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/2776223002/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_metrics_unittest.cc:4412: TEST_F(AutofillMetricsTest, RecordDeveloperEnagementMetric) { On 2017/03/27 18:58:55, Mathieu wrote: > *engagement Done.
The CQ bit was checked by csashi@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: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
https://codereview.chromium.org/2776223002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/2776223002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics_unittest.cc:416: scoped_feature_list_.InitAndEnableFeature(kAutofillUkmLogging); Sorry that I didn't catch this earlier, but you might want to check with privard@ whether this new UKM can fall under the existing kAutofillUkmLogging feature or we should create a new one. AFAIK, they would like granular control over which UKM is allowed.
https://codereview.chromium.org/2776223002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/2776223002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics_unittest.cc:416: scoped_feature_list_.InitAndEnableFeature(kAutofillUkmLogging); On 2017/03/27 23:59:31, Mathieu wrote: > Sorry that I didn't catch this earlier, but you might want to check with > privard@ whether this new UKM can fall under the existing kAutofillUkmLogging > feature or we should create a new one. AFAIK, they would like granular control > over which UKM is allowed. Acknowledged. I remembered that you told me to add a feature but because the UKM logging feature flag name was generic, I thought the new metric would also be covered. Will check anyway.
The CQ bit was checked by csashi@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.
Hi Sashi, please see my comment inline. https://codereview.chromium.org/2776223002/diff/140001/components/autofill/co... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/2776223002/diff/140001/components/autofill/co... components/autofill/core/browser/form_structure.cc:369: {AutofillMetrics::FILLABLE_FORM_CONTAINS_TYPE_HINTS, by looking at the comments of these constants, it seems that FILLABLE_FOMR_CONTAINS_TYPE_HINTS is a superset of FILLABLE_FORM_PARSED. https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil... Do you think we could only log one? Maybe Math knows why it's logged this way now? That would remove the need to have multiple times the same metric in a UKM_Entry, which is something I find a bit weird. Thanks!
Hi Sebastien, Please take a look. Thanks! -sashi. https://codereview.chromium.org/2776223002/diff/140001/components/autofill/co... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/2776223002/diff/140001/components/autofill/co... components/autofill/core/browser/form_structure.cc:369: {AutofillMetrics::FILLABLE_FORM_CONTAINS_TYPE_HINTS, On 2017/03/29 13:52:58, sebsg wrote: > by looking at the comments of these constants, it seems that > FILLABLE_FOMR_CONTAINS_TYPE_HINTS is a superset of FILLABLE_FORM_PARSED. > > https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil... > > Do you think we could only log one? Maybe Math knows why it's logged this way > now? > > That would remove the need to have multiple times the same metric in a > UKM_Entry, which is something I find a bit weird. > > Thanks! Yes, CONTAINS_TYPE_HINTS are a subset of FORM_PARSED. I am logging both to be consistent with UMA metrics. If consistency with UMA is not important, I can log just one. BTW, the log schema seems to be specifically designed for multiple enums for the same metric. See https://codesearch.chromium.org/chromium/src/components/metrics/proto/ukm/ent...
https://codereview.chromium.org/2776223002/diff/140001/components/autofill/co... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/2776223002/diff/140001/components/autofill/co... components/autofill/core/browser/form_structure.cc:369: {AutofillMetrics::FILLABLE_FORM_CONTAINS_TYPE_HINTS, On 2017/03/29 16:15:11, csashi wrote: > On 2017/03/29 13:52:58, sebsg wrote: > > by looking at the comments of these constants, it seems that > > FILLABLE_FOMR_CONTAINS_TYPE_HINTS is a superset of FILLABLE_FORM_PARSED. > > > > > https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil... > > > > Do you think we could only log one? Maybe Math knows why it's logged this way > > now? > > > > That would remove the need to have multiple times the same metric in a > > UKM_Entry, which is something I find a bit weird. > > > > Thanks! > > Yes, CONTAINS_TYPE_HINTS are a subset of FORM_PARSED. I am logging both to be > consistent with UMA metrics. If consistency with UMA is not important, I can log > just one. > > BTW, the log schema seems to be specifically designed for multiple enums for the > same metric. See > https://codesearch.chromium.org/chromium/src/components/metrics/proto/ukm/ent... I see, I really prefer logging only once if it's a subset, since it's then simpler to parse when analyzing the metrics. I just want to wait for Math to see if he has another reason to keep them separated. I'll ping him :)
https://codereview.chromium.org/2776223002/diff/140001/components/autofill/co... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/2776223002/diff/140001/components/autofill/co... components/autofill/core/browser/form_structure.cc:369: {AutofillMetrics::FILLABLE_FORM_CONTAINS_TYPE_HINTS, On 2017/03/29 17:55:13, sebsg wrote: > On 2017/03/29 16:15:11, csashi wrote: > > On 2017/03/29 13:52:58, sebsg wrote: > > > by looking at the comments of these constants, it seems that > > > FILLABLE_FOMR_CONTAINS_TYPE_HINTS is a superset of FILLABLE_FORM_PARSED. > > > > > > > > > https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil... > > > > > > Do you think we could only log one? Maybe Math knows why it's logged this > way > > > now? > > > > > > That would remove the need to have multiple times the same metric in a > > > UKM_Entry, which is something I find a bit weird. > > > > > > Thanks! > > > > Yes, CONTAINS_TYPE_HINTS are a subset of FORM_PARSED. I am logging both to be > > consistent with UMA metrics. If consistency with UMA is not important, I can > log > > just one. > > > > BTW, the log schema seems to be specifically designed for multiple enums for > the > > same metric. See > > > https://codesearch.chromium.org/chromium/src/components/metrics/proto/ukm/ent... > > I see, I really prefer logging only once if it's a subset, since it's then > simpler to parse when analyzing the metrics. I just want to wait for Math to see > if he has another reason to keep them separated. I'll ping him :) Ok I talked with Math. Would you mind renaming the enum and changing the logic a bit? The idea would be to make them mutually exclusive, so something like FILLABLE_FORM_PARSED_WITH_HINTS and FILLABLE_FORM_PARSED_NO_HINT. WDYT?
On 2017/03/29 18:46:10, sebsg wrote: > https://codereview.chromium.org/2776223002/diff/140001/components/autofill/co... > File components/autofill/core/browser/form_structure.cc (right): > > https://codereview.chromium.org/2776223002/diff/140001/components/autofill/co... > components/autofill/core/browser/form_structure.cc:369: > {AutofillMetrics::FILLABLE_FORM_CONTAINS_TYPE_HINTS, > On 2017/03/29 17:55:13, sebsg wrote: > > On 2017/03/29 16:15:11, csashi wrote: > > > On 2017/03/29 13:52:58, sebsg wrote: > > > > by looking at the comments of these constants, it seems that > > > > FILLABLE_FOMR_CONTAINS_TYPE_HINTS is a superset of FILLABLE_FORM_PARSED. > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil... > > > > > > > > Do you think we could only log one? Maybe Math knows why it's logged this > > way > > > > now? > > > > > > > > That would remove the need to have multiple times the same metric in a > > > > UKM_Entry, which is something I find a bit weird. > > > > > > > > Thanks! > > > > > > Yes, CONTAINS_TYPE_HINTS are a subset of FORM_PARSED. I am logging both to > be > > > consistent with UMA metrics. If consistency with UMA is not important, I can > > log > > > just one. > > > > > > BTW, the log schema seems to be specifically designed for multiple enums for > > the > > > same metric. See > > > > > > https://codesearch.chromium.org/chromium/src/components/metrics/proto/ukm/ent... > > > > I see, I really prefer logging only once if it's a subset, since it's then > > simpler to parse when analyzing the metrics. I just want to wait for Math to > see > > if he has another reason to keep them separated. I'll ping him :) > > Ok I talked with Math. Would you mind renaming the enum and changing the logic a > bit? The idea would be to make them mutually exclusive, so something like > FILLABLE_FORM_PARSED_WITH_HINTS and FILLABLE_FORM_PARSED_NO_HINT. WDYT? Removing myself, Seb has all the context :)
mathp@chromium.org changed reviewers: - mathp@chromium.org
On 2017/03/29 20:32:05, Mathieu wrote: > On 2017/03/29 18:46:10, sebsg wrote: > > > https://codereview.chromium.org/2776223002/diff/140001/components/autofill/co... > > File components/autofill/core/browser/form_structure.cc (right): > > > > > https://codereview.chromium.org/2776223002/diff/140001/components/autofill/co... > > components/autofill/core/browser/form_structure.cc:369: > > {AutofillMetrics::FILLABLE_FORM_CONTAINS_TYPE_HINTS, > > On 2017/03/29 17:55:13, sebsg wrote: > > > On 2017/03/29 16:15:11, csashi wrote: > > > > On 2017/03/29 13:52:58, sebsg wrote: > > > > > by looking at the comments of these constants, it seems that > > > > > FILLABLE_FOMR_CONTAINS_TYPE_HINTS is a superset of FILLABLE_FORM_PARSED. > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil... > > > > > > > > > > Do you think we could only log one? Maybe Math knows why it's logged > this > > > way > > > > > now? > > > > > > > > > > That would remove the need to have multiple times the same metric in a > > > > > UKM_Entry, which is something I find a bit weird. > > > > > > > > > > Thanks! > > > > > > > > Yes, CONTAINS_TYPE_HINTS are a subset of FORM_PARSED. I am logging both to > > be > > > > consistent with UMA metrics. If consistency with UMA is not important, I > can > > > log > > > > just one. > > > > > > > > BTW, the log schema seems to be specifically designed for multiple enums > for > > > the > > > > same metric. See > > > > > > > > > > https://codesearch.chromium.org/chromium/src/components/metrics/proto/ukm/ent... > > > > > > I see, I really prefer logging only once if it's a subset, since it's then > > > simpler to parse when analyzing the metrics. I just want to wait for Math to > > see > > > if he has another reason to keep them separated. I'll ping him :) > > > > Ok I talked with Math. Would you mind renaming the enum and changing the logic > a > > bit? The idea would be to make them mutually exclusive, so something like > > FILLABLE_FORM_PARSED_WITH_HINTS and FILLABLE_FORM_PARSED_NO_HINT. WDYT? > > Removing myself, Seb has all the context :) Please create a crbug.com for this.
Description was changed from ========== Adds UKM for autofill attributes in form_structure. BUG= ========== to ========== Adds UKM for autofill attributes in form_structure. BUG=706646 ==========
Hi Sebastien, Please take a look. Thanks. -sashi. https://codereview.chromium.org/2776223002/diff/140001/components/autofill/co... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/2776223002/diff/140001/components/autofill/co... components/autofill/core/browser/form_structure.cc:369: {AutofillMetrics::FILLABLE_FORM_CONTAINS_TYPE_HINTS, On 2017/03/29 18:46:09, sebsg wrote: > On 2017/03/29 17:55:13, sebsg wrote: > > On 2017/03/29 16:15:11, csashi wrote: > > > On 2017/03/29 13:52:58, sebsg wrote: > > > > by looking at the comments of these constants, it seems that > > > > FILLABLE_FOMR_CONTAINS_TYPE_HINTS is a superset of FILLABLE_FORM_PARSED. > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil... > > > > > > > > Do you think we could only log one? Maybe Math knows why it's logged this > > way > > > > now? > > > > > > > > That would remove the need to have multiple times the same metric in a > > > > UKM_Entry, which is something I find a bit weird. > > > > > > > > Thanks! > > > > > > Yes, CONTAINS_TYPE_HINTS are a subset of FORM_PARSED. I am logging both to > be > > > consistent with UMA metrics. If consistency with UMA is not important, I can > > log > > > just one. > > > > > > BTW, the log schema seems to be specifically designed for multiple enums for > > the > > > same metric. See > > > > > > https://codesearch.chromium.org/chromium/src/components/metrics/proto/ukm/ent... > > > > I see, I really prefer logging only once if it's a subset, since it's then > > simpler to parse when analyzing the metrics. I just want to wait for Math to > see > > if he has another reason to keep them separated. I'll ping him :) > > Ok I talked with Math. Would you mind renaming the enum and changing the logic a > bit? The idea would be to make them mutually exclusive, so something like > FILLABLE_FORM_PARSED_WITH_HINTS and FILLABLE_FORM_PARSED_NO_HINT. WDYT? Done.
The CQ bit was checked by csashi@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.
Thanks for fixing this. LGTM % a small comment. Could you please just update the name of the enums in histograms.xml? Here are the lines: https://cs.chromium.org/chromium/src/tools/metrics/histograms/histograms.xml?... Thank you!
On 2017/03/30 12:49:14, sebsg wrote: > Thanks for fixing this. LGTM % a small comment. > > Could you please just update the name of the enums in histograms.xml? Here are > the lines: > https://cs.chromium.org/chromium/src/tools/metrics/histograms/histograms.xml?... > > Thank you! Done.
csashi@google.com changed reviewers: + holte@chromium.org
Hi Steven, Can you please approve the change to histograms.xml? Thanks, -sashi.
histograms lgtm
The CQ bit was checked by csashi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org Link to the patchset: https://codereview.chromium.org/2776223002/#ps180001 (title: "Makes FORM_PARSED with and without hints as exclusive metrics.")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by csashi@google.com
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by csashi@google.com
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by csashi@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...
csashi@google.com changed reviewers: + rogerm@chromium.org
Hi Roger, My change crossed your changed which added another entry to DeveloperEngagementMetric - Can you confirm that the FORM_CONTAINS_UPI_VPA_HINT may be true, but the form is not autofillable? Otherwise, we can have 3 mutually exclusive metrics: 1. NO_HINTS 2. AUTOCOMPLETE_HINTS 3. UPI_VPA_HINT I suppose it is possible that a form has both autocomplete and UPI_VPA hint but if the UPI_VPA_HINT is a stronger hint than the autocomplete hint we can log UPI_VPA_HINT and not log AUTOCOMPLETE_HINTS. Thanks! -sashi.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by csashi@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.
The CQ bit was checked by csashi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2776223002/#ps220001 (title: "Resolves merge conflicts with FORM_CONTAINS_UPI_VPA_HINT.")
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": 220001, "attempt_start_ts": 1490928630449520,
"parent_rev": "76768cef1733de30bbb78ad0c596e45cc164d36c", "commit_rev":
"c6d68221043cb769554616d3658fd99b299024a2"}
Message was sent while issue was closed.
Description was changed from ========== Adds UKM for autofill attributes in form_structure. BUG=706646 ========== to ========== Adds UKM for autofill attributes in form_structure. BUG=706646 Review-Url: https://codereview.chromium.org/2776223002 Cr-Commit-Position: refs/heads/master@{#461005} Committed: https://chromium.googlesource.com/chromium/src/+/c6d68221043cb769554616d3658f... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/c6d68221043cb769554616d3658f...
Message was sent while issue was closed.
On 2017/03/30 23:13:15, csashi wrote: > Hi Roger, > My change crossed your changed which added another entry to > DeveloperEngagementMetric - Can you confirm that the FORM_CONTAINS_UPI_VPA_HINT > may be true, but the form is not autofillable? Otherwise, we can have 3 mutually > exclusive metrics: > 1. NO_HINTS > 2. AUTOCOMPLETE_HINTS > 3. UPI_VPA_HINT > > I suppose it is possible that a form has both autocomplete and UPI_VPA hint but > if the UPI_VPA_HINT is a stronger hint than the autocomplete hint we can log > UPI_VPA_HINT and not log AUTOCOMPLETE_HINTS. > > Thanks! > -sashi. Hi Roger, I committed this patch to make the Fri. cutoff. I will incorporate the VPA HINT in a follow up CL. -sashi.
Message was sent while issue was closed.
Description was changed from ========== Adds UKM for autofill attributes in form_structure. BUG=706646 Review-Url: https://codereview.chromium.org/2776223002 Cr-Commit-Position: refs/heads/master@{#461005} Committed: https://chromium.googlesource.com/chromium/src/+/c6d68221043cb769554616d3658f... ========== to ========== Adds UKM for autofill attributes in form_structure. BUG=706646 Review-Url: https://codereview.chromium.org/2776223002 Cr-Commit-Position: refs/heads/master@{#461005} Committed: https://chromium.googlesource.com/chromium/src/+/c6d68221043cb769554616d3658f... ==========
Message was sent while issue was closed.
csashi@google.com changed reviewers: - rogerm@chromium.org
Message was sent while issue was closed.
On 2017/03/31 03:59:37, csashi wrote: > On 2017/03/30 23:13:15, csashi wrote: > > Hi Roger, > > My change crossed your changed which added another entry to > > DeveloperEngagementMetric - Can you confirm that the > FORM_CONTAINS_UPI_VPA_HINT > > may be true, but the form is not autofillable? Otherwise, we can have 3 > mutually > > exclusive metrics: > > 1. NO_HINTS > > 2. AUTOCOMPLETE_HINTS > > 3. UPI_VPA_HINT > > > > I suppose it is possible that a form has both autocomplete and UPI_VPA hint > but > > if the UPI_VPA_HINT is a stronger hint than the autocomplete hint we can log > > UPI_VPA_HINT and not log AUTOCOMPLETE_HINTS. > > > > Thanks! > > -sashi. > > Hi Roger, > I committed this patch to make the Fri. cutoff. I will incorporate the VPA HINT > in > a follow up CL. > -sashi. UPI-VPA hint is independent of whether or not the form is fillable. This was not a mutually exclusive metric when my CL was written. As you've formulated above, UPI-VPA is a subset of times AUTOCOMPLETE_HINTS is incremented. |
