Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(15)

Issue 2780003003: Send an event to the page load metrics to track resource starting. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 3 weeks ago by Pete Williamson
Modified:
6 months ago
CC:
chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org, loading-reviews_chromium.org, mmenke, Randy Smith (Not in Mondays), speed-metrics-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Send an event to the page load metrics to track resource starting. This will eventually allow page load metrics consumers to calculate a percentage of known resource request completion dynamically. Design doc here: https://docs.google.com/document/d/1dxGOMsUkmxfoNUfQKqZRi4qAhWVGh9DZDiFI1arkiJ0 BUG=699313 Review-Url: https://codereview.chromium.org/2780003003 Cr-Commit-Position: refs/heads/master@{#467011} Committed: https://chromium.googlesource.com/chromium/src/+/5d47d0d535ba6b9eff503faaf6af0769faa8b8c3

Patch Set 1 #

Total comments: 6

Patch Set 2 : Not ready for full review, but uploading so we can review the approach. #

Patch Set 3 : Unit test working #

Patch Set 4 : Fix browser tests #

Total comments: 12

Patch Set 5 : Fix browser tests #

Patch Set 6 : CR feedback per RyanSturm #

Total comments: 14

Patch Set 7 : Second round of RyanSturm feedback #

Total comments: 15

Patch Set 8 : CR feedback per bmcquade #

Total comments: 6

Patch Set 9 : Add comments per bmcquade #

Patch Set 10 : FREEZE.unindexed - took merge and fixed it up #

