Create Private Data for InMemoryURLIndex (in Preparation for SQLite Cache)

Create Private Data for InMemoryURLIndex (in Preparation for SQLite Cache) 

1. Encapsulate the private, persistent data for the InMemoryURLIndex in a new class, URLIndexPrivateData (found in in_memory_url_index_types.h).
2. Move most of the support types, including the new URLIndexPrivateData class, into in_memory_url_index_types.h. 
3. Correctly handle the adding and removing of page title words when a URL change is detected. 
4. Replace static class member functions with non-friend, non-class functions for better flexibility. 
5. Move convenience types out from InMemoryURLIndex class up into history namespace. 
6. Rename convenience types to generalize their intent. 

BUG=92718
TEST=Enhanced unit tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107893

[mrossetti, 2011-10-20 23:44:08]

This, again, is essentially a subset of the same code you previous reviewed (in
http://codereview.chromium.org/8120004/). I have added a new unit test for
proving that changing the title of a history item results in the expected search
results. I also introduced a URLRow::set_id function which when previously
suggested a year ago caused some concern.

[mrossetti, 2011-10-21 19:43:57]

All bots green.

[Peter Kasting, 2011-10-24 22:01:40]

LGTM (I kinda skimmed it)

http://codereview.chromium.org/8359019/diff/1/chrome/browser/history/history_...
File chrome/browser/history/history_types.h (right):

http://codereview.chromium.org/8359019/diff/1/chrome/browser/history/history_...
chrome/browser/history/history_types.h:77: void set_id(URLID id) { id_ = id; }
Nit: Given that the comments on |id_| below talk about this being immutable, it
seems like you should both update those comments and add a note here about how
this should be correctly used?

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

http://codereview.chromium.org/8359019/diff/1/chrome/browser/history/in_memor...
chrome/browser/history/in_memory_url_index.cc:190: return true;
Nit: I just noticed that this function (old and new versions) never returns
false.  Seems like it should be void then.

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

http://codereview.chromium.org/8359019/diff/1/chrome/browser/history/in_memor...
chrome/browser/history/in_memory_url_index_types.h:30: : term_num(term_num),
offset(offset), length(length) {}
Nit: My reading is that each member needs its own line if the declaration +
initializers don't all fit on one line (as they do above).

[mrossetti, 2011-10-25 16:15:05]

Thanks for the review.

As soon as I can satisfy the Win trybot I'll commit.

http://codereview.chromium.org/8359019/diff/1/chrome/browser/history/history_...
File chrome/browser/history/history_types.h (right):

http://codereview.chromium.org/8359019/diff/1/chrome/browser/history/history_...
chrome/browser/history/history_types.h:77: void set_id(URLID id) { id_ = id; }
On 2011/10/24 22:01:40, Peter Kasting wrote:
> Nit: Given that the comments on |id_| below talk about this being immutable,
it
> seems like you should both update those comments and add a note here about how
> this should be correctly used?

Done.

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

http://codereview.chromium.org/8359019/diff/1/chrome/browser/history/in_memor...
chrome/browser/history/in_memory_url_index.cc:190: return true;
On 2011/10/24 22:01:40, Peter Kasting wrote:
> Nit: I just noticed that this function (old and new versions) never returns
> false.  Seems like it should be void then.

Done.

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

http://codereview.chromium.org/8359019/diff/1/chrome/browser/history/in_memor...
chrome/browser/history/in_memory_url_index_types.h:30: : term_num(term_num),
offset(offset), length(length) {}
On 2011/10/24 22:01:40, Peter Kasting wrote:
> Nit: My reading is that each member needs its own line if the declaration +
> initializers don't all fit on one line (as they do above).

Done.