A working implementation of AQS (Assisted Query Stats).

chrome/browser/autocomplete/autocomplete_unittest.cc

 // Copyright (c) 2012 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/bind.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "base/string16.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
@@ skipping 19 matching lines @@
 }
 
 namespace {
 const size_t kResultsPerProvider = 3;
 }
 
 // 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 string16& prefix)
-      : AutocompleteProvider(NULL, NULL, ""),
+  TestProvider(int relevance, const string16& prefix, Profile* profile,
+               const string16 match_keyword)

[Peter Kasting, 2012/06/18 19:45:52]

Nit: One arg per line

[Bart N, 2012/06/18 20:34:18]

Done.

+      : AutocompleteProvider(NULL, profile, ""),
         relevance_(relevance),
-        prefix_(prefix) {
+        prefix_(prefix),
+        match_keyword_(match_keyword) {
   }
 
   virtual void Start(const AutocompleteInput& input,
                      bool minimal_changes);
 
   void set_listener(ACProviderListener* listener) {
     listener_ = listener;
   }
 
  private:
   ~TestProvider() {}
 
   void Run();
 
   void AddResults(int start_at, int num);
+  void AddResultsWithSearchTermsArgs(
+      int start_at,
+      int num,
+      AutocompleteMatch::Type type,
+      const TemplateURLRef::SearchTermsArgs& search_terms_args);
 
   int relevance_;
   const string16 prefix_;
+  const string16 match_keyword_;
 };
 
 void TestProvider::Start(const AutocompleteInput& input,
                          bool minimal_changes) {
   if (minimal_changes)
     return;
 
   matches_.clear();
 
   // Generate one result synchronously, the rest later.

[Peter Kasting, 2012/06/18 19:45:52]

This comment is now wrong?

[Bart N, 2012/06/18 20:34:18]

Done.

   AddResults(0, 1);
+  AddResultsWithSearchTermsArgs(
+      1, 1, AutocompleteMatch::SEARCH_WHAT_YOU_TYPED,
+      TemplateURLRef::SearchTermsArgs(ASCIIToUTF16("echo")));
+  AddResultsWithSearchTermsArgs(
+      2, 1, AutocompleteMatch::NAVSUGGEST,
+      TemplateURLRef::SearchTermsArgs(ASCIIToUTF16("nav")));
+  AddResultsWithSearchTermsArgs(
+      3, 1, AutocompleteMatch::SEARCH_SUGGEST,
+      TemplateURLRef::SearchTermsArgs(ASCIIToUTF16("query")));
 
   if (input.matches_requested() == AutocompleteInput::ALL_MATCHES) {
     done_ = false;
     MessageLoop::current()->PostTask(FROM_HERE, base::Bind(&TestProvider::Run,
                                                            this));
   }
 }
 
 void TestProvider::Run() {
   DCHECK_GT(kResultsPerProvider, 0U);
   AddResults(1, kResultsPerProvider);
   done_ = true;
   DCHECK(listener_);
   listener_->OnProviderUpdate(true);
 }
 
 void TestProvider::AddResults(int start_at, int num) {
+  AddResultsWithSearchTermsArgs(start_at,
+                                num,
+                                AutocompleteMatch::URL_WHAT_YOU_TYPED,
+                                TemplateURLRef::SearchTermsArgs(string16()));
+}
+
+void TestProvider::AddResultsWithSearchTermsArgs(
+    int start_at,
+    int num,
+    AutocompleteMatch::Type type,
+    const TemplateURLRef::SearchTermsArgs& search_terms_args) {
   for (int i = start_at; i < num; i++) {
-    AutocompleteMatch match(this, relevance_ - i, false,
-                            AutocompleteMatch::URL_WHAT_YOU_TYPED);
+    AutocompleteMatch match(this, relevance_ - i, false, type);
 
     match.fill_into_edit = prefix_ + UTF8ToUTF16(base::IntToString(i));
     match.destination_url = GURL(UTF16ToUTF8(match.fill_into_edit));
 
     match.contents = match.fill_into_edit;
     match.contents_class.push_back(
         ACMatchClassification(0, ACMatchClassification::NONE));
     match.description = match.fill_into_edit;
     match.description_class.push_back(
         ACMatchClassification(0, ACMatchClassification::NONE));
+    match.search_terms_args.reset(
+        new TemplateURLRef::SearchTermsArgs(search_terms_args));
+    if (!match_keyword_.empty()) {
+      match.keyword = match_keyword_;
+      ASSERT_TRUE(match.GetTemplateURL(profile_) != NULL);
+    }
 
     matches_.push_back(match);
   }
 }
 
 class AutocompleteProviderTest : public testing::Test,
                                  public content::NotificationObserver {
  protected:
   struct KeywordTestData {
     const string16 fill_into_edit;
     const string16 keyword;
     const bool expected_keyword_result;
   };
 
+  struct AssistedQueryStatsTestData {
+    const AutocompleteMatch::Type match_type;
+    const std::string expected_aqs;
+  };
+
  protected:
   void ResetControllerWithTestProviders(bool same_destinations);
 
   // Runs a query on the input "a", and makes sure both providers' input is
   // properly collected.
   void RunTest();
 
   void RunRedundantKeywordTest(const KeywordTestData* match_data, size_t size);
 
+  void RunAssistedQueryStatsTest(
+      const AssistedQueryStatsTestData* aqs_test_data,
+      size_t size);
+
   void RunQuery(const string16 query);
 
   void ResetControllerWithTestProvidersWithKeywordAndSearchProviders();
   void ResetControllerWithKeywordProvider();
   void RunExactKeymatchTest(bool allow_exact_keyword_match);
 
   // These providers are owned by the controller once it's created.
   ACProviders providers_;
 
   AutocompleteResult result_;
   scoped_ptr<AutocompleteController> controller_;
 
  private:
   // content::NotificationObserver
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details);
 
   MessageLoopForUI message_loop_;
   content::NotificationRegistrar registrar_;
   TestingProfile profile_;
 };
 
 void AutocompleteProviderTest::ResetControllerWithTestProviders(
     bool same_destinations) {
   // Forget about any existing providers.  The controller owns them and will
   // Release() them below, when we delete it during the call to reset().
   providers_.clear();
 
+  // Register a TemplateURL for test keyword "t".

[Peter Kasting, 2012/06/18 19:45:52]

Can we do this in the UpdateAssistedQueryStats test instead?

[Bart N, 2012/06/18 20:34:18]

I still wanted a full end-to-end test rather than UpdateAssistedQueryStatsTest
only (which is testing a small fraction of the whole flow).

[Peter Kasting, 2012/06/18 20:49:50]

Sure, but can't we do that by just moving this setup code down to the test that
actually references this TemplateURL?  It's not critical that we setup this
TemplateURL in this function, is it?

[Bart N, 2012/06/18 22:26:24]

I tried, but there are some major implications, e.g. a match can't set keyword
unless it's registered, otherwise GetTemplateURL crashes.

I left a TODO and will address it in AQ cleanup, if you don't mind...

+  const string16 test_keyword(ASCIIToUTF16("t"));
+  TemplateURLServiceFactory::GetInstance()->SetTestingFactoryAndUse(
+      &profile_, &TemplateURLServiceFactory::BuildInstanceFor);
+  TemplateURLData data;
+  data.SetURL("http://aqs/{searchTerms}/{google:assistedQueryStats}");
+  data.SetKeyword(test_keyword);
+  TemplateURL* default_t_url = new TemplateURL(&profile_, data);
+  TemplateURLService* turl_model =
+      TemplateURLServiceFactory::GetForProfile(&profile_);
+  turl_model->Add(default_t_url);
+  turl_model->SetDefaultSearchProvider(default_t_url);
+  TemplateURLID default_provider_id = default_t_url->id();
+  ASSERT_NE(0, default_provider_id);
+
   // Construct two new providers, with either the same or different prefixes.
   TestProvider* providerA = new TestProvider(kResultsPerProvider,
-                                             ASCIIToUTF16("http://a"));
+                                             ASCIIToUTF16("http://a"),
+                                             &profile_,
+                                             test_keyword);
   providerA->AddRef();
   providers_.push_back(providerA);
 
