A large part of this change is adding the 1st unit test for TemplateURLFetcher.

Also did some refactoring of TemplateURLFetcher to allow for a future
modification in which I make it able to start the load of TemplateURLModel and
wait for that to finish before processing the fetched url.

BUG=38475
TEST=unit_test --gtest_filter=TemplateURLFetcher*

Review URL: http://codereview.chromium.org/3548017

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@61727 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 223 insertions, 89 deletions

diff --git a/chrome/browser/search_engines/template_url_fetcher.cc b/chrome/browser/search_engines/template_url_fetcher.cc
index f5a3878..e6ab568 100644
--- a/chrome/browser/search_engines/template_url_fetcher.cc
+++ b/chrome/browser/search_engines/template_url_fetcher.cc
@@ -69,8 +69,11 @@ class TemplateURLFetcher::RequestDelegate : public URLFetcher::Delegate,
   const std::wstring keyword() const { return keyword_; }
 
  private:
+  void AddSearchProvider();
+
   URLFetcher url_fetcher_;
   TemplateURLFetcher* fetcher_;
+  scoped_ptr<TemplateURL> template_url_;
   std::wstring keyword_;
   const GURL osdd_url_;
   const GURL favicon_url_;
@@ -94,81 +97,88 @@ void TemplateURLFetcher::RequestDelegate::OnURLFetchComplete(
     int response_code,
     const ResponseCookies& cookies,
     const std::string& data) {
+  template_url_.reset(new TemplateURL());
+
+  // Validation checks.
   // Make sure we can still replace the keyword, i.e. the fetch was successful.
   // If the OSDD file was loaded HTTP, we also have to check the response_code.
   // For other schemes, e.g. when the OSDD file is bundled with an extension,
-  // the response_code is not applicable and should be -1.
+  // the response_code is not applicable and should be -1. Also, ensure that
+  // the returned information results in a valid search URL.
   if (!status.is_success() ||
-      ((response_code != -1) && (response_code != 200))) {
+      ((response_code != -1) && (response_code != 200)) ||
+      !TemplateURLParser::Parse(
+          reinterpret_cast<const unsigned char*>(data.c_str()),
+          data.length(),
+          NULL,
+          template_url_.get()) ||
+      !template_url_->url() || !template_url_->url()->SupportsReplacement()) {
     fetcher_->RequestCompleted(this);
     // WARNING: RequestCompleted deletes us.
     return;
   }
+  AddSearchProvider();
+  // WARNING: AddSearchProvider deletes us.
+}
 
