[AiS] Provide Answers queries with a session token.

chrome/browser/autocomplete/search_provider.cc

Index: chrome/browser/autocomplete/search_provider.cc
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index 3c6a77d235c939175a6063732b08c586767e04a1..a3941a4036b6953b20d7168ca828b15991f1a8a2 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -7,13 +7,16 @@
 #include <algorithm>
 #include <cmath>
 
+#include "base/base64.h"
 #include "base/callback.h"
+#include "base/command_line.h"
 #include "base/i18n/break_iterator.h"
 #include "base/i18n/case_conversion.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
+#include "base/rand_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier.h"
@@ -34,6 +37,7 @@
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ui/search/instant_controller.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/user_metrics.h"
@@ -637,6 +641,11 @@ net::URLFetcher* SearchProvider::CreateSuggestFetcher(
   TemplateURLRef::SearchTermsArgs search_term_args(input.text());
   search_term_args.cursor_position = input.cursor_position();
   search_term_args.page_classification = input.current_page_classification();
+#if defined(OS_ANDROID)

[Peter Kasting, 2014/05/09 21:35:07]

Why is this ifdef necessary?  It seems harmless to run this code for other
platforms.

[groby-ooo-7-16, 2014/05/09 21:50:25]

That's because the switch is Android-only :) 

It looks like Answers in general will have a small code footprint, so unless
that raises serious concerns, I'll just make it a cross-platform flag.

Would you mind a follow-up CL, since there are still CLs in flight with #if
OS_ANDROID?

[Peter Kasting, 2014/05/09 22:29:24]

Fine with me.

+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableAnswersInSuggest))

[Peter Kasting, 2014/05/09 21:35:07]

Nit: Indent 4, not 8

[groby-ooo-7-16, 2014/05/09 22:18:09]

Done.

+    search_term_args.session_token = GetSessionToken();
+#endif
   GURL suggest_url(template_url->suggestions_url_ref().ReplaceSearchTerms(
       search_term_args));
   if (!suggest_url.is_valid())
@@ -1130,3 +1139,18 @@ void SearchProvider::UpdateDone() {
   // pending, and we're not waiting on Instant.
   done_ = !timer_.IsRunning() && (suggest_results_pending_ == 0);
 }
+
+std::string SearchProvider::GetSessionToken() {
+  // Renew token if it expired.
+  if (base::Time::Now() > token_expiration_time_) {

[Peter Kasting, 2014/05/09 21:35:07]

Time::Now() can go backward (e.g. for DST), leaving you with the same token for
a long time.  Perhaps you want to use TimeTicks, which won't decrease.  (Even
that could cease to increment for arbitrarily long times while the computer is
suspended -- is that a problem?)

[groby-ooo-7-16, 2014/05/09 21:50:25]

I picked Time because it's not affected by suspension. Long-lived tokens are not
really an issue - the server-side cache will just expire anyways. The only
reason to expire the token quickly is to not tie together search query history
unnecessarily. I don't think if it's an hour on rare cases causes significant
issues there.

[groby-ooo-7-16, 2014/05/09 22:18:09]

Done - TimeTicks

+    const int kTokenBytes = 12;

[Peter Kasting, 2014/05/09 21:35:07]

This should be a size_t

[groby-ooo-7-16, 2014/05/09 22:18:09]

Done.

+    std::string raw_data;
+    base::RandBytes(WriteInto(&raw_data, kTokenBytes + 1), kTokenBytes);
+    base::Base64Encode(raw_data, &current_token_);
+  }
+
+  // Extend expiration time another 60 seconds.
+  token_expiration_time_ = base::Time::Now() + base::TimeDelta::FromSeconds(60);

[Peter Kasting, 2014/05/09 21:35:07]

Fetch the time once atop the function instead of calling Now() repeatedly.

[groby-ooo-7-16, 2014/05/09 22:18:09]

Done.

+
+  return current_token_;
+}