predictors: ResourcePrefetchPredictorBrowserTest improvements.

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 7b292b3557914a5c40237583016d77edd2ec23fa..f539c21b0985fc27c76d051bda5b6be848b5a54c 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
@@ -57,17 +57,24 @@ const char kHtmlIframePath[] = "/predictors/html_iframe.html";
 
 struct ResourceSummary {
   ResourceSummary()
-      : is_no_store(false),
-        version(0),
+      : version(0),
+        is_no_store(false),
         is_external(false),
-        should_be_recorded(true) {}
+        is_observable(true),
+        is_request_prohibited(false) {}
 
   ResourcePrefetchPredictor::URLRequestSummary request;
   std::string content;
-  bool is_no_store;
+  // 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;
-  bool should_be_recorded;
+  // True iff the LearningObserver must observe this resource.
+  bool is_observable;
+  // True iff a test must fail on request to this resource.

[pasko, 2016/12/26 14:12:16]

nit: since we are in ResourceSummary, we could remove some redundancy with
s/is_request_prohibited/is_prohibited/. Up to you.

[pasko, 2016/12/26 14:12:17]

I'd rephrase slightly (because a test can fail for other reasons):
// A request with |is_request_prohibited| set to true makes the test (that
originates the request) fail.

[alexilin, 2016/12/26 17:33:22]

Done.

+  bool is_request_prohibited;
 };
 
 struct RedirectEdge {
@@ -168,6 +175,10 @@ void CompareSubresources(std::vector<URLRequestSummary> actual_subresources,
               testing::UnorderedElementsAreArray(expected_subresources));
 }
 
+std::string GetVersionnedETag(size_t version, const std::string& path) {

[pasko, 2016/12/26 14:12:17]

s/GetVersionned/MakeVersioned/

(we are not looking up the tag anywhere, so I think 'make' is more applicable)

[alexilin, 2016/12/26 17:33:22]

I'd prefer to name it CreateVersionnedETag since it more consistent with other
helper functions in resource_prefetch_predictor_test_utils.

[pasko, 2016/12/26 17:56:49]

sgtm on the "Create" part, but s/Versionned/Versioned/

[alexilin, 2016/12/26 18:27:07]

Done.

+  return base::StringPrintf("'%zu%s'", version, path.c_str());
+}
+
 }  // namespace
 
 // Helper class to track and allow waiting for a single OnNavigationLearned
@@ -253,6 +264,9 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     embedded_test_server()->RegisterRequestHandler(
         base::Bind(&ResourcePrefetchPredictorBrowserTest::HandleResourceRequest,
                    base::Unretained(this)));
+    embedded_test_server()->RegisterRequestMonitor(base::Bind(
+        &ResourcePrefetchPredictorBrowserTest::MonitorResourceRequest,
+        base::Unretained(this)));
     ASSERT_TRUE(embedded_test_server()->Start());
     predictor_ =
         ResourcePrefetchPredictorFactory::GetForProfile(browser()->profile());
@@ -270,7 +284,17 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     // 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.
+    for (auto& kv : resources_) {
+      if (kv.second.is_observable)
+        kv.second.is_request_prohibited = true;
+    }
     NavigateToURLAndCheckSubresources(main_frame_url);
