Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2755093002:
predictors: Mark before_first_contentful_paint for resources fetched before fcp. (Closed)
|
Created:
3 years, 9 months ago by trevordixon Modified:
3 years, 7 months 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. |
Descriptionpredictors: 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 #Messages
Total messages: 53 (20 generated)
Description was changed from ========== Mark before_first_contentful_paint. BUG= ========== to ========== Mark before_first_contentful_paint. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Mark before_first_contentful_paint. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== predictors: Mark before_first_contentful_paint for resources fetched before fcp. BUG=631966 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
trevordixon@chromium.org changed reviewers: + alexilin@chromium.org
Thanks! Don't forget to add some unittests! https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/page_loa... 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_loa... chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc:33: predictors::NavigationID navigation_id(web_contents); I'm afraid it won't work correctly. The observer is created before navigation commit, at this point we don't know navigation final destination. NavigationID contains main_frame_url it has to be created after navigation commit. You could keep a pointer to web_contents inside the class and create NavigationID directly in OnFirstContentfulPaint(). https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h (right): https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h:9: #include "chrome/browser/predictors/resource_prefetch_predictor.h" nit: NavigationID is declared in resource_prefetch_common.h. You could include resource_prefetch_common.h. for NavigationID. and keep forward declaration for ResourcePrefetchPredictor. https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:585: if (nav_it == inflight_navigations_.end()) return; nit: Return statement should occupy a separate line. Please, run `git cl format` to automatically reformat code according to style guides. https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:588: request_summary.before_first_contentful_paint = true; You can't assume that Observer gets an event at the same time when first contentful paint happens. The flow is following: (1) Navigation start =====> (2) Navigation commit =====> (3) First contentful paint =====> (4) PageLoadMetricsObserver::OnFirstContentfulPaint() is called I've made a little experiment showing that the time between (3) and (4) could be significant (>1s) You need to use PageLoadTiming::first_contentful_paint to determine when (3) exactly happened. And it could be very tricky if OnLoad() event handlers complete between (3) and (4). We'll write everything in the database assuming that all resources are before fcp while some resources could be loaded after (3). https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:115: bool before_first_contentful_paint; You need to initialize this field in URLRequestSummary constructors. https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:140: base::TimeDelta first_contentful_paint_time; Unused field. https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:208: void RecordFirstContentfulPaint(const NavigationID navigation_id); Please, add a comment to this function. https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:7: // resource_prefetch_predictor_tables.h. Please, bump kDatabaseVersion in resource_prefetch_predictor_tables.h.
https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/page_loa... 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_loa... 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: > I'm afraid it won't work correctly. > The observer is created before navigation commit, at this point we don't know navigation final destination. NavigationID contains main_frame_url it has to be created after navigation commit. > > You could keep a pointer to web_contents inside the class and create NavigationID directly in OnFirstContentfulPaint(). Done. https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h (right): https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h:9: #include "chrome/browser/predictors/resource_prefetch_predictor.h" On 2017/03/17 at 14:46:46, alexilin wrote: > nit: > NavigationID is declared in resource_prefetch_common.h. > You could include resource_prefetch_common.h. for NavigationID. and keep forward declaration for ResourcePrefetchPredictor. Done. https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:585: if (nav_it == inflight_navigations_.end()) return; On 2017/03/17 at 14:46:46, alexilin wrote: > nit: > Return statement should occupy a separate line. > Please, run `git cl format` to automatically reformat code according to style guides. Done. https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:588: request_summary.before_first_contentful_paint = true; On 2017/03/17 at 14:46:46, alexilin wrote: > You can't assume that Observer gets an event at the same time when first contentful paint happens. > The flow is following: > (1) Navigation start =====> (2) Navigation commit =====> (3) First contentful paint =====> (4) PageLoadMetricsObserver::OnFirstContentfulPaint() is called > > I've made a little experiment showing that the time between (3) and (4) could be significant (>1s) > You need to use PageLoadTiming::first_contentful_paint to determine when (3) exactly happened. > > And it could be very tricky if OnLoad() event handlers complete between (3) and (4). We'll write everything in the database assuming that all resources are before fcp while some resources could be loaded after (3). Maybe fixed. How likely is it that OnFirstContentfulPaint could be called after onload? If it's sufficiently rare, it should work fine to ignore the possibility. https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:115: bool before_first_contentful_paint; On 2017/03/17 at 14:46:46, alexilin wrote: > You need to initialize this field in URLRequestSummary constructors. Done. https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:140: base::TimeDelta first_contentful_paint_time; On 2017/03/17 at 14:46:46, alexilin wrote: > Unused field. Oops. https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:208: void RecordFirstContentfulPaint(const NavigationID navigation_id); On 2017/03/17 at 14:46:46, alexilin wrote: > Please, add a comment to this function. Done. https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:7: // resource_prefetch_predictor_tables.h. On 2017/03/17 at 14:46:46, alexilin wrote: > Please, bump kDatabaseVersion in resource_prefetch_predictor_tables.h. Done.
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
trevordixon@chromium.org changed reviewers: + lizeb@chromium.org
Thanks! High level comment: the timings from PageLoadMetrics are offsets relative to navigation_start. Later on you take this offset and add it to a creation time. Are you sure that the two "origins" match? That is, is creation time the same as navigation start?
Thanks! Adding to Benoit's comment: You can access navigation_start time from page_load_metrics::PageLoadExtraInfo struct in OnFirstContentfulPaint(). Don't know how it behaves with redirects though. https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2755093002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:588: request_summary.before_first_contentful_paint = true; On 2017/03/27 12:30:08, trevordixon wrote: > On 2017/03/17 at 14:46:46, alexilin wrote: > > You can't assume that Observer gets an event at the same time when first > contentful paint happens. > > The flow is following: > > (1) Navigation start =====> (2) Navigation commit =====> (3) First contentful > paint =====> (4) PageLoadMetricsObserver::OnFirstContentfulPaint() is called > > > > I've made a little experiment showing that the time between (3) and (4) could > be significant (>1s) > > You need to use PageLoadTiming::first_contentful_paint to determine when (3) > exactly happened. > > > > And it could be very tricky if OnLoad() event handlers complete between (3) > and (4). We'll write everything in the database assuming that all resources are > before fcp while some resources could be loaded after (3). > > Maybe fixed. How likely is it that OnFirstContentfulPaint could be called after > onload? If it's sufficiently rare, it should work fine to ignore the > possibility. Well, it's definitely not rare for "fast" pages for which the time between fcp and onload is short. Then it could happen that onload appears before OnFirstContentfulPaint event. Examples: google properties, facebook, wikipedia. We don't target them, though. So, maybe, it's fine and isn't worth complexity. https://codereview.chromium.org/2755093002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2755093002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:1261: Need to update before_first_contentful_paint property here as well. This code corresponds the case when we had an entry in the database for the current host with this resource. For priority field we save the latest value seen so it makes sense to do so for before_first_contentful_paint as well. https://codereview.chromium.org/2755093002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:1280: ResourceData* resource_to_add = data.add_resources(); Need to set_before_first_contentful_paint() here too. This code corresponds the case when we had an entry in the database for the current host but the resource is new. https://codereview.chromium.org/2755093002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2755093002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:127: static constexpr int kDatabaseVersion = 8; nit: Why 8 after 6? :)
https://codereview.chromium.org/2755093002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2755093002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:1261: On 2017/03/27 at 15:31:59, alexilin wrote: > Need to update before_first_contentful_paint property here as well. > This code corresponds the case when we had an entry in the database for the current host with this resource. > > For priority field we save the latest value seen so it makes sense to do so for before_first_contentful_paint as well. Done. https://codereview.chromium.org/2755093002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:1280: ResourceData* resource_to_add = data.add_resources(); On 2017/03/27 at 15:31:59, alexilin wrote: > Need to set_before_first_contentful_paint() here too. > This code corresponds the case when we had an entry in the database for the current host but the resource is new. Done. https://codereview.chromium.org/2755093002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2755093002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:127: static constexpr int kDatabaseVersion = 8; On 2017/03/27 at 15:32:00, alexilin wrote: > nit: > Why 8 after 6? :) Forgot I already updated that!
On 2017/03/27 at 13:36:41, lizeb wrote: > Thanks! > > High level comment: the timings from PageLoadMetrics are offsets relative to navigation_start. > > Later on you take this offset and add it to a creation time. Are you sure that the two "origins" match? That is, is creation time the same as navigation start? I forget why I thought that made sense. Now I'm comparing base::Time timing.navigation_start + timing.first_contentful_paint.value() to base::Time request.response_time, but I see that for cached responses, "this is the last time the cache entry was validated." If it's cached, it makes more sense to compare to base::TimeTick request.creation_time, but are base::Time and base::TimeTick comparable?
Can you add tests (browser tests in particular)? That would help checking the actual behavior.
As discussed offline it would be better to mark all resources as before_first_contentful_paint in the case when we didn't observe OnFirstContentfulPaint before OnNavigationComplete. Add as Benoit said, you need to add some tests. (unittests firstly, browsertests may be hard to make non-flaky due timings) https://codereview.chromium.org/2755093002/diff/220001/chrome/browser/page_lo... 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_lo... chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc:76: timing.navigation_start + timing.first_contentful_paint.value()); PageLoadTiming::navigation_start marked as "should not be used by PageLoadMetricsObservers". Could you use PageLoadExtraInfo::navigation_start instead? Then your problem with creation_time vs. response_time will be resolved.
Patchset #6 (id:240001) has been deleted
Except for missing tests, I think the issues are fixed. https://codereview.chromium.org/2755093002/diff/220001/chrome/browser/page_lo... 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_lo... chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc:76: timing.navigation_start + timing.first_contentful_paint.value()); On 2017/03/28 at 12:58:51, alexilin wrote: > PageLoadTiming::navigation_start marked as "should not be used by PageLoadMetricsObservers". Could you use PageLoadExtraInfo::navigation_start instead? > Then your problem with creation_time vs. response_time will be resolved. Cool, I'll use extra_info.navigation_start. The following quick test showed differences in the hundreds of microseconds--close enough not to matter. base::TimeDelta time_diff = base::Time::Now() - timing.navigation_start; base::TimeDelta tick_diff = base::TimeTicks::Now() - extra_info.navigation_start; LOG(WARNING) << (time_diff - tick_diff);
On 2017/03/29 09:26:55, trevordixon wrote: > Except for missing tests, I think the issues are fixed. > > https://codereview.chromium.org/2755093002/diff/220001/chrome/browser/page_lo... > 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_lo... > chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc:76: > timing.navigation_start + timing.first_contentful_paint.value()); > On 2017/03/28 at 12:58:51, alexilin wrote: > > PageLoadTiming::navigation_start marked as "should not be used by > PageLoadMetricsObservers". Could you use PageLoadExtraInfo::navigation_start > instead? > > Then your problem with creation_time vs. response_time will be resolved. > > Cool, I'll use extra_info.navigation_start. The following quick test showed > differences in the hundreds of microseconds--close enough not to matter. > > base::TimeDelta time_diff = base::Time::Now() - timing.navigation_start; > base::TimeDelta tick_diff = base::TimeTicks::Now() - > extra_info.navigation_start; > LOG(WARNING) << (time_diff - tick_diff); Great, thanks. Sorry about the vagueness, but there is the resource timing API, which is exposed to Javascript. With luck, would some of the fields be accessible from the browser process? If that's the case, then we don't have to worry about offsets at all, otherwise this is probably OK (even though the offset is going to be larger if we have thread contention).
PTAL at browser test.
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?
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( On 2017/04/21 at 13:49:05, alexilin wrote: > It has been lost :( Oh no, I did a bad merge. Restored. 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( On 2017/04/21 at 13:49:05, alexilin wrote: > Ditto. Fixed. 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()); On 2017/04/21 at 13:49:05, alexilin wrote: > Do you still need this? Nope. 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) { On 2017/04/21 at 13:49:05, alexilin wrote: > Please add following lines to the beginning of the method: > DCHECK_CURRENTLY_ON(BrowserThread::UI); > if (initialization_state_ != INITIALIZED) > return; Done. https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:657: if (nav_it != inflight_navigations_.end()) { On 2017/04/21 at 13:49:05, alexilin wrote: > very tiny nit: > You could omit curly braces. OK. 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()) { On 2017/04/21 at 13:49:05, alexilin wrote: > This is not needed. base::TimeDelta isn't build-in type. Deleted. https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:399: NavigateToURLAndCheckSubresourcesAllCached(main_frame_url); On 2017/04/21 at 13:49:05, alexilin wrote: > You could pass match_before_first_contentful_paint here as well. All resources are before_first_contentful_paint after PrefetchURL, so at this point, we never want to match_before_first_contentful_paint. 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); On 2017/04/21 at 13:49:05, alexilin wrote: > 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. Is 1500 milliseconds a good guess? Might as well go longer? 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); On 2017/04/21 at 13:49:05, alexilin wrote: > 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. I needed a real image for another reason, but that reason went away, so the empty response image should work fine. 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'; On 2017/04/21 at 13:49:05, alexilin wrote: > Why do you need to change backgroundColor? Doesn't fcp happen without it? Doesn't appear to be necessary. Deleted.
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/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2755093002/diff/320001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:864: // Set before_first_contentful paint for each resource. If I understand the code correctly, this will slightly overestimate the resources we get before first contentful paint, because we take the time at which we start reading the response as the reference, not the time at which we are done with a resource. This is fine, and doing otherwise would complicate the code. If my understanding is correct, can you add a comment making that explicit? https://codereview.chromium.org/2755093002/diff/320001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2755093002/diff/320001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:106: base::TimeDelta delay; nit: add a comment? https://codereview.chromium.org/2755093002/diff/320001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:698: if (!summary.delay.is_zero()) { nit: no braces
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()); On 2017/04/25 12:46:09, trevordixon wrote: > On 2017/04/21 at 13:49:05, alexilin wrote: > > Do you still need this? > > Nope. Nit: remove #include "chrome/browser/sessions/session_tab_helper.h" as it's unused. 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:782: long_script->delay = base::TimeDelta::FromMilliseconds(1500); On 2017/04/25 12:46:10, trevordixon wrote: > On 2017/04/21 at 13:49:05, alexilin wrote: > > 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. > > Is 1500 milliseconds a good guess? Might as well go longer? It should be enough. But if this test turns out to be flaky it'd be ok to make it even longer. https://cs.chromium.org/chromium/src/base/test/test_timeouts.h?l=31&gs=cpp%25... Could you also add a comment to the code explaining why we need this delay? Something like: // Delay to ensure that the renderer performs first contentful paint before it gets the response to the long_script. https://codereview.chromium.org/2755093002/diff/320001/chrome/test/data/predi... File chrome/test/data/predictors/subresource_fcp_order.html (right): https://codereview.chromium.org/2755093002/diff/320001/chrome/test/data/predi... chrome/test/data/predictors/subresource_fcp_order.html:10: <script id="script"> I don't completely understand why your test is working. Scripts are executed from top to bottom. Thus this inline script should be executed before the parser stops at long-script. Hence the image tag should be inserted and processed before the parser reaches long-script. What am I missing? I'd be less surprising for me to see inline script after long-script.
On 2017/04/25 at 15:30:37, lizeb wrote: > Additionally, can you add the field to the dump in tools/resource_prefetch_predictor/prefetch_predictor_tool.py? It already shows the before_first_contentful_paint from the proto; is there another place it should be handled?
On 2017/04/25 at 18:42:21, alexilin wrote: > 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()); > On 2017/04/25 12:46:09, trevordixon wrote: > > On 2017/04/21 at 13:49:05, alexilin wrote: > > > Do you still need this? > > > > Nope. > > Nit: > remove #include "chrome/browser/sessions/session_tab_helper.h" as it's unused. > > 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:782: long_script->delay = base::TimeDelta::FromMilliseconds(1500); > On 2017/04/25 12:46:10, trevordixon wrote: > > On 2017/04/21 at 13:49:05, alexilin wrote: > > > 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. > > > > Is 1500 milliseconds a good guess? Might as well go longer? > > It should be enough. But if this test turns out to be flaky it'd be ok to make it even longer. https://cs.chromium.org/chromium/src/base/test/test_timeouts.h?l=31&gs=cpp%25... > > Could you also add a comment to the code explaining why we need this delay? Something like: > // Delay to ensure that the renderer performs first contentful paint before it gets the response to the long_script. > > https://codereview.chromium.org/2755093002/diff/320001/chrome/test/data/predi... > File chrome/test/data/predictors/subresource_fcp_order.html (right): > > https://codereview.chromium.org/2755093002/diff/320001/chrome/test/data/predi... > chrome/test/data/predictors/subresource_fcp_order.html:10: <script id="script"> > I don't completely understand why your test is working. Scripts are executed from top to bottom. Thus this inline script should be executed before the parser stops at long-script. Hence the image tag should be inserted and processed before the parser reaches long-script. > What am I missing? > I'd be less surprising for me to see inline script after long-script. The first two requests in the head should happen before fcp; the other two at the end of the body should happen after. The order of the image and script at the end of the body doesn't really matter as long as both happen after fcp. In fact, the sooner that image is inserted and loaded, the better it tests that fcp is happening at the right time. The delay on the script is there just to ensure there's enough time to trigger RecordFirstContentfulPaint before the page load finishes.
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:383: bool match_before_first_contentful_paint = false) { On 2017/04/21 at 13:49:05, alexilin wrote: > Maybe as a global/class member to avoid this plumbing? Most of the plumbing goes through LearningObserver and, I think, is unavoidable. The first couple of hops are easier to track as method parameters in my opinion, so I'd opt to keep it like this. You think it would be better as a flag on the class? 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); On 2017/04/25 at 18:42:21, alexilin wrote: > On 2017/04/25 12:46:10, trevordixon wrote: > > On 2017/04/21 at 13:49:05, alexilin wrote: > > > 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. > > > > Is 1500 milliseconds a good guess? Might as well go longer? > > It should be enough. But if this test turns out to be flaky it'd be ok to make it even longer. https://cs.chromium.org/chromium/src/base/test/test_timeouts.h?l=31&gs=cpp%25... > > Could you also add a comment to the code explaining why we need this delay? Something like: > // Delay to ensure that the renderer performs first contentful paint before it gets the response to the long_script. Added a comment. https://codereview.chromium.org/2755093002/diff/320001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2755093002/diff/320001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:864: // Set before_first_contentful paint for each resource. On 2017/04/25 at 15:30:36, Benoit L wrote: > If I understand the code correctly, this will slightly overestimate the resources we get before first contentful paint, because we take the time at which we start reading the response as the reference, not the time at which we are done with a resource. > > This is fine, and doing otherwise would complicate the code. > > If my understanding is correct, can you add a comment making that explicit? OK, documented above where response_time is recorded. https://codereview.chromium.org/2755093002/diff/320001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2755093002/diff/320001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:106: base::TimeDelta delay; On 2017/04/25 at 15:30:37, Benoit L wrote: > nit: add a comment? Done. https://codereview.chromium.org/2755093002/diff/320001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:698: if (!summary.delay.is_zero()) { On 2017/04/25 at 15:30:36, Benoit L wrote: > nit: no braces Done.
browsertest: L G T M 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:383: bool match_before_first_contentful_paint = false) { On 2017/04/25 20:23:55, trevordixon wrote: > On 2017/04/21 at 13:49:05, alexilin wrote: > > Maybe as a global/class member to avoid this plumbing? > > Most of the plumbing goes through LearningObserver and, I think, is unavoidable. > The first couple of hops are easier to track as method parameters in my opinion, > so I'd opt to keep it like this. You think it would be better as a flag on the > class? Fair enough! sgtm. https://codereview.chromium.org/2755093002/diff/300001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:399: NavigateToURLAndCheckSubresourcesAllCached(main_frame_url); On 2017/04/25 12:46:09, trevordixon wrote: > On 2017/04/21 at 13:49:05, alexilin wrote: > > You could pass match_before_first_contentful_paint here as well. > > All resources are before_first_contentful_paint after PrefetchURL, so at this > point, we never want to match_before_first_contentful_paint. Yes, you're right. It'd be better to not race with cache response time. https://codereview.chromium.org/2755093002/diff/320001/chrome/test/data/predi... File chrome/test/data/predictors/subresource_fcp_order.html (right): https://codereview.chromium.org/2755093002/diff/320001/chrome/test/data/predi... chrome/test/data/predictors/subresource_fcp_order.html:10: <script id="script"> On 2017/04/25 18:42:21, alexilin wrote: > I don't completely understand why your test is working. Scripts are executed > from top to bottom. Thus this inline script should be executed before the parser > stops at long-script. Hence the image tag should be inserted and processed > before the parser reaches long-script. > What am I missing? > I'd be less surprising for me to see inline script after long-script. Discussed offline: This version of test had a complicated behavior. There is a race between preload scanner that issues a request for long-script.js and html-parser that executes the inline script and issues a request for image.png. If the request for long-script.js arrives to the embedded HTTP server earlier than the request for image.png then the test works as expected. Because the HTTP server synchronously waits for a delay before it sends a response for long-script.js and a response for image.png always sent after. Anyway, it's possible that HTTP server gets a request for image.png before, then the test could fail.
Unit test added.
Thanks! https://codereview.chromium.org/2755093002/diff/380001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2755093002/diff/380001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:2106: auto res1_time = start + 1 * second; nit: You could use base::TimeTicks::FromInternalValue(x) instead of using Now() + delta. https://codereview.chromium.org/2755093002/diff/380001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:2146: resource3.before_first_contentful_paint = true; Predictor doesn't keep references to URLRequestSummary that was passed to it as well as gmock macros. So this change doesn't affect neither the state of the predictor nor the set expectation. To make it work you need to call EXPECT_CALL(mock_observer,...) with correct parameters first, then you need to reset before_first_contentful_paint and call predictor_->RecordURLResponse(...) for all resources. As an alternative, I'd suggest you to not use mock_observer and do not set before_first_contentful_paint explicitly for any URLRequestSummary. The check that the data passed to database is correct would be enough.
https://codereview.chromium.org/2755093002/diff/380001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2755093002/diff/380001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:2106: auto res1_time = start + 1 * second; On 2017/04/27 at 13:16:39, alexilin wrote: > nit: > You could use base::TimeTicks::FromInternalValue(x) instead of using Now() + delta. Done. https://codereview.chromium.org/2755093002/diff/380001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:2146: resource3.before_first_contentful_paint = true; On 2017/04/27 at 13:16:39, alexilin wrote: > Predictor doesn't keep references to URLRequestSummary that was passed to it as well as gmock macros. So this change doesn't affect neither the state of the predictor nor the set expectation. > To make it work you need to call EXPECT_CALL(mock_observer,...) with correct parameters first, then you need to reset before_first_contentful_paint and call predictor_->RecordURLResponse(...) for all resources. > > As an alternative, I'd suggest you to not use mock_observer and do not set before_first_contentful_paint explicitly for any URLRequestSummary. The check that the data passed to database is correct would be enough. OK, removed.
lgtm, thanks. https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:146: static constexpr int kDatabaseVersion = 8; Note that this needs to be increased further, as Alex's CL landed before. I'm mentioning that as I don't think the rebase will show a conflict.
lgtm, thank you! https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/page_lo... 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_lo... 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 this include? https://codereview.chromium.org/2755093002/diff/400001/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/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer_unittest.cc:11: #include "chrome/browser/sessions/session_tab_helper.h" nit: Do we need this include? https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:472: response_time(base::TimeTicks()), nit: Unnecessary call of the copy constructor. Please remove or call it without parameters.
https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/page_lo... 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_lo... 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: > nit: > Do we need this include? Deleted. https://codereview.chromium.org/2755093002/diff/400001/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/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer_unittest.cc:11: #include "chrome/browser/sessions/session_tab_helper.h" On 2017/04/28 at 11:22:03, alexilin wrote: > nit: > Do we need this include? Deleted. https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:472: response_time(base::TimeTicks()), On 2017/04/28 at 11:22:04, alexilin wrote: > nit: > Unnecessary call of the copy constructor. > Please remove or call it without parameters. Deleted.
https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2755093002/diff/400001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:146: static constexpr int kDatabaseVersion = 8; On 2017/04/28 at 11:19:19, Benoit L wrote: > Note that this needs to be increased further, as Alex's CL landed before. I'm mentioning that as I don't think the rebase will show a conflict. Thanks. FYI, rebasing did surface the conflict.
trevordixon@chromium.org changed reviewers: + bauerb@chromium.org, jkarlin@chromium.org
bauerb and jkarlin for OWNERS review. Thanks in advance.
chrome/browser/page_load_metrics/ lgtm
LGTM
The CQ bit was checked by trevordixon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by trevordixon@chromium.org
The CQ bit was checked by trevordixon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org, alexilin@chromium.org, bauerb@chromium.org, jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2755093002/#ps500001 (title: "Bump kDatabaseVersion")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 500001, "attempt_start_ts": 1493728044767540,
"parent_rev": "f07593458743da2a55f1d5e7ef7b9f92d02ad0e4", "commit_rev":
"ba9f7b9f476e705395814878d9e2401118384b60"}
Message was sent while issue was closed.
Description was changed from ========== predictors: Mark before_first_contentful_paint for resources fetched before fcp. BUG=631966 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== 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/+/ba9f7b9f476e705395814878d9e2... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:500001) as https://chromium.googlesource.com/chromium/src/+/ba9f7b9f476e705395814878d9e2... |
