Implemented substring matching within page titles. Fixed bug where URL was be...

Implemented substring matching within page titles. Fixed bug where URL was being lower-cased. Added unit tests for page title matching as well as unit tests for various static functions and other areas not previously unit tested.

BUG=58958, 57853, 58559, 60107, 75245
TEST=Many unit tests added.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=77646

[Peter Kasting, 2011-03-09 01:57:53]

You don't mean bug 58753.

[Peter Kasting, 2011-03-09 02:31:48]

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_quick_provider.cc:110:
AutocompleteMatch::Type match_type = history_match.url_matches.empty() ?
Nit: Just inline this into the next expression.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_quick_provider.cc:179: const
history::TermMatches& matches, size_t text_length, size_t adjust) {
Nit: One arg per line

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_quick_provider.cc:190: size_t offset =
matches[i].offset - adjust;
It's not obvious from the surrounding code that |adjust| must not be greater
than matches[i].offset.  e.g. What happens when the user types "h", "t", or "p"?

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_quick_provider.cc:194: offset +=
matches[i].length;
Nit: Shorter:

    do {
      offset += matches[i].length;
      ++i;
    } while ((i < match_count) && (offset == matches[i].offset - adjust));

Doing this means you need to remove the "++i" from the outermost for().

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_quick_provider.h (right):

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_quick_provider.h:68: const
history::TermMatches& matches, size_t text_length, size_t adjust);
Nit: One arg per line

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index.cc (right):

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:95: int AddumulateMatchLength(int
total, const TermMatch& match) {
Nit: Addumulate -> Accumulate

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:419: string16
lower_string(upper_string);
Nit: How about:

  string16 lower_string;
  lower_string.reserve(upper_string.length());
  std::transform(upper_string.begin(), upper_string.end(),
std::back_inserter(lower_string), ...);

(Avoids a copy)

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:423: tolower);
Is this calling the C tolower() function?  If so, don't we want to be using
l10n_util or ICU instead?

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:443:
base::BreakIterator::BREAK_SPACE :
Nit: You can collapse lines 2 & 3 to a single line by indenting 4:

  base::BreakIterator iter(&uni_string, break_on_space ?
      base::BreakIterator::BREAK_SPACE : base::BreakIterator::BREAK_WORD);

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:451: if (word.size())
Nit: !word.empty()

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:458:
words.push_back(iter.GetString());
I confess, I'm a little confused why one arm of the conditional does this but
the other doesn't.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:725: if (matches[i-1].term_num >
matches[i].term_num)
Nit: Spaces around '-'

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:726: ++out_of_order;
Nit: Indent 2

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:739:
static_cast<float>(kMaxSignificantStart) * kStartMaxValue;
Nit: Is this second cast needed?

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index.h (right):

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.h:49: : term_num(term_num),
offset(offset), length(length) {}
Nit: One initializer per line when they don't all fit on the same line as the
function name.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index_unittest.cc (right):

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:105: URLRow
MakeURLRow(const char* url, const char* title, int visit_count,
Nit: One arg per line

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:224: // Insure title is
being searched.
Nit: Insure -> Ensure

[Paweł Hajdan Jr., 2011-03-09 10:16:35]

Drive-by with testing comments. This is mostly about the gyp change, I added the
second nit because I was looking at the test files anyway.

Please fix the issues, no need to wait for me.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index_unittest.cc (right):

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:136: return
FILE_PATH_LITERAL("url_history_provider_test_limited.db.txt");
nit: Please fix indentation, should be just 2 spaces not 4.

http://codereview.chromium.org/6652008/diff/6001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/6652008/diff/6001/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:1809:
'test/data/History/url_history_provider_test.db.txt',
Why are those data files being added to gyp? We have many many other data files
in chrome/tests that rather shouldn't be in gyp. Could you remove those two
lines?

I guess the resource.rc file just below might be somehow special/magical.

[mrossetti_google.com, 2011-03-09 17:01:11]

Fixed. Should have been 57853.

On Tue, Mar 8, 2011 at 6:57 PM, <pkasting@chromium.org> wrote:

> You don't mean bug 58753.
>
>
> http://codereview.chromium.org/6652008/
>

[mrossetti, 2011-03-10 01:31:14]

Ready for another go!

Thanks for that great catch! I changed how the 'http://' prefix was being
handled.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_quick_provider.cc:110:
AutocompleteMatch::Type match_type = history_match.url_matches.empty() ?
On 2011/03/09 02:31:48, Peter Kasting wrote:
> Nit: Just inline this into the next expression.

Done.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_quick_provider.cc:179: const
history::TermMatches& matches, size_t text_length, size_t adjust) {
On 2011/03/09 02:31:48, Peter Kasting wrote:
> Nit: One arg per line

Yep, I reread the style guide and get it now.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_quick_provider.cc:190: size_t offset =
matches[i].offset - adjust;
On 2011/03/09 02:31:48, Peter Kasting wrote:
> It's not obvious from the surrounding code that |adjust| must not be greater
> than matches[i].offset.  e.g. What happens when the user types "h", "t", or
"p"?

Ah! Excellent catch! I've modified the scoring function to eliminate
consideration of a prefix of 'http://'. Thank you.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_quick_provider.cc:194: offset +=
matches[i].length;
On 2011/03/09 02:31:48, Peter Kasting wrote:
> Nit: Shorter:
> 
>     do {
>       offset += matches[i].length;
>       ++i;
>     } while ((i < match_count) && (offset == matches[i].offset - adjust));
> 
> Doing this means you need to remove the "++i" from the outermost for().

Sweet!

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_quick_provider.h (right):

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_quick_provider.h:68: const
history::TermMatches& matches, size_t text_length, size_t adjust);
On 2011/03/09 02:31:48, Peter Kasting wrote:
> Nit: One arg per line

