Remove AddSearchProvider

chrome/browser/search_engines/template_url_fetcher_unittest.cc

Index: chrome/browser/search_engines/template_url_fetcher_unittest.cc
diff --git a/chrome/browser/search_engines/template_url_fetcher_unittest.cc b/chrome/browser/search_engines/template_url_fetcher_unittest.cc
index e760c440d05d114049029c6222d8fe7e7dffc0e8..e7bc75880eb3f86c0d61172824e6be179394db5c 100644
--- a/chrome/browser/search_engines/template_url_fetcher_unittest.cc
+++ b/chrome/browser/search_engines/template_url_fetcher_unittest.cc
@@ -25,8 +25,32 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
+namespace {
+
 using base::ASCIIToUTF16;
 
+class TestTemplateUrlFetcher : public TemplateURLFetcher {
+ public:
+  TestTemplateUrlFetcher(TemplateURLService* template_url_service,
+                         net::URLRequestContextGetter* request_context,
+                         const base::Closure& request_completed_callback)
+      : TemplateURLFetcher(template_url_service, request_context),
+        callback_(request_completed_callback) {}
+  ~TestTemplateUrlFetcher() override {}
+
+ protected:
+  void RequestCompleted(RequestDelegate* request) override {
+    callback_.Run();
+    TemplateURLFetcher::RequestCompleted(request);
+  }
+
+ private:
+  // Callback to be run when a request completes.
+  base::Closure callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestTemplateUrlFetcher);
+};
+
 // Basic set-up for TemplateURLFetcher tests.
 class TemplateURLFetcherTest : public testing::Test {
  public:
@@ -35,8 +59,10 @@ class TemplateURLFetcherTest : public testing::Test {
   void SetUp() override {
     TestingProfile* profile = test_util_.profile();
     ASSERT_TRUE(profile->GetRequestContext());
-    template_url_fetcher_.reset(new TemplateURLFetcher(
-        test_util_.model(), profile->GetRequestContext()));
+    template_url_fetcher_.reset(new TestTemplateUrlFetcher(
+        test_util_.model(), profile->GetRequestContext(),
+        base::Bind(&TemplateURLFetcherTest::RequestCompletedCallback,
+                   base::Unretained(this))));
 
     ASSERT_TRUE(test_server_.Start());
   }
@@ -45,19 +71,12 @@ class TemplateURLFetcherTest : public testing::Test {
     ASSERT_TRUE(test_server_.ShutdownAndWaitUntilComplete());
   }
 
-  // Called when the callback is destroyed.
-  void DestroyedCallback();
-
-  // TemplateURLFetcherCallbacks implementation.  (Although not derived from
-  // this class, this method handles those calls for the test.)
-  void ConfirmAddSearchProvider(
-      base::ScopedClosureRunner* callback_destruction_notifier,
-      std::unique_ptr<TemplateURL> template_url);
+  // Called when a request completes.
+  void RequestCompletedCallback();
 
   // Schedules the download of the url.
   void StartDownload(const base::string16& keyword,
                      const std::string& osdd_file_name,
-                     TemplateURLFetcher::ProviderType provider_type,
                      bool check_that_file_exists);
 
   // Waits for any downloads to finish.
@@ -67,11 +86,7 @@ class TemplateURLFetcherTest : public testing::Test {
   TemplateURLFetcher* template_url_fetcher() {
     return template_url_fetcher_.get();
   }
-  const TemplateURL* last_callback_template_url() const {
-    return last_callback_template_url_.get();
-  }
-  int callbacks_destroyed() const { return callbacks_destroyed_; }
-  int add_provider_called() const { return add_provider_called_; }
+  int requests_completed() const { return requests_completed_; }
 
  private:
   content::TestBrowserThreadBundle thread_bundle_;  // To set up BrowserThreads.
@@ -79,14 +94,8 @@ class TemplateURLFetcherTest : public testing::Test {
   std::unique_ptr<TemplateURLFetcher> template_url_fetcher_;
   net::EmbeddedTestServer test_server_;
 
-  // The last TemplateURL to come from a callback.
-  std::unique_ptr<TemplateURL> last_callback_template_url_;
-
-  // How many TemplateURLFetcherTestCallbacks have been destructed.
-  int callbacks_destroyed_;
-
-  // How many times ConfirmAddSearchProvider has been called.
-  int add_provider_called_;
+  // How many TemplateURKFetcher::RequestDelegate requests have completed.
+  int requests_completed_;
 
   // Is the code in WaitForDownloadToFinish in a message loop waiting for a
   // callback to finish?
@@ -108,31 +117,22 @@ bool GetTestDataDir(base::FilePath* dir) {
 
 TemplateURLFetcherTest::TemplateURLFetcherTest()
     : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
-      callbacks_destroyed_(0),
-      add_provider_called_(0),
+      requests_completed_(0),
       waiting_for_download_(false) {
   base::FilePath test_data_dir;
   CHECK(GetTestDataDir(&test_data_dir));
   test_server_.ServeFilesFromDirectory(test_data_dir);
 }
 
-void TemplateURLFetcherTest::DestroyedCallback() {
-  callbacks_destroyed_++;
+void TemplateURLFetcherTest::RequestCompletedCallback() {
+  requests_completed_++;
   if (waiting_for_download_)
     base::MessageLoop::current()->QuitWhenIdle();
 }
 
-void TemplateURLFetcherTest::ConfirmAddSearchProvider(
-    base::ScopedClosureRunner* callback_destruction_notifier,
-    std::unique_ptr<TemplateURL> template_url) {
-  last_callback_template_url_ = std::move(template_url);
-  add_provider_called_++;
-}
-
 void TemplateURLFetcherTest::StartDownload(
     const base::string16& keyword,
     const std::string& osdd_file_name,
-    TemplateURLFetcher::ProviderType provider_type,
     bool check_that_file_exists) {
   if (check_that_file_exists) {
     base::FilePath osdd_full_path;
@@ -145,18 +145,9 @@ void TemplateURLFetcherTest::StartDownload(
   // Start the fetch.
   GURL osdd_url = test_server_.GetURL("/" + osdd_file_name);
   GURL favicon_url;
-  base::ScopedClosureRunner* callback_destruction_notifier =
-      new base::ScopedClosureRunner(
-          base::Bind(&TemplateURLFetcherTest::DestroyedCallback,
-                     base::Unretained(this)));
-
   template_url_fetcher_->ScheduleDownload(
       keyword, osdd_url, favicon_url,
-      TemplateURLFetcher::URLFetcherCustomizeCallback(),
-      base::Bind(&TemplateURLFetcherTest::ConfirmAddSearchProvider,
-                 base::Unretained(this),
-                 base::Owned(callback_destruction_notifier)),
-      provider_type);
+      TemplateURLFetcher::URLFetcherCustomizeCallback());
 }
 
 void TemplateURLFetcherTest::WaitForDownloadToFinish() {
@@ -173,14 +164,11 @@ TEST_F(TemplateURLFetcherTest, BasicAutodetectedTest) {
   ASSERT_FALSE(test_util()->model()->GetTemplateURLForKeyword(keyword));
 
   std::string osdd_file_name("simple_open_search.xml");
-  StartDownload(keyword, osdd_file_name,
-                TemplateURLFetcher::AUTODETECTED_PROVIDER, true);
-  ASSERT_EQ(0, add_provider_called());
-  ASSERT_EQ(0, callbacks_destroyed());
+  StartDownload(keyword, osdd_file_name, true);
+  ASSERT_EQ(0, requests_completed());

[Peter Kasting, 2016/05/09 22:23:59]

How come here and the next two modified lines following are ASSERT instead of
EXPECT?

[Evan Stade, 2016/05/09 22:54:53]

no reason, should be expect.

 
   WaitForDownloadToFinish();
-  ASSERT_EQ(0, add_provider_called());
-  ASSERT_EQ(1, callbacks_destroyed());
+  ASSERT_EQ(1, requests_completed());
 
   const TemplateURL* t_url = test_util()->model()->GetTemplateURLForKeyword(
       keyword);
@@ -199,97 +187,38 @@ TEST_F(TemplateURLFetcherTest, DuplicatesThrownAway) {
   ASSERT_FALSE(test_util()->model()->GetTemplateURLForKeyword(keyword));
 
   std::string osdd_file_name("simple_open_search.xml");
-  StartDownload(keyword, osdd_file_name,
-                TemplateURLFetcher::AUTODETECTED_PROVIDER, true);
-  ASSERT_EQ(0, add_provider_called());
-  ASSERT_EQ(0, callbacks_destroyed());
+  StartDownload(keyword, osdd_file_name, true);
+  ASSERT_EQ(0, requests_completed());
 
   struct {
     std::string description;
     std::string osdd_file_name;
     base::string16 keyword;
-    TemplateURLFetcher::ProviderType provider_type;
   } test_cases[] = {
-      { "Duplicate osdd url with autodetected provider.", osdd_file_name,
-        keyword + ASCIIToUTF16("1"),
-        TemplateURLFetcher::AUTODETECTED_PROVIDER },
-      { "Duplicate keyword with autodetected provider.", osdd_file_name + "1",
-        keyword, TemplateURLFetcher::AUTODETECTED_PROVIDER },
-      { "Duplicate osdd url with explicit provider.", osdd_file_name,
-        base::string16(), TemplateURLFetcher::EXPLICIT_PROVIDER },
+      {"Duplicate osdd url with autodetected provider.", osdd_file_name,
+       keyword + ASCIIToUTF16("1")},
+      {"Duplicate keyword with autodetected provider.", osdd_file_name + "1",
+       keyword},
   };
 
   for (size_t i = 0; i < arraysize(test_cases); ++i) {
-    StartDownload(test_cases[i].keyword, test_cases[i].osdd_file_name,
-                  test_cases[i].provider_type, false);
-    ASSERT_EQ(1, template_url_fetcher()->requests_count())
+    StartDownload(test_cases[i].keyword, test_cases[i].osdd_file_name, false);
+    EXPECT_EQ(1, template_url_fetcher()->requests_count())
         << test_cases[i].description;
-    ASSERT_EQ(i + 1, static_cast<size_t>(callbacks_destroyed()));
   }
 
   WaitForDownloadToFinish();
-  ASSERT_EQ(1 + arraysize(test_cases),
-            static_cast<size_t>(callbacks_destroyed()));
-  ASSERT_EQ(0, add_provider_called());
-}
-
-TEST_F(TemplateURLFetcherTest, BasicExplicitTest) {
-  base::string16 keyword(ASCIIToUTF16("test"));
-
-  test_util()->ChangeModelToLoadState();
-  ASSERT_FALSE(test_util()->model()->GetTemplateURLForKeyword(keyword));
-
-  std::string osdd_file_name("simple_open_search.xml");
-  StartDownload(keyword, osdd_file_name,
-                TemplateURLFetcher::EXPLICIT_PROVIDER, true);
-  ASSERT_EQ(0, add_provider_called());
-  ASSERT_EQ(0, callbacks_destroyed());
-
-  WaitForDownloadToFinish();
-  ASSERT_EQ(1, add_provider_called());
-  ASSERT_EQ(1, callbacks_destroyed());
-
-  ASSERT_TRUE(last_callback_template_url());
-  EXPECT_EQ(ASCIIToUTF16("http://example.com/%s/other_stuff"),
-            last_callback_template_url()->url_ref().DisplayURL(
-                test_util()->model()->search_terms_data()));
-  EXPECT_EQ(ASCIIToUTF16("example.com"),
-            last_callback_template_url()->keyword());
-  EXPECT_FALSE(last_callback_template_url()->safe_for_autoreplace());
+  EXPECT_EQ(1, requests_completed());
 }
 
 TEST_F(TemplateURLFetcherTest, AutodetectedBeforeLoadTest) {
   base::string16 keyword(ASCIIToUTF16("test"));
-  ASSERT_FALSE(test_util()->model()->GetTemplateURLForKeyword(keyword));
+  EXPECT_FALSE(test_util()->model()->GetTemplateURLForKeyword(keyword));
 
   std::string osdd_file_name("simple_open_search.xml");
-  StartDownload(keyword, osdd_file_name,
-                TemplateURLFetcher::AUTODETECTED_PROVIDER, true);
-  ASSERT_EQ(0, add_provider_called());
-  ASSERT_EQ(1, callbacks_destroyed());
-}
-
-TEST_F(TemplateURLFetcherTest, ExplicitBeforeLoadTest) {
-  base::string16 keyword(ASCIIToUTF16("test"));
-  ASSERT_FALSE(test_util()->model()->GetTemplateURLForKeyword(keyword));
-
-  std::string osdd_file_name("simple_open_search.xml");
-  StartDownload(keyword, osdd_file_name,
-                TemplateURLFetcher::EXPLICIT_PROVIDER, true);
-  ASSERT_EQ(0, add_provider_called());
-  ASSERT_EQ(0, callbacks_destroyed());
-
-  WaitForDownloadToFinish();
-  ASSERT_EQ(1, add_provider_called());
-  ASSERT_EQ(1, callbacks_destroyed());
-
-  ASSERT_TRUE(last_callback_template_url());
-  EXPECT_EQ(ASCIIToUTF16("http://example.com/%s/other_stuff"),
-            last_callback_template_url()->url_ref().DisplayURL(
-                test_util()->model()->search_terms_data()));
-  EXPECT_EQ(ASCIIToUTF16("example.com"),
-            last_callback_template_url()->keyword());
-  EXPECT_FALSE(last_callback_template_url()->safe_for_autoreplace());
+  StartDownload(keyword, osdd_file_name, true);
+  EXPECT_EQ(0, template_url_fetcher()->requests_count());
+  EXPECT_EQ(0, requests_completed());

[Peter Kasting, 2016/05/09 22:23:59]

It seems like this test gets the same results as the StartDownload() call in the
next test, even though in that case the keyword already exists and in this case
it doesn't.  This at least deserves an explanation as to why requests_count()
doesn't go up, and maybe an expansion of the test to cover a way in which this
case is handled differently than the one below.

[Evan Stade, 2016/05/09 22:54:53]

I think the name of the test explains why the requests count doesn't go up. Both
this and the next test are countered by BasicAutodetectedTest. They happen to
have the same outcome but for different reasons. Not sure how to elaborate them
to demonstrate that better?

[Peter Kasting, 2016/05/09 22:57:32]

Maybe just add a comment like "This should bail because the TemplateURLService
is not yet loaded."?

[Evan Stade, 2016/05/10 16:51:13]

Done.

 }
 
 TEST_F(TemplateURLFetcherTest, DuplicateKeywordsTest) {
@@ -301,35 +230,31 @@ TEST_F(TemplateURLFetcherTest, DuplicateKeywordsTest) {
   test_util()->model()->Add(new TemplateURL(data));
   test_util()->ChangeModelToLoadState();
 
-  ASSERT_TRUE(test_util()->model()->GetTemplateURLForKeyword(keyword));
+  EXPECT_TRUE(test_util()->model()->GetTemplateURLForKeyword(keyword));
 
   // This should bail because the keyword already exists.
   std::string osdd_file_name("simple_open_search.xml");
-  StartDownload(keyword, osdd_file_name,
-                TemplateURLFetcher::AUTODETECTED_PROVIDER, true);
-  ASSERT_EQ(0, add_provider_called());
-  ASSERT_EQ(1, callbacks_destroyed());
-  ASSERT_FALSE(last_callback_template_url());
+  StartDownload(keyword, osdd_file_name, true);
+  EXPECT_EQ(0, template_url_fetcher()->requests_count());
+  EXPECT_EQ(0, requests_completed());
 }
 
 TEST_F(TemplateURLFetcherTest, DuplicateDownloadTest) {
+  test_util()->ChangeModelToLoadState();
+
   base::string16 keyword(ASCIIToUTF16("test"));
   std::string osdd_file_name("simple_open_search.xml");
-  StartDownload(keyword, osdd_file_name,
-                TemplateURLFetcher::EXPLICIT_PROVIDER, true);
-  ASSERT_EQ(0, add_provider_called());
-  ASSERT_EQ(0, callbacks_destroyed());
+  StartDownload(keyword, osdd_file_name, true);
+  EXPECT_EQ(1, template_url_fetcher()->requests_count());
+  EXPECT_EQ(0, requests_completed());
 
   // This should bail because the keyword already has a pending download.
-  StartDownload(keyword, osdd_file_name,
-                TemplateURLFetcher::EXPLICIT_PROVIDER, true);
-  ASSERT_EQ(0, add_provider_called());
-  ASSERT_EQ(1, callbacks_destroyed());
+  StartDownload(keyword, osdd_file_name, true);
+  EXPECT_EQ(1, template_url_fetcher()->requests_count());
+  EXPECT_EQ(0, requests_completed());
 
   WaitForDownloadToFinish();
-  ASSERT_EQ(1, add_provider_called());
-  ASSERT_EQ(2, callbacks_destroyed());
-  ASSERT_TRUE(last_callback_template_url());
+  EXPECT_EQ(1, requests_completed());
 }
 
 TEST_F(TemplateURLFetcherTest, UnicodeTest) {
@@ -339,11 +264,12 @@ TEST_F(TemplateURLFetcherTest, UnicodeTest) {
   ASSERT_FALSE(test_util()->model()->GetTemplateURLForKeyword(keyword));
 
   std::string osdd_file_name("unicode_open_search.xml");
-  StartDownload(keyword, osdd_file_name,
-                TemplateURLFetcher::AUTODETECTED_PROVIDER, true);
+  StartDownload(keyword, osdd_file_name, true);
   WaitForDownloadToFinish();
   const TemplateURL* t_url =
       test_util()->model()->GetTemplateURLForKeyword(keyword);
   EXPECT_EQ(base::UTF8ToUTF16("\xd1\x82\xd0\xb5\xd1\x81\xd1\x82"),
             t_url->short_name());
 }
+
+}  // namespace