Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(180)

Issue 2909033002: Move UKM PageLoad SourceId generation to PageLoadTracker. (Closed)

Created:
3 years, 6 months ago by Bryan McQuade
Modified:
3 years, 6 months ago
Reviewers:
RyanSturm
CC:
cbentzel+watch_chromium.org, chromium-reviews, csharrison+watch_chromium.org, gavinp+prer_chromium.org, loading-reviews+metrics_chromium.org, speed-metrics-reviews_chromium.org, subresource-filter-reviews_chromium.org, tbansal+watch-data-reduction-proxy_chromium.org, tburkard+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Move UKM PageLoad SourceId generation to PageLoadTracker. We'd like to enable all observers to record UKM data sharing the same SourceId, so metrics from different observers can be queried/joined together. This change migrates SourceId generation from the UKM observer to the PageLoadTracker, so all observers can share the same SourceId. BUG=727286 Review-Url: https://codereview.chromium.org/2909033002 Cr-Commit-Position: refs/heads/master@{#475606} Committed: https://chromium.googlesource.com/chromium/src/+/0bff3c633799c676eadce377337cc24a27795151

Patch Set 1 #

Patch Set 2 : cleanup #

Total comments: 9

Patch Set 3 : address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+137 lines, -64 lines) Patch
M chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc View 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/google_captcha_observer.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/google_captcha_observer.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/omnibox_suggestion_used_page_load_metrics_observer.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/protocol_page_load_metrics_observer.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/protocol_page_load_metrics_observer.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h View 4 chunks +5 lines, -6 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc View 7 chunks +12 lines, -14 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_metrics_observer.h View 1 5 chunks +10 lines, -2 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_metrics_observer.cc View 5 chunks +15 lines, -7 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_tracker.h View 3 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/page_load_tracker.cc View 1 5 chunks +25 lines, -6 lines 0 comments Download

Messages

Total messages: 23 (15 generated)
RyanSturm
lgtm % a couple of comments, but most of them are no-ops. https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc ...
3 years, 6 months ago (2017-05-30 16:26:23 UTC) #9
Bryan McQuade
Thanks! https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc#newcode104 chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:104: ukm::SourceId source_id) override { On 2017/05/30 at 16:26:23, ...
3 years, 6 months ago (2017-05-30 17:17:06 UTC) #12
RyanSturm
still lgtm https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_load_metrics/page_load_tracker.cc File chrome/browser/page_load_metrics/page_load_tracker.cc (right): https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_load_metrics/page_load_tracker.cc#newcode375 chrome/browser/page_load_metrics/page_load_tracker.cc:375: ukm_recorder->UpdateSourceURL(source_id_, start_url_); On 2017/05/30 17:17:06, Bryan McQuade ...
3 years, 6 months ago (2017-05-30 17:27:11 UTC) #13
Bryan McQuade
On 2017/05/30 at 17:27:11, ryansturm wrote: > still lgtm > > https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_load_metrics/page_load_tracker.cc > File chrome/browser/page_load_metrics/page_load_tracker.cc ...
3 years, 6 months ago (2017-05-30 17:29:13 UTC) #14
RyanSturm
On 2017/05/30 17:29:13, Bryan McQuade wrote: > On 2017/05/30 at 17:27:11, ryansturm wrote: > > ...
3 years, 6 months ago (2017-05-30 17:36:56 UTC) #15
RyanSturm
On 2017/05/30 17:36:56, RyanSturm wrote: > On 2017/05/30 17:29:13, Bryan McQuade wrote: > > On ...
3 years, 6 months ago (2017-05-30 17:38:33 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2909033002/40001
3 years, 6 months ago (2017-05-30 18:27:26 UTC) #20
commit-bot: I haz the power
3 years, 6 months ago (2017-05-30 18:31:58 UTC) #23
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0bff3c633799c676eadce377337c...

Powered by Google App Engine
This is Rietveld 408576698