Done.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index.cc (right):

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:95: int AddumulateMatchLength(int
total, const TermMatch& match) {
On 2011/03/09 02:31:48, Peter Kasting wrote:
> Nit: Addumulate -> Accumulate

LOL

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:419: string16
lower_string(upper_string);
On 2011/03/09 02:31:48, Peter Kasting wrote:
> Nit: How about:
> 
>   string16 lower_string;
>   lower_string.reserve(upper_string.length());
>   std::transform(upper_string.begin(), upper_string.end(),
> std::back_inserter(lower_string), ...);
> 
> (Avoids a copy)

This function replaced with l10n_util::ToLower().

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:423: tolower);
On 2011/03/09 02:31:48, Peter Kasting wrote:
> Is this calling the C tolower() function?  If so, don't we want to be using
> l10n_util or ICU instead?

Done. Replaced function.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:443:
base::BreakIterator::BREAK_SPACE :
On 2011/03/09 02:31:48, Peter Kasting wrote:
> Nit: You can collapse lines 2 & 3 to a single line by indenting 4:
> 
>   base::BreakIterator iter(&uni_string, break_on_space ?
>       base::BreakIterator::BREAK_SPACE : base::BreakIterator::BREAK_WORD);

Done.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:451: if (word.size())
On 2011/03/09 02:31:48, Peter Kasting wrote:
> Nit: !word.empty()

Done.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:458:
words.push_back(iter.GetString());
On 2011/03/09 02:31:48, Peter Kasting wrote:
> I confess, I'm a little confused why one arm of the conditional does this but
> the other doesn't.

The iterator behaves differently depending on the breaking strategy, which was a
surprise to me, too. The BreakIterator unit tests do not properly test this case
as its basic word and space tests always use a test string starting with a
space; I verified this behavior through experimentation and implementing
specific unit tests in in_memory_url_index_unittest.cc. And I was loathe to
modify the behavior of BreakIterator as it would touch a lot of code.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:725: if (matches[i-1].term_num >
matches[i].term_num)
On 2011/03/09 02:31:48, Peter Kasting wrote:
> Nit: Spaces around '-'

Done.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:726: ++out_of_order;
On 2011/03/09 02:31:48, Peter Kasting wrote:
> Nit: Indent 2

Done.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:739:
static_cast<float>(kMaxSignificantStart) * kStartMaxValue;
On 2011/03/09 02:31:48, Peter Kasting wrote:
> Nit: Is this second cast needed?

Yes.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index.h (right):

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.h:49: : term_num(term_num),
offset(offset), length(length) {}
On 2011/03/09 02:31:48, Peter Kasting wrote:
> Nit: One initializer per line when they don't all fit on the same line as the
> function name.

