Removing bad dependencies from autocomplete_provider_unittest

chrome/browser/autocomplete/autocomplete_provider_unittest.cc

Index: chrome/browser/autocomplete/autocomplete_provider_unittest.cc
diff --git a/chrome/browser/autocomplete/autocomplete_provider_unittest.cc b/chrome/browser/autocomplete/autocomplete_provider_unittest.cc
index 3c876a2adcc19879021a5786e22cb09d44bf4cbb..9d597a9e4c9499f1ff5b9abc3b83c7a1b5fcb677 100644
--- a/chrome/browser/autocomplete/autocomplete_provider_unittest.cc
+++ b/chrome/browser/autocomplete/autocomplete_provider_unittest.cc
@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
+#include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string16.h"
@@ -14,28 +15,27 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/thread_task_runner_handle.h"
-#include "chrome/browser/autocomplete/chrome_autocomplete_provider_client.h"
-#include "chrome/browser/autocomplete/chrome_autocomplete_scheme_classifier.h"
 #include "chrome/browser/chrome_notification_types.h"
-#include "chrome/browser/search_engines/template_url_service_factory.h"
-#include "chrome/test/base/testing_browser_process.h"
-#include "chrome/test/base/testing_profile.h"
 #include "components/metrics/proto/omnibox_event.pb.h"
 #include "components/omnibox/browser/autocomplete_controller.h"
 #include "components/omnibox/browser/autocomplete_input.h"
 #include "components/omnibox/browser/autocomplete_match.h"
 #include "components/omnibox/browser/autocomplete_provider_listener.h"
 #include "components/omnibox/browser/keyword_provider.h"
