Do not delay most visited results.

chrome/browser/autocomplete/zero_suggest_provider_unittest.cc

Index: chrome/browser/autocomplete/zero_suggest_provider_unittest.cc
diff --git a/chrome/browser/autocomplete/zero_suggest_provider_unittest.cc b/chrome/browser/autocomplete/zero_suggest_provider_unittest.cc
index b6574a716129e56bff97cdf297546c28fa22a0e8..1b1e4a987654405cd7913021e4550d6f73f7b371 100644
--- a/chrome/browser/autocomplete/zero_suggest_provider_unittest.cc
+++ b/chrome/browser/autocomplete/zero_suggest_provider_unittest.cc
@@ -10,6 +10,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier_factory.h"
 #include "chrome/browser/autocomplete/chrome_autocomplete_scheme_classifier.h"
+#include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_profile.h"
@@ -24,6 +25,87 @@
 #include "net/url_request/test_url_fetcher_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace {
+class FakeTopSites : public history::TopSites {

[Mark P, 2014/10/22 19:11:24]

optional nit: FakeTopSite implies that at minimum it returns some fake URLs for
top sites.  Consider another name, perhaps FakeEmptyTopSites or
FakeTopSitesForCheckingCallbackOnly, that kind of thing.

[Maria, 2014/10/23 20:34:26]

Done.

+ public:
+  FakeTopSites() {
+  }
+  virtual ~FakeTopSites() {
+  }
+  // history::TopSites:
+  bool SetPageThumbnail(const GURL& url, const gfx::Image& thumbnail,
+                        const ThumbnailScore& score) override {
+    return false;
+  }
+  bool SetPageThumbnailToJPEGBytes(const GURL& url,
+                                   const base::RefCountedMemory* memory,
+                                   const ThumbnailScore& score) override {
+    return false;
+  }
+  void GetMostVisitedURLs(const GetMostVisitedURLsCallback& callback,
+                          bool include_forced_urls) override;
+  bool GetPageThumbnail(const GURL& url, bool prefix_match,
+                        scoped_refptr<base::RefCountedMemory>* bytes) override {
+    return false;
+  }
+  bool GetPageThumbnailScore(const GURL& url, ThumbnailScore* score) override {
+    return false;
+  }
+  bool GetTemporaryPageThumbnailScore(const GURL& url, ThumbnailScore* score)
+      override {
+    return false;
+  }
+  void SyncWithHistory() override {}
+  bool HasBlacklistedItems() const override {
+    return false;
+  }
+  void AddBlacklistedURL(const GURL& url) override {}
+  void RemoveBlacklistedURL(const GURL& url) override {}
+  bool IsBlacklisted(const GURL& url) override {
+    return false;
+  }
+  void ClearBlacklistedURLs() override {}
+  void Shutdown() override {}
+  base::CancelableTaskTracker::TaskId StartQueryForMostVisited() override {
+    return 0;
+  }
+  bool IsKnownURL(const GURL& url) override {
+    return false;
+  }
+  const std::string& GetCanonicalURLString(const GURL& url) const override {
+    return NULL;
+  }
+  bool IsNonForcedFull() override {
+    return false;
+  }
+  bool IsForcedFull() override {
+    return false;
+  }
+  bool loaded() const override {
+    return false;
+  }
+  history::MostVisitedURLList GetPrepopulatePages() override {
+    return history::MostVisitedURLList();
+  }
+  bool AddForcedURL(const GURL& url, const base::Time& time) override {
+    return false;
+  }
+  // content::NotificationObserver:
+  void Observe(int type,
+               const content::NotificationSource& source,
+               const content::NotificationDetails& details) override {}
+  GetMostVisitedURLsCallback mv_callback;

[Mark P, 2014/10/22 19:11:23]

optional nit: comment this new local variable about what it does / why you have
it.

[Maria, 2014/10/23 20:34:26]

Done.

+};
+
+void FakeTopSites::GetMostVisitedURLs(
+    const GetMostVisitedURLsCallback& callback,
+    bool include_forced_urls)  {
+  mv_callback = callback;
+}
+
+} // namespace
+
+
 class ZeroSuggestProviderTest : public testing::Test,
                                 public AutocompleteProviderListener {
  public:
@@ -39,6 +121,7 @@ class ZeroSuggestProviderTest : public testing::Test,
   void ResetFieldTrialList();
 
   void CreatePersonalizedFieldTrial();
+  void CreateMostVisitedFieldTrial();
 
   // Set up threads for testing; this needs to be instantiated before
   // |profile_|.
@@ -86,6 +169,8 @@ void ZeroSuggestProviderTest::SetUp() {
   turl_model->Add(default_t_url_);
   turl_model->SetUserSelectedDefaultSearchProvider(default_t_url_);
 
+  profile_.SetTopSites(new FakeTopSites());
+
   provider_ = ZeroSuggestProvider::Create(this, turl_model, &profile_);
 }
 
@@ -117,6 +202,47 @@ void ZeroSuggestProviderTest::CreatePersonalizedFieldTrial() {
       OmniboxFieldTrial::kBundledExperimentFieldTrialName, "A");
 }
 