-  TestProvider* providerB = new TestProvider(kResultsPerProvider * 2,
-      same_destinations ? ASCIIToUTF16("http://a") : ASCIIToUTF16("http://b"));
+  TestProvider* providerB = new TestProvider(
+      kResultsPerProvider * 2,
+      same_destinations ? ASCIIToUTF16("http://a") : ASCIIToUTF16("http://b"),
+      &profile_,
+      string16());
   providerB->AddRef();
   providers_.push_back(providerB);
 
   // Reset the controller to contain our new providers.
   AutocompleteController* controller =
       new AutocompleteController(providers_, &profile_);
   controller_.reset(controller);
   providerA->set_listener(controller);
   providerB->set_listener(controller);
 
@@ skipping 37 matching lines @@
   AutocompleteProvider* keyword_provider = new KeywordProvider(NULL, &profile_);
   keyword_provider->AddRef();
   providers_.push_back(keyword_provider);
   AutocompleteProvider* search_provider = new SearchProvider(NULL, &profile_);
   search_provider->AddRef();
   providers_.push_back(search_provider);
 
   controller_.reset(new AutocompleteController(providers_, &profile_));
 }
 
-void AutocompleteProviderTest::
-    ResetControllerWithKeywordProvider() {
+void AutocompleteProviderTest::ResetControllerWithKeywordProvider() {
   TemplateURLServiceFactory::GetInstance()->SetTestingFactoryAndUse(
       &profile_, &TemplateURLServiceFactory::BuildInstanceFor);
 
   TemplateURLService* turl_model =
       TemplateURLServiceFactory::GetForProfile(&profile_);
 
   // Create a TemplateURL for KeywordProvider.
   TemplateURLData data;
   data.short_name = ASCIIToUTF16("foo.com");
   data.SetKeyword(ASCIIToUTF16("foo.com"));
@@ skipping 41 matching lines @@
     match.keyword = match_data[i].keyword;
     matches.push_back(match);
   }
 
   AutocompleteResult result;
   result.AppendMatches(matches);
   controller_->UpdateAssociatedKeywords(&result);
 
   for (size_t j = 0; j < result.size(); ++j) {
     EXPECT_EQ(match_data[j].expected_keyword_result,
-        result.match_at(j).associated_keyword.get() != NULL);
+        result.match_at(j)->associated_keyword.get() != NULL);
   }
 }
 
