Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737953002:
UKM: Remove UkmService::RecordSource (Closed)
DescriptionUKM: Remove UkmService::RecordSource
This CL removes the old API UkmService::RecordSource. It also changes
the source vector to a map.
BUG=678682
Review-Url: https://codereview.chromium.org/2737953002
Cr-Commit-Position: refs/heads/master@{#456922}
Committed: https://chromium.googlesource.com/chromium/src/+/c5e49a3f0d49efa6d313f165599e697289c5468e
Patch Set 1 #
Total comments: 6
Patch Set 2 : review fix #
Total comments: 2
Patch Set 3 : review fix #Patch Set 4 : rebase and fix autofill #Patch Set 5 : fix compile error on win bot #Patch Set 6 : fix error #Patch Set 7 : rebase #Patch Set 8 : fix error from rebase #
Messages
Total messages: 41 (20 generated)
Description was changed from ========== UKM: Remove UkmService::RecordSource BUG=678682 ========== to ========== UKM: Remove UkmService::RecordSource This CL removes the old API UkmService::RecordSource. It also changes the source vector to a map. BUG=678682 ==========
zhenw@chromium.org changed reviewers: + bmcquade@chromium.org, holte@chromium.org, rkaplow@chromium.org
ptal
https://codereview.chromium.org/2737953002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2737953002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc:48: const std::map<int32_t, std::unique_ptr<ukm::UkmSource>>& sources = auto? https://codereview.chromium.org/2737953002/diff/1/components/ukm/ukm_service.cc File components/ukm/ukm_service.cc (right): https://codereview.chromium.org/2737953002/diff/1/components/ukm/ukm_service.... components/ukm/ukm_service.cc:451: if (sources_.find(source_id) != sources_.end()) { can use ContainsKey https://codesearch.chromium.org/chromium/src/base/stl_util.h?type=cs&q=stl+co... https://codereview.chromium.org/2737953002/diff/1/components/ukm/ukm_service.... components/ukm/ukm_service.cc:452: DCHECK(sources_[source_id]); i don't think this dcheck is needed, by definition of how we're finding
https://codereview.chromium.org/2737953002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2737953002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc:48: const std::map<int32_t, std::unique_ptr<ukm::UkmSource>>& sources = On 2017/03/08 18:49:20, rkaplow (slow) wrote: > auto? Done. https://codereview.chromium.org/2737953002/diff/1/components/ukm/ukm_service.cc File components/ukm/ukm_service.cc (right): https://codereview.chromium.org/2737953002/diff/1/components/ukm/ukm_service.... components/ukm/ukm_service.cc:451: if (sources_.find(source_id) != sources_.end()) { On 2017/03/08 18:49:20, rkaplow (slow) wrote: > can use ContainsKey > https://codesearch.chromium.org/chromium/src/base/stl_util.h?type=cs&q=stl+co... Done. https://codereview.chromium.org/2737953002/diff/1/components/ukm/ukm_service.... components/ukm/ukm_service.cc:452: DCHECK(sources_[source_id]); On 2017/03/08 18:49:20, rkaplow (slow) wrote: > i don't think this dcheck is needed, by definition of how we're finding Done.
lgtm
Bryan, can you take a look at the page load metrics part? Thanks!
On 2017/03/09 at 16:51:14, zhenw wrote: > Bryan, can you take a look at the page load metrics part? Thanks! Sorry, I missed this. This basically looks good - just one small requested change in the test. Thanks!
oops, here are comments https://codereview.chromium.org/2737953002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2737953002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc:45: const ukm::UkmSource* GetUkmSource(size_t source_index) { Now that we're using a map<> and map<> iteration order is a function of key sort order, and over time the source_ids used as keys may not be monotonically increasing as they are allocated, looking up by index may no longer be safe / stable. Given this, let's change this method signature to: const ukm::UkmSource* GetUkmSourceForUrl(const char* url); Technically there can be more than one source for a given URL, but I think it's reasonable to assume one source per URL for the scope of this test class. You can then update the call sites accordingly. For example, the lookup code in MultiplePageLoads changes from: EXPECT_EQ(2ul, ukm_source_count()); const ukm::UkmSource* source1 = GetUkmSource(0); const ukm::UkmSource* source2 = GetUkmSource(1); EXPECT_EQ(GURL(kTestUrl1), source1->url()); EXPECT_EQ(GURL(kTestUrl2), source2->url()); EXPECT_NE(source1->id(), source2->id()); to: EXPECT_EQ(2ul, ukm_source_count()); const ukm::UkmSource* source1 = GetUkmSourceForUrl(kTestUrl1); const ukm::UkmSource* source2 = GetUkmSourceForUrl(kTestUrl2); EXPECT_NE(source1->id(), source2->id());
https://codereview.chromium.org/2737953002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2737953002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc:45: const ukm::UkmSource* GetUkmSource(size_t source_index) { On 2017/03/13 17:55:38, Bryan McQuade wrote: > Now that we're using a map<> and map<> iteration order is a function of key sort > order, and over time the source_ids used as keys may not be monotonically > increasing as they are allocated, looking up by index may no longer be safe / > stable. > > Given this, let's change this method signature to: > const ukm::UkmSource* GetUkmSourceForUrl(const char* url); > > Technically there can be more than one source for a given URL, but I think it's > reasonable to assume one source per URL for the scope of this test class. > > You can then update the call sites accordingly. For example, the lookup code in > MultiplePageLoads changes from: > > EXPECT_EQ(2ul, ukm_source_count()); > const ukm::UkmSource* source1 = GetUkmSource(0); > const ukm::UkmSource* source2 = GetUkmSource(1); > EXPECT_EQ(GURL(kTestUrl1), source1->url()); > EXPECT_EQ(GURL(kTestUrl2), source2->url()); > EXPECT_NE(source1->id(), source2->id()); > > to: > > EXPECT_EQ(2ul, ukm_source_count()); > const ukm::UkmSource* source1 = GetUkmSourceForUrl(kTestUrl1); > const ukm::UkmSource* source2 = GetUkmSourceForUrl(kTestUrl2); > EXPECT_NE(source1->id(), source2->id()); Done.
lgtm
The CQ bit was checked by zhenw@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...
zhenw@chromium.org changed reviewers: + sebsg@chromium.org
+sebsg for //components/autofill/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
LGTM for Autofill
The CQ bit was checked by zhenw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, bmcquade@chromium.org Link to the patchset: https://codereview.chromium.org/2737953002/#ps60001 (title: "rebase and fix autofill")
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by zhenw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, sebsg@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2737953002/#ps80001 (title: "fix compile error on win bot")
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by zhenw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, sebsg@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2737953002/#ps100001 (title: "fix error")
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/03/14 at 22:07:33, commit-bot wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Hi Zhen, I just landed https://codereview.chromium.org/2747173003 which will require you to re-sync and update this change. There are 2 new tests in ukm_page_load_metrics_observer_unittest.cc that will require updating to use the new method. Just wanted to let you know.
On 2017/03/14 22:27:42, Bryan McQuade wrote: > On 2017/03/14 at 22:07:33, commit-bot wrote: > > Try jobs failed on following builders: > > linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux > (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > Hi Zhen, > > I just landed https://codereview.chromium.org/2747173003 which will require you > to re-sync and update this change. There are 2 new tests in > ukm_page_load_metrics_observer_unittest.cc that will require updating to use the > new method. Just wanted to let you know. Thanks for the notice! Just rebased and will commit again.
The CQ bit was checked by zhenw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, sebsg@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2737953002/#ps140001 (title: "fix error from rebase")
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": 140001, "attempt_start_ts": 1489533004054250,
"parent_rev": "b520415c483d14ee79d8fb82accc63aedd78b755", "commit_rev":
"c5e49a3f0d49efa6d313f165599e697289c5468e"}
Message was sent while issue was closed.
Description was changed from ========== UKM: Remove UkmService::RecordSource This CL removes the old API UkmService::RecordSource. It also changes the source vector to a map. BUG=678682 ========== to ========== UKM: Remove UkmService::RecordSource This CL removes the old API UkmService::RecordSource. It also changes the source vector to a map. BUG=678682 Review-Url: https://codereview.chromium.org/2737953002 Cr-Commit-Position: refs/heads/master@{#456922} Committed: https://chromium.googlesource.com/chromium/src/+/c5e49a3f0d49efa6d313f165599e... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/c5e49a3f0d49efa6d313f165599e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
