Omnibox refactor, introduced OmniboxController.

chrome/browser/ui/omnibox/omnibox_controller_unittest.cc

Index: chrome/browser/ui/omnibox/omnibox_controller_unittest.cc
diff --git a/chrome/browser/ui/omnibox/omnibox_controller_unittest.cc b/chrome/browser/ui/omnibox/omnibox_controller_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ad77738b72fe3b900eea06fcadcf5d2e97a424ce
--- /dev/null
+++ b/chrome/browser/ui/omnibox/omnibox_controller_unittest.cc
@@ -0,0 +1,105 @@
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.

[sreeram, 2013/04/24 20:25:03]

No "(c)".

[beaudoin, 2013/04/25 14:57:54]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/command_line.h"
+#include "base/prefs/pref_service.h"
+#include "chrome/browser/autocomplete/autocomplete_controller.h"
+#include "chrome/browser/autocomplete/autocomplete_provider.h"
+#include "chrome/browser/ui/omnibox/omnibox_controller.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/testing_profile.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+class OmniboxControllerTest : public testing::Test {
+ public:
+  virtual void SetUp() OVERRIDE {
+    profile_.reset(new TestingProfile);
+  }
+
+  virtual void TearDown() OVERRIDE {
+    omnibox_controller_.reset();
+    profile_.reset();
+  }
+
+ protected:
+  void CreateController() {
+    omnibox_controller_.reset(new OmniboxController(NULL, profile_.get()));
+  }
+
+  void AssertProviders(int expected_providers);
+
+  scoped_ptr<TestingProfile> profile_;
+  scoped_ptr<OmniboxController> omnibox_controller_;
+};
+
+// Checks that the list of autocomplete providers used by the OmniboxController
+// matches the one in the |expected_providers| bit field.
+void OmniboxControllerTest::AssertProviders(int expected_providers) {
+  const ACProviders* providers =
+      omnibox_controller_->autocomplete_controller()->providers();
+
+  for (size_t i = 0; i < providers->size(); ++i) {
+    // Ensure this is a provider we wanted.
+    int type = providers->at(i)->type();
+    ASSERT_EQ(type, expected_providers & type);
+
+    // Remove it from expectations so we fail if it's there twice.
+    expected_providers &= ~type;
+  }
+
+  // Ensure we saw all the providers we expected.
+  ASSERT_EQ(0, expected_providers);
+}
+
+TEST_F(OmniboxControllerTest, CheckDefaultAutocompleteProviders) {
+  CreateController();
+  // List of the autocomplete providers we expect by default.
+  int expected_providers =
+      AutocompleteProvider::TYPE_BOOKMARK |
+      AutocompleteProvider::TYPE_BUILTIN |
+      AutocompleteProvider::TYPE_HISTORY_CONTENTS |
+      AutocompleteProvider::TYPE_HISTORY_QUICK |
+      AutocompleteProvider::TYPE_HISTORY_URL |
+      AutocompleteProvider::TYPE_KEYWORD |
+      AutocompleteProvider::TYPE_SEARCH |
+      AutocompleteProvider::TYPE_SHORTCUTS;

[sreeram, 2013/04/24 20:25:03]

I don't this test buys us much.

In my opinion, the main point of such a test is to ensure that the two lists
(instant-extended and non-instant-extended) are kept "in sync". So, for example,
if somebody adds or removes a provider from the non-instant-extended list, and
they forget to do so similarly for the instant-extended list, the test should
alert them to it.

With this test, they'll be reminded to update the list in the test here (in
CheckDefaultAutocompleteProviders), but they may still not realize that they
have to update the other list too.

So, I think a better technique is to just have one test, like so:

TEST_F(OmniboxControllerTest, DefaultVersusInstantAutocompleteProviders) {
  int default_providers = AutocompleteClassifier::kDefaultOmniboxProviders;
  int instant_providers =
AutocompleteClassifier::kInstantExtendedOmniboxProviders;
  int expected_difference = AutocompleteProvider::TYPE_HISTORY_CONTENTS | ...;
  EXPECT_EQ(expected_difference, default_providers - instant_providers);
}

This doesn't even need you to enable the Instant extended API, set up a profile,
or any such thing. WDYT?

[beaudoin, 2013/04/25 14:57:54]

Not sure this test should live in the AutocompleteController since it only
checks the constants and "hopes" the AutocompleteController are using them. If
that's really what we want we should write a test in AutocompleteClassifier
where the constants live.

I've taken a different approach where I check that the delta between the
actually instantiated providers in both cases are different. It should catch the
problem you're thinking of and any other that could stem from the instantiation
process.

As for the Profile and all, I already need them in tests I've written in the
follow-up patch. :)

+  AssertProviders(expected_providers);
+
+  // With a valid kInstantUIZeroSuggestUrlPrefix we also expect
+  // TYPE_ZERO_SUGGEST.
+  profile_->GetPrefs()->SetString(prefs::kInstantUIZeroSuggestUrlPrefix,
+                                  std::string("http://dummy.url.com/"));
+  CreateController();
+  AssertProviders(expected_providers | AutocompleteProvider::TYPE_ZERO_SUGGEST);
+}
+
+TEST_F(OmniboxControllerTest, CheckAutocompleteProvidersWithInstant) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableInstantExtendedAPI);

[sreeram, 2013/04/24 20:25:03]

You might also instead consider #including "c/b/search/search.h" and calling the
EnableInstantExtendedAPIForTesting() method instead. (Of course, if you accept
the suggestion in my other comment in this file, this becomes unnecessary.)

[beaudoin, 2013/04/25 14:57:54]

Done.

+
+  CreateController();
+  // List of the autocomplete providers we expect by default.
+  int expected_providers =
+    AutocompleteProvider::TYPE_BOOKMARK |
+    AutocompleteProvider::TYPE_BUILTIN |
+    AutocompleteProvider::TYPE_HISTORY_QUICK |
+    AutocompleteProvider::TYPE_HISTORY_URL |
+    AutocompleteProvider::TYPE_KEYWORD |
+    // TODO(beaudoin): remove TYPE_SEARCH once it's no longer needed to pass
+    // the Instant suggestion through via FinalizeInstantQuery.
+    AutocompleteProvider::TYPE_SEARCH;
+  AssertProviders(expected_providers);
+
+  // With a valid kInstantUIZeroSuggestUrlPrefix we also expect
+  // TYPE_ZERO_SUGGEST.
+  profile_->GetPrefs()->SetString(prefs::kInstantUIZeroSuggestUrlPrefix,
+                                  std::string("http://dummy.url.com/"));
+  CreateController();
+  AssertProviders(expected_providers | AutocompleteProvider::TYPE_ZERO_SUGGEST);
+}
+
+}  // namespace