+void AutocompleteProviderTest::RunAssistedQueryStatsTest(
+    const AssistedQueryStatsTestData* aqs_test_data,
+    size_t size) {
+  // Prepare input.
+  ACMatches matches;
+  for (size_t i = 0; i < size; ++i) {
+    AutocompleteMatch match(NULL, -i, false, aqs_test_data[i].match_type);

[Peter Kasting, 2012/06/18 19:45:52]

Nit: "-i" for a size_t is pretty weird, and risks overrunning any signal value
that might be present at (-1).  Can we just pick a constant for the highest
relevance and then do (kMaxRelevance - i) instead?

[Bart N, 2012/06/18 20:34:18]

Done.

[Bart N, 2012/06/18 20:34:18]

Done.

+    match.keyword = ASCIIToUTF16("t");
+    match.search_terms_args.reset(
+        new TemplateURLRef::SearchTermsArgs(string16()));
+    matches.push_back(match);
+  }
+  result_.Reset();
+  result_.AppendMatches(matches);
+
+  // Update AQS.
+  controller_->UpdateAssistedQueryStats(&result_);
+
+  // Verify data.
+  for (size_t i = 0; i < size; ++i) {
+    EXPECT_EQ(aqs_test_data[i].expected_aqs,
+              result_.match_at(i)->search_terms_args->assisted_query_stats);
+  }
+}
 
 void AutocompleteProviderTest::RunQuery(const string16 query) {
   result_.Reset();
   controller_->Start(query, string16(), true, false, true,
       AutocompleteInput::ALL_MATCHES);
 
   if (!controller_->done())
     // The message loop will terminate when all autocomplete input has been
     // collected.
     MessageLoop::current()->Run();
@@ skipping 31 matching lines @@
   ResetControllerWithTestProviders(false);
   RunTest();
 
   // Make sure the default match gets set to the highest relevance match.  The
   // highest relevance matches should come from the second provider.
   EXPECT_EQ(kResultsPerProvider * 2, result_.size());  // two providers
   ASSERT_NE(result_.end(), result_.default_match());
   EXPECT_EQ(providers_[1], result_.default_match()->provider);
 }
 
+// Tests assisted query stats.
+TEST_F(AutocompleteProviderTest, AssistedQueryStats) {
+  ResetControllerWithTestProviders(false);
+  RunTest();
+
+  EXPECT_EQ(kResultsPerProvider * 2, result_.size());  // two providers
+
+  // Now, check the results from the second provider, as they should not have
+  // assisted query stats set.
+  for (size_t i = 0; i < kResultsPerProvider; ++i) {
+    EXPECT_TRUE(result_.match_at(i)->search_terms_args->assisted_query_stats

[Peter Kasting, 2012/06/18 19:45:52]

Nit: Break after '(' instead (2 places)

[Bart N, 2012/06/18 20:34:18]

Done.

+                .empty());
+  }
+  // The first provider has a test keyword, so AQS should be non-empty.
+  for (size_t i = kResultsPerProvider; i < kResultsPerProvider * 2; ++i) {
+    EXPECT_FALSE(result_.match_at(i)->search_terms_args->assisted_query_stats
+                 .empty());
+  }
+}
+
 TEST_F(AutocompleteProviderTest, RemoveDuplicates) {
   ResetControllerWithTestProviders(true);
   RunTest();
 
   // Make sure all the first provider's results were eliminated by the second
   // provider's.
   EXPECT_EQ(kResultsPerProvider, result_.size());
   for (AutocompleteResult::const_iterator i(result_.begin());
        i != result_.end(); ++i)
     EXPECT_EQ(providers_[1], i->provider);
@@ skipping 40 matching lines @@
       { ASCIIToUTF16("foo.com"), string16(), false },
       { ASCIIToUTF16("bar.com"), string16(), true },
     };
 
     SCOPED_TRACE("Duplicate url with multiple keywords");
     RunRedundantKeywordTest(multiple_keyword,
         ARRAYSIZE_UNSAFE(multiple_keyword));
   }
 }
 
