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

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:
4 months, 3 weeks ago by Pete Williamson
Modified:
3 months, 3 weeks 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 ...
4 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 ...
4 months 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 ...
4 months 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, ...
4 months 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 ...
4 months 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: // ...
4 months ago (2017-04-17 20:46:36 UTC) #20
Bryan McQuade
Adding csharrison for chrome/browser/loader/ changes.
4 months 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 ...
4 months 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 ...
4 months 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?
4 months 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: > ...
4 months 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: > ...
4 months 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 ...
4 months 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 ...
4 months 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 ...
4 months 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 ...
4 months 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 ...
3 months, 3 weeks 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 ...
3 months, 3 weeks 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
3 months, 3 weeks 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, ...
3 months, 3 weeks 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
3 months, 3 weeks 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
3 months, 3 weeks ago (2017-04-25 16:29:01 UTC) #57
Pete Williamson
2 months 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 b40b6558b