A working implementation of AQS (Assisted Query Stats).

chrome/browser/autocomplete/autocomplete.cc

Index: chrome/browser/autocomplete/autocomplete.cc
diff --git a/chrome/browser/autocomplete/autocomplete.cc b/chrome/browser/autocomplete/autocomplete.cc
index 4f87500830bd40e23c106072b9e2452f9e475de3..6ce6336e3b890e43c6d18ffc4ae31e2b27222ea8 100644
--- a/chrome/browser/autocomplete/autocomplete.cc
+++ b/chrome/browser/autocomplete/autocomplete.cc
@@ -14,6 +14,7 @@
 #include "base/metrics/histogram.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
+#include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete_controller_delegate.h"
 #include "chrome/browser/autocomplete/autocomplete_match.h"
@@ -53,6 +54,45 @@
 
 using base::TimeDelta;
 
+namespace {
+
+// Converts the given type to an integer based on the AQS specification.

[Peter Kasting, 2012/06/14 01:04:23]

We either need a copy of or link to the spec, or a copy of the actual table of
values as constants, or something.  These magic numbers are too magic.

[Bart N, 2012/06/15 18:07:34]

Done.

+int AutocompleteMatchToAssistedQueryType(const AutocompleteMatch::Type& type) {
+  switch (type) {
+    case AutocompleteMatch::SEARCH_SUGGEST:

[Peter Kasting, 2012/06/14 01:04:23]

Nit: I suggest putting the return statements on the same lines as the case
statements, all aligned

[Bart N, 2012/06/15 18:07:34]

Done.

+      return 0;
+    case AutocompleteMatch::NAVSUGGEST:
+      return 5;
+    case AutocompleteMatch::SEARCH_WHAT_YOU_TYPED:
+      return 57;
+    case AutocompleteMatch::URL_WHAT_YOU_TYPED:
+      return 58;
+    case AutocompleteMatch::SEARCH_HISTORY:
+      return 59;
+    case AutocompleteMatch::HISTORY_URL:
+      return 60;
+    case AutocompleteMatch::HISTORY_TITLE:
+      return 61;
+    case AutocompleteMatch::HISTORY_BODY:
+      return 62;
+    case AutocompleteMatch::HISTORY_KEYWORD:
+      return 63;
+    default:
+      return 64;
+  }
+}
+
+// Appends query stats of the given type and number to the existing AQS string.
+void AppendQueryStats(int type, int count, std::string* aqs) {
+  if (!aqs->empty())
+    aqs->append("j");
+  base::StringAppendF(aqs, "%d", type);
+  if (count > 1)
+    base::StringAppendF(aqs, "l%d", count);
+}
+
+}  // namespace
+
 // AutocompleteInput ----------------------------------------------------------
 
 AutocompleteInput::AutocompleteInput()