Done.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index_unittest.cc (right):

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:105: URLRow
MakeURLRow(const char* url, const char* title, int visit_count,
On 2011/03/09 02:31:48, Peter Kasting wrote:
> Nit: One arg per line

Done.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:136: return
FILE_PATH_LITERAL("url_history_provider_test_limited.db.txt");
On 2011/03/09 10:16:35, Paweł Hajdan Jr. wrote:
> nit: Please fix indentation, should be just 2 spaces not 4.

Done.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:224: // Insure title is
being searched.
On 2011/03/09 02:31:48, Peter Kasting wrote:
> Nit: Insure -> Ensure

Done.

http://codereview.chromium.org/6652008/diff/6001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/6652008/diff/6001/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:1809:
'test/data/History/url_history_provider_test.db.txt',
On 2011/03/09 10:16:35, Paweł Hajdan Jr. wrote:
> Why are those data files being added to gyp? We have many many other data
files
> in chrome/tests that rather shouldn't be in gyp. Could you remove those two
> lines?
> 
> I guess the resource.rc file just below might be somehow special/magical.

Done.

[Peter Kasting, 2011-03-10 02:13:43]

LGTM

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_quick_provider.cc:190: size_t offset =
matches[i].offset - adjust;
On 2011/03/10 01:31:14, mrossetti wrote:
> On 2011/03/09 02:31:48, Peter Kasting wrote:
> > It's not obvious from the surrounding code that |adjust| must not be greater
> > than matches[i].offset.  e.g. What happens when the user types "h", "t", or
> "p"?
> 
> Ah! Excellent catch! I've modified the scoring function to eliminate
> consideration of a prefix of 'http://'. Thank you.

BTW, something you should know about scoring.

Prefixes like the scheme or an initial "ftp." or "www." matter to scoring.  We
want to score matches that don't match in those sections better than ones that
do.  For example, if I type "w", "washingtonmutual.com" is a better match than
"www.foo.com".

This detail is important because it prevents "w" or "h" from always
inline-autocompleting to the most-commonly visited URL (which is generally
"http://www....").  With Instant on, this is really noticeable.  There's
currently a bug against the HistoryURLProvider where we're not rigorous enough
about taking that into account.

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index.cc (right):

http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:458:
words.push_back(iter.GetString());
On 2011/03/10 01:31:14, mrossetti wrote:
> On 2011/03/09 02:31:48, Peter Kasting wrote:
> > I confess, I'm a little confused why one arm of the conditional does this
but
> > the other doesn't.
> 
> The iterator behaves differently depending on the breaking strategy, which was
a
> surprise to me, too. The BreakIterator unit tests do not properly test this
case
> as its basic word and space tests always use a test string starting with a
> space; I verified this behavior through experimentation and implementing
> specific unit tests in in_memory_url_index_unittest.cc. And I was loathe to
> modify the behavior of BreakIterator as it would touch a lot of code.

If BreakIterator should be changed, can you file a bug for that so we can at
least do it separately?

If not, maybe add some comments in here?

[mrossetti, 2011-03-10 16:10:26]

On 2011/03/10 02:13:43, Peter Kasting wrote:
> LGTM
> 
>
http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
> File chrome/browser/autocomplete/history_quick_provider.cc (right):
> 
>
http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
> chrome/browser/autocomplete/history_quick_provider.cc:190: size_t offset =
> matches[i].offset - adjust;
> On 2011/03/10 01:31:14, mrossetti wrote:
> > On 2011/03/09 02:31:48, Peter Kasting wrote:
> > > It's not obvious from the surrounding code that |adjust| must not be
greater
> > > than matches[i].offset.  e.g. What happens when the user types "h", "t",
or
> > "p"?
> > 
> > Ah! Excellent catch! I've modified the scoring function to eliminate
> > consideration of a prefix of 'http://'. Thank you.
> 
> BTW, something you should know about scoring.
> 
> Prefixes like the scheme or an initial "ftp." or "www." matter to scoring.  We
> want to score matches that don't match in those sections better than ones that
> do.  For example, if I type "w", "washingtonmutual.com" is a better match than
> "www.foo.com".
> 
> This detail is important because it prevents "w" or "h" from always
> inline-autocompleting to the most-commonly visited URL (which is generally
> "http://www....").  With Instant on, this is really noticeable.  There's
> currently a bug against the HistoryURLProvider where we're not rigorous enough
> about taking that into account.
> 
>
http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
> File chrome/browser/history/in_memory_url_index.cc (right):
> 
>
http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
> chrome/browser/history/in_memory_url_index.cc:458:
> words.push_back(iter.GetString());
> On 2011/03/10 01:31:14, mrossetti wrote:
> > On 2011/03/09 02:31:48, Peter Kasting wrote:
> > > I confess, I'm a little confused why one arm of the conditional does this
> but
> > > the other doesn't.
> > 
> > The iterator behaves differently depending on the breaking strategy, which
was
> a
> > surprise to me, too. The BreakIterator unit tests do not properly test this
> case
> > as its basic word and space tests always use a test string starting with a
> > space; I verified this behavior through experimentation and implementing
> > specific unit tests in in_memory_url_index_unittest.cc. And I was loathe to
> > modify the behavior of BreakIterator as it would touch a lot of code.
> 
> If BreakIterator should be changed, can you file a bug for that so we can at
> least do it separately?
> 
> If not, maybe add some comments in here?

I'm going to jostle the BreakIterator unit tests this morning to see what ought
to be done, write up a bug from that effort, and also add a TODO in here to come
back and update this if changes are made to the iterator.