A working implementation of AQS (Assisted Query Stats).

chrome/browser/autocomplete/autocomplete_unittest.cc

Index: chrome/browser/autocomplete/autocomplete_unittest.cc
diff --git a/chrome/browser/autocomplete/autocomplete_unittest.cc b/chrome/browser/autocomplete/autocomplete_unittest.cc
index 61d984f75496f96e9c543851e0d146a8e5e4b113..14d981db9ead18403dd617a7557e55f2cef2b6f3 100644
--- a/chrome/browser/autocomplete/autocomplete_unittest.cc
+++ b/chrome/browser/autocomplete/autocomplete_unittest.cc
@@ -37,10 +37,12 @@ const size_t kResultsPerProvider = 3;
 // 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,
@@ -56,9 +58,15 @@ class TestProvider : public AutocompleteProvider {
   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,
@@ -70,6 +78,15 @@ void TestProvider::Start(const AutocompleteInput& input,
 
   // 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;
@@ -87,9 +104,19 @@ void TestProvider::Run() {
 }
 
 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));
@@ -100,6 +127,12 @@ void TestProvider::AddResults(int start_at, int num) {
     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);
   }
@@ -114,6 +147,11 @@ class AutocompleteProviderTest : public testing::Test,
     const bool expected_keyword_result;
   };
 
+  struct AssistedQueryStatsTestData {
+    const AutocompleteMatch::Type match_type;
+    const std::string expected_aqs;
+  };
+
  protected:
   void ResetControllerWithTestProviders(bool same_destinations);
 
@@ -123,6 +161,10 @@ class AutocompleteProviderTest : public testing::Test,
 
   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();
@@ -152,14 +194,34 @@ void AutocompleteProviderTest::ResetControllerWithTestProviders(
   // 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);
 
@@ -217,8 +279,7 @@ void AutocompleteProviderTest::
   controller_.reset(new AutocompleteController(providers_, &profile_));
 }
 
-void AutocompleteProviderTest::
-    ResetControllerWithKeywordProvider() {
+void AutocompleteProviderTest::ResetControllerWithKeywordProvider() {
   TemplateURLServiceFactory::GetInstance()->SetTestingFactoryAndUse(
       &profile_, &TemplateURLServiceFactory::BuildInstanceFor);
 
@@ -280,10 +341,34 @@ void AutocompleteProviderTest::RunRedundantKeywordTest(
 
   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();
@@ -335,6 +420,26 @@ TEST_F(AutocompleteProviderTest, Query) {
   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();
@@ -395,6 +500,39 @@ TEST_F(AutocompleteProviderTest, RedundantKeywordsIgnoredInResult) {
   }
 }
 
+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) {