@@ -1044,6 +1084,7 @@ void AutocompleteController::UpdateResult(bool is_synchronous_pass) {
 
   UpdateKeywordDescriptions(&result_);
   UpdateAssociatedKeywords(&result_);
+  UpdateAssistedQueryStats(&result_);
 
   bool notify_default_match = is_synchronous_pass;
   if (!is_synchronous_pass) {
@@ -1100,6 +1141,45 @@ void AutocompleteController::UpdateAssociatedKeywords(
   }
 }
 
+void AutocompleteController::UpdateAssistedQueryStats(
+    AutocompleteResult* result) {
+  if (result->empty())
+    return;
+
+  // Build the impressions string (the AQS part after ".").
+  std::string aqs;
+  int count = 0;
+  int last_type = -1;
+  for (ACMatches::iterator match(result->begin()); match != result->end();
+       ++match) {
+    int type = AutocompleteMatchToAssistedQueryType(match->type);
+    if (last_type != -1 && type != last_type) {
+      AppendQueryStats(last_type, count, &aqs);
+      count = 1;
+    } else {
+      count++;
+    }
+    last_type = type;
+  }
+  AppendQueryStats(last_type, count, &aqs);
+
+  // Go over all matches and set AQS if the match supports it.

[Peter Kasting, 2012/06/14 01:04:23]

Don't do things this way.  Instead let TemplateURLRef substitute the correct
string into the URL, and pass the data you need to it (by which I mean, a const
AutocompleteResult*) through ReplaceSearchTerms(), the way we currently do for
the existing suggest data.

Now, implementing this will be a bit of a trick.  I suggest adding a
normally-empty string16 member on AutocompleteMatch called
|query_for_later_replace|, and where SearchProvider() currently calls
ReplaceSearchTerms(), instead omit the call and set this member.  Then this
function (UpdateAssistedQueryStats) can simply loop through all matches, and for
each one with a non-empty value here, call
ReplaceSearchTerms(match.query_for_later_replace, result).

This will also eliminate the need for the SupportsAssistedQueryStats() method
you've added.

Note that this comment assumes we're replacing the existing aq/oq stuff with
aqs, see comment elsewhere.

[Bart N, 2012/06/14 01:22:13]

I really wanted to do it that way, but I ran into several issues.
I don't think this would work. The problem is that AQS string can only be
computed in this method, as it is order sensitive and needs global access to all
providers. On the other hand, we still need to replace all other params (e.g.
oq, search terms etc.), which requires knowing the substitutes, currently only
known locally (at the time when ReplaceSearchTerms is called). 

Probably a better approach would be to decouple setting replacements from
actually applying them. For instance, we could set template params instead of
calling ReplaceSearchTerms in two places:
(1) as they are currently (in search provider)
(2) in this method (aqs)

Finally, we could call something like "ApplyReplacements" (or similar) which
would essentially go over all params and map them onto the final URL. This would
also imply that we don't set destination_url until the very last moment, which
would be here, but I can't tell if there are any consequences of doing so...
Unfortunately, we still need oq stuff and aqs alone is not sufficient. See my
proposal above.

[Peter Kasting, 2012/06/14 03:18:17]

Right; I was attempting to address that, but assumed only the query string
needed to be preserved of the three existing args.  It sounds like the "oq"
needs to be preserved too (see bottom of this comment).

There are two ways to handle this.  One is to use a route like I've suggested
and simply preserve the "oq" on the AutocompleteMatch as well.  The other is to
do something like you've suggested and cache the "query" and "oq" values in the
TemplateURLRef, making it stateful, then allow a caller to get back a URL with
those values substituted in later.

Practically speaking, these are really almost the same, they just differ in
where the state is preserved.  Because TemplateURLRef doesn't currently have any
state like this, and it has a lot of different users all over the codebase
(while AutocompleteMatch is restricted to just this subsystem), I'd feel a
little better if the state was preserved on AutocompleteMatch.

Now, if keeping two members instead of one feels inelegant, there's another
change we could make; we could define a struct (SearchTermsArgs) that
ReplaceSearchTerms() can take, and that AutocompleteMatch can keep as a
scoped_ptr (so NULL means no need to call ReplaceSearchTerms() here).  Then if
we give the struct a one-arg constructor, we can replace almost all the calls in
the rest of the codebase, which currently do something akin to
"ReplaceSearchTerms(terms, NO_SUGGESTIONS_AVAILABLE, std::string())" with the
somewhat-more-readable-and-less-confusing
"ReplaceSearchTerms(SearchTermsArgs(terms))".  This makes the overall change a
bit less hacky, at the cost of requiring you to touch more code :)
Can we at least eliminate the stuff dealing with aq, leaving oq and aqs?

[Bart N, 2012/06/15 18:07:34]

Done, as we discussed.
On 2012/06/14 03:18:17, Peter Kasting wrote:

+  int index = 0;
+  for (ACMatches::iterator match(result->begin()); match != result->end();
+       ++match, ++index) {
+    if (match->provider != search_provider_)
+      continue;
+    const TemplateURL* template_url = match->GetTemplateURL(profile_);
+    if (!template_url || !template_url->url_ref().SupportsAssistedQueryStats())
+      continue;
+    std::string url(match->destination_url.spec());
+    if (url.find("&aqs=") != std::string::npos)
+      continue;
+    url.append(base::StringPrintf("&aqs=%d", index)).append(".").append(aqs);
+    match->destination_url = GURL(url);
+  }
+}
+
 void AutocompleteController::UpdateKeywordDescriptions(
     AutocompleteResult* result) {
   string16 last_keyword;