Gather word-start Information to Aid in Scoring.

Gather word-start Information to Aid in Scoring.

This change implements word start gathering and provides those word starts to the scoring routine but does not yet implement consideration of those word starts in the actual score calculation.

BUG=NONE
TEST=Added and updated unit tests.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126661

[mrossetti, 2012-03-08 21:26:40]

pkasting, please give the basic logic of collecting word starts and the unit
tests a close look.

brettw or sky, I need an owner to look this over, too.

[Peter Kasting, 2012-03-09 02:37:43]

LGTM

https://chromiumcodereview.appspot.com/9655003/diff/1/chrome/browser/history/...
File chrome/browser/history/in_memory_url_index_types.cc (right):

https://chromiumcodereview.appspot.com/9655003/diff/1/chrome/browser/history/...
chrome/browser/history/in_memory_url_index_types.cc:136: string16
original_word(iter.GetString());
Nit: Shorter:

  if (trimmed & TRIM_LEADING)
    word_start += iter.GetString().find(word);

I'm a little concerned about the time cost of the find() call.  I wonder if it
doesn't make sense to do something more akin to:

  string16 word(iter.GetString());
  size_t initial_whitespace = 0;
  if (break_on_space) {
    string16 trimmed_word;
    TrimWhitespace(word, TRIM_LEADING, &trimmed_word);
    initial_whitespace = word.length() - trimmed_word.length();
    TrimWhitespace(trimmed_word, TRIM_TRAILING, &word);
  }

I don't know whether this makes much practical difference.

https://chromiumcodereview.appspot.com/9655003/diff/1/chrome/browser/history/...
File chrome/browser/history/in_memory_url_index_types.h (right):

https://chromiumcodereview.appspot.com/9655003/diff/1/chrome/browser/history/...
chrome/browser/history/in_memory_url_index_types.h:91: // is not NULL then
clears and pushes the word starts onto |word_starts|.
Nit: You might want to change "word starts" to something more descriptive, e.g.
"offsets within |uni_string| at which each word starts".  Also, if that's a
correct description, then you should be using size_t for these instead of int.

https://chromiumcodereview.appspot.com/9655003/diff/1/chrome/browser/history/...
File chrome/browser/history/in_memory_url_index_types_unittest.cc (right):