+    for (auto& kv : resources_) {
+      if (kv.second.is_observable)
+        kv.second.is_request_prohibited = false;

[pasko, 2016/12/26 14:12:17]

why should this be returned back to false? Are we doing more than one
TestLearningAndPrefetching() in a single test?

[alexilin, 2016/12/26 17:33:22]

Potentially yes. For example, we already have CrossSiteNavigation test but it
cleans resources between two function calls so it doesn't count.
I just think that it'd be nice to return the system to the initial state after
we've done here.

[pasko, 2016/12/26 17:56:49]

OK, makes sense, agreed.

+    }
   }
 
   void NavigateToURLAndCheckSubresources(
@@ -279,22 +303,26 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     GURL endpoint_url = GetRedirectEndpoint(main_frame_url);
     std::vector<URLRequestSummary> url_request_summaries;
     for (const auto& kv : resources_) {
-      if (kv.second.is_no_store || !kv.second.should_be_recorded)
-        continue;
-      url_request_summaries.push_back(
-          GetURLRequestSummaryForResource(endpoint_url, kv.second));
+      if (kv.second.is_observable) {
+        url_request_summaries.push_back(
+            GetURLRequestSummaryForResource(endpoint_url, kv.second));
+      }
     }
+
+    bool match_navigation_id =

[pasko, 2016/12/26 14:12:17]

this part is already committed in crrev.com/62729f8a, please rebase

[alexilin, 2016/12/26 17:33:23]

Done.

+        disposition == WindowOpenDisposition::CURRENT_TAB;
+
     LearningObserver observer(
         predictor_, UpdateAndGetVisitCount(main_frame_url),
         CreatePageRequestSummary(endpoint_url.spec(), main_frame_url.spec(),
                                  url_request_summaries),
-        true);  // Matching navigation id by default
+        match_navigation_id);
     ui_test_utils::NavigateToURLWithDisposition(
         browser(), main_frame_url, disposition,
         ui_test_utils::BROWSER_TEST_NONE);
     observer.Wait();
     for (auto& kv : resources_) {
-      if (!kv.second.is_no_store && kv.second.should_be_recorded)
+      if (kv.second.is_observable)
         kv.second.request.was_cached = true;
     }
   }
@@ -304,7 +332,7 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     predictor_->StartPrefetching(main_frame_url, PrefetchOrigin::EXTERNAL);
     observer.Wait();
     for (auto& kv : resources_) {
-      if (!kv.second.is_no_store && kv.second.should_be_recorded)
+      if (kv.second.is_observable)
         kv.second.request.was_cached = true;
     }
   }
@@ -332,12 +360,12 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     return resource;
   }
 
-  void AddUnrecordedResources(const std::vector<GURL>& resource_urls) {
+  void AddUnobservableResources(const std::vector<GURL>& resource_urls) {
     for (const GURL& resource_url : resource_urls) {
       auto resource =
           AddResource(resource_url, content::RESOURCE_TYPE_SUB_RESOURCE,
                       net::DEFAULT_PRIORITY);
-      resource->should_be_recorded = false;
+      resource->is_observable = false;
     }
   }
 
@@ -382,6 +410,9 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     https_server()->RegisterRequestHandler(
         base::Bind(&ResourcePrefetchPredictorBrowserTest::HandleResourceRequest,
                    base::Unretained(this)));
+    https_server()->RegisterRequestMonitor(base::Bind(
+        &ResourcePrefetchPredictorBrowserTest::MonitorResourceRequest,
+        base::Unretained(this)));
     ASSERT_TRUE(https_server()->Start());
   }
 
@@ -434,6 +465,18 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     return current;
   }
 