+#include "components/omnibox/browser/mock_autocomplete_provider_client.h"
 #include "components/omnibox/browser/search_provider.h"
 #include "components/search_engines/search_engines_switches.h"
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/template_url_service.h"
+#include "components/search_engines/template_url_service_client.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using testing::NiceMock;
+
 static std::ostream& operator<<(std::ostream& os,
                                 const AutocompleteResult::const_iterator& it) {
   return os << static_cast<const AutocompleteMatch*>(&(*it));
@@ -44,21 +44,32 @@ static std::ostream& operator<<(std::ostream& os,
 namespace {
 const size_t kResultsPerProvider = 3;
 const char kTestTemplateURLKeyword[] = "t";
-}
+
+class TestingSchemeClassifier : public AutocompleteSchemeClassifier {
+ public:
+  metrics::OmniboxInputType::Type GetInputTypeForScheme(
+      const std::string& scheme) const override {
+    if (net::URLRequest::IsHandledProtocol(scheme))
+      return metrics::OmniboxInputType::URL;
+    return metrics::OmniboxInputType::INVALID;
+  }
+};
+
+} // namespace
 
 // Autocomplete provider that provides known results. Note that this is
 // refcounted so that it can also be a task on the message loop.
 class TestProvider : public AutocompleteProvider {
  public:
   TestProvider(int relevance, const base::string16& prefix,
-               Profile* profile,
-               const base::string16 match_keyword)
+               const base::string16 match_keyword,
+               AutocompleteProviderClient* client)
       : AutocompleteProvider(AutocompleteProvider::TYPE_SEARCH),
         listener_(NULL),
-        profile_(profile),
         relevance_(relevance),
         prefix_(prefix),
-        match_keyword_(match_keyword) {
+        match_keyword_(match_keyword),
+        client_(client) {
   }
 
   void Start(const AutocompleteInput& input, bool minimal_changes) override;
@@ -80,10 +91,10 @@ class TestProvider : public AutocompleteProvider {
       const TemplateURLRef::SearchTermsArgs& search_terms_args);
 
   AutocompleteProviderListener* listener_;
-  Profile* profile_;
   int relevance_;
   const base::string16 prefix_;
   const base::string16 match_keyword_;
+  AutocompleteProviderClient* client_;
 };
 
 void TestProvider::Start(const AutocompleteInput& input, bool minimal_changes) {
@@ -152,8 +163,7 @@ void TestProvider::AddResultsWithSearchTermsArgs(
         new TemplateURLRef::SearchTermsArgs(search_terms_args));
     if (!match_keyword_.empty()) {
       match.keyword = match_keyword_;
-      TemplateURLService* service =
-          TemplateURLServiceFactory::GetForProfile(profile_);
+      TemplateURLService* service = client_->GetTemplateURLService();
       ASSERT_TRUE(match.GetTemplateURL(service, false) != NULL);
     }
 
@@ -163,6 +173,9 @@ void TestProvider::AddResultsWithSearchTermsArgs(
 
 class AutocompleteProviderTest : public testing::Test,
                                  public content::NotificationObserver {
+ public:
+  ~AutocompleteProviderTest() override {}
+
  protected:
   struct KeywordTestData {
     const base::string16 fill_into_edit;
@@ -186,6 +199,8 @@ class AutocompleteProviderTest : public testing::Test,
                                         TestProvider** provider1_ptr,
                                         TestProvider** provider2_ptr);
 
+  void SetUp() override;
+
   // Runs a query on the input "a", and makes sure both providers' input is
   // properly collected.
   void RunTest();
@@ -236,24 +251,25 @@ class AutocompleteProviderTest : public testing::Test,
 
   content::TestBrowserThreadBundle thread_bundle_;
   content::NotificationRegistrar registrar_;
-  TestingProfile profile_;
   scoped_ptr<AutocompleteController> controller_;
+  scoped_ptr<NiceMock<MockAutocompleteProviderClient>> client_;
 };

[droger, 2015/11/02 10:20:33]

add blank line here

[Abhishek, 2015/11/03 13:34:33]

Done.

+void AutocompleteProviderTest::SetUp() {

[droger, 2015/11/02 10:20:33]

Nit: in general, using the constructor is preferred over using the SetUp method.

See
https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de...

[Abhishek, 2015/11/03 13:34:33]

Done.

+  scoped_ptr<TemplateURLService> template_url_service(
+      new TemplateURLService(nullptr, 0));
+  client_.reset(new NiceMock<MockAutocompleteProviderClient>());
+  client_->set_template_url_service(template_url_service.Pass());
+}
 
 void AutocompleteProviderTest::RegisterTemplateURL(
     const base::string16 keyword,
     const std::string& template_url) {
-  if (TemplateURLServiceFactory::GetForProfile(&profile_) == NULL) {
-    TemplateURLServiceFactory::GetInstance()->SetTestingFactoryAndUse(
-        &profile_, &TemplateURLServiceFactory::BuildInstanceFor);
-  }
   TemplateURLData data;
   data.SetURL(template_url);
   data.SetShortName(keyword);
   data.SetKeyword(keyword);
   TemplateURL* default_t_url = new TemplateURL(data);
-  TemplateURLService* turl_model =
-      TemplateURLServiceFactory::GetForProfile(&profile_);
+  TemplateURLService* turl_model = client_->GetTemplateURLService();
   turl_model->Add(default_t_url);
   turl_model->SetUserSelectedDefaultSearchProvider(default_t_url);
   turl_model->Load();
@@ -284,23 +300,19 @@ void AutocompleteProviderTest::ResetControllerWithTestProviders(
   TestProvider* provider1 = new TestProvider(
       kResultsPerProvider,
       base::ASCIIToUTF16("http://a"),
-      &profile_,
-      base::ASCIIToUTF16(kTestTemplateURLKeyword));
+      base::ASCIIToUTF16(kTestTemplateURLKeyword), client_.get());
   providers.push_back(provider1);
 
   TestProvider* provider2 = new TestProvider(
       kResultsPerProvider * 2,
       same_destinations ? base::ASCIIToUTF16("http://a")
                         : base::ASCIIToUTF16("http://b"),
-      &profile_,
-      base::string16());
+      base::string16(), client_.get());
   providers.push_back(provider2);
 
   // Reset the controller to contain our new providers.
-  controller_.reset(new AutocompleteController(
-
-      make_scoped_ptr(new ChromeAutocompleteProviderClient(&profile_)), NULL,
-      0));
+  controller_.reset(new AutocompleteController(make_scoped_ptr(client_.get()),
+      NULL, 0));
   // We're going to swap the providers vector, but the old vector should be
   // empty so no elements need to be freed at this point.
   EXPECT_TRUE(controller_->providers_.empty());
@@ -322,17 +334,13 @@ void AutocompleteProviderTest::ResetControllerWithTestProviders(
 
 void AutocompleteProviderTest::
     ResetControllerWithKeywordAndSearchProviders() {
-  TemplateURLServiceFactory::GetInstance()->SetTestingFactoryAndUse(
-      &profile_, &TemplateURLServiceFactory::BuildInstanceFor);
-
   // Reset the default TemplateURL.
   TemplateURLData data;
   data.SetShortName(base::ASCIIToUTF16("default"));
   data.SetKeyword(base::ASCIIToUTF16("default"));
   data.SetURL("http://defaultturl/{searchTerms}");
   TemplateURL* default_t_url = new TemplateURL(data);
-  TemplateURLService* turl_model =
-      TemplateURLServiceFactory::GetForProfile(&profile_);
+  TemplateURLService* turl_model = client_->GetTemplateURLService();
   turl_model->Add(default_t_url);
   turl_model->SetUserSelectedDefaultSearchProvider(default_t_url);
   TemplateURLID default_provider_id = default_t_url->id();
@@ -348,17 +356,12 @@ void AutocompleteProviderTest::
   ASSERT_NE(0, keyword_t_url->id());
 
   controller_.reset(new AutocompleteController(
-
-      make_scoped_ptr(new ChromeAutocompleteProviderClient(&profile_)), NULL,
+      make_scoped_ptr(client_.get()), NULL,

[droger, 2015/11/02 10:20:33]

Here and everywhere you call make_scoped_ptr(client_.get()):
this is a problem, because make_scoped_ptr() takes ownership of the client, but
the client is already owned by AutocompleteProviderTest. This will cause a
double free.

I'm not sure what is the best way to fix that.
One way would be to do 
new AutocompleteController(client_.Pass(), ...);
This will reset |client_| to null though, so you may need to keep a weak
reference to it (or to the template service).

[Abhishek, 2015/11/03 13:34:33]

Thanks for letting me know about it!
I'm checking how to create weak reference to it!
Actually if I try making client_ as weekPtr then I'm not able pass here. And if
I try making template_url_service as weekPtr then I'm not able to set it into
client_ :/

[droger, 2015/11/03 14:27:48]

Just to make sure there is no misunderstanding: You cannot use base::WeakPtr
here because the client does not support it (it would need to inherit from
base::SupportsWeakPtr).

What I meant here was to keep a raw pointer, in addition to the scoped pointer.
I'm not sure of the details though, and maybe we don't even need it.

One thing is sure though: you cannot put the same object in two different
scoped_ptrs, because they will both delete the object and it will crash.

[Abhishek, 2015/11/04 11:57:11]

Thanks! I've updated it to raw pointer.

       AutocompleteProvider::TYPE_KEYWORD | AutocompleteProvider::TYPE_SEARCH));
 }
 
 void AutocompleteProviderTest::ResetControllerWithKeywordProvider() {
-  TemplateURLServiceFactory::GetInstance()->SetTestingFactoryAndUse(
-      &profile_, &TemplateURLServiceFactory::BuildInstanceFor);
-
-  TemplateURLService* turl_model =
-      TemplateURLServiceFactory::GetForProfile(&profile_);
+  TemplateURLService* turl_model = client_->GetTemplateURLService();
 
   // Create a TemplateURL for KeywordProvider.
   TemplateURLData data;
@@ -387,7 +390,7 @@ void AutocompleteProviderTest::ResetControllerWithKeywordProvider() {
   ASSERT_NE(0, keyword_t_url->id());
 
   controller_.reset(new AutocompleteController(
-      make_scoped_ptr(new ChromeAutocompleteProviderClient(&profile_)), NULL,
+      make_scoped_ptr(client_.get()), NULL,
       AutocompleteProvider::TYPE_KEYWORD));
 }
 
@@ -413,11 +416,10 @@ void AutocompleteProviderTest::RunKeywordTest(const base::string16& input,
       input, base::string16::npos, std::string(), GURL(),
       metrics::OmniboxEventProto::INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS,
       false, true, true, true, false,
-      ChromeAutocompleteSchemeClassifier(&profile_));
+      TestingSchemeClassifier());
   AutocompleteResult result;
   result.AppendMatches(controller_->input_, matches);
   controller_->UpdateAssociatedKeywords(&result);
-
   for (size_t j = 0; j < result.size(); ++j) {
     EXPECT_EQ(match_data[j].expected_associated_keyword,
               result.match_at(j)->associated_keyword.get() ?
@@ -459,7 +461,7 @@ void AutocompleteProviderTest::RunQuery(const base::string16 query) {
   controller_->Start(AutocompleteInput(
       query, base::string16::npos, std::string(), GURL(),
       metrics::OmniboxEventProto::INVALID_SPEC, true, false, true, true, false,
-      ChromeAutocompleteSchemeClassifier(&profile_)));
+      TestingSchemeClassifier()));
 
   if (!controller_->done())
     // The message loop will terminate when all autocomplete input has been
@@ -479,7 +481,7 @@ void AutocompleteProviderTest::RunExactKeymatchTest(
       base::ASCIIToUTF16("k test"), base::string16::npos, std::string(), GURL(),
       metrics::OmniboxEventProto::INVALID_SPEC, true, false,
       allow_exact_keyword_match, false, false,
-      ChromeAutocompleteSchemeClassifier(&profile_)));
+      TestingSchemeClassifier()));
   EXPECT_TRUE(controller_->done());
   EXPECT_EQ(AutocompleteProvider::TYPE_SEARCH,
       controller_->result().default_match()->provider->type());
@@ -635,7 +637,6 @@ TEST_F(AutocompleteProviderTest, ExactMatchKeywords) {
       { base::ASCIIToUTF16("foo.com"), base::string16(),
         base::ASCIIToUTF16("foo.com") }
     };
-
     SCOPED_TRACE("keyword match as usual");
     RunKeywordTest(base::ASCIIToUTF16("fo"), keyword_match,
                    arraysize(keyword_match));