predictors: ResourcePrefetchPredictorBrowserTest improvements.

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 "base/command_line.h"
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
 #include "chrome/browser/browsing_data/browsing_data_remover.h"
 #include "chrome/browser/browsing_data/browsing_data_remover_factory.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor_factory.h"
@@ skipping 39 matching lines @@
 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";
 
 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;
+  // Allows to update HTTP ETag.
+  size_t version;
+  // True iff "Cache-control: no-store" header is present.
   bool is_no_store;
-  size_t version;
+  // 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 {
   // This response code should be returned by previous url in the chain.
   net::HttpStatusCode code;
   GURL url;
 };
 
 // Helper class to track and allow waiting for ResourcePrefetchPredictor
 // initialization. WARNING: OnPredictorInitialized event will not be fired if
@@ skipping 80 matching lines @@
 
   for (auto& subresource : actual_subresources)
     ModifySubresourceForComparison(&subresource, match_navigation_id);
   for (auto& subresource : expected_subresources)
     ModifySubresourceForComparison(&subresource, match_navigation_id);
 
   EXPECT_THAT(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
 // 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,
@@ skipping 65 matching lines @@
         switches::kSpeculativeResourcePrefetchingEnabledExternal);
   }
 
   void SetUpOnMainThread() override {
     embedded_test_server()->RegisterRequestHandler(
         base::Bind(&ResourcePrefetchPredictorBrowserTest::HandleRedirectRequest,
                    base::Unretained(this)));
     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());
     ASSERT_TRUE(predictor_);
     EnsurePredictorInitialized();
   }
 
   void TestLearningAndPrefetching(const GURL& main_frame_url) {
     // Navigate to |main_frame_url| and check all the expectations.
     NavigateToURLAndCheckSubresources(main_frame_url);
     ClearCache();
     // It is needed to have at least two resource hits to trigger prefetch.
     NavigateToURLAndCheckSubresources(main_frame_url);
     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.
+    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(
       const GURL& main_frame_url,
       WindowOpenDisposition disposition = WindowOpenDisposition::CURRENT_TAB) {
     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;
     }
   }
 
   void PrefetchURL(const GURL& main_frame_url) {
     PrefetchingObserver observer(predictor_, main_frame_url);
     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;
     }
   }
 
   ResourceSummary* AddResource(const GURL& resource_url,
                                content::ResourceType resource_type,
                                net::RequestPriority priority) {
     auto pair_and_whether_inserted =
         resources_.insert(std::make_pair(resource_url, ResourceSummary()));
     EXPECT_TRUE(pair_and_whether_inserted.second) << resource_url
                                                   << " was inserted twice";
     ResourceSummary* resource = &pair_and_whether_inserted.first->second;
     resource->request.resource_url = resource_url;
     resource->request.resource_type = resource_type;
     resource->request.priority = priority;
     resource->request.has_validators = true;
     return resource;
   }
 
   ResourceSummary* AddExternalResource(const GURL& resource_url,
                                        content::ResourceType resource_type,
                                        net::RequestPriority priority) {
     auto resource = AddResource(resource_url, resource_type, priority);
     resource->is_external = true;
     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;
     }
   }
 
   void AddRedirectChain(const GURL& initial_url,
                         const std::vector<RedirectEdge>& redirect_chain) {
     ASSERT_FALSE(redirect_chain.empty());
     GURL current = initial_url;
     for (const auto& edge : redirect_chain) {
       auto result = redirects_.insert(std::make_pair(current, edge));
       EXPECT_TRUE(result.second) << current << " already has a redirect.";
@@ skipping 24 matching lines @@
     https_server_ = base::MakeUnique<net::EmbeddedTestServer>(
         net::EmbeddedTestServer::TYPE_HTTPS);
     https_server()->AddDefaultHandlers(
         base::FilePath(FILE_PATH_LITERAL("chrome/test/data")));
     https_server()->RegisterRequestHandler(
         base::Bind(&ResourcePrefetchPredictorBrowserTest::HandleRedirectRequest,
                    base::Unretained(this)));
     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());
   }
 
   // Returns the embedded test server working over HTTPS. Must be enabled by
   // calling EnableHttpsServer() before use.
   const net::EmbeddedTestServer* https_server() const {
     return https_server_.get();
   }
 
   net::EmbeddedTestServer* https_server() { return https_server_.get(); }
@@ skipping 32 matching lines @@
     GURL current = initial_url;
     while (true) {
       auto it = redirects_.find(current);
       if (it == redirects_.end())
         break;
       current = it->second.url;
     }
     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());
     if (resource_it == resources_.end())
       return nullptr;
 
     const ResourceSummary& summary = resource_it->second;
     if (summary.is_external)
       return nullptr;
 
     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())
       http_response->set_content(summary.content);
     if (summary.is_no_store)
       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");
     else
       http_response->AddCustomHeader("Cache-Control", "max-age=2147483648");
     return std::move(http_response);
   }
 
   std::unique_ptr<net::test_server::HttpResponse> HandleRedirectRequest(
       const net::test_server::HttpRequest& request) const {
@@ skipping 106 matching lines @@
 IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
                        JavascriptInnerHtml) {
   auto externalScript = AddExternalResource(
       GetURL(kScriptInnerHtmlPath), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
   externalScript->request.mime_type = kJavascriptMime;
   AddResource(GetURL(kImagePath), content::RESOURCE_TYPE_IMAGE, net::LOWEST);
   AddResource(GetURL(kStylePath), content::RESOURCE_TYPE_STYLESHEET,
               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));
 }
 
 // Requests originated by XMLHttpRequest have content::RESOURCE_TYPE_XHR.
 // Actual resource type is inferred from the mime-type.
 IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, JavascriptXHR) {
   auto externalScript = AddExternalResource(
       GetURL(kScriptXHRPath), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
   externalScript->request.mime_type = kJavascriptMime;
   auto image = AddResource(GetURL(kImagePath), content::RESOURCE_TYPE_IMAGE,
                            net::HIGHEST);
   image->request.mime_type = kImageMime;
   auto style = AddResource(GetURL(kStylePath),
                            content::RESOURCE_TYPE_STYLESHEET, net::HIGHEST);
   style->request.mime_type = kStyleMime;
   auto script = AddResource(GetURL(kScriptPath), content::RESOURCE_TYPE_SCRIPT,
                             net::HIGHEST);
   script->request.mime_type = kJavascriptMime;
   TestLearningAndPrefetching(GetURL(kHtmlXHRPath));
 }
 
 // ResourcePrefetchPredictor ignores all resources requested from subframes.
 IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
                        IframeShouldBeIgnored) {
   // Included from html_iframe.html.
   AddResource(GetURL(kImagePath2), content::RESOURCE_TYPE_IMAGE, net::LOWEST);
   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