Removing bad dependencies from autocomplete_provider_unittest

components/omnibox/browser/autocomplete_provider_unittest.cc

Index: components/omnibox/browser/autocomplete_provider_unittest.cc
diff --git a/chrome/browser/autocomplete/autocomplete_provider_unittest.cc b/components/omnibox/browser/autocomplete_provider_unittest.cc
similarity index 85%
rename from chrome/browser/autocomplete/autocomplete_provider_unittest.cc
rename to components/omnibox/browser/autocomplete_provider_unittest.cc
index 3c876a2adcc19879021a5786e22cb09d44bf4cbb..4a2534fef9c82ab4f1efd3f55a2adf7a128f9aea 100644
--- a/chrome/browser/autocomplete/autocomplete_provider_unittest.cc
+++ b/components/omnibox/browser/autocomplete_provider_unittest.cc
@@ -8,32 +8,25 @@
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #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 "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 "components/search_engines/template_url_service_client.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 static std::ostream& operator<<(std::ostream& os,
@@ -44,22 +37,55 @@ static std::ostream& operator<<(std::ostream& os,
 namespace {

[Peter Kasting, 2015/11/19 19:04:41]

Nit: Blank line below

[Abhishek, 2015/11/20 09:37:20]

Done.

 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;

[Peter Kasting, 2015/11/19 19:04:41]

Nit: Simpler:

    return net::URLRequest::IsHandledProtocol(scheme) ?
        metrics::OmniboxInputType::URL : metrics::OmniboxInputType::INVALID;

[Abhishek, 2015/11/20 09:37:21]

Done.

+  }
+};

[Peter Kasting, 2015/11/19 19:04:41]

Nit: DISALLOW_COPY_AND_ASSIGN

[Abhishek, 2015/11/20 09:37:20]

Done.

+
+// AutocompleteProviderClient implementation that calls the specified closure
+// when the result is ready.
+class AutocompleteProviderClientWithClosure
+    : public MockAutocompleteProviderClient {
+ public:
+  void set_closure(const base::Closure& closure) { closure_ = closure; }
+
+ private:
+  void OnAutocompleteControllerResultReady(
+      AutocompleteController* controller) override {
+    if (closure_.is_null())
+      return;

[Peter Kasting, 2015/11/19 19:04:41]

Don't we still need to call QuitWhenIdle() if the closure is null?

[Abhishek, 2015/11/20 09:37:20]

I think we can't do that, because tests are getting crashed if I add it.

[Peter Kasting, 2015/11/20 19:52:18]

Can you investigate to the point of understanding why?  If it turns out there's
a good reason that's happening, we should put a comment here explaining why we
shouldn't do this; or if it turns out the crashes are due to a bug, we should
fix that.

[Abhishek, 2015/11/21 12:11:20]

AutocompleteProviderTest.AllowExactKeywordMatch
AutocompleteProviderTest.ExtraQueryParams

These are the cases when closure_ is null.

If we call QuitWhenIdle when closure_ is null then in release build it's not
crashing, but in debug build it crashes.

I think I'll add TODO for me and create a bug for it.
WDYT?

[Peter Kasting, 2015/11/21 21:08:42]

Crashes how?  DCHECK failure?  I think you should continue investigating this at
least until you completely understand the crash before checking this patch in.

[Abhishek, 2015/11/23 15:22:07]

Actually its crashing in both the cases(either closure_ is null or not null)
into debug build.
Basically its a CHECK failed terminate inside the MessageLoop while calling
QuitWhenIdle().

Here are the crash logs from my local:
--
[32077:32077:1123/200225:11232244126621:FATAL:message_loop.cc(302)] Check
failed: false. Must be inside Run to call Quit
#0 0x00000182c70e base::debug::StackTrace::StackTrace()
#1 0x0000017473bf logging::LogMessage::~LogMessage()
#2 0x00000175024c base::MessageLoop::QuitWhenIdle()
#3 0x000000cd5b69 (anonymous
namespace)::AutocompleteProviderClientWithClosure::OnAutocompleteControllerResultReady()
#4 0x00000223d24f AutocompleteController::NotifyChanged()
#5 0x00000223b9e6 AutocompleteController::UpdateResult()
#6 0x00000223afd9 AutocompleteController::Start()
#7 0x000000cd2686 AutocompleteProviderTest::RunExactKeymatchTest()
--
Other function BT is also same.
No.
When I google it I found similar callstack related to MessageLoop at
https://crbug.com/541737. But I'm not sure how much its applicable here.

Could you please help me to understand this debug build terminate issue?

[droger, 2015/11/23 16:11:15]

I don't know much about the specifics of this test, but the change that is done
in this CL is preserving the existing behavior.

Before this CL, this code was run once the test registered to the
NOTIFICATION_AUTOCOMPLETE_CONTROLLER_RESULT_READY notification, and was not run
at all when the notification was not observed.

The new code calls set_closure() instead of registering for the notification,
but the behavior is otherwise similar.
You can see that set_closure() is called at the same place where the
notification was registered (line 338 below).

Removing the early return causes the code to be called at some time when it was
not run before.

+
+    if (controller->done()) {
+      closure_.Run();
+      base::MessageLoop::current()->QuitWhenIdle();
+    }
+  }
+
+  base::Closure closure_;
+};

