predictors: Mark before_first_contentful_paint for resources fetched before fcp.

chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc

Index: chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
diff --git a/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc b/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
index cf9bbc2877e3801e8a24ab6dffb402cec4a338d3..afa0da9082d86968377974649b28074c23620298 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
@@ -54,6 +54,7 @@ const char kStylePath[] = "/handled-by-test/style.css";
 const char kStylePath2[] = "/handled-by-test/style2.css";
 const char kScriptPath[] = "/handled-by-test/script.js";
 const char kScriptPath2[] = "/handled-by-test/script2.js";
+const char kScriptLongPath[] = "/handled-by-test/long-script.js";
 const char kFontPath[] = "/handled-by-test/font.ttf";
 const char kRedirectPath[] = "/handled-by-test/redirect.html";
 const char kRedirectPath2[] = "/handled-by-test/redirect2.html";
@@ -72,6 +73,8 @@ const char kScriptXHRPath[] = "/predictors/xhr.js";
 const char kHtmlIframePath[] = "/predictors/html_iframe.html";
 const char kHtmlJavascriptRedirectPath[] =
     "/predictors/javascript_redirect.html";
+const char kHtmlFcpOrderPath[] = "/predictors/subresource_fcp_order.html";
+const char kImageRealPath[] = "/predictors/google.png";
 
 // Host, path.
 const char* kScript[2] = {kFooHost, kScriptPath};
@@ -85,7 +88,10 @@ struct ResourceSummary {
         is_no_store(false),
         is_external(false),
         is_observable(true),
-        is_prohibited(false) {}
+        is_prohibited(false),
+        delay(base::TimeDelta()) {

[alexilin, 2017/04/21 13:49:05]

This is not needed. base::TimeDelta isn't build-in type.

[trevordixon, 2017/04/25 12:46:09]

Deleted.

+    request.before_first_contentful_paint = true;
+  }
 
   ResourcePrefetchPredictor::URLRequestSummary request;
   // Allows to update HTTP ETag.
@@ -99,6 +105,7 @@ struct ResourceSummary {
   // A request with |is_prohibited| set to true makes the test that originates
   // the request fail.
   bool is_prohibited;
+  base::TimeDelta delay;
 };
 
 struct RedirectEdge {
@@ -169,9 +176,12 @@ void SetValidNavigationID(NavigationID* navigation_id) {
 }
 
 void ModifySubresourceForComparison(URLRequestSummary* subresource,
-                                    bool match_navigation_id) {
+                                    bool match_navigation_id,
+                                    bool match_before_first_contentful_paint) {
   if (!match_navigation_id)
     SetValidNavigationID(&subresource->navigation_id);
+  if (!match_before_first_contentful_paint)
+    subresource->before_first_contentful_paint = true;
   if (subresource->resource_type == content::RESOURCE_TYPE_IMAGE &&
       subresource->priority == net::LOWEST) {
     // Fuzzy comparison for images because an image priority can be
@@ -185,15 +195,18 @@ void ModifySubresourceForComparison(URLRequestSummary* subresource,
 // and fail the test if the expectation is not met.
 void CompareSubresources(std::vector<URLRequestSummary> actual_subresources,
                          std::vector<URLRequestSummary> expected_subresources,
-                         bool match_navigation_id) {
+                         bool match_navigation_id,
+                         bool match_before_first_contentful_paint) {
   // Duplicate resources can be observed in a single navigation but
   // ResourcePrefetchPredictor only cares about the first occurrence of each.
   RemoveDuplicateSubresources(&actual_subresources);
 
   for (auto& subresource : actual_subresources)
-    ModifySubresourceForComparison(&subresource, match_navigation_id);
+    ModifySubresourceForComparison(&subresource, match_navigation_id,
+                                   match_before_first_contentful_paint);
   for (auto& subresource : expected_subresources)
-    ModifySubresourceForComparison(&subresource, match_navigation_id);
+    ModifySubresourceForComparison(&subresource, match_navigation_id,
+                                   match_before_first_contentful_paint);
 
   EXPECT_THAT(actual_subresources,
               testing::UnorderedElementsAreArray(expected_subresources));
@@ -233,11 +246,14 @@ class LearningObserver : public TestObserver {
   LearningObserver(ResourcePrefetchPredictor* predictor,
                    const size_t expected_url_visit_count,
                    const PageRequestSummary& expected_summary,
-                   bool match_navigation_id)
+                   bool match_navigation_id,
+                   bool match_before_first_contentful_paint)
       : TestObserver(predictor),
         url_visit_count_(expected_url_visit_count),
         summary_(expected_summary),
-        match_navigation_id_(match_navigation_id) {}
+        match_navigation_id_(match_navigation_id),
+        match_before_first_contentful_paint_(
+            match_before_first_contentful_paint) {}
 
   // TestObserver:
   void OnNavigationLearned(size_t url_visit_count,
@@ -248,7 +264,8 @@ class LearningObserver : public TestObserver {
     for (const auto& resource : summary.subresource_requests)
       current_navigation_ids_.insert(resource.navigation_id);
     CompareSubresources(summary.subresource_requests,
-                        summary_.subresource_requests, match_navigation_id_);
+                        summary_.subresource_requests, match_navigation_id_,
+                        match_before_first_contentful_paint_);
     run_loop_.Quit();
   }
 
@@ -263,6 +280,7 @@ class LearningObserver : public TestObserver {
   size_t url_visit_count_;
   PageRequestSummary summary_;
   bool match_navigation_id_;
+  bool match_before_first_contentful_paint_;
   std::set<NavigationID> current_navigation_ids_;
 
   DISALLOW_COPY_AND_ASSIGN(LearningObserver);
@@ -360,12 +378,18 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     ResourcePrefetchPredictor::SetAllowPortInUrlsForTesting(false);
   }
 
-  void TestLearningAndPrefetching(const GURL& main_frame_url) {
+  void TestLearningAndPrefetching(
+      const GURL& main_frame_url,
+      bool match_before_first_contentful_paint = false) {

[alexilin, 2017/04/21 13:49:05]

Maybe as a global/class member to avoid this plumbing?

[trevordixon, 2017/04/25 20:23:55]

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?

[alexilin, 2017/04/26 09:25:01]

Fair enough! sgtm. 

     // Navigate to |main_frame_url| and check all the expectations.
-    NavigateToURLAndCheckSubresources(main_frame_url);
+    NavigateToURLAndCheckSubresources(main_frame_url,
+                                      WindowOpenDisposition::CURRENT_TAB,
+                                      match_before_first_contentful_paint);
     ClearCache();
     // It is needed to have at least two resource hits to trigger prefetch.
-    NavigateToURLAndCheckSubresources(main_frame_url);
+    NavigateToURLAndCheckSubresources(main_frame_url,
+                                      WindowOpenDisposition::CURRENT_TAB,
+                                      match_before_first_contentful_paint);
     ClearCache();
     // Prefetch all needed resources and change expectations so that all
     // cacheable resources should be served from cache next navigation.
@@ -389,7 +413,8 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
 
   void NavigateToURLAndCheckSubresources(
       const GURL& navigation_url,
-      WindowOpenDisposition disposition = WindowOpenDisposition::CURRENT_TAB) {
+      WindowOpenDisposition disposition = WindowOpenDisposition::CURRENT_TAB,
+      bool match_before_first_contentful_paint = false) {
     GURL initial_url = GetLastClientSideRedirectEndpoint(navigation_url);
     GURL main_frame_url = GetRedirectEndpoint(navigation_url);
     std::vector<URLRequestSummary> url_request_summaries;
@@ -407,7 +432,7 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
         predictor_, UpdateAndGetVisitCount(initial_url),
         CreatePageRequestSummary(main_frame_url.spec(), initial_url.spec(),
                                  url_request_summaries),
-        match_navigation_id);
+        match_navigation_id, match_before_first_contentful_paint);
     ui_test_utils::NavigateToURLWithDisposition(
         browser(), navigation_url, disposition,
         ui_test_utils::BROWSER_TEST_NONE);
@@ -672,6 +697,10 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     // 0kB.
     http_response->set_content(std::string(1024, ' '));
 
+    if (!summary.delay.is_zero()) {
+      base::PlatformThread::Sleep(summary.delay);
+    }
+
     return std::move(http_response);
   }
 
@@ -742,6 +771,24 @@ IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, Simple) {
       internal::kResourcePrefetchPredictorPrefetchHitsSize, 4, 1);
 }
 
+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
+                       SubresourceFcpOrder) {
+  AddResource(GetURL(kStylePath), content::RESOURCE_TYPE_STYLESHEET,
+              net::HIGHEST);
+  AddResource(GetURL(kScriptPath), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
+
+  ResourceSummary* long_script = AddResource(
+      GetURL(kScriptLongPath), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
+  long_script->delay = base::TimeDelta::FromMilliseconds(1500);

[alexilin, 2017/04/21 13:49:05]

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. 

[trevordixon, 2017/04/25 12:46:10]

Is 1500 milliseconds a good guess? Might as well go longer?

[alexilin, 2017/04/25 18:42:21]

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.

[trevordixon, 2017/04/25 20:23:55]

Added a comment.

+  long_script->request.before_first_contentful_paint = false;
+
+  ResourceSummary* image = AddResource(
+      GetURL(kImageRealPath), content::RESOURCE_TYPE_IMAGE, net::LOWEST);

[alexilin, 2017/04/21 13:49:05]

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.

[trevordixon, 2017/04/25 12:46:10]

I needed a real image for another reason, but that reason went away, so the
empty response image should work fine.

+  image->request.before_first_contentful_paint = false;
+
+  TestLearningAndPrefetching(GetURL(kHtmlFcpOrderPath), true);
+}
+
 IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, Redirect) {
   GURL initial_url = GetURLWithHost(kFooHost, kRedirectPath);
   GURL redirected_url =