+void ZeroSuggestProviderTest::CreateMostVisitedFieldTrial() {
+  std::map<std::string, std::string> params;
+  params[std::string(OmniboxFieldTrial::kZeroSuggestRule)] = "true";
+  params[std::string(OmniboxFieldTrial::kZeroSuggestVariantRule)] =
+      "MostVisited";
+  variations::AssociateVariationParams(
+      OmniboxFieldTrial::kBundledExperimentFieldTrialName, "A", params);
+  base::FieldTrialList::CreateFieldTrial(
+      OmniboxFieldTrial::kBundledExperimentFieldTrialName, "A");
+}
+
+TEST_F(ZeroSuggestProviderTest, TestMostVisitedCallback) {
+  CreateMostVisitedFieldTrial();
+
+  std::string input_url("http://www.cnn.com");

[Mark P, 2014/10/22 19:11:24]

nit: add trailing slash to make a valid URL, not requiring autocomplete to fix
it up.
ditto with foo.com

[Maria, 2014/10/23 20:34:27]

Done.

+  AutocompleteInput input(base::ASCIIToUTF16(input_url), base::string16::npos,
+                          std::string(), GURL(input_url),

[Mark P, 2014/10/22 19:11:24]

optional nit: This GURL doesn't have to be the same as the input.  Consider
using something else (and an appropriate page classification to go with it).

+                          metrics::OmniboxEventProto::INVALID_SPEC, true, false,

[Mark P, 2014/10/22 19:11:24]

Typically prevent_inline_autocomplete is false, not true.

[Maria, 2014/10/23 20:34:26]

Done.

+                          true, true,
+                          ChromeAutocompleteSchemeClassifier(&profile_));
+  history::MostVisitedURLList urls;
+  history::MostVisitedURL url;
+  url.url = GURL("http://foo.com");

[Mark P, 2014/10/22 19:11:23]

Please use one of the MostVisitedURL constructors.  Don't muck with internal
fields unless necessary.

[Maria, 2014/10/23 20:34:26]

Done.

+  urls.push_back(url);
+
+  provider_->Start(input, false);
+  EXPECT_TRUE(provider_->matches().empty());
+  ((FakeTopSites*) profile_.GetTopSites())->mv_callback.Run(urls);

[Mark P, 2014/10/22 19:11:23]

style guide request: I think we're supposed to explicitly use the various
casting options.  I think this is ought to be a dynamic_cast.

[Maria, 2014/10/23 20:34:26]

That's what I had here at first, but it refuses to compile saying that
dynamic_cast is disallowed by the compiler flags we use. Also not seeing
dynamic_cast being used in the codebase, so not sure how to fix this.

[Mark P, 2014/10/23 21:33:52]

From the style guide, I guess we're supposed to use static_cast:
When casting to and from different types, use static_cast<>() when you know the
conversion is safe; also consider checked_cast<>() and saturated_cast<>() in
base/numerics/safe_conversions.h, which allow you to enforce via CHECK() that
the cast did not change the value or to clamp out-of-range values, respectively.

[Maria, 2014/10/23 21:53:20]

It looks to me like safe conversions is used for numeric types rather than
arbitrary class pointers (at least from the uses in the codebase), but it's good
to know. Switched to using static_cast.

+  // Should have verbatim match + most visited url match

[Mark P, 2014/10/22 19:11:23]

nit: period at end of comment

[Maria, 2014/10/23 20:34:26]

Done.

[Maria, 2014/10/23 20:34:27]

Done.

+  EXPECT_EQ(2U, provider_->matches().size());
+  provider_->Stop(false);

[Mark P, 2014/10/22 19:11:24]

Oftentimes the autocomplete system Start()s a new input without properly
Stop()ing the old.  Perhaps we should omit this Stop() call for testing
purposes?

If you omit it, you might want a brief comment explaining why.

[Maria, 2014/10/23 20:34:26]

I think this will be taken care of by the new test case.

+
+  provider_->Start(input, false);
+  provider_->Stop(false);
+  EXPECT_TRUE(provider_->matches().empty());
+  // Most visited results arriving after Stop() has been called, ensure they
+  // are not displayed.
+  ((FakeTopSites*) profile_.GetTopSites())->mv_callback.Run(urls);
+  EXPECT_TRUE(provider_->matches().empty());
+}

[Mark P, 2014/10/22 19:11:23]

I'd also like to see a test for the case I mentioned in the original comment
about testing this:
Start()
Start() again, this time on a page that should NOT launch a MV request
callback
EXPECT empty matches.
(In this new test, please omit all Stop() calls.  They're not dependably called
by the framework.)

[Maria, 2014/10/23 20:34:26]

Done.

+
 TEST_F(ZeroSuggestProviderTest, TestPsuggestZeroSuggestCachingFirstRun) {
   CreatePersonalizedFieldTrial();