+  void MonitorResourceRequest(
+      const net::test_server::HttpRequest& request) const {
+    auto resource_it = resources_.find(request.GetURL());

[pasko, 2016/12/26 14:12:17]

since the type for |resources_| is declared elsewhere, it is not obvious what
the type of |resource_it| is. Please declare the type explicitly.

[alexilin, 2016/12/26 17:33:23]

It is debatable whether "std::map<X, Y>::const_iterator" is more readable than
"auto" with a proper variable name. It's a little bit better with typedef for
the map type.

I also fixed other similar cases in this file.

[pasko, 2016/12/26 17:56:49]

Arguably finding the definition of ResourceMap is extra work when reading, and
it is somewhere in the middle of the file. The style guide recommends declaring
typedefs/using near the top of the 'private' section:
https://google.github.io/styleguide/cppguide.html#Declaration_Order

ResourceMap is too ambiguous, I would agree with GurlToResourceSummaryMap, if
put in the right order.

[alexilin, 2016/12/26 18:27:08]

GurlToResourceSummaryMap1234567
std::map<GURL, ResourceSummary>

7 chars win. I'd prefer to leave full type definition then.
Done.

+    if (resource_it == resources_.end())
+      return;
+
+    const ResourceSummary& summary = resource_it->second;
+    EXPECT_FALSE(summary.is_request_prohibited) << request.GetURL().spec()
+                                                << "\n"
+                                                << request.all_headers;
+  }
+
   std::unique_ptr<net::test_server::HttpResponse> HandleResourceRequest(

[pasko, 2016/12/26 14:12:17]

I think it is time for a brief comment of how requests are handled:
1. where it takes the data to serve requests
2. what parts of |request| it uses
3. mention that it always responds with HTTP 304 on validation requests

what else would be useful?

[alexilin, 2016/12/26 17:33:23]

Done.
+ added remark about the execution thread.
I think it's too verbose to describe how each field of ResourceSummary is used
to form a response.

[pasko, 2016/12/26 17:56:49]

Thanks!

       const net::test_server::HttpRequest& request) const {
     auto resource_it = resources_.find(request.GetURL());
@@ -446,7 +489,15 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
 
     auto http_response =
         base::MakeUnique<net::test_server::BasicHttpResponse>();
-    http_response->set_code(net::HTTP_OK);
+
+    if (request.headers.find("If-None-Match") != request.headers.end() &&
+        request.headers.at("If-None-Match") ==
+            GetVersionnedETag(summary.version, request.relative_url)) {
+      http_response->set_code(net::HTTP_NOT_MODIFIED);
+    } else {
+      http_response->set_code(net::HTTP_OK);
+    }
+
     if (!summary.request.mime_type.empty())
       http_response->set_content_type(summary.request.mime_type);
     if (!summary.content.empty())
@@ -455,8 +506,7 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
       http_response->AddCustomHeader("Cache-Control", "no-store");
     if (summary.request.has_validators) {
       http_response->AddCustomHeader(
-          "ETag", base::StringPrintf("'%zu%s'", summary.version,
-                                     request.relative_url.c_str()));
+          "ETag", GetVersionnedETag(summary.version, request.relative_url));
     }
     if (summary.request.always_revalidate)
       http_response->AddCustomHeader("Cache-Control", "no-cache");
@@ -583,7 +633,7 @@ IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
               net::HIGHEST);
   // https://www.w3.org/TR/2014/REC-html5-20141028/scripting-1.html#the-script-element
   // Script elements don't execute when inserted using innerHTML attribute.
-  AddUnrecordedResources({GetURL(kScriptPath)});
+  AddUnobservableResources({GetURL(kScriptPath)});
   TestLearningAndPrefetching(GetURL(kHtmlInnerHtmlPath));
 }
 
@@ -613,10 +663,27 @@ IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
   AddResource(GetURL(kStylePath2), content::RESOURCE_TYPE_STYLESHEET,
               net::HIGHEST);
   AddResource(GetURL(kScriptPath2), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
-  // Included from <iframe src="html_subresources.html"> and not recored.
-  AddUnrecordedResources({GetURL(kImagePath), GetURL(kStylePath),
-                          GetURL(kScriptPath), GetURL(kFontPath)});
+  // Included from <iframe src="html_subresources.html"> and shouldn't be
+  // observed.
+  AddUnobservableResources({GetURL(kImagePath), GetURL(kStylePath),
+                            GetURL(kScriptPath), GetURL(kFontPath)});
   TestLearningAndPrefetching(GetURL(kHtmlIframePath));
 }
 
+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, AlwaysRevalidate) {
+  std::vector<ResourceSummary*> resources = {
+      AddResource(GetURL(kImagePath), content::RESOURCE_TYPE_IMAGE,
+                  net::LOWEST),
+      AddResource(GetURL(kStylePath), content::RESOURCE_TYPE_STYLESHEET,
+                  net::HIGHEST),
+      AddResource(GetURL(kScriptPath), content::RESOURCE_TYPE_SCRIPT,
+                  net::MEDIUM),
+      AddResource(GetURL(kFontPath), content::RESOURCE_TYPE_FONT_RESOURCE,
+                  net::HIGHEST),
+  };
+  for (auto& resource : resources)
+    resource->request.always_revalidate = true;
+  TestLearningAndPrefetching(GetURL(kHtmlSubresourcesPath));
+}
+
 }  // namespace predictors