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..a3f913514cfd4606fac75b6f165143f054813a8e
--- /dev/null
+++ b/chrome/browser/ui/omnibox/omnibox_controller_unittest.cc
@@ -0,0 +1,96 @@
+// Copyright 2013 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/prefs/pref_service.h"
+#include "chrome/browser/autocomplete/autocomplete_controller.h"
+#include "chrome/browser/autocomplete/autocomplete_provider.h"
+#include "chrome/browser/search/search.h"
+#include "chrome/browser/ui/omnibox/omnibox_controller.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 {

[Peter Kasting, 2013/04/25 18:24:55]

Nit: Do not define virtual (and other non-cheap) functions inline, even in test
files.

[beaudoin, 2013/04/25 20:34:51]

Done.

+    profile_.reset(new TestingProfile);
+  }
+
+  virtual void TearDown() OVERRIDE {
+    omnibox_controller_.reset();
+    profile_.reset();

[Peter Kasting, 2013/04/25 18:24:55]

Nit: Do we actually need to tear these down here instead of in the destructor? 
If so, add a comment saying why.

[beaudoin, 2013/04/25 20:34:51]

Done.

+  }
+
+ protected:
+  void CreateController() {
+    omnibox_controller_.reset(new OmniboxController(NULL, profile_.get()));
+  }
+
+  void AssertProviders(int expected_providers);
+
+  scoped_ptr<TestingProfile> profile_;

[Peter Kasting, 2013/04/25 18:24:55]

Google style bans non-private data members in classes.  Make these private and
use accessors or define functions on the class to do what you need.

[beaudoin, 2013/04/25 20:34:51]

Done.

+  scoped_ptr<OmniboxController> omnibox_controller_;
+};

[Peter Kasting, 2013/04/25 18:24:55]

Nit: DISALLOW_COPY_AND_ASSIGN

[beaudoin, 2013/04/25 20:34:51]

Forced me to add an empty constructor though, so I added a virtual destructor
too.

Done.

+
+// 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);

[Peter Kasting, 2013/04/25 18:24:55]

Nit: What about just ASSERT_TRUE(expected_providers & type);?

[beaudoin, 2013/04/25 20:34:51]

Done.

+
+    // 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;

[Peter Kasting, 2013/04/25 18:24:55]

I'm a little unhappy about this.  It doesn't seem like someone changing the
provider sets should have to go change the test to be in sync as well -- in
other words, this seems like too low-level of a thing to be testing.

I read the previous review traffic on this and honestly I think you should just
nuke this test entirely.  I don't even think we should test that the various
lists are "in-sync" with each other.

[sreeram, 2013/04/25 18:47:51]

I think the test is valuable. If somebody changes the non-instant provider set,
this test will force them to think, "Does this provider change apply to the
instant set as well?". If it does, they just have to add it to the instant set,
no need to change the test. If it doesn't, they have to update the test; thus,
they are making a conscious decision to exclude the change from the instant set.

[Peter Kasting, 2013/04/25 19:05:07]

We define the two sets right next to each other.  You'd have to be blind not to
see one set while changing the other :).  So I think the value is extremely low,
whereas the hassle factor is not (you're guaranteed to have to change this file
on literally every change).  I've been burned enough by crappy low-level
make-work tests with Mac code before that I'm very cognizant of that at this
point.

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

What if I only checked the difference, instead of looking at what is contained
in the first set? This would break only if you break the expectation (ie.
instant-extended is exactly the regular providers minus a couple) instead of
breaking whenever we change something.

I'll do this and send a new patchset.#

[Peter Kasting, 2013/04/25 19:45:27]

That would improve things.  I still feel like it's unnecessary given that we
intend to change the instant set to support the shortcuts provider at some point
(which will require updating this test), and we'll be removing the non-instant
set later as well (which will make the test meaningless).  The chances of adding
a new provider before those happen seems miniscule.

[beaudoin, 2013/04/25 20:34:51]

I'm leaving it in, if only because it tests that the instantiation process
actually worked. :)

+  AssertProviders(expected_providers);
+
+  // With a valid kInstantUIZeroSuggestUrlPrefix we expect the exact same set
+  // in addition to TYPE_ZERO_SUGGEST.
+  int providers_with_zero_suggest =
+      expected_providers | AutocompleteProvider::TYPE_ZERO_SUGGEST;
+  profile_->GetPrefs()->SetString(prefs::kInstantUIZeroSuggestUrlPrefix,
+                                  "http://dummy.url.com/");
+  CreateController();
+  AssertProviders(providers_with_zero_suggest);
+  profile_->GetPrefs()->SetString(prefs::kInstantUIZeroSuggestUrlPrefix,
+                                  std::string());
+
+  // With instant extended we expect the exact same set, save from a couple of
+  // providers that should not be running.
+  int providers_with_instant_extended =
+      expected_providers &
+      ~AutocompleteProvider::TYPE_HISTORY_CONTENTS &
+      ~AutocompleteProvider::TYPE_SHORTCUTS;
+  // TODO(beaudoin): remove TYPE_SEARCH once it's no longer needed to pass
+  // the Instant suggestion through via FinalizeInstantQuery.
+  chrome::EnableInstantExtendedAPIForTesting();
+  CreateController();
+  AssertProviders(providers_with_instant_extended);
+
+}
+
+}  // namespace