-  scoped_ptr<TemplateURL> template_url(new TemplateURL());
-  if (TemplateURLParser::Parse(
-          reinterpret_cast<const unsigned char*>(data.c_str()),
-                                                 data.length(),
-                                                 NULL,
-                                                 template_url.get()) &&
-      template_url->url() && template_url->url()->SupportsReplacement()) {
-    if (!autodetected_ || keyword_.empty()) {
-      // Generate new keyword from URL in OSDD for none autodetected case.
-      // Previous keyword was generated from URL where OSDD was placed and
-      // it gives wrong result when OSDD is located on third party site that
-      // has nothing in common with search engine in OSDD.
-      GURL keyword_url(template_url->url()->url());
-      std::wstring new_keyword = TemplateURLModel::GenerateKeyword(
-          keyword_url, false);
-      if (!new_keyword.empty())
-        keyword_ = new_keyword;
-    }
-    TemplateURLModel* model = fetcher_->profile()->GetTemplateURLModel();
-    const TemplateURL* existing_url;
-    if (keyword_.empty() ||
-        !model || !model->loaded() ||
-        !model->CanReplaceKeyword(keyword_, GURL(template_url->url()->url()),
-                                  &existing_url)) {
-      if (autodetected_ || !model || !model->loaded()) {
-        fetcher_->RequestCompleted(this);
-        // WARNING: RequestCompleted deletes us.
-        return;
-      }
-      // If we're coming from JS (neither autodetected nor failure to load the
-      // template URL model) and this URL already exists in the model, we bring
-      // up the EditKeywordController to edit it.  This is helpful feedback in
-      // the case of clicking a button twice, and annoying in the case of a
-      // page that calls AddSearchProvider() in JS without a user action.
-      keyword_.clear();
-      existing_url = NULL;
+void TemplateURLFetcher::RequestDelegate::AddSearchProvider() {
+  DCHECK(template_url_.get());
+  if (!autodetected_ || keyword_.empty()) {
+    // Generate new keyword from URL in OSDD for none autodetected case.
+    // Previous keyword was generated from URL where OSDD was placed and
+    // it gives wrong result when OSDD is located on third party site that
+    // has nothing in common with search engine in OSDD.
+    GURL keyword_url(template_url_->url()->url());
+    std::wstring new_keyword = TemplateURLModel::GenerateKeyword(
+        keyword_url, false);
+    if (!new_keyword.empty())
+      keyword_ = new_keyword;
+  }
+  TemplateURLModel* model = fetcher_->profile()->GetTemplateURLModel();
+  const TemplateURL* existing_url;
+  if (keyword_.empty() ||
+      !model || !model->loaded() ||
+      !model->CanReplaceKeyword(keyword_, GURL(template_url_->url()->url()),
+                                &existing_url)) {
+    if (autodetected_ || !model || !model->loaded()) {
+      fetcher_->RequestCompleted(this);
+      // WARNING: RequestCompleted deletes us.
+      return;
     }
+    // If we're coming from JS (neither autodetected nor failure to load the
+    // template URL model) and this URL already exists in the model, we bring
+    // up the EditKeywordController to edit it.  This is helpful feedback in
+    // the case of clicking a button twice, and annoying in the case of a
+    // page that calls AddSearchProvider() in JS without a user action.
+    keyword_.clear();
+    existing_url = NULL;
+  }
 
-    if (existing_url)
-      model->Remove(existing_url);
-
-    // The short name is what is shown to the user. We preserve original names
-    // since it is better when generated keyword in many cases.
-    template_url->set_keyword(keyword_);
-    template_url->set_originating_url(osdd_url_);
-
-    // The page may have specified a URL to use for favicons, if not, set it.
-    if (!template_url->GetFavIconURL().is_valid())
-      template_url->SetFavIconURL(favicon_url_);
-
-    if (autodetected_) {
-      // Mark the keyword as replaceable so it can be removed if necessary.
-      template_url->set_safe_for_autoreplace(true);
-      model->Add(template_url.release());
-    } else if (source_ && source_->delegate()) {
-      // Confirm addition and allow user to edit default choices. It's ironic
-      // that only *non*-autodetected additions get confirmed, but the user
-      // expects feedback that his action did something.
-      // The source TabContents' delegate takes care of adding the URL to the
-      // model, which takes ownership, or of deleting it if the add is
-      // cancelled.
-      source_->delegate()->ConfirmAddSearchProvider(template_url.release(),
-                                                    fetcher_->profile());
-    }
+  if (existing_url)
+    model->Remove(existing_url);
+
+  // The short name is what is shown to the user. We preserve original names
+  // since it is better when generated keyword in many cases.
+  template_url_->set_keyword(keyword_);
+  template_url_->set_originating_url(osdd_url_);
+
+  // The page may have specified a URL to use for favicons, if not, set it.
+  if (!template_url_->GetFavIconURL().is_valid())
+    template_url_->SetFavIconURL(favicon_url_);
+
+  if (autodetected_) {
+    // Mark the keyword as replaceable so it can be removed if necessary.
+    template_url_->set_safe_for_autoreplace(true);
+    model->Add(template_url_.release());
+  } else if (source_ && source_->delegate()) {
+    // Confirm addition and allow user to edit default choices. It's ironic
+    // that only *non*-autodetected additions get confirmed, but the user
+    // expects feedback that his action did something.
+    // The source TabContents' delegate takes care of adding the URL to the
+    // model, which takes ownership, or of deleting it if the add is
+    // cancelled.
+    source_->delegate()->ConfirmAddSearchProvider(template_url_.release(),
+                                                  fetcher_->profile());
   }
   fetcher_->RequestCompleted(this);
   // WARNING: RequestCompleted deletes us.
diff --git a/chrome/browser/search_engines/template_url_fetcher_unittest.cc b/chrome/browser/search_engines/template_url_fetcher_unittest.cc
new file mode 100644
index 0000000..1b28b5a
--- /dev/null
+++ b/chrome/browser/search_engines/template_url_fetcher_unittest.cc
@@ -0,0 +1,109 @@
+// Copyright (c) 2010 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/platform_thread.h"
+#include "base/scoped_ptr.h"
+#include "base/thread.h"
+#include "chrome/browser/search_engines/template_url.h"
+#include "chrome/browser/search_engines/template_url_fetcher.h"
+#include "chrome/browser/search_engines/template_url_model.h"
+#include "chrome/browser/search_engines/template_url_model_test_util.h"
+#include "chrome/test/testing_profile.h"
+#include "googleurl/src/gurl.h"
+#include "net/test/test_server.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+// Quits the current message loop as soon as there is a change to the
+// TemplateURLModel.
+class QuitOnChangedObserver : public TemplateURLModelObserver {
+ public:
+  explicit QuitOnChangedObserver(TemplateURLModel* model);
+  virtual ~QuitOnChangedObserver();
+
+  // TemplateURLModelObserver implemementation.
+  virtual void OnTemplateURLModelChanged();
+
+ private:
+  TemplateURLModel* model_;
+};
+
+QuitOnChangedObserver::QuitOnChangedObserver(TemplateURLModel* model)
+    : model_(model) {
+  model_->AddObserver(this);
+}
+
+QuitOnChangedObserver::~QuitOnChangedObserver() {
+  model_->RemoveObserver(this);
+}
+
+void QuitOnChangedObserver::OnTemplateURLModelChanged() {
+  MessageLoop::current()->Quit();
+}
+
+// Basic set-up for TemplateURLFetcher tests.
+class TemplateURLFetcherTest : public testing::Test {
+ public:
+  TemplateURLFetcherTest();
+
+  virtual void SetUp() {
+    ASSERT_TRUE(test_server_.Start());
+    test_util_.SetUp();
+
+    io_thread_.reset(new ChromeThread(ChromeThread::IO));
+    base::Thread::Options options;
+    options.message_loop_type = MessageLoop::TYPE_IO;
+    io_thread_->StartWithOptions(options);
+
+    test_util_.profile()->CreateTemplateURLFetcher();
+    ASSERT_TRUE(test_util_.profile()->GetTemplateURLFetcher());
+
+    test_util_.profile()->CreateRequestContext();
+    ASSERT_TRUE(test_util_.profile()->GetRequestContext());
+  }
+
+  virtual void TearDown() {
+    io_thread_->Stop();
+    io_thread_.reset();
+    test_util_.TearDown();
+  }
+
+ protected:
+  TemplateURLModelTestUtil test_util_;
+  net::TestServer test_server_;
+  scoped_ptr<ChromeThread> io_thread_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TemplateURLFetcherTest);
+};
+
+TemplateURLFetcherTest::TemplateURLFetcherTest()
+    : test_server_(net::TestServer::TYPE_HTTP,
+                   FilePath(FILE_PATH_LITERAL("chrome/test/data"))) {
+}
+
+TEST_F(TemplateURLFetcherTest, BasicTest) {
+  std::wstring keyword(L"test");
+
+  test_util_.ChangeModelToLoadState();
+  test_util_.ResetObserverCount();
+  ASSERT_FALSE(test_util_.model()->GetTemplateURLForKeyword(keyword));
+
+  GURL osdd_url = test_server_.GetURL("files/osdd/dictionary.xml");
+  GURL favicon_url;
+  test_util_.profile()->GetTemplateURLFetcher()->ScheduleDownload(
+      keyword, osdd_url, favicon_url, NULL, true);
+
+  {  // Wait for the url to be downloaded.
+    QuitOnChangedObserver quit_on_changed_observer(test_util_.model());
+    MessageLoop::current()->Run();
+  }
+
+  const TemplateURL* t_url = test_util_.model()->GetTemplateURLForKeyword(
+      keyword);
+  ASSERT_TRUE(t_url);
+  EXPECT_STREQ(L"http://dictionary.reference.com/browse/%s?r=75",
+               t_url->url()->DisplayURL().c_str());
+  EXPECT_EQ(true, t_url->safe_for_autoreplace());
+}
+
diff --git a/chrome/browser/search_engines/template_url_model_test_util.cc b/chrome/browser/search_engines/template_url_model_test_util.cc
index 35e87e8..480a6ee 100644
--- a/chrome/browser/search_engines/template_url_model_test_util.cc
+++ b/chrome/browser/search_engines/template_url_model_test_util.cc
@@ -123,8 +123,8 @@ TemplateURLModelTestUtil::~TemplateURLModelTestUtil() {
 void TemplateURLModelTestUtil::SetUp() {
   profile_.reset(new TemplateURLModelTestingProfile());
   profile_->SetUp();
-  model_.reset(new TestingTemplateURLModel(profile_.get()));
-  model_->AddObserver(this);
+  profile_->SetTemplateURLModel(new TestingTemplateURLModel(profile_.get()));
+  profile_->GetTemplateURLModel()->AddObserver(this);
 }
 
 void TemplateURLModelTestUtil::TearDown() {
@@ -157,33 +157,34 @@ void TemplateURLModelTestUtil::BlockTillIOThreadProcessesRequests() {
 }
 
 void TemplateURLModelTestUtil::VerifyLoad() {
-  ASSERT_FALSE(model_->loaded());
-  model_->Load();
+  ASSERT_FALSE(model()->loaded());
+  model()->Load();
   BlockTillServiceProcessesRequests();
   VerifyObserverCount(1);
 }
 
 void TemplateURLModelTestUtil::ChangeModelToLoadState() {
-  model_->ChangeToLoadedState();
+  model()->ChangeToLoadedState();
   // Initialize the web data service so that the database gets updated with
   // any changes made.
-  model_->service_ = profile_->GetWebDataService(Profile::EXPLICIT_ACCESS);
+  model()->service_ = profile_->GetWebDataService(Profile::EXPLICIT_ACCESS);
 }
 
 void TemplateURLModelTestUtil::ClearModel() {
-  model_.reset(NULL);
+  profile_->SetTemplateURLModel(NULL);
 }
 
 void TemplateURLModelTestUtil::ResetModel(bool verify_load) {
-  model_.reset(new TestingTemplateURLModel(profile_.get()));
-  model_->AddObserver(this);
+  profile_->SetTemplateURLModel(new TestingTemplateURLModel(profile_.get()));
+  model()->AddObserver(this);
   changed_count_ = 0;
   if (verify_load)
     VerifyLoad();
 }
 
 std::wstring TemplateURLModelTestUtil::GetAndClearSearchTerm() {
-  return model_->GetAndClearSearchTerm();
+  return
+      static_cast<TestingTemplateURLModel*>(model())->GetAndClearSearchTerm();
 }
 
 void TemplateURLModelTestUtil::SetGoogleBaseURL(
@@ -199,7 +200,7 @@ WebDataService* TemplateURLModelTestUtil::GetWebDataService() {
 }
 
 TemplateURLModel* TemplateURLModelTestUtil::model() const {
-  return model_.get();
+  return profile_->GetTemplateURLModel();
 }
 
 TestingProfile* TemplateURLModelTestUtil::profile() const {
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 1b8a70b..fbf0eede 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1403,6 +1403,7 @@
         'browser/search_engines/keyword_editor_controller_unittest.cc',
         'browser/search_engines/search_host_to_urls_map_unittest.cc',
         'browser/search_engines/search_provider_install_data_unittest.cc',
+        'browser/search_engines/template_url_fetcher_unittest.cc',
         'browser/search_engines/template_url_model_test_util.cc',
         'browser/search_engines/template_url_model_test_util.h',
         'browser/search_engines/template_url_model_unittest.cc',
diff --git a/chrome/test/testing_profile.cc b/chrome/test/testing_profile.cc
index 359f64d..a7ad89a 100644
--- a/chrome/test/testing_profile.cc
+++ b/chrome/test/testing_profile.cc
@@ -29,6 +29,7 @@
 #include "chrome/browser/net/gaia/token_service.h"
 #include "chrome/browser/notifications/desktop_notification_service.h"
 #include "chrome/browser/prefs/browser_prefs.h"
+#include "chrome/browser/search_engines/template_url_fetcher.h"
 #include "chrome/browser/search_engines/template_url_model.h"
 #include "chrome/browser/sessions/session_service.h"
 #include "chrome/browser/sync/profile_sync_service_mock.h"
@@ -40,6 +41,7 @@
 #include "chrome/test/testing_pref_service.h"
 #include "net/base/cookie_monster.h"
 #include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_unittest.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "webkit/database/database_tracker.h"
 
@@ -108,16 +110,6 @@ class BookmarkLoadObserver : public BookmarkModelObserver {
   DISALLOW_COPY_AND_ASSIGN(BookmarkLoadObserver);
 };
 
-// This context is used to assist testing the CookieMonster by providing a
-// valid CookieStore. This can probably be expanded to test other aspects of
-// the context as well.
-class TestURLRequestContext : public URLRequestContext {
- public:
-  TestURLRequestContext() {
-    cookie_store_ = new net::CookieMonster(NULL, NULL);
-  }
-};
-
 // Used to return a dummy context (normally the context is on the IO thread).
 // The one here can be run on the main test thread. Note that this can lead to
 // a leak if your test does not have a ChromeThread::IO in it because
@@ -311,8 +303,16 @@ void TestingProfile::BlockUntilBookmarkModelLoaded() {
   DCHECK(bookmark_bar_model_->IsLoaded());
 }
 
+void TestingProfile::CreateTemplateURLFetcher() {
+  template_url_fetcher_.reset(new TemplateURLFetcher(this));
+}
+
 void TestingProfile::CreateTemplateURLModel() {
-  template_url_model_.reset(new TemplateURLModel(this));
+  SetTemplateURLModel(new TemplateURLModel(this));
+}
+
+void TestingProfile::SetTemplateURLModel(TemplateURLModel* model) {
+  template_url_model_.reset(model);
 }
 
 void TestingProfile::UseThemeProvider(BrowserThemeProvider* theme_provider) {
diff --git a/chrome/test/testing_profile.h b/chrome/test/testing_profile.h
index d84079f..dbd03a8 100644
--- a/chrome/test/testing_profile.h
+++ b/chrome/test/testing_profile.h
@@ -80,9 +80,16 @@ class TestingProfile : public Profile {
   // CreateBookmarkModel.
   void BlockUntilBookmarkModelLoaded();
 
-  // Creates a TemplateURLModel. If not invoked the TemplateURLModel is NULL.
+  // Creates a TemplateURLFetcher. If not invoked, the TemplateURLFetcher is
+  // NULL.
+  void CreateTemplateURLFetcher();
+
+  // Creates a TemplateURLModel. If not invoked, the TemplateURLModel is NULL.
   void CreateTemplateURLModel();
 
+  // Sets the TemplateURLModel. Takes ownership of it.
+  void SetTemplateURLModel(TemplateURLModel* model);
+
   // Uses a specific theme provider for this profile. TestingProfile takes
   // ownership of |theme_provider|.
   void UseThemeProvider(BrowserThemeProvider* theme_provider);
@@ -165,7 +172,9 @@ class TestingProfile : public Profile {
   virtual TemplateURLModel* GetTemplateURLModel() {
     return template_url_model_.get();
   }
-  virtual TemplateURLFetcher* GetTemplateURLFetcher() { return NULL; }
+  virtual TemplateURLFetcher* GetTemplateURLFetcher() {
+    return template_url_fetcher_.get();
+  }
   virtual history::TopSites* GetTopSites();
   virtual DownloadManager* GetDownloadManager() { return NULL; }
   virtual PersonalDataManager* GetPersonalDataManager() { return NULL; }
@@ -315,7 +324,11 @@ class TestingProfile : public Profile {
   // The WebDataService.  Only created if CreateWebDataService is invoked.
   scoped_refptr<WebDataService> web_data_service_;
 
-  // The TemplateURLFetcher. Only created if CreateTemplateURLModel is invoked.
+  // The TemplateURLFetcher. Only created if CreateTemplateURLFetcher is
+  // invoked.
+  scoped_ptr<TemplateURLFetcher> template_url_fetcher_;
+
+  // The TemplateURLModel. Only created if CreateTemplateURLModel is invoked.
   scoped_ptr<TemplateURLModel> template_url_model_;
 
   scoped_ptr<NTPResourceCache> ntp_resource_cache_;