Unified diffs Side-by-side diffs Delta from patch set Stats (+405 lines, -94 lines) Patch
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc View 1 2 3 4 5 6 7 8 9 2 chunks +31 lines, -0 lines 0 comments Download
M chrome/browser/page_load_metrics/metrics_web_contents_observer.h View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/page_load_metrics/metrics_web_contents_observer.cc View 1 2 3 4 5 6 7 8 9 2 chunks +21 lines, -3 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -4 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 3 chunks +15 lines, -9 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc View 1 2 3 4 5 6 7 8 9 1 chunk +11 lines, -9 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 3 chunks +21 lines, -12 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.h View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.cc View 1 2 3 4 5 1 chunk +5 lines, -4 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +9 lines, -5 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h View 1 1 chunk +4 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -1 line 0 comments Download
A chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h View 1 2 3 4 5 6 7 1 chunk +49 lines, -0 lines 0 comments Download
A chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc View 1 2 3 4 5 6 7 1 chunk +39 lines, -0 lines 0 comments Download
A chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +71 lines, -0 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.h View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc View 1 2 3 4 5 1 chunk +5 lines, -4 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 4 chunks +12 lines, -6 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.h View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.cc View 1 2 3 4 5 1 chunk +5 lines, -4 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +9 lines, -5 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_metrics_observer.h View 1 2 3 4 5 6 7 8 9 3 chunks +39 lines, -10 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_metrics_observer.cc View 1 2 3 4 5 6 7 8 9 1 chunk +14 lines, -4 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_tracker.h View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/page_load_tracker.cc View 1 2 3 4 5 6 7 8 1 chunk +9 lines, -2 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 58 (35 generated)
RyanSturm
https://codereview.chromium.org/2780003003/diff/1/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2780003003/diff/1/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc#newcode359 chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:359: const GURL& url, Are you using url here? https://codereview.chromium.org/2780003003/diff/1/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc ...
6 months, 3 weeks ago (2017-03-29 19:03:47 UTC) #2
Pete Williamson
OK, the unit tests are working, I think this is ready for a first look ...
6 months, 1 week ago (2017-04-13 23:10:21 UTC) #4
RyanSturm
It seems like this change does nothing, no UMA or initializing the observer (chrome/browser/page_load_metrics/page_load_metrics_initialize.cc). I'm ...
6 months, 1 week ago (2017-04-14 20:08:43 UTC) #13
Pete Williamson
Addressed all outstanding feedback. I understand this would be better with a client using it, ...
6 months, 1 week ago (2017-04-17 18:53:12 UTC) #18
RyanSturm
lgtm % nits Can you wait for bmcquade before submitting? https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc#newcode468 ...
6 months, 1 week ago (2017-04-17 19:57:08 UTC) #19
Pete Williamson
Waiting on BMcquade1's feedback Fixed all identified issues. https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc#newcode468 chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:468: // ...
6 months, 1 week ago (2017-04-17 20:46:36 UTC) #20
Bryan McQuade
Adding csharrison for chrome/browser/loader/ changes.
6 months, 1 week ago (2017-04-17 23:22:56 UTC) #27
Bryan McQuade
Thanks! A couple comments - I think this seems reasonable. We'll need chrome/browser/loader owners review ...
6 months, 1 week ago (2017-04-17 23:32:14 UTC) #28
RyanSturm
https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_load_metrics/page_load_metrics_observer.h File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right): https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_load_metrics/page_load_metrics_observer.h#newcode236 chrome/browser/page_load_metrics/page_load_metrics_observer.h:236: // The type of the request as gleaned from ...
6 months, 1 week ago (2017-04-17 23:37:32 UTC) #29
Charlie Harrison
https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc#newcode348 chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:348: void NotifyUIThreadOfRequestStarted( Can we use WebContentsObserver::DidGetResourceResponseStart instead?
6 months, 1 week ago (2017-04-18 16:51:33 UTC) #30
RyanSturm
https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc#newcode348 chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:348: void NotifyUIThreadOfRequestStarted( On 2017/04/18 16:51:32, Charlie Harrison wrote: > ...
6 months, 1 week ago (2017-04-18 17:23:58 UTC) #31
Charlie Harrison
https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc#newcode348 chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:348: void NotifyUIThreadOfRequestStarted( On 2017/04/18 17:23:57, Ryan Sturm wrote: > ...
6 months, 1 week ago (2017-04-18 17:38:35 UTC) #32
Charlie Harrison
c/b/l LGTM but I only looked at that file. I in servicing your TODO it ...
6 months, 1 week ago (2017-04-18 17:40:43 UTC) #33
Pete Williamson
On 2017/04/18 17:40:43, Charlie Harrison wrote: > c/b/l LGTM but I only looked at that ...
6 months, 1 week ago (2017-04-18 18:16:43 UTC) #34
Pete Williamson
Addressed BMcQuade comments. https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h (right): https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h#newcode14 chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:14: class ResourceTrackingPageLoadMetricsObserver On 2017/04/17 23:32:13, Bryan ...
6 months, 1 week ago (2017-04-18 18:17:27 UTC) #35
RyanSturm
https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_load_metrics/page_load_metrics_observer.h File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right): https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_load_metrics/page_load_metrics_observer.h#newcode236 chrome/browser/page_load_metrics/page_load_metrics_observer.h:236: // The type of the request as gleaned from ...
6 months, 1 week ago (2017-04-18 18:37:13 UTC) #36
Bryan McQuade
LGTM, thanks! A couple comments - feel free to commit once you've decided how to ...
6 months ago (2017-04-24 13:56:56 UTC) #37
Pete Williamson
https://codereview.chromium.org/2780003003/diff/140001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2780003003/diff/140001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#newcode259 chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:259: // Note for the main HTML page, the tracker ...
6 months ago (2017-04-24 19:53:14 UTC) #38
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/2780003003/160001
6 months ago (2017-04-24 19:54:32 UTC) #41
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/254223) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, ...
6 months ago (2017-04-24 20:00:14 UTC) #43
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/2780003003/180001
6 months ago (2017-04-25 16:24:11 UTC) #54
commit-bot: I haz the power
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/5d47d0d535ba6b9eff503faaf6af0769faa8b8c3
6 months ago (2017-04-25 16:29:01 UTC) #57
Pete Williamson
4 months, 1 week ago (2017-06-12 23:20:46 UTC) #58
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2940503003/ by petewil@chromium.org.

The reason for reverting is: After talking with the chrome loading team, they
expressed a preference to not use PageLoadMetrics for things happening on the
background thread, so we are changing our approach..
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa