predictors: Mark before_first_contentful_paint for resources fetched before fcp.

chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <set>
 
 #include "base/command_line.h"
@@ skipping 36 matching lines @@
 const char kJavascriptMime[] = "application/javascript";
 
 // Paths to resources handled by a custom request handler. They return empty
 // responses with controllable response headers.
 const char kImagePath[] = "/handled-by-test/image.png";
 const char kImagePath2[] = "/handled-by-test/image2.png";
 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";
 const char kRedirectPath3[] = "/handled-by-test/redirect3.html";
 
 // These are loaded from a file by the test server.
 const char kHtmlSubresourcesPath[] = "/predictors/html_subresources.html";
 const char kHtmlDocumentWritePath[] = "/predictors/document_write.html";
 const char kScriptDocumentWritePath[] = "/predictors/document_write.js";
 const char kHtmlAppendChildPath[] = "/predictors/append_child.html";
 const char kScriptAppendChildPath[] = "/predictors/append_child.js";
 const char kHtmlInnerHtmlPath[] = "/predictors/inner_html.html";
 const char kScriptInnerHtmlPath[] = "/predictors/inner_html.js";
 const char kHtmlXHRPath[] = "/predictors/xhr.html";
 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};
 const char* kImage[2] = {kBarHost, kImagePath};
 const char* kFont[2] = {kFooHost, kFontPath};
 const char* kStyle[2] = {kBazHost, kStylePath};
 
 struct ResourceSummary {
   ResourceSummary()
       : version(0),
         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.
   size_t version;
   // True iff "Cache-control: no-store" header is present.
   bool is_no_store;
   // True iff a request for this resource must be ignored by the custom handler.
   bool is_external;
   // True iff the LearningObserver must observe this resource.
   bool is_observable;
   // A request with |is_prohibited| set to true makes the test that originates
   // the request fail.
   bool is_prohibited;
+  base::TimeDelta delay;
 };
 
 struct RedirectEdge {
   // This response code should be returned by previous url in the chain.
   net::HttpStatusCode code;
   GURL url;
   bool is_client_side;
 };
 
 // Helper class to track and allow waiting for ResourcePrefetchPredictor
@@ skipping 50 matching lines @@
 }
 
 // Fill a NavigationID with "empty" data that does not trigger
 // the is_valid DCHECK(). Allows comparing.
 void SetValidNavigationID(NavigationID* navigation_id) {
   navigation_id->tab_id = 0;
   navigation_id->main_frame_url = GURL("http://127.0.0.1");
 }
 
 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
     // boosted during layout via
     // ResourceFetcher::updateAllImageResourcePriorities().
     subresource->priority = net::MEDIUM;
   }
 }
 
 // Does a custom comparison of subresources of URLRequestSummary
 // 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));
 }
 
 std::string CreateVersionedETag(size_t version, const std::string& path) {
   return base::StringPrintf("'%zu%s'", version, path.c_str());
 }
 
 GURL GetRequestURL(const net::test_server::HttpRequest& request) {
@@ skipping 19 matching lines @@
 // Helper class to track and allow waiting for a single OnNavigationLearned
 // event. The information provided by this event is also used to verify that
 // ResourcePrefetchPredictor works as expected.
 class LearningObserver : public TestObserver {
  public:
   using PageRequestSummary = ResourcePrefetchPredictor::PageRequestSummary;
 
   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,
                            const PageRequestSummary& summary) override {
     EXPECT_EQ(url_visit_count, url_visit_count_);
     EXPECT_EQ(summary.main_frame_url, summary_.main_frame_url);
     EXPECT_EQ(summary.initial_url, summary_.initial_url);
     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();
   }
 
   void Wait() { run_loop_.Run(); }
 
   std::set<NavigationID>& current_navigation_ids() {
     return current_navigation_ids_;
   }
 
  private:
   base::RunLoop run_loop_;
   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);
 };
 
 // Helper class to track and allow waiting for a single OnPrefetchingFinished
 // event. Checks also that {Start,Stop}Prefetching are called with the right
 // argument.
 class PrefetchingObserver : public TestObserver {
  public:
@@ skipping 77 matching lines @@
     // URLs from the test server contain a port number.
     ResourcePrefetchPredictor::SetAllowPortInUrlsForTesting(true);
     EnsurePredictorInitialized();
     histogram_tester_.reset(new base::HistogramTester());
   }
 
   void TearDownOnMainThread() override {
     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.
     PrefetchURL(main_frame_url);
     // To be sure that the browser send no requests to the server after
     // prefetching.
     NavigateToURLAndCheckSubresourcesAllCached(main_frame_url);

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

You could pass match_before_first_contentful_paint here as well.

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

All resources are before_first_contentful_paint after PrefetchURL, so at this
point, we never want to match_before_first_contentful_paint.

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

Yes, you're right. It'd be better to not race with cache response time.

   }
 
   void NavigateToURLAndCheckSubresourcesAllCached(const GURL& navigation_url) {
     for (auto& kv : resources_) {
       if (kv.second.is_observable)
         kv.second.is_prohibited = true;
     }
     NavigateToURLAndCheckSubresources(navigation_url);
     for (auto& kv : resources_) {
       if (kv.second.is_observable)
         kv.second.is_prohibited = false;
     }
   }
 
   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;
     for (const auto& kv : resources_) {
       if (kv.second.is_observable) {
         url_request_summaries.push_back(
             GetURLRequestSummaryForResource(main_frame_url, kv.second));
       }
     }
 
     bool match_navigation_id =
         disposition == WindowOpenDisposition::CURRENT_TAB;
 
     LearningObserver observer(
         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);
     observer.Wait();
 
     for (auto& kv : resources_) {
       if (kv.second.is_observable)
         kv.second.request.was_cached = true;
     }
     for (const auto& nav : observer.current_navigation_ids())
@@ skipping 244 matching lines @@
     }
     if (summary.request.always_revalidate)
       http_response->AddCustomHeader("Cache-Control", "no-cache");
     else
       http_response->AddCustomHeader("Cache-Control", "max-age=2147483648");
 
     // Add some content, otherwise the prefetch size histogram rounds down to
     // 0kB.
     http_response->set_content(std::string(1024, ' '));
 
+    if (!summary.delay.is_zero()) {
+      base::PlatformThread::Sleep(summary.delay);
+    }
+
     return std::move(http_response);
   }
 
   // The custom handler for redirect requests from the browser to an
   // EmbeddedTestServer. Running on the EmbeddedTestServer IO thread.
   // Finds the data to serve requests in |redirects_| map keyed by a request
   // URL.
   std::unique_ptr<net::test_server::HttpResponse> HandleRedirectRequest(
       const net::test_server::HttpRequest& request) const {
     std::map<GURL, RedirectEdge>::const_iterator redirect_it =
@@ skipping 50 matching lines @@
   histogram_tester_->ExpectBucketCount(
       internal::kResourcePrefetchPredictorPrefetchHitsCountNotCached, 4, 1);
 
   histogram_tester_->ExpectBucketCount(
       internal::kResourcePrefetchPredictorPrefetchMissesSize, 0, 1);
   // Each request is ~1k, see HandleResourceRequest() above.
   histogram_tester_->ExpectBucketCount(
       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 =
       GetPageURLWithReplacements(kBarHost, kHtmlSubresourcesPath);
   AddRedirectChain(initial_url,
                    {{net::HTTP_MOVED_PERMANENTLY, redirected_url}});
   AddResourcesFromSubresourceHtml();
   TestLearningAndPrefetching(initial_url);
 }
 
@@ skipping 207 matching lines @@
   AddResourcesFromSubresourceHtml();
 
   NavigateToURLAndCheckSubresources(initial_url);
   ClearCache();
   NavigateToURLAndCheckSubresources(initial_url);
   ClearCache();
   NavigateToURLAndCheckPrefetching(initial_url);
 }
 
 }  // namespace predictors