[AiS] Provide Answers queries with a session token.

[AiS] Provide Answers queries with a session token.

Answers requires a session token to utilize the caching for Answers.
The token has a lifetime of one minute, and the expiration time gets
refreshed with each new query sent.

BUG=370192
R=pkasting@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270307

[groby-ooo-7-16, 2014-05-09 20:58:29]

If I had known where the tree was headed, I would've rolled it into the previous
CL. (And yes, the version is not updated. I'll wait till the previous one lands
before doing that...)

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

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:644: #if defined(OS_ANDROID)
Why is this ifdef necessary?  It seems harmless to run this code for other
platforms.

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:646:
switches::kEnableAnswersInSuggest))
Nit: Indent 4, not 8

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:1145: if (base::Time::Now() >
token_expiration_time_) {
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?)

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:1146: const int kTokenBytes = 12;
This should be a size_t

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:1153: token_expiration_time_ =
base::Time::Now() + base::TimeDelta::FromSeconds(60);
Fetch the time once atop the function instead of calling Now() repeatedly.

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.h:67:
FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, SessionToken);
Nit: List tests here in the same order as the test file

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.h:275: // Obtain a session token,
regenerating if necessary.
Nit: Obtains

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider_unittest.cc:3097: // Make sure the
default providers suggest service was queried.
Nit: provider's

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider_unittest.cc:3104:
search_terms_args.session_token = provider_->current_token_;
Can this use GetSessionToken()?  If so, perhaps these tests can be un-friended
just by making that function public.

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider_unittest.cc:3109: // complete
running the fetcher to clean up.
Nit: Capitalize

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider_unittest.cc:3117: const
base::TimeDelta small_delta = base::TimeDelta::FromMilliseconds(1);
Nit: Declare this just above first use

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider_unittest.cc:3130:
provider_->current_token_ = "";
Nit: .clear()

https://codereview.chromium.org/276073002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url.cc (right):

https://codereview.chromium.org/276073002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/template_url.cc:973: std::string token =
search_terms_args.session_token;
Nit: Either inline this directly into the uses below, or use a const string&.

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

While I'm working on the fixes, a few quick questions

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:644: #if defined(OS_ANDROID)
On 2014/05/09 21:35:07, Peter Kasting wrote:
> Why is this ifdef necessary?  It seems harmless to run this code for other
> platforms.

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?

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:1145: if (base::Time::Now() >
token_expiration_time_) {
On 2014/05/09 21:35:07, Peter Kasting wrote:
> 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?)

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.

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.h:67:
FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, SessionToken);
On 2014/05/09 21:35:07, Peter Kasting wrote:
> Nit: List tests here in the same order as the test file

Ah. I assumed alphabetical order.

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider_unittest.cc:3104:
search_terms_args.session_token = provider_->current_token_;
On 2014/05/09 21:35:07, Peter Kasting wrote:
> Can this use GetSessionToken()?  If so, perhaps these tests can be un-friended
> just by making that function public.

Second test needs access to expiration time, so we can't do that. This one
could, but it relies on there not being 60 seconds between QueryForInput and the
check. Which means both this and the next test could flake around DST :(

In light of that, I think you're right, TimeTicks is the better choice, even
though it means clock suspension extends the lifetime of the token.

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

And here are the fixes - PTAL

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:646:
switches::kEnableAnswersInSuggest))
On 2014/05/09 21:35:07, Peter Kasting wrote:
> Nit: Indent 4, not 8

Done.

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:1145: if (base::Time::Now() >
token_expiration_time_) {
On 2014/05/09 21:50:25, groby wrote:
> On 2014/05/09 21:35:07, Peter Kasting wrote:
> > 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?)
> 
> 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.

Done - TimeTicks

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:1146: const int kTokenBytes = 12;
On 2014/05/09 21:35:07, Peter Kasting wrote:
> This should be a size_t

Done.

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:1153: token_expiration_time_ =
base::Time::Now() + base::TimeDelta::FromSeconds(60);
On 2014/05/09 21:35:07, Peter Kasting wrote:
> Fetch the time once atop the function instead of calling Now() repeatedly.

Done.

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.h:67:
FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, SessionToken);
On 2014/05/09 21:50:25, groby wrote:
> On 2014/05/09 21:35:07, Peter Kasting wrote:
> > Nit: List tests here in the same order as the test file
> 
> Ah. I assumed alphabetical order.

Done.

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.h:275: // Obtain a session token,
regenerating if necessary.
On 2014/05/09 21:35:07, Peter Kasting wrote:
> Nit: Obtains

Done.

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider_unittest.cc:3097: // Make sure the
default providers suggest service was queried.
On 2014/05/09 21:35:07, Peter Kasting wrote:
> Nit: provider's

Done.

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider_unittest.cc:3109: // complete
running the fetcher to clean up.
On 2014/05/09 21:35:07, Peter Kasting wrote:
> Nit: Capitalize

Done.

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider_unittest.cc:3117: const
base::TimeDelta small_delta = base::TimeDelta::FromMilliseconds(1);
On 2014/05/09 21:35:07, Peter Kasting wrote:
> Nit: Declare this just above first use

Done.

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider_unittest.cc:3130:
provider_->current_token_ = "";
On 2014/05/09 21:35:07, Peter Kasting wrote:
> Nit: .clear()

Done.

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

LGTM

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/276073002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:644: #if defined(OS_ANDROID)
On 2014/05/09 21:50:25, groby wrote:
> On 2014/05/09 21:35:07, Peter Kasting wrote:
> > Why is this ifdef necessary?  It seems harmless to run this code for other
> > platforms.
> 
> 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?

Fine with me.

https://codereview.chromium.org/276073002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/276073002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:3130: const
base::TimeDelta small_delta = base::TimeDelta::FromMilliseconds(1);
Nit: Could probably be kSmallDelta?  (I'm not 100% clear on the rules)

[groby-ooo-7-16, 2014-05-09 23:22:41]

https://codereview.chromium.org/276073002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/276073002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:3130: const
base::TimeDelta small_delta = base::TimeDelta::FromMilliseconds(1);
On 2014/05/09 22:29:24, Peter Kasting wrote:
> Nit: Could probably be kSmallDelta?  (I'm not 100% clear on the rules)

kSmallDelta indeed. Fixed. (All constants are k, even local ones)

[groby-ooo-7-16, 2014-05-09 23:22:45]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2014-05-09 23:27:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/groby@chromium.org/276073002/40001

[commit-bot: I haz the power, 2014-05-10 02:52:13]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-10 03:02:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-10 03:02:03]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[groby-ooo-7-16, 2014-05-13 23:30:05]

FYI: Updated version number.

[groby-ooo-7-16, 2014-05-13 23:30:10]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2014-05-13 23:30:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/groby@chromium.org/276073002/60001

[commit-bot: I haz the power, 2014-05-14 02:57:45]

Change committed as 270307