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

Issue 2755093002: predictors: Mark before_first_contentful_paint for resources fetched before fcp.

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by trevordixon
Modified:
3 days, 3 hours ago
Reviewers:
Benoit L, alexilin
CC:
arv+watch_chromium.org, chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org, speed-metrics-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

predictors: Mark before_first_contentful_paint for resources fetched before fcp. BUG=631966 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Patch Set 1 : Mark before_first_contentful_paint. #

Total comments: 17

Patch Set 2 : Mark before_first_contentful_paint. #

Total comments: 6

Patch Set 3 : Mark before_first_contentful_paint. #

Patch Set 4 : Mark before_first_contentful_paint. #

Patch Set 5 : Set before_first_contentful_paint for new resources on existing hosts. Only increment version once. #

Total comments: 2

Patch Set 6 : Use TimeTicks::Now() for request response time in SummarizeResponse. #

Total comments: 2

Patch Set 7 : before_first_contentful_paint browser_test #

Patch Set 8 : before_first_contentful_paint browser_test #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+159 lines, -34 lines) Patch
M chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h View 1 3 chunks +5 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc View 1 2 3 4 5 6 2 chunks +28 lines, -14 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer_unittest.cc View 1 2 3 4 5 6 2 chunks +3 lines, -1 line 1 comment Download
M chrome/browser/predictors/resource_prefetch_predictor.h View 1 2 3 4 5 6 3 chunks +8 lines, -0 lines 0 comments Download
M chrome/browser/predictors/resource_prefetch_predictor.cc View 1 2 3 4 5 6 6 chunks +23 lines, -1 line 2 comments Download
M chrome/browser/predictors/resource_prefetch_predictor.proto View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc View 1 2 3 4 5 6 7 14 chunks +60 lines, -13 lines 5 comments Download
M chrome/browser/predictors/resource_prefetch_predictor_tables.h View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/predictors/resource_prefetch_predictor_test_util.cc View 1 2 3 4 5 6 2 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/resources/predictors/resource_prefetch_predictor.html View 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/resources/predictors/resource_prefetch_predictor.js View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/predictors/predictors_handler.cc View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
A chrome/test/data/predictors/google.png View 1 2 3 4 5 6 Binary file 0 comments Download
A chrome/test/data/predictors/subresource_fcp_order.html View 1 2 3 4 5 6 7 1 chunk +18 lines, -0 lines 1 comment Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 26 (12 generated)
trevordixon
1 month, 1 week ago (2017-03-17 12:36:43 UTC) #4
alexilin
Thanks! Don't forget to add some unittests! https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc File chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc#newcode33 chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc:33: predictors::NavigationID navigation_id(web_contents); ...
1 month, 1 week ago (2017-03-17 14:46:47 UTC) #5
trevordixon
https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc File chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc#newcode33 chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc:33: predictors::NavigationID navigation_id(web_contents); On 2017/03/17 at 14:46:46, alexilin wrote: > ...
4 weeks ago (2017-03-27 12:30:08 UTC) #6
trevordixon
4 weeks ago (2017-03-27 12:44:50 UTC) #15
Benoit L
Thanks! High level comment: the timings from PageLoadMetrics are offsets relative to navigation_start. Later on ...
4 weeks ago (2017-03-27 13:36:41 UTC) #16
alexilin
Thanks! Adding to Benoit's comment: You can access navigation_start time from page_load_metrics::PageLoadExtraInfo struct in OnFirstContentfulPaint(). ...
4 weeks ago (2017-03-27 15:32:00 UTC) #17
trevordixon
https://codereview.chromium.org/2755093002/diff/160001/chrome/browser/predictors/resource_prefetch_predictor.cc File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2755093002/diff/160001/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode1261 chrome/browser/predictors/resource_prefetch_predictor.cc:1261: On 2017/03/27 at 15:31:59, alexilin wrote: > Need to ...
3 weeks, 6 days ago (2017-03-28 11:10:41 UTC) #18
trevordixon
On 2017/03/27 at 13:36:41, lizeb wrote: > Thanks! > > High level comment: the timings ...
3 weeks, 6 days ago (2017-03-28 11:21:03 UTC) #19
Benoit L
Can you add tests (browser tests in particular)? That would help checking the actual behavior.
3 weeks, 6 days ago (2017-03-28 12:14:11 UTC) #20
alexilin
As discussed offline it would be better to mark all resources as before_first_contentful_paint in the ...
3 weeks, 6 days ago (2017-03-28 12:58:51 UTC) #21
trevordixon
Except for missing tests, I think the issues are fixed. https://codereview.chromium.org/2755093002/diff/220001/chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc File chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2755093002/diff/220001/chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc#newcode76 ...
3 weeks, 5 days ago (2017-03-29 09:26:55 UTC) #23
Benoit L
On 2017/03/29 09:26:55, trevordixon wrote: > Except for missing tests, I think the issues are ...
3 weeks, 5 days ago (2017-03-29 11:19:21 UTC) #24
trevordixon
PTAL at browser test.
3 days, 5 hours ago (2017-04-21 12:00:59 UTC) #25
alexilin
3 days, 3 hours ago (2017-04-21 13:49:06 UTC) #26
Thanks!
Browsertest looks good, I have just a few comments.
Please, add some unittests in resource_prefetch_predictor_unittest.cc.

https://codereview.chromium.org/2755093002/diff/260001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2755093002/diff/260001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:1211:
resource_to_add->set_before_first_contentful_paint(
It has been lost :(

https://codereview.chromium.org/2755093002/diff/260001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:1262:
old_resource->set_before_first_contentful_paint(
Ditto.

https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer_unittest.cc:50:
// SessionTabHelper::CreateForWebContents(web_contents());
Do you still need this?

https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:655: const
base::TimeTicks& first_contentful_paint) {
Please add following lines to the beginning of the method:
  DCHECK_CURRENTLY_ON(BrowserThread::UI);
  if (initialization_state_ != INITIALIZED)
    return;

https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:657: if (nav_it !=
inflight_navigations_.end()) {
very tiny nit:
You could omit curly braces.

https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:92:
delay(base::TimeDelta()) {
This is not needed. base::TimeDelta isn't build-in type.

https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:383: bool
match_before_first_contentful_paint = false) {
Maybe as a global/class member to avoid this plumbing?

https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:399:
NavigateToURLAndCheckSubresourcesAllCached(main_frame_url);
You could pass match_before_first_contentful_paint here as well.

https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:782:
long_script->delay = base::TimeDelta::FromMilliseconds(1500);
We'll hope that this is enough for the slowest test bot.
As an option, we could make http server thread waiting until the UI thread gets
fcp signal. Sounds like overkill, though.

https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:786:
GetURL(kImageRealPath), content::RESOURCE_TYPE_IMAGE, net::LOWEST);
Is there any special reason why you need to have a real image? I suppose fcp
should happen before the response from the long script, right? Just curious.

https://codereview.chromium.org/2755093002/diff/300001/chrome/test/data/predi...
File chrome/test/data/predictors/subresource_fcp_order.html (right):

https://codereview.chromium.org/2755093002/diff/300001/chrome/test/data/predi...
chrome/test/data/predictors/subresource_fcp_order.html:11:
document.body.style.backgroundColor = 'red';
Why do you need to change backgroundColor? Doesn't fcp happen without it?
Sign in to reply to this message.

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