+TEST_F(AutocompleteProviderTest, UpdateAssistedQueryStats) {
+  ResetControllerWithTestProviders(false);
+
+  {
+    AssistedQueryStatsTestData test_data[] = { };
+    RunAssistedQueryStatsTest(test_data, ARRAYSIZE_UNSAFE(test_data));

[Peter Kasting, 2012/06/18 19:45:52]

Nit: I think you can use arraysize() here instead since the struct type is
named?  (3 places)

[Peter Kasting, 2012/06/18 20:49:50]

Was this false or did you miss it?

[Bart N, 2012/06/18 21:20:52]

I missed my reply. Here it goes:

// One caveat is that, for C++03, arraysize() doesn't accept any array of
// an anonymous type or a type defined inside a function.  In these rare
// cases, you have to use the unsafe ARRAYSIZE() macro below.  This is
// due to a limitation in C++03's template system.  The limitation has
// been removed in C++11.

In short - no, I can't use it here as it doesn't compile.

[Peter Kasting, 2012/06/18 21:29:21]

Huh.  I thought AssistedQueryStatsTestData was defined in the class instead of
in the function, so it would compile here.  But if you tested and it doesn't, I
guess I'm wrong :)

+    SCOPED_TRACE("No matches");

[Peter Kasting, 2012/06/18 19:45:52]

Nit: Doesn't this need to be above the RunAssistedQueryStatsTest() call?  (3
places)

[Bart N, 2012/06/18 20:34:18]

Ah yes. Done.

+  }
+
+  {
+    AssistedQueryStatsTestData test_data[] = {
+      { AutocompleteMatch::SEARCH_WHAT_YOU_TYPED, "chrome.0.57" }
+    };
+    RunAssistedQueryStatsTest(test_data, ARRAYSIZE_UNSAFE(test_data));
+    SCOPED_TRACE("One match");
+  }
+
+  {
+    AssistedQueryStatsTestData test_data[] = {
+      { AutocompleteMatch::SEARCH_WHAT_YOU_TYPED, "chrome.0.57j58j5l2j0l3j59" },
+      { AutocompleteMatch::URL_WHAT_YOU_TYPED, "chrome.1.57j58j5l2j0l3j59" },
+      { AutocompleteMatch::NAVSUGGEST, "chrome.2.57j58j5l2j0l3j59" },
+      { AutocompleteMatch::NAVSUGGEST, "chrome.3.57j58j5l2j0l3j59" },
+      { AutocompleteMatch::SEARCH_SUGGEST, "chrome.4.57j58j5l2j0l3j59" },
+      { AutocompleteMatch::SEARCH_SUGGEST, "chrome.5.57j58j5l2j0l3j59" },
+      { AutocompleteMatch::SEARCH_SUGGEST, "chrome.6.57j58j5l2j0l3j59" },
+      { AutocompleteMatch::SEARCH_HISTORY, "chrome.7.57j58j5l2j0l3j59" },
+    };
+    RunAssistedQueryStatsTest(test_data, ARRAYSIZE_UNSAFE(test_data));
+    SCOPED_TRACE("Multiple matches");
+  }
+}
+
 typedef testing::Test AutocompleteTest;
 
 TEST_F(AutocompleteTest, InputType) {
   struct test_data {
     const string16 input;
     const AutocompleteInput::Type type;
   } input_cases[] = {
     { string16(), AutocompleteInput::INVALID },
     { ASCIIToUTF16("?"), AutocompleteInput::FORCED_QUERY },
     { ASCIIToUTF16("?foo"), AutocompleteInput::FORCED_QUERY },
@@ skipping 199 matching lines @@
                                                    &scheme,
                                                    &host);
     AutocompleteInput input(input_cases[i].input, string16(), true, false,
                             true, AutocompleteInput::ALL_MATCHES);
     EXPECT_EQ(input_cases[i].scheme.begin, scheme.begin);
     EXPECT_EQ(input_cases[i].scheme.len, scheme.len);
     EXPECT_EQ(input_cases[i].host.begin, host.begin);
     EXPECT_EQ(input_cases[i].host.len, host.len);
   }
 }