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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by trevordixon
Modified:
3 weeks, 5 days ago
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 Review-Url: https://codereview.chromium.org/2755093002 Cr-Commit-Position: refs/heads/master@{#468621} Committed: https://chromium.googlesource.com/chromium/src/+/ba9f7b9f476e705395814878d9e2401118384b60

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: 4

Patch Set 7 : before_first_contentful_paint browser_test #

Patch Set 8 : before_first_contentful_paint browser_test #

Total comments: 23

Patch Set 9 : Respond to feedback. #

Total comments: 8

Patch Set 10 : Comments, style. #

Patch Set 11 : Simplify HTML. #

Patch Set 12 : Unit test. #

Total comments: 4

Patch Set 13 : Just check db. #

Total comments: 8

Patch Set 14 : Delete unnecessary stuff. #

Patch Set 15 : Delete unnecessary stuff. #

Patch Set 16 : Bump kDatabaseVersion #

Patch Set 17 : Rebase #

Patch Set 18 : Bump kDatabaseVersion #

Unified diffs Side-by-side diffs Delta from patch set Stats (+223 lines, -34 lines) Patch
M chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +4 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 7 8 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 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/predictors/resource_prefetch_predictor.h View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +10 lines, -0 lines 0 comments Download
M chrome/browser/predictors/resource_prefetch_predictor.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 8 chunks +36 lines, -1 line 0 comments Download
M chrome/browser/predictors/resource_prefetch_predictor.proto View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 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 8 9 10 11 12 13 14 13 chunks +56 lines, -13 lines 0 comments Download
M chrome/browser/predictors/resource_prefetch_predictor_tables.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 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 7 8 9 10 11 12 13 14 6 chunks +11 lines, -3 lines 0 comments Download
M chrome/browser/predictors/resource_prefetch_predictor_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +53 lines, -0 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/subresource_fcp_order.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +16 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 53 (20 generated)
trevordixon
2 months, 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); ...
2 months, 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: > ...
2 months ago (2017-03-27 12:30:08 UTC) #6
trevordixon
2 months 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 ...
2 months 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(). ...
2 months 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 ...
2 months 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 ...
2 months 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.
2 months 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 ...
2 months 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 ...
2 months 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 ...
2 months ago (2017-03-29 11:19:21 UTC) #24
trevordixon
PTAL at browser test.
1 month, 1 week ago (2017-04-21 12:00:59 UTC) #25
alexilin
Thanks! Browsertest looks good, I have just a few comments. Please, add some unittests in ...
1 month, 1 week ago (2017-04-21 13:49:06 UTC) #26
trevordixon
https://codereview.chromium.org/2755093002/diff/260001/chrome/browser/predictors/resource_prefetch_predictor.cc File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2755093002/diff/260001/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode1211 chrome/browser/predictors/resource_prefetch_predictor.cc:1211: resource_to_add->set_before_first_contentful_paint( On 2017/04/21 at 13:49:05, alexilin wrote: > It ...
1 month ago (2017-04-25 12:46:10 UTC) #27
Benoit L
Thanks! Only nits. Additionally, can you add the field to the dump in tools/resource_prefetch_predictor/prefetch_predictor_tool.py? https://codereview.chromium.org/2755093002/diff/320001/chrome/browser/predictors/resource_prefetch_predictor.cc ...
1 month ago (2017-04-25 15:30:37 UTC) #28
alexilin
https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer_unittest.cc 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_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer_unittest.cc#newcode50 chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer_unittest.cc:50: // SessionTabHelper::CreateForWebContents(web_contents()); On 2017/04/25 12:46:09, trevordixon wrote: > On ...
1 month ago (2017-04-25 18:42:21 UTC) #29
trevordixon
On 2017/04/25 at 15:30:37, lizeb wrote: > Additionally, can you add the field to the ...
1 month ago (2017-04-25 20:07:39 UTC) #30
trevordixon
On 2017/04/25 at 18:42:21, alexilin wrote: > https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer_unittest.cc > 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_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer_unittest.cc#newcode50 ...
1 month ago (2017-04-25 20:23:46 UTC) #31
trevordixon
https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc#newcode383 chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:383: bool match_before_first_contentful_paint = false) { On 2017/04/21 at 13:49:05, ...
1 month ago (2017-04-25 20:23:55 UTC) #32
alexilin
browsertest: L G T M https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc#newcode383 chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:383: bool match_before_first_contentful_paint = false) ...
1 month ago (2017-04-26 09:25:01 UTC) #33
trevordixon
Unit test added.
1 month ago (2017-04-27 12:01:12 UTC) #34
alexilin
Thanks! https://codereview.chromium.org/2755093002/diff/380001/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2755093002/diff/380001/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc#newcode2106 chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:2106: auto res1_time = start + 1 * second; ...
1 month ago (2017-04-27 13:16:40 UTC) #35
trevordixon
https://codereview.chromium.org/2755093002/diff/380001/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2755093002/diff/380001/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc#newcode2106 chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:2106: auto res1_time = start + 1 * second; On ...
1 month ago (2017-04-28 09:44:06 UTC) #36
Benoit L
lgtm, thanks. https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/predictors/resource_prefetch_predictor_tables.h File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/predictors/resource_prefetch_predictor_tables.h#newcode146 chrome/browser/predictors/resource_prefetch_predictor_tables.h:146: static constexpr int kDatabaseVersion = 8; Note ...
1 month ago (2017-04-28 11:19:19 UTC) #37
alexilin
lgtm, thank you! https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h File chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h (right): https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h#newcode9 chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h:9: #include "chrome/browser/predictors/resource_prefetch_common.h" nit: Do we need ...
1 month ago (2017-04-28 11:22:04 UTC) #38
trevordixon
https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h File chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h (right): https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h#newcode9 chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h:9: #include "chrome/browser/predictors/resource_prefetch_common.h" On 2017/04/28 at 11:22:03, alexilin wrote: > ...
1 month ago (2017-04-28 12:39:54 UTC) #39
trevordixon
https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/predictors/resource_prefetch_predictor_tables.h File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/predictors/resource_prefetch_predictor_tables.h#newcode146 chrome/browser/predictors/resource_prefetch_predictor_tables.h:146: static constexpr int kDatabaseVersion = 8; On 2017/04/28 at ...
1 month ago (2017-04-28 12:46:42 UTC) #40
trevordixon
bauerb and jkarlin for OWNERS review. Thanks in advance.
1 month ago (2017-04-28 19:25:53 UTC) #42
jkarlin
chrome/browser/page_load_metrics/ lgtm
3 weeks, 6 days ago (2017-05-01 11:40:47 UTC) #43
Bernhard Bauer
LGTM
3 weeks, 5 days ago (2017-05-02 09:06:39 UTC) #44
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/2755093002/500001
3 weeks, 5 days ago (2017-05-02 12:27:40 UTC) #50
commit-bot: I haz the power
3 weeks, 5 days ago (2017-05-02 13:41:14 UTC) #53
Message was sent while issue was closed.
Committed patchset #18 (id:500001) as
https://chromium.googlesource.com/chromium/src/+/ba9f7b9f476e705395814878d9e2...
Sign in to reply to this message.

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