Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740633002:
[Autofill] Add upstreaming UKM (Closed)
|
Created:
3 years, 9 months ago by sebsg Modified:
3 years, 9 months ago CC:
chromium-reviews, pkl (ping after 24h if needed), droger+watchlist_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, sdefresne+watchlist_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, marq+watch_chromium.org, noyau+watch_chromium.org, blundell+watchlist_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, sync-reviews_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Autofill] Add upstreaming UKM
BUG=699256
Review-Url: https://codereview.chromium.org/2740633002
Cr-Commit-Position: refs/heads/master@{#456173}
Committed: https://chromium.googlesource.com/chromium/src/+/adcdf2d7c58a495e0f917a50148c2f041a1a6e85
Patch Set 1 #
Total comments: 9
Patch Set 2 : Re-do #
Total comments: 10
Patch Set 3 : Addressed comments + feature flag #
Total comments: 6
Patch Set 4 : Addressed comments #
Total comments: 1
Patch Set 5 : Added tests #
Total comments: 4
Patch Set 6 : bool -> void return value #Patch Set 7 : Fix for Android and iOS #
Total comments: 14
Patch Set 8 : Addressed rkaplow's comments #Patch Set 9 : Fixed comments #Messages
Total messages: 93 (69 generated)
Description was changed from ========== DS BUG= ========== to ========== [Autofill] Add upstreaming UKM BUG=699256 ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by sebsg@chromium.org 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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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_...)
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by sebsg@chromium.org 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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sebsg@chromium.org 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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sebsg@chromium.org 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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by sebsg@chromium.org 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...
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Hi Math, PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hey Math, I changed the code in autofill_ukm to be way more general. WDYT?
Let's try to keep the PersonalDataManager out of this.
In AutofillManager, when we need to log we could call the static (or anonymous
namespace) method
LogUkm(client_->GetUkmService(),
const GURL& url,
const std::string& ukm,
const std::map<std::string, int>& metrics)
Sorry I just thought now, but GetUkmService should be a method on AutofillClient
same as the rappor service.
https://cs.chromium.org/chromium/src/chrome/browser/ui/autofill/chrome_autofi...
https://codereview.chromium.org/2740633002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.cc (right):
https://codereview.chromium.org/2740633002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:1065: }
At this point I would call personal_data_->ResetUkmSession() which guarantees no
logging for the wrong URL.
ResetUkmSession would do autofill_ukm_session_.reset(base::MakeUnique...)
https://codereview.chromium.org/2740633002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:1224:
personal_data_->ResetUkmSession();
https://codereview.chromium.org/2740633002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_ukm.cc (right):
https://codereview.chromium.org/2740633002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_ukm.cc:20:
card_upload_decision_url_.reset(new GURL(url));
I think I would prefer if |card_upload_decision_url_| was just a member that you
can assign.
Below you can check for .is_empty() to know whether we should log.
https://codereview.chromium.org/2740633002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_ukm.h (right):
https://codereview.chromium.org/2740633002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_ukm.h:19: // A class to log Autofill
related Url Keyed Metrics.
Mention why it exists: sometimes the URL is not in the same codepath as the
metric being logged.
https://codereview.chromium.org/2740633002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_ukm.h:20: class AutofillUkm {
Would rename this AutofillUkmSession
https://codereview.chromium.org/2740633002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_ukm.h:43:
DISALLOW_IMPLICIT_CONSTRUCTORS(AutofillUkm);
curious, why not DISALLOW_COPY_AND_ASSIGN?
https://codereview.chromium.org/2740633002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_ukm_unittest.cc (right):
https://codereview.chromium.org/2740633002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_ukm_unittest.cc:38:
autofill_ukm_.reset(new AutofillUkm(GetTestUkmService()));
you can inline SetUp and TearDown above
https://codereview.chromium.org/2740633002/diff/100001/components/autofill/co...
File components/autofill/core/browser/personal_data_manager.cc (right):
https://codereview.chromium.org/2740633002/diff/100001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:272: autofill_ukm_(
autofill_ukm_(base::MakeUnique<AutofillUkm>(ukm_service))
https://codereview.chromium.org/2740633002/diff/100001/ios/chrome/browser/aut...
File ios/chrome/browser/autofill/personal_data_manager_factory.cc (right):
https://codereview.chromium.org/2740633002/diff/100001/ios/chrome/browser/aut...
ios/chrome/browser/autofill/personal_data_manager_factory.cc:54: nullptr /*
ukm::UkmService */));
is there no ukm service on iOS?
Oh no! Many draft comments were sent! Ignore more of them.
Patchset #3 (id:140001) has been deleted
Got a bit confused by the comments :P By I think this patch is much better now. WDYT?
+added bonus, let's put a base::Feature around the code (inside the static function that I'm suggesting below). https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:216: if (upload_decision >= AutofillMetrics::NUM_CARD_UPLOAD_DECISION_METRICS) DCHECK(ukm_service); https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:219: std::map<std::string, int> metrics; is there a fancy initialization like: const std::map<std::string, int> metrics{{"UploadDecision", static_cast<int>(upload_decision}}; ? https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... File components/autofill/core/browser/autofill_ukm.cc (right): https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_ukm.cc:21: DVLOG(3) << "No UKM service."; these DVLOG are not very useful? https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... File components/autofill/core/browser/autofill_ukm.h (right): https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_ukm.h:20: class AutofillUkm { I don't think we need the class, right? LogUkm could just sit in AutofillMetrics as a static. https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... File components/autofill/core/browser/test_autofill_client.h (right): https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... components/autofill/core/browser/test_autofill_client.h:20: //#include "components/ukm/ukm_source.h" fix
Patchset #4 (id:180001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Another look? https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:216: if (upload_decision >= AutofillMetrics::NUM_CARD_UPLOAD_DECISION_METRICS) On 2017/03/09 18:40:49, Mathieu Perreault wrote: > DCHECK(ukm_service); As discussed offline, will not add this. https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:219: std::map<std::string, int> metrics; On 2017/03/09 18:40:49, Mathieu Perreault wrote: > is there a fancy initialization like: > > const std::map<std::string, int> metrics{{"UploadDecision", > static_cast<int>(upload_decision}}; > > ? Done. https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... File components/autofill/core/browser/autofill_ukm.cc (right): https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_ukm.cc:21: DVLOG(3) << "No UKM service."; On 2017/03/09 18:40:49, Mathieu Perreault wrote: > these DVLOG are not very useful? Done. https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... File components/autofill/core/browser/autofill_ukm.h (right): https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_ukm.h:20: class AutofillUkm { On 2017/03/09 18:40:49, Mathieu Perreault wrote: > I don't think we need the class, right? LogUkm could just sit in AutofillMetrics > as a static. Done. https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... File components/autofill/core/browser/test_autofill_client.h (right): https://codereview.chromium.org/2740633002/diff/160001/components/autofill/co... components/autofill/core/browser/test_autofill_client.h:20: //#include "components/ukm/ukm_source.h" On 2017/03/09 18:40:49, Mathieu Perreault wrote: > fix Done.
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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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_...)
https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... File components/autofill/core/browser/autofill_experiments.cc (right): https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... components/autofill/core/browser/autofill_experiments.cc:35: const base::Feature kAutofillUkmLogging{"AutofillLogUkm", AutofillUkmLogging is also a good feature name https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... File components/autofill/core/browser/autofill_experiments.h (right): https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... components/autofill/core/browser/autofill_experiments.h:105: bool IsLoggingUkmEnabled(); *IsUkmLoggingEnabled to be consistent with the name of the feature. https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:1089: // TODO(jdonnelly): Log duration. can we add a success log here?
On 2017/03/09 20:13:26, Mathieu Perreault wrote: > https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... > File components/autofill/core/browser/autofill_experiments.cc (right): > > https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... > components/autofill/core/browser/autofill_experiments.cc:35: const base::Feature > kAutofillUkmLogging{"AutofillLogUkm", > AutofillUkmLogging is also a good feature name > > https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... > File components/autofill/core/browser/autofill_experiments.h (right): > > https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... > components/autofill/core/browser/autofill_experiments.h:105: bool > IsLoggingUkmEnabled(); > *IsUkmLoggingEnabled to be consistent with the name of the feature. > > https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... > File components/autofill/core/browser/autofill_manager.cc (right): > > https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... > components/autofill/core/browser/autofill_manager.cc:1089: // TODO(jdonnelly): > Log duration. > can we add a success log here? Let's also revert the about flag
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
PTAL? https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... File components/autofill/core/browser/autofill_experiments.cc (right): https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... components/autofill/core/browser/autofill_experiments.cc:35: const base::Feature kAutofillUkmLogging{"AutofillLogUkm", On 2017/03/09 20:13:26, Mathieu Perreault wrote: > AutofillUkmLogging is also a good feature name Done. https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... File components/autofill/core/browser/autofill_experiments.h (right): https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... components/autofill/core/browser/autofill_experiments.h:105: bool IsLoggingUkmEnabled(); On 2017/03/09 20:13:26, Mathieu Perreault wrote: > *IsUkmLoggingEnabled to be consistent with the name of the feature. Done. https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2740633002/diff/200001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:1089: // TODO(jdonnelly): Log duration. On 2017/03/09 20:13:26, Mathieu Perreault wrote: > can we add a success log here? We can but in a different set of metrics? WDYT?
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/2740633002/diff/220001/components/ukm/ukm_ser... File components/ukm/ukm_service.h (right): https://codereview.chromium.org/2740633002/diff/220001/components/ukm/ukm_ser... components/ukm/ukm_service.h:167: int32_t session_id_; Oops, this came with the rebase.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #6 (id:240001) has been deleted
Added a bunch of tests, PTAL?
Patchset #6 (id:240001) has been deleted
The CQ bit was checked by sebsg@chromium.org 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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
lgtm with nit https://codereview.chromium.org/2740633002/diff/260001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2740633002/diff/260001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:1042: AutofillMetrics::LogCardUploadDecisionMetricUkm( we are not using the return value (bool) so consider making it void. https://codereview.chromium.org/2740633002/diff/260001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2740633002/diff/260001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:715: bool AutofillMetrics::LogUkm(ukm::UkmService* ukm_service, We can keep the return value for unit testing, I guess?
Patchset #2 (id:120001) has been deleted
The CQ bit was checked by sebsg@chromium.org 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 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 sebsg@chromium.org 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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) 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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #7 (id:300001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sebsg@chromium.org changed reviewers: + rkaplow@chromium.org
Hi Rob, PTAL? https://codereview.chromium.org/2740633002/diff/260001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2740633002/diff/260001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:1042: AutofillMetrics::LogCardUploadDecisionMetricUkm( On 2017/03/10 15:30:22, Mathieu Perreault wrote: > we are not using the return value (bool) so consider making it void. Done. https://codereview.chromium.org/2740633002/diff/260001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2740633002/diff/260001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:715: bool AutofillMetrics::LogUkm(ukm::UkmService* ukm_service, On 2017/03/10 15:30:22, Mathieu Perreault wrote: > We can keep the return value for unit testing, I guess? Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:1042: AutofillMetrics::LogCardUploadDecisionMetricUkm( small optional comment - since the structure of AutofillMetrics::LogCardUploadDecisionMetricUkm( client_->GetUkmService(), pending_upload_request_url_, AutofillMetrics::something) appears over and over, you may want to move that to a local function and hide the GetUkmService / pending_upload_request_url_ detail https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_manager_unittest.cc:1038: // Check if there is an entry for card upload decisions. nit for these two, the expectation should appear first (expected, actual) https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:707: std::map<std::string, int> metrics = { hm, right now LogUKM supports a map, but we're only ever adding a single key/value pair. Is this expected to change shortly? https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:700: // Logs the the |ukm| with the specified |url| and the specified |metrics|. ukm as as a string isn't super clear. Ukm Entry? https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_metrics_unittest.cc:4399: EXPECT_EQ(entry_proto.event_hash(), similar comment re: expect ordering
The CQ bit was checked by sebsg@chromium.org 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 checked by sebsg@chromium.org to run a CQ dry run
Patchset #8 (id:340001) has been deleted
Patchset #8 (id:360001) has been deleted
Thanks, another look? https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:1042: AutofillMetrics::LogCardUploadDecisionMetricUkm( On 2017/03/10 19:00:27, rkaplow (slow) wrote: > small optional comment - since the structure of > > AutofillMetrics::LogCardUploadDecisionMetricUkm( > client_->GetUkmService(), pending_upload_request_url_, > AutofillMetrics::something) > > appears over and over, you may want to move that to a local function and hide > the GetUkmService / pending_upload_request_url_ detail Done. https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_manager_unittest.cc:1038: // Check if there is an entry for card upload decisions. On 2017/03/10 19:00:27, rkaplow (slow) wrote: > nit for these two, the expectation should appear first (expected, actual) Done. https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:707: std::map<std::string, int> metrics = { On 2017/03/10 19:00:27, rkaplow (slow) wrote: > hm, right now LogUKM supports a map, but we're only ever adding a single > key/value pair. Is this expected to change shortly? The Payment Request ukm that I'm adding now in another CL will save 2 metrics per entry. I was also wondering if this is something that would be worth exposing in the UKM API? It sounds like something that will be done often. https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:700: // Logs the the |ukm| with the specified |url| and the specified |metrics|. On 2017/03/10 19:00:27, rkaplow (slow) wrote: > ukm as as a string isn't super clear. Ukm Entry? Done. https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_metrics_unittest.cc:4399: EXPECT_EQ(entry_proto.event_hash(), On 2017/03/10 19:00:27, rkaplow (slow) wrote: > similar comment re: expect ordering Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sebsg@chromium.org changed reviewers: + boliu@chromium.org, jdonnelly@chromium.org
jdonnelly@, could you please take a look at autofill_client_ios? boliu@, could you please take a look at aw_autofill_client? Thanks!
autofill_client_ios lgtm
dumb question.. what is UKM and why doesn't webview need it too?
rs lgtm after chat
lgtm https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:707: std::map<std::string, int> metrics = { On 2017/03/10 19:25:33, sebsg wrote: > On 2017/03/10 19:00:27, rkaplow (slow) wrote: > > hm, right now LogUKM supports a map, but we're only ever adding a single > > key/value pair. Is this expected to change shortly? > > The Payment Request ukm that I'm adding now in another CL will save 2 metrics > per entry. I was also wondering if this is something that would be worth > exposing in the UKM API? It sounds like something that will be done often. yes, we can probably do that though. Can you add a comment that you set it up as a map as more metrics will be added? Right now it seems wasteful https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:700: // Logs the the |ukm| with the specified |url| and the specified |metrics|. On 2017/03/10 19:25:33, sebsg wrote: > On 2017/03/10 19:00:27, rkaplow (slow) wrote: > > ukm as as a string isn't super clear. Ukm Entry? > > Done. please adjust the comment too
Thanks! Sending to CQ https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:707: std::map<std::string, int> metrics = { On 2017/03/10 20:26:16, rkaplow (slow) wrote: > On 2017/03/10 19:25:33, sebsg wrote: > > On 2017/03/10 19:00:27, rkaplow (slow) wrote: > > > hm, right now LogUKM supports a map, but we're only ever adding a single > > > key/value pair. Is this expected to change shortly? > > > > The Payment Request ukm that I'm adding now in another CL will save 2 metrics > > per entry. I was also wondering if this is something that would be worth > > exposing in the UKM API? It sounds like something that will be done often. > > yes, we can probably do that though. > Can you add a comment that you set it up as a map as more metrics will be added? > Right now it seems wasteful Done. https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2740633002/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:700: // Logs the the |ukm| with the specified |url| and the specified |metrics|. On 2017/03/10 20:26:16, rkaplow (slow) wrote: > On 2017/03/10 19:25:33, sebsg wrote: > > On 2017/03/10 19:00:27, rkaplow (slow) wrote: > > > ukm as as a string isn't super clear. Ukm Entry? > > > > Done. > > please adjust the comment too Done.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, rkaplow@chromium.org, boliu@chromium.org, jdonnelly@chromium.org Link to the patchset: https://codereview.chromium.org/2740633002/#ps400001 (title: "Fixed 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": 400001, "attempt_start_ts": 1489178305807010,
"parent_rev": "073706fee0c989101c5c28391400ddeb0a5a4127", "commit_rev":
"adcdf2d7c58a495e0f917a50148c2f041a1a6e85"}
Message was sent while issue was closed.
Description was changed from ========== [Autofill] Add upstreaming UKM BUG=699256 ========== to ========== [Autofill] Add upstreaming UKM BUG=699256 Review-Url: https://codereview.chromium.org/2740633002 Cr-Commit-Position: refs/heads/master@{#456173} Committed: https://chromium.googlesource.com/chromium/src/+/adcdf2d7c58a495e0f917a50148c... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:400001) as https://chromium.googlesource.com/chromium/src/+/adcdf2d7c58a495e0f917a50148c... |