[Peter Kasting, 2015/11/19 19:04:41]

Nit: DISALLOW_COPY_AND_ASSIGN

[Abhishek, 2015/11/20 09:37:21]

Done.

+
+}  // 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)
+  TestProvider(int relevance,
+               const base::string16& prefix,
+               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 +106,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_;
 };

[Peter Kasting, 2015/11/19 19:04:41]

Nit: DISALLOW_COPY_AND_ASSIGN

[Abhishek, 2015/11/20 09:37:21]

Done.

 
 void TestProvider::Start(const AutocompleteInput& input, bool minimal_changes) {
@@ -123,11 +149,9 @@ void TestProvider::Run() {
 }
 
 void TestProvider::AddResults(int start_at, int num) {
-  AddResultsWithSearchTermsArgs(start_at,
-                                num,
-                                AutocompleteMatchType::URL_WHAT_YOU_TYPED,
-                                TemplateURLRef::SearchTermsArgs(
-                                    base::string16()));
+  AddResultsWithSearchTermsArgs(
+      start_at, num, AutocompleteMatchType::URL_WHAT_YOU_TYPED,
+      TemplateURLRef::SearchTermsArgs(base::string16()));
 }
 
 void TestProvider::AddResultsWithSearchTermsArgs(
@@ -152,8 +176,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);
     }
 
@@ -161,8 +184,11 @@ void TestProvider::AddResultsWithSearchTermsArgs(
   }
 }
 
