Omnibox: Truncate HistoryContents Searches at 10k Characters

Omnibox: Truncate HistoryContents Searches at 10k Characters

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

[Peter Kasting, 2013-04-17 22:34:57]

https://codereview.chromium.org/14330012/diff/1/chrome/browser/autocomplete/h...
File chrome/browser/autocomplete/history_contents_provider.cc (right):

https://codereview.chromium.org/14330012/diff/1/chrome/browser/autocomplete/h...
chrome/browser/autocomplete/history_contents_provider.cc:41: const size_t
kMaxCharactersToConsider = 10000u;
Even 10K seems like it will take a while.  What about truncating at like... 2K? 
I can't imagine there'd be a noticeable quality difference between the two.

[Mark P, 2013-04-17 22:58:22]

https://codereview.chromium.org/14330012/diff/1/chrome/browser/autocomplete/h...
File chrome/browser/autocomplete/history_contents_provider.cc (right):

https://codereview.chromium.org/14330012/diff/1/chrome/browser/autocomplete/h...
chrome/browser/autocomplete/history_contents_provider.cc:41: const size_t
kMaxCharactersToConsider = 10000u;
On 2013/04/17 22:34:57, Peter Kasting wrote:
> Even 10K seems like it will take a while.  What about truncating at like...
2K? 
> I can't imagine there'd be a noticeable quality difference between the two.

2k is also okay with me.  Done.

[Peter Kasting, 2013-04-17 22:59:45]

LGTM

https://codereview.chromium.org/14330012/diff/6001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/history_contents_provider.cc (right):

https://codereview.chromium.org/14330012/diff/6001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_contents_provider.cc:41: const size_t
kMaxCharactersToConsider = 2000u;
Nit: Since this is only used in one function I suggest declaring it in the most
local scope there.  (This could be done with kDaysToSearch too.)

[Mark P, 2013-04-17 23:06:48]

https://codereview.chromium.org/14330012/diff/6001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/history_contents_provider.cc (right):

https://codereview.chromium.org/14330012/diff/6001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_contents_provider.cc:41: const size_t
kMaxCharactersToConsider = 2000u;
On 2013/04/17 22:59:45, Peter Kasting wrote:
> Nit: Since this is only used in one function I suggest declaring it in the
most
> local scope there.  (This could be done with kDaysToSearch too.)

I kind of like having parameters called out somewhere, like this, at the top of
a file.  Besides that function is kind of long already; this would add another
doezen lines to it.

If you insist I'll do it.

[Peter Kasting, 2013-04-17 23:09:01]

https://codereview.chromium.org/14330012/diff/6001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/history_contents_provider.cc (right):

https://codereview.chromium.org/14330012/diff/6001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_contents_provider.cc:41: const size_t
kMaxCharactersToConsider = 2000u;
On 2013/04/17 23:06:48, Mark P wrote:
> On 2013/04/17 22:59:45, Peter Kasting wrote:
> > Nit: Since this is only used in one function I suggest declaring it in the
> most
> > local scope there.  (This could be done with kDaysToSearch too.)
> 
> I kind of like having parameters called out somewhere, like this, at the top
of
> a file.  Besides that function is kind of long already; this would add another
> doezen lines to it.
> 
> If you insist I'll do it.

I don't care strongly.  I always read the style guide's "declare things in the
most local scope possible" as applying to constants too, so that's the source of
the suggestion.  Up to you whether you agree with that interpretation.

[Mark P, 2013-04-17 23:13:28]

https://codereview.chromium.org/14330012/diff/6001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/history_contents_provider.cc (right):

https://codereview.chromium.org/14330012/diff/6001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_contents_provider.cc:41: const size_t
kMaxCharactersToConsider = 2000u;
On 2013/04/17 23:09:01, Peter Kasting wrote:
> On 2013/04/17 23:06:48, Mark P wrote:
> > On 2013/04/17 22:59:45, Peter Kasting wrote:
> > > Nit: Since this is only used in one function I suggest declaring it in the
> > most
> > > local scope there.  (This could be done with kDaysToSearch too.)
> > 
> > I kind of like having parameters called out somewhere, like this, at the top
> of
> > a file.  Besides that function is kind of long already; this would add
another
> > doezen lines to it.
> > 
> > If you insist I'll do it.
> 
> I don't care strongly.  I always read the style guide's "declare things in the
> most local scope possible" as applying to constants too, so that's the source
of
> the suggestion.  Up to you whether you agree with that interpretation.

Okay, done.

[commit-bot: I haz the power, 2013-04-18 15:08:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/14330012/5002

[commit-bot: I haz the power, 2013-04-18 17:33:33]

Change committed as 194960