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..7d57a3c7a2edd552dbb889257ad2d7d696372798 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"
@@ -1044,6 +1045,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 +1102,86 @@ void AutocompleteController::UpdateAssociatedKeywords(
   }
 }
 
+namespace {

[msw, 2012/06/13 22:39:33]

anon namespaces and contents belong at the top of the file; ~line 57

[Bart N, 2012/06/13 23:17:13]

Done.

+
+// Converts the given type to an integer based on the AQS specification.

[msw, 2012/06/13 22:39:33]

Are these magic numbers really necessary? Why not use the existing
AutocompleteMatch::Type values?

[Bart N, 2012/06/13 23:17:13]

These values must map to existing enum on the server side. I can't reuse
existing values. One question though - should I create constants for these
values and put it somewhere in anon namespace?

[msw, 2012/06/13 23:42:08]

I wouldn't bother, but maybe add a vague statement about where they map from.
(can't link to server code, but you can say "These numbers match those in server
code, see <enum name>." or similar.

+int AutocompletMatchToAssistedQueryType(const AutocompleteMatch::Type& type) {

[msw, 2012/06/13 22:39:33]

Spelling: Autocomplet*e*MatchToAssistedQueryType

[Bart N, 2012/06/13 23:17:13]

Done.

+  switch (type) {
+    case AutocompleteMatch::SEARCH_SUGGEST:
+      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
+
+void AutocompleteController::UpdateAssistedQueryStats(
+    AutocompleteResult* result) {
+  if (result->empty())
+    return;
+
+  // Build the impressions string (the AQS part after ".").
+  std::string aqs;

[msw, 2012/06/13 22:39:33]

Use string16 if possible.

[Bart N, 2012/06/13 23:17:13]

Hmmm.. a few methods broke when I switched to string16. For instance, I'm using
StringAppendF which doesn't support string16, is there any alternative for that?

[msw, 2012/06/13 23:42:08]

Use + and += operators, IIRC. See base/string16.h and base/string_util.h, etc.
for more as needed. 

+  int count = 0;
+  int last_type = -1;
+  for (ACMatches::iterator match(result->begin()); match != result->end();

[msw, 2012/06/13 22:39:33]

Is there a reason for combining consecutive matches by type here? Does it make
sense to just list each match type here and analyze them as needed offline?

[Bart N, 2012/06/13 23:17:13]

Yes, it's required by the spec. For instance, instead of 0j0j0, we have to send
0l3 (length=3).

[msw, 2012/06/13 23:42:08]

Fair enough.

+       ++match) {
+    int type = AutocompletMatchToAssistedQueryType(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);

[msw, 2012/06/13 22:39:33]

Should this be done even if there were no matches?

[Bart N, 2012/06/13 23:17:13]

There is always at least one. See the check at the top of this function.

[msw, 2012/06/13 23:42:08]

Doh! Thanks for the heads up on the empty check.

+
+  // Go over all matches and set AQS if the match supports it.
+  int index = 0;
+  for (AutocompleteResult::iterator i = result->begin(); i != result->end();

[msw, 2012/06/13 22:39:33]

nit: consistency is nice, consider matching your loop above.

[Bart N, 2012/06/13 23:17:13]

Ah yes, done.

+       ++i, ++index) {
+    if (i->provider != search_provider_)

[msw, 2012/06/13 22:39:33]

It'd be nice if we could put this logic in SearchProvider match generation
itself, but I guess that doesn't work if we need to know the final aggregated
ordering of all the match types from all the providers.

[Bart N, 2012/06/13 23:17:13]

Currently, I rely on the fact that we check for SupportsAssistedQueryStats.
Actually,  this check may be entirely unnecessary, because only Google Provider,
which is a SearchProvider, will have it set. Anyway, let me know.

[msw, 2012/06/13 23:42:08]

So, I'm not terribly familiar with AQS or what you're trying to achieve. It
looks like you need data about all the aggregated matches from various
providers, right? If so, then this seems to be the right place for this code. If
you only need to know about the matches that the SearchProvider creates (and not
know for example, that a history match listed above/between them, or some search
matches weren't included in the displayed set, etc.), then move this code to
SearchProvider.

+      continue;
+    const TemplateURL* template_url = i->GetTemplateURL(profile_);
+    if (!template_url || !template_url->url_ref().SupportsAssistedQueryStats())
+      continue;
+    std::string url(i->destination_url.spec());
+    if (url.find("&aqs=") != std::string::npos)
+      continue;
+    url.append(base::StringPrintf("&aqs=%d", index)).append(".").append(aqs);

[msw, 2012/06/13 22:39:33]

Whoa, we're sending the ordered list of match types to the search provider? Has
that gone through privacy review?

[Bart N, 2012/06/13 23:17:13]

Yes. It was approved today, I can forward an email if you are interested in.

[msw, 2012/06/13 23:42:08]

You should link this CL to a chromium bug (set BUG=###### in the CL
description). In my experience, there's security/privacy review tags there (or
in a corresponding bug) as needed; that'd bring peace of mind to your reviewers
:)

+    i->destination_url = GURL(url);
+
+    VLOG(1) << "[" << i->provider->name() << "] URL = " << i->destination_url;

[msw, 2012/06/13 22:39:33]

Will this LOG help end users? if not, please remove.

[Bart N, 2012/06/13 23:17:13]

Removed.

+  }
+}
+
 void AutocompleteController::UpdateKeywordDescriptions(
     AutocompleteResult* result) {
   string16 last_keyword;