Create LoadingDataCollector class and have observers rely on it instead of ResourcePrefetchPredictor

chrome/browser/predictors/loading_data_collector_unittest.cc

+// Copyright 2014 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 "chrome/browser/predictors/loading_data_collector.h"
+
+#include <iostream>
+#include <memory>
+#include <utility>
+
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/run_loop.h"
+#include "base/test/histogram_tester.h"
+#include "base/time/time.h"
+#include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/predictors/resource_prefetch_predictor_tables.h"

[Benoit L, 2017/06/02 12:43:59]

Is this include needed?

Please run "git cl lint", although I'm not sure it would point out unnecessary
includes.

[trevordixon, 2017/06/06 13:08:03]

I think I got rid of all the unnecessary ones.

+#include "chrome/browser/predictors/resource_prefetch_predictor_test_util.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/history/core/browser/history_service.h"
+#include "components/history/core/browser/history_types.h"
+#include "components/sessions/core/session_id.h"
+#include "content/public/browser/resource_request_info.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "net/http/http_response_headers.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_job.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::StrictMock;
+
+namespace predictors {
+
+using URLRequestSummary = ResourcePrefetchPredictor::URLRequestSummary;
+
+class EmptyURLRequestDelegate : public net::URLRequest::Delegate {
+  void OnResponseStarted(net::URLRequest* request, int net_error) override {}
+  void OnReadCompleted(net::URLRequest* request, int bytes_read) override {}
+};
+
+class LoadingDataCollectorTest : public testing::Test {
+ public:
+  LoadingDataCollectorTest();
+  ~LoadingDataCollectorTest() override;
+  void SetUp() override;
+  void TearDown() override;
+
+ protected:
+  std::unique_ptr<net::URLRequest> CreateURLRequest(

[Benoit L, 2017/06/02 12:43:59]

See comment below, is it possible to avoid duplicating this function?

+      const GURL& url,
+      net::RequestPriority priority,
+      content::ResourceType resource_type,
+      bool is_main_frame) {
+    std::unique_ptr<net::URLRequest> request =
+        url_request_context_.CreateRequest(url, priority,
+                                           &url_request_delegate_);
+    request->set_first_party_for_cookies(url);
+    content::ResourceRequestInfo::AllocateForTesting(
+        request.get(), resource_type, nullptr, -1, -1, -1, is_main_frame, false,
+        false, true, content::PREVIEWS_OFF);
+    request->Start();
+    return request;
+  }
+
+  void InitializePredictor() {
+    predictor_->StartInitialization();
+    base::RunLoop loop;
+    loop.RunUntilIdle();  // Runs the DB lookup.
+    profile_->BlockUntilHistoryProcessesPendingRequests();
+  }
+
+  void ResetPredictor() {
+    LoadingPredictorConfig config;

[Benoit L, 2017/06/02 12:43:59]

There is now PopulateTestConfig() to replace this code.

[trevordixon, 2017/06/06 13:08:04]

Done.

+    config.max_urls_to_track = 3;
+    config.max_hosts_to_track = 2;
+    config.min_url_visit_count = 2;
+    config.max_resources_per_entry = 4;
+    config.max_consecutive_misses = 2;
+    config.max_redirect_consecutive_misses = 2;
+    config.min_resource_confidence_to_trigger_prefetch = 0.5;
+    config.is_url_learning_enabled = true;
+    config.is_manifests_enabled = true;
+    config.is_origin_learning_enabled = true;
+
+    config.mode |= LoadingPredictorConfig::LEARNING;
+    predictor_.reset(new ResourcePrefetchPredictor(config, profile_.get()));

[Benoit L, 2017/06/02 12:43:59]

nit:

predictor_ = base::MakeUnique<>()...

[trevordixon, 2017/06/06 13:08:04]

Done.

+  }
+
+  content::TestBrowserThreadBundle thread_bundle_;
+  std::unique_ptr<TestingProfile> profile_;
+  net::TestURLRequestContext url_request_context_;
+
+  std::unique_ptr<ResourcePrefetchPredictor> predictor_;
+
+  MockURLRequestJobFactory url_request_job_factory_;
+  EmptyURLRequestDelegate url_request_delegate_;
+
+  std::unique_ptr<base::HistogramTester> histogram_tester_;
+};
+
+LoadingDataCollectorTest::LoadingDataCollectorTest()
+    : thread_bundle_(), profile_(new TestingProfile()) {}
+
+LoadingDataCollectorTest::~LoadingDataCollectorTest() {
+  profile_.reset(NULL);
+  base::RunLoop().RunUntilIdle();
+}
+
+void LoadingDataCollectorTest::SetUp() {
+  ASSERT_TRUE(profile_->CreateHistoryService(true, false));
+  profile_->BlockUntilHistoryProcessesPendingRequests();
+  EXPECT_TRUE(HistoryServiceFactory::GetForProfile(
+      profile_.get(), ServiceAccessType::EXPLICIT_ACCESS));
+  ResetPredictor();

[Benoit L, 2017/06/02 12:43:59]

nit: These two functions are called only once, inline?

[trevordixon, 2017/06/06 13:08:03]

Done.

+  InitializePredictor();
+
+  url_request_context_.set_job_factory(&url_request_job_factory_);
+
+  histogram_tester_.reset(new base::HistogramTester());

[Benoit L, 2017/06/02 12:43:59]

nit:

histogram_tester_ = base::MakeUnique<base::HistogramTester>();

[trevordixon, 2017/06/06 13:08:04]

Done.

+}
+
+void LoadingDataCollectorTest::TearDown() {
+  predictor_.reset(NULL);
+  profile_->DestroyHistoryService();
+}
+
+TEST_F(LoadingDataCollectorTest, HandledResourceTypes) {

[Benoit L, 2017/06/02 12:43:59]

Is it correct that all the tests below only test static functions?

If so, you don't need any of the machinery above, right?

I suspect you will eventually need some of it at least once the function
actually doing the recording are moved to LoadingDataCollector, but it seems
strange to have it without any use.

[trevordixon, 2017/06/06 13:08:04]

You're right. At one point I had the RecordFirstContentfulPaint test here, but
removing that meant I should have gotten rid of a bunch of stuff up there. 

+  EXPECT_TRUE(LoadingDataCollector::IsHandledResourceType(
+      content::RESOURCE_TYPE_STYLESHEET, "bogus/mime-type"));
+  EXPECT_TRUE(LoadingDataCollector::IsHandledResourceType(
+      content::RESOURCE_TYPE_STYLESHEET, ""));
+  EXPECT_FALSE(LoadingDataCollector::IsHandledResourceType(
+      content::RESOURCE_TYPE_WORKER, "text/css"));
+  EXPECT_FALSE(LoadingDataCollector::IsHandledResourceType(
+      content::RESOURCE_TYPE_WORKER, ""));
+  EXPECT_TRUE(LoadingDataCollector::IsHandledResourceType(
+      content::RESOURCE_TYPE_PREFETCH, "text/css"));
+  EXPECT_FALSE(LoadingDataCollector::IsHandledResourceType(
+      content::RESOURCE_TYPE_PREFETCH, "bogus/mime-type"));
+  EXPECT_FALSE(LoadingDataCollector::IsHandledResourceType(
+      content::RESOURCE_TYPE_PREFETCH, ""));
+  EXPECT_TRUE(LoadingDataCollector::IsHandledResourceType(
+      content::RESOURCE_TYPE_PREFETCH, "application/font-woff"));
+  EXPECT_TRUE(LoadingDataCollector::IsHandledResourceType(
+      content::RESOURCE_TYPE_PREFETCH, "font/woff2"));
+  EXPECT_FALSE(LoadingDataCollector::IsHandledResourceType(
+      content::RESOURCE_TYPE_XHR, ""));
+  EXPECT_FALSE(LoadingDataCollector::IsHandledResourceType(
+      content::RESOURCE_TYPE_XHR, "bogus/mime-type"));
+  EXPECT_TRUE(LoadingDataCollector::IsHandledResourceType(
+      content::RESOURCE_TYPE_XHR, "application/javascript"));
+}
+
+TEST_F(LoadingDataCollectorTest, ShouldRecordRequestMainFrame) {
+  std::unique_ptr<net::URLRequest> http_request =
+      CreateURLRequest(GURL("http://www.google.com"), net::MEDIUM,
+                       content::RESOURCE_TYPE_IMAGE, true);
+  EXPECT_TRUE(LoadingDataCollector::ShouldRecordRequest(
+      http_request.get(), content::RESOURCE_TYPE_MAIN_FRAME));
+
+  std::unique_ptr<net::URLRequest> https_request =
+      CreateURLRequest(GURL("https://www.google.com"), net::MEDIUM,
+                       content::RESOURCE_TYPE_IMAGE, true);
+  EXPECT_TRUE(LoadingDataCollector::ShouldRecordRequest(
+      https_request.get(), content::RESOURCE_TYPE_MAIN_FRAME));
+
+  std::unique_ptr<net::URLRequest> file_request =
+      CreateURLRequest(GURL("file://www.google.com"), net::MEDIUM,
+                       content::RESOURCE_TYPE_IMAGE, true);
+  EXPECT_FALSE(LoadingDataCollector::ShouldRecordRequest(
+      file_request.get(), content::RESOURCE_TYPE_MAIN_FRAME));
+
+  std::unique_ptr<net::URLRequest> https_request_with_port =
+      CreateURLRequest(GURL("https://www.google.com:666"), net::MEDIUM,
+                       content::RESOURCE_TYPE_IMAGE, true);
+  EXPECT_FALSE(LoadingDataCollector::ShouldRecordRequest(
+      https_request_with_port.get(), content::RESOURCE_TYPE_MAIN_FRAME));
+}
+
+TEST_F(LoadingDataCollectorTest, ShouldRecordRequestSubResource) {
+  std::unique_ptr<net::URLRequest> http_request =
+      CreateURLRequest(GURL("http://www.google.com/cat.png"), net::MEDIUM,
+                       content::RESOURCE_TYPE_IMAGE, false);
+  EXPECT_FALSE(LoadingDataCollector::ShouldRecordRequest(
+      http_request.get(), content::RESOURCE_TYPE_IMAGE));
+
+  std::unique_ptr<net::URLRequest> https_request =
+      CreateURLRequest(GURL("https://www.google.com/cat.png"), net::MEDIUM,
+                       content::RESOURCE_TYPE_IMAGE, false);
+  EXPECT_FALSE(LoadingDataCollector::ShouldRecordRequest(
+      https_request.get(), content::RESOURCE_TYPE_IMAGE));
+
+  std::unique_ptr<net::URLRequest> file_request =
+      CreateURLRequest(GURL("file://www.google.com/cat.png"), net::MEDIUM,
+                       content::RESOURCE_TYPE_IMAGE, false);
+  EXPECT_FALSE(LoadingDataCollector::ShouldRecordRequest(
+      file_request.get(), content::RESOURCE_TYPE_IMAGE));
+
+  std::unique_ptr<net::URLRequest> https_request_with_port =
+      CreateURLRequest(GURL("https://www.google.com:666/cat.png"), net::MEDIUM,
+                       content::RESOURCE_TYPE_IMAGE, false);
+  EXPECT_FALSE(LoadingDataCollector::ShouldRecordRequest(
+      https_request_with_port.get(), content::RESOURCE_TYPE_IMAGE));
+}
+
+TEST_F(LoadingDataCollectorTest, ShouldRecordResponseMainFrame) {
+  net::HttpResponseInfo response_info;
+  response_info.headers = MakeResponseHeaders("");
+  url_request_job_factory_.set_response_info(response_info);
+
+  std::unique_ptr<net::URLRequest> http_request =
+      CreateURLRequest(GURL("http://www.google.com"), net::MEDIUM,
+                       content::RESOURCE_TYPE_MAIN_FRAME, true);
+  EXPECT_TRUE(LoadingDataCollector::ShouldRecordResponse(http_request.get()));
+
+  std::unique_ptr<net::URLRequest> https_request =
+      CreateURLRequest(GURL("https://www.google.com"), net::MEDIUM,
+                       content::RESOURCE_TYPE_MAIN_FRAME, true);
+  EXPECT_TRUE(LoadingDataCollector::ShouldRecordResponse(https_request.get()));
+
+  std::unique_ptr<net::URLRequest> file_request =
+      CreateURLRequest(GURL("file://www.google.com"), net::MEDIUM,
+                       content::RESOURCE_TYPE_MAIN_FRAME, true);
+  EXPECT_FALSE(LoadingDataCollector::ShouldRecordResponse(file_request.get()));
+
+  std::unique_ptr<net::URLRequest> https_request_with_port =
+      CreateURLRequest(GURL("https://www.google.com:666"), net::MEDIUM,
+                       content::RESOURCE_TYPE_MAIN_FRAME, true);
+  EXPECT_FALSE(LoadingDataCollector::ShouldRecordResponse(
+      https_request_with_port.get()));
+}
+
+TEST_F(LoadingDataCollectorTest, ShouldRecordResponseSubresource) {
+  net::HttpResponseInfo response_info;
+  response_info.headers =
+      MakeResponseHeaders("HTTP/1.1 200 OK\n\nSome: Headers\n");
+  response_info.was_cached = true;
+  url_request_job_factory_.set_response_info(response_info);
+
+  // Protocol.
+  std::unique_ptr<net::URLRequest> http_image_request =
+      CreateURLRequest(GURL("http://www.google.com/cat.png"), net::MEDIUM,
+                       content::RESOURCE_TYPE_IMAGE, true);
+  EXPECT_TRUE(
+      LoadingDataCollector::ShouldRecordResponse(http_image_request.get()));
+
+  std::unique_ptr<net::URLRequest> https_image_request =
+      CreateURLRequest(GURL("https://www.google.com/cat.png"), net::MEDIUM,
+                       content::RESOURCE_TYPE_IMAGE, true);
+  EXPECT_TRUE(
+      LoadingDataCollector::ShouldRecordResponse(https_image_request.get()));
+
+  std::unique_ptr<net::URLRequest> https_image_request_with_port =
+      CreateURLRequest(GURL("https://www.google.com:666/cat.png"), net::MEDIUM,
+                       content::RESOURCE_TYPE_IMAGE, true);
+  EXPECT_FALSE(LoadingDataCollector::ShouldRecordResponse(
+      https_image_request_with_port.get()));
+
+  std::unique_ptr<net::URLRequest> file_image_request =
+      CreateURLRequest(GURL("file://www.google.com/cat.png"), net::MEDIUM,
+                       content::RESOURCE_TYPE_IMAGE, true);
+  EXPECT_FALSE(
+      LoadingDataCollector::ShouldRecordResponse(file_image_request.get()));
+
+  // ResourceType.
+  std::unique_ptr<net::URLRequest> sub_frame_request =
+      CreateURLRequest(GURL("http://www.google.com/frame.html"), net::MEDIUM,
+                       content::RESOURCE_TYPE_SUB_FRAME, true);
+  EXPECT_FALSE(
+      LoadingDataCollector::ShouldRecordResponse(sub_frame_request.get()));
+
+  std::unique_ptr<net::URLRequest> font_request =
+      CreateURLRequest(GURL("http://www.google.com/comic-sans-ms.woff"),
+                       net::MEDIUM, content::RESOURCE_TYPE_FONT_RESOURCE, true);
+  EXPECT_TRUE(LoadingDataCollector::ShouldRecordResponse(font_request.get()));
+
+  // From MIME Type.
+  url_request_job_factory_.set_mime_type("image/png");
+  std::unique_ptr<net::URLRequest> prefetch_image_request =
+      CreateURLRequest(GURL("http://www.google.com/cat.png"), net::MEDIUM,
+                       content::RESOURCE_TYPE_PREFETCH, true);
+  EXPECT_TRUE(
+      LoadingDataCollector::ShouldRecordResponse(prefetch_image_request.get()));
+
+  url_request_job_factory_.set_mime_type("image/my-wonderful-format");
+  std::unique_ptr<net::URLRequest> prefetch_unknown_image_request =
+      CreateURLRequest(GURL("http://www.google.com/cat.png"), net::MEDIUM,
+                       content::RESOURCE_TYPE_PREFETCH, true);
+  EXPECT_FALSE(LoadingDataCollector::ShouldRecordResponse(
+      prefetch_unknown_image_request.get()));
+
+  url_request_job_factory_.set_mime_type("font/woff");
+  std::unique_ptr<net::URLRequest> prefetch_font_request =
+      CreateURLRequest(GURL("http://www.google.com/comic-sans-ms.woff"),
+                       net::MEDIUM, content::RESOURCE_TYPE_PREFETCH, true);
+  EXPECT_TRUE(
+      LoadingDataCollector::ShouldRecordResponse(prefetch_font_request.get()));
+
+  url_request_job_factory_.set_mime_type("font/woff-woff");
+  std::unique_ptr<net::URLRequest> prefetch_unknown_font_request =
+      CreateURLRequest(GURL("http://www.google.com/comic-sans-ms.woff"),
+                       net::MEDIUM, content::RESOURCE_TYPE_PREFETCH, true);
+  EXPECT_FALSE(LoadingDataCollector::ShouldRecordResponse(
+      prefetch_unknown_font_request.get()));
+
+  // Not main frame.
+  std::unique_ptr<net::URLRequest> font_request_sub_frame = CreateURLRequest(
+      GURL("http://www.google.com/comic-sans-ms.woff"), net::MEDIUM,
+      content::RESOURCE_TYPE_FONT_RESOURCE, false);
+  EXPECT_FALSE(
+      LoadingDataCollector::ShouldRecordResponse(font_request_sub_frame.get()));
+}
+
+}  // namespace predictors