https://chromiumcodereview.appspot.com/9655003/diff/1/chrome/browser/history/...
chrome/browser/history/in_memory_url_index_types_unittest.cc:18: const int*
expected, size_t expected_size, const T& actual) {
Nit: One arg per line if they don't all fit on the first line

https://chromiumcodereview.appspot.com/9655003/diff/1/chrome/browser/history/...
chrome/browser/history/in_memory_url_index_types_unittest.cc:46: int
expected_starts_a[] = {0, 7, 11, 18, 23, 31, 35};
Nit: Use size_t in all these too

https://chromiumcodereview.appspot.com/9655003/diff/1/chrome/browser/history/...
File chrome/browser/history/url_index_private_data.cc (right):

https://chromiumcodereview.appspot.com/9655003/diff/1/chrome/browser/history/...
chrome/browser/history/url_index_private_data.cc:1048: // allow testing of cache
file upgrading in ReadFromFile().
Nit: Is this the best way of doing this?  For most other versioned data storage
migration tests, we keep a copy of an old version in chrome/test/data and then
write code that directly opens and migrates it to check that it does what we
want.  This avoids being as intrusive to the real code that saves these.

https://chromiumcodereview.appspot.com/9655003/diff/1/chrome/browser/history/...
chrome/browser/history/url_index_private_data.cc:1284: for
(RepeatedField<int32>::const_iterator jiter = url_starts.begin();
Nit: Argh... use |i| and |j| rather than |iter| and |jiter|, as |jiter| looks
like a misspelling of "jitter"

[sky, 2012-03-09 16:56:07]

Rubber stamp LGTM from me

[mrossetti, 2012-03-14 23:23:49]

Cleaning out draft comments.

http://codereview.chromium.org/9655003/diff/1/chrome/browser/history/in_memor...
File chrome/browser/history/in_memory_url_index_types.cc (right):

http://codereview.chromium.org/9655003/diff/1/chrome/browser/history/in_memor...
chrome/browser/history/in_memory_url_index_types.cc:136: string16
original_word(iter.GetString());
Done! That's a great approach.

On 2012/03/09 02:37:43, Peter Kasting wrote:
> Nit: Shorter:
> 
>   if (trimmed & TRIM_LEADING)
>     word_start += iter.GetString().find(word);
> 
> I'm a little concerned about the time cost of the find() call.  I wonder if it
> doesn't make sense to do something more akin to:
> 
>   string16 word(iter.GetString());
>   size_t initial_whitespace = 0;
>   if (break_on_space) {
>     string16 trimmed_word;
>     TrimWhitespace(word, TRIM_LEADING, &trimmed_word);
>     initial_whitespace = word.length() - trimmed_word.length();
>     TrimWhitespace(trimmed_word, TRIM_TRAILING, &word);
>   }
> 
> I don't know whether this makes much practical difference.

http://codereview.chromium.org/9655003/diff/1/chrome/browser/history/in_memor...
File chrome/browser/history/in_memory_url_index_types.h (right):

http://codereview.chromium.org/9655003/diff/1/chrome/browser/history/in_memor...
chrome/browser/history/in_memory_url_index_types.h:91: // is not NULL then
clears and pushes the word starts onto |word_starts|.
On 2012/03/09 02:37:43, Peter Kasting wrote:
> Nit: You might want to change "word starts" to something more descriptive,
e.g.
> "offsets within |uni_string| at which each word starts".  Also, if that's a
> correct description, then you should be using size_t for these instead of int.

Comment updated to be more clear. Also changed to size_t. I'm also introducing a
type (WordStarts) so that we will only have to change this in one place if a
change is called for in the future.

http://codereview.chromium.org/9655003/diff/1/chrome/browser/history/in_memor...
File chrome/browser/history/in_memory_url_index_types_unittest.cc (right):

http://codereview.chromium.org/9655003/diff/1/chrome/browser/history/in_memor...
chrome/browser/history/in_memory_url_index_types_unittest.cc:18: const int*
expected, size_t expected_size, const T& actual) {
On 2012/03/09 02:37:43, Peter Kasting wrote:
> Nit: One arg per line if they don't all fit on the first line

Done.

http://codereview.chromium.org/9655003/diff/1/chrome/browser/history/in_memor...
chrome/browser/history/in_memory_url_index_types_unittest.cc:46: int
expected_starts_a[] = {0, 7, 11, 18, 23, 31, 35};
On 2012/03/09 02:37:43, Peter Kasting wrote:
> Nit: Use size_t in all these too

Done.

http://codereview.chromium.org/9655003/diff/1/chrome/browser/history/url_inde...
File chrome/browser/history/url_index_private_data.cc (right):

http://codereview.chromium.org/9655003/diff/1/chrome/browser/history/url_inde...
chrome/browser/history/url_index_private_data.cc:1048: // allow testing of cache
file upgrading in ReadFromFile().
On 2012/03/09 02:37:43, Peter Kasting wrote:
> Nit: Is this the best way of doing this?  For most other versioned data
storage
> migration tests, we keep a copy of an old version in chrome/test/data and then
> write code that directly opens and migrates it to check that it does what we
> want.  This avoids being as intrusive to the real code that saves these.

I hear you, and understand. Since saving using protobuf is going to be replaced
in the near future (for some definition of 'near') and because this is pretty
complex data I'm going to leave it as-is for now and see how difficult it would
be to take the proper approach once this caching has moved over to SQLite. I've
added a TODO to remind me of this.