-class AutocompleteProviderTest : public testing::Test,
-                                 public content::NotificationObserver {
+class AutocompleteProviderTest : public testing::Test {
+ public:
+  ~AutocompleteProviderTest() override;
+  AutocompleteProviderTest();

[Peter Kasting, 2015/11/19 19:04:41]

Nit: Constructor before destructor

[Abhishek, 2015/11/20 09:37:20]

Done.

+
  protected:
   struct KeywordTestData {
     const base::string16 fill_into_edit;
@@ -176,7 +202,7 @@ class AutocompleteProviderTest : public testing::Test,
   };
 
  protected:

[Peter Kasting, 2015/11/19 19:04:41]

Nit: Remove this duplicate line

[Abhishek, 2015/11/20 09:37:21]

Done.

-   // Registers a test TemplateURL under the given keyword.
+  // Registers a test TemplateURL under the given keyword.
   void RegisterTemplateURL(const base::string16 keyword,
                            const std::string& template_url);
 
@@ -229,31 +255,35 @@ class AutocompleteProviderTest : public testing::Test,
   AutocompleteResult result_;
 
  private:
-  // content::NotificationObserver:
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override;
-
-  content::TestBrowserThreadBundle thread_bundle_;
-  content::NotificationRegistrar registrar_;
-  TestingProfile profile_;
+  base::MessageLoop message_loop_;
   scoped_ptr<AutocompleteController> controller_;
+  // Owned by |controller_|.
+  AutocompleteProviderClientWithClosure* client_;
+  // Sanity check to ensure that |client_| ownership has been passed to

[Peter Kasting, 2015/11/19 19:04:40]

Nit: Sanity check -> Used

[Abhishek, 2015/11/20 09:37:20]

Done.

+  // |controller_| exactly once.
+  bool client_owned_;
 };

[Peter Kasting, 2015/11/19 19:04:41]

Nit: DISALLOW_COPY_AND_ASSIGN

[Abhishek, 2015/11/20 09:37:20]

Done.

 
+AutocompleteProviderTest::AutocompleteProviderTest()
+    : client_(new AutocompleteProviderClientWithClosure), client_owned_(false) {
+  scoped_ptr<TemplateURLService> template_url_service(
+      new TemplateURLService(nullptr, 0));
+  client_->set_template_url_service(template_url_service.Pass());
+}
+
+AutocompleteProviderTest::~AutocompleteProviderTest() {
+  DCHECK(client_owned_);

[Peter Kasting, 2015/11/19 19:04:41]

Avoid using DCHECK in tests; failed DCHECKs crash the test and leave the bot in
a bad state.  Find ways to test this using ASSERT_ macros instead.  (Several
places)

[Abhishek, 2015/11/20 09:37:20]

As per the gtest.h we can't use ASSERT_* directly from destructor. So I'm doing
this inside TearDown().
Please correct me if I'm wrong here!

[Peter Kasting, 2015/11/20 19:52:18]

Avoiding ASSERT in destructors is a good move.  I believe you can safely use
EXPECT in a destructor, though, which is probably the right thing to do anyway
here, since failing this check shouldn't instantly halt the test.

[Abhishek, 2015/11/21 12:11:20]

Done.

+}
+
 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();
@@ -281,26 +311,23 @@ void AutocompleteProviderTest::ResetControllerWithTestProviders(
   AutocompleteController::Providers providers;
 
   // Construct two new providers, with either the same or different prefixes.
-  TestProvider* provider1 = new TestProvider(
-      kResultsPerProvider,
-      base::ASCIIToUTF16("http://a"),
-      &profile_,
-      base::ASCIIToUTF16(kTestTemplateURLKeyword));
+  TestProvider* provider1 =
+      new TestProvider(kResultsPerProvider, base::ASCIIToUTF16("http://a"),
+                       base::ASCIIToUTF16(kTestTemplateURLKeyword), client_);
   providers.push_back(provider1);
 
-  TestProvider* provider2 = new TestProvider(
-      kResultsPerProvider * 2,
-      same_destinations ? base::ASCIIToUTF16("http://a")
-                        : base::ASCIIToUTF16("http://b"),
-      &profile_,
-      base::string16());
+  TestProvider* provider2 =
+      new TestProvider(kResultsPerProvider * 2,
+                       same_destinations ? base::ASCIIToUTF16("http://a")
+                                         : base::ASCIIToUTF16("http://b"),
+                       base::string16(), client_);

[Peter Kasting, 2015/11/19 19:04:41]

Nit: Shorter:

  TestProvider* provider2 = new TestProvider(
      kResultsPerProvider * 2,
      base::ASCIIToUTF16(same_destinations ? "http://a" : "http://b"),
      base::string16(), client_);

[Abhishek, 2015/11/20 09:37:21]

Done.

   providers.push_back(provider2);
 
+  DCHECK(!client_owned_);
   // 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_), NULL, 0));
+  client_owned_ = true;

[Peter Kasting, 2015/11/19 19:04:41]

Nit: Three times you write a block of code to do:

  DCHECK(!client_owned_);
  controller_.reset(new AutocompleteController(make_scoped_ptr(client_),
                    nullptr, type));
  client_owned_ = true;

Where the only difference is |type|.  Factor this out into a helper function.

(Note use of nullptr for NULL as well)

[Abhishek, 2015/11/20 09:37:21]

Done.

   // 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());
@@ -308,11 +335,8 @@ void AutocompleteProviderTest::ResetControllerWithTestProviders(
   provider1->set_listener(controller_.get());
   provider2->set_listener(controller_.get());
 
-  // The providers don't complete synchronously, so listen for "result updated"
-  // notifications.
-  registrar_.Add(this,
-                 chrome::NOTIFICATION_AUTOCOMPLETE_CONTROLLER_RESULT_READY,
-                 content::Source<AutocompleteController>(controller_.get()));
+  client_->set_closure(base::Bind(&AutocompleteProviderTest::CopyResults,
+                                  base::Unretained(this)));
 
   if (provider1_ptr)
     *provider1_ptr = provider1;
@@ -320,19 +344,14 @@ void AutocompleteProviderTest::ResetControllerWithTestProviders(
     *provider2_ptr = provider2;
 }
 
-void AutocompleteProviderTest::
-    ResetControllerWithKeywordAndSearchProviders() {
-  TemplateURLServiceFactory::GetInstance()->SetTestingFactoryAndUse(
-      &profile_, &TemplateURLServiceFactory::BuildInstanceFor);
-
+void AutocompleteProviderTest::ResetControllerWithKeywordAndSearchProviders() {
   // 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();
@@ -347,18 +366,15 @@ void AutocompleteProviderTest::
   turl_model->Add(keyword_t_url);
   ASSERT_NE(0, keyword_t_url->id());
 
+  DCHECK(!client_owned_);
   controller_.reset(new AutocompleteController(
-
-      make_scoped_ptr(new ChromeAutocompleteProviderClient(&profile_)), NULL,
+      make_scoped_ptr(client_), NULL,
       AutocompleteProvider::TYPE_KEYWORD | AutocompleteProvider::TYPE_SEARCH));
+  client_owned_ = true;
 }
 
 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;
@@ -386,9 +402,10 @@ void AutocompleteProviderTest::ResetControllerWithKeywordProvider() {
   turl_model->Add(keyword_t_url);
   ASSERT_NE(0, keyword_t_url->id());
 
+  DCHECK(!client_owned_);
   controller_.reset(new AutocompleteController(
-      make_scoped_ptr(new ChromeAutocompleteProviderClient(&profile_)), NULL,
-      AutocompleteProvider::TYPE_KEYWORD));
+      make_scoped_ptr(client_), NULL, AutocompleteProvider::TYPE_KEYWORD));
+  client_owned_ = true;
 }
 
 void AutocompleteProviderTest::RunTest() {
@@ -412,12 +429,10 @@ void AutocompleteProviderTest::RunKeywordTest(const base::string16& input,
   controller_->input_ = AutocompleteInput(
       input, base::string16::npos, std::string(), GURL(),
       metrics::OmniboxEventProto::INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS,
-      false, true, true, true, false,
-      ChromeAutocompleteSchemeClassifier(&profile_));
+      false, true, true, true, false, 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() ?
@@ -456,10 +471,10 @@ void AutocompleteProviderTest::RunAssistedQueryStatsTest(
 
 void AutocompleteProviderTest::RunQuery(const base::string16 query) {
   result_.Reset();
-  controller_->Start(AutocompleteInput(
-      query, base::string16::npos, std::string(), GURL(),
-      metrics::OmniboxEventProto::INVALID_SPEC, true, false, true, true, false,
-      ChromeAutocompleteSchemeClassifier(&profile_)));
+  controller_->Start(
+      AutocompleteInput(query, base::string16::npos, std::string(), GURL(),
+                        metrics::OmniboxEventProto::INVALID_SPEC, true, false,
+                        true, true, false, TestingSchemeClassifier()));
 
   if (!controller_->done())
     // The message loop will terminate when all autocomplete input has been
@@ -478,8 +493,7 @@ void AutocompleteProviderTest::RunExactKeymatchTest(
   controller_->Start(AutocompleteInput(
       base::ASCIIToUTF16("k test"), base::string16::npos, std::string(), GURL(),
       metrics::OmniboxEventProto::INVALID_SPEC, true, false,
-      allow_exact_keyword_match, false, false,
-      ChromeAutocompleteSchemeClassifier(&profile_)));
+      allow_exact_keyword_match, false, false, TestingSchemeClassifier()));
   EXPECT_TRUE(controller_->done());
   EXPECT_EQ(AutocompleteProvider::TYPE_SEARCH,
       controller_->result().default_match()->provider->type());
@@ -501,16 +515,6 @@ GURL AutocompleteProviderTest::GetDestinationURL(
   return match.destination_url;
 }
 
-void AutocompleteProviderTest::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  if (controller_->done()) {
-    CopyResults();
-    base::MessageLoop::current()->QuitWhenIdle();
-  }
-}
-
 // Tests that the default selection is set properly when updating results.
 TEST_F(AutocompleteProviderTest, Query) {
   TestProvider* provider1 = NULL;
@@ -635,7 +639,6 @@ TEST_F(AutocompleteProviderTest, ExactMatchKeywords) {
       { base::ASCIIToUTF16("foo.com"), base::string16(),
         base::ASCIIToUTF16("foo.com") }
     };
-

[Peter Kasting, 2015/11/19 19:04:41]

Nit: Don't remove this

[Abhishek, 2015/11/20 09:37:21]

Done.

     SCOPED_TRACE("keyword match as usual");
     RunKeywordTest(base::ASCIIToUTF16("fo"), keyword_match,
                    arraysize(keyword_match));