HQP Refactoring (in Preparation for SQLite Cache)

HQP Refactoring (in Preparation for SQLite Cache) 

1. Move ownership of the InMemoryURLIndex from the InMemoryHistoryBackend to the HistoryBackend where it truly belongs. 
2. Encapsulate the private, persistent data for the InMemoryURLIndex in a new class, URLIndexPrivateData. 
3. Handle (by notification) URL visits, updates and deletes. Refactor use of NOTIFICATION_HISTORY_URLS_DELETED to provide the deleted URLRow so that row ID is available. 
4. Correctly handle the adding and removing of page title words when a URL change is detected. 
5. Move most of the support types, including the new URLIndexPrivateData class, into a new file, in_memory_url_index_types.h. 
6. Replace static class member functions with non-friend, non-class functions for better flexibility. 
7. Move convenience types out from InMemoryURLIndex class up into history namespace. 
8. Rename convenience types to generalize their intent. 
9. Other small cleanups. 

BUG=96731, 92718
TEST=Unit tests updated. 
TBR=atwilson,brettw

[mrossetti, 2011-10-03 19:44:05]

Most of these changes have been reviewed previously. The major, new changes of
note are:

1) the proper handling of words that may have changed in a page title,
2) properly deleting words from page titles for deleted visits, (see
InMemoryURLIndex::AddRowWordsToIndex, InMemoryURLIndex::RemoveRowFromIndex, and
RemoveRowWordsFromIndex) and
3) the addition of a private data class for encapsulating IMUI's persistent data
(see in_memory_url_index_types.h/.cc).

[GeorgeY, 2011-10-04 18:13:28]

LGTM

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index_types.cc (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.cc:154:
new_word_id_set.begin()));
I wonder how fast it is going to be:
looks like O(N*log(M)), where N == term_chars.size() and M is the number of
words with a letter. The question is how big are N and M?

[Peter Kasting, 2011-10-05 00:11:42]

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete.cc:508: << name()
Nit: Also acceptable to put this on the end of the previous line.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_provider.cc (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_provider.cc:34: DCHECK(history_service &&
match.destination_url.is_valid());
Nit: I'd use two DCHECKs.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_provider.h (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_provider.h:58: // Find and remove the match
from the current collection of matches.
Nit: This comment doesn't mention removing the match from any backing data
structures/databases.

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

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_quick_provider.h:53: static const int
kMaxNonInliningScore;
I suspect this is leftover from merges, since we removed this constant in the
static initializer purge (
http://src.chromium.org/viewvc/chrome?view=rev&revision=99036 ).

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/histo...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/histo...
chrome/browser/history/history_backend.cc:220:
switches::kDisableHistoryQuickProvider)) {
Nit: No need for {}

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/histo...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/histo...
chrome/browser/history/history_backend.h:553: Profile* profile_;  // The profile
may be NULL during unittests.
Where do you use this?

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

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:120: : profile_(profile),
Nit: It doesn't seem like you need to keep this as a member, just use it when
constructing |source| below?

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:124: Source<Profile>
source(profile_);
Seems like we should take explicit action when |profile| is NULL, either to not
register listeners or to register for all sources (or something)?

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:194: row !=
details.changed_urls.end(); ++row) {
Nit: No {} necessary (2 places)

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

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.h:263: void OnURLsVisited(const
URLVisitedDetails& details);
Nit: Should be singular?

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index_types.cc (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.cc:43:
clean_matches.push_back(last_match);
Nit: You can remove this and the +1 below if you remove the "=
sorted_matches[0]" in the line above and add a NULL-constructor to TermMatch. 
Maybe not a net win.  Your choice.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.cc:176: uni_string,
Nit: This could go on previous line

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.cc:177: break_on_space ?
base::i18n::BreakIterator::BREAK_SPACE
Nit: I normally suggest placing the ':' on the end of the first line

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index_types.h (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.h:42: // other terms.
Nit: Might want to describe 2K clamp behavior

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.h:49: // WAS: SortAndDeoverlap
Nit: Remove this line?

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.h:52: // Extract and return the
offsets from |matches|.
Nit: Extracts and returns

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.h:55: // Replace the offsets in
|matches| with those given in |offsets|, deleting
Nit: Replaces and returns

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.h:95: // resulting list will
contain strings broken at whitespace.
Nit: This description seems incomplete given your example below, which
apparently breaks after "http://" with this argument set to true.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index_types_unittest.cc (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types_unittest.cc:78: const size_t
expected_offsets[] = { 1, 3, 5, 7, 9, 12, 14, 17, 19, 21, 24 };
Nit: If you place this above the ASSERT_EQ you can use arraysize() there and
below.

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

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:175:
ASSERT_TRUE(actual.end() != actual_iter);
Nit: If you can't use ASSERT_NE(), then I suggest reversing the order of these
two values.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:181:
EXPECT_GT(actual_values.count(*set_iter), 0U);
This doesn't do a precise equivalence comparison.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:636: // available_words_
will already be empty since we have freshly built the
Nit: Would it be better to just EXPECT_TRUE() this?

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:684:
ASSERT_FALSE(private_data.history_info_map_.end() == actual);
Nit: Same comment as earlier regarding variable order.

[mrossetti, 2011-10-07 17:04:14]

This is ready for review. Thanks for all of the great comments!

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete.cc:508: << name()
On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: Also acceptable to put this on the end of the previous line.

Done.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_provider.cc (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_provider.cc:34: DCHECK(history_service &&
match.destination_url.is_valid());
On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: I'd use two DCHECKs.

Done.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_provider.h (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_provider.h:58: // Find and remove the match
from the current collection of matches.
On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: This comment doesn't mention removing the match from any backing data
> structures/databases.

Done.

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

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_quick_provider.h:53: static const int
kMaxNonInliningScore;
On 2011/10/05 00:11:42, Peter Kasting wrote:
> I suspect this is leftover from merges, since we removed this constant in the
> static initializer purge (
> http://src.chromium.org/viewvc/chrome?view=rev&revision=99036 ).

Done.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/histo...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/histo...
chrome/browser/history/history_backend.cc:220:
switches::kDisableHistoryQuickProvider)) {
On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: No need for {}

Done.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/histo...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/histo...
chrome/browser/history/history_backend.h:553: Profile* profile_;  // The profile
may be NULL during unittests.
Only for initializing the InMemoryURLIndex instance, which takes place in the
ctor, so I've removed the data member.

On 2011/10/05 00:11:42, Peter Kasting wrote:
> Where do you use this?

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

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:120: : profile_(profile),
Removed for now. In the next CL it'll probably be coming back.

On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: It doesn't seem like you need to keep this as a member, just use it when
> constructing |source| below?

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:124: Source<Profile>
source(profile_);
On 2011/10/05 00:11:42, Peter Kasting wrote:
> Seems like we should take explicit action when |profile| is NULL, either to
not
> register listeners or to register for all sources (or something)?

Done.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.cc:194: row !=
details.changed_urls.end(); ++row) {
On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: No {} necessary (2 places)

Gotcha! I was under the mistaken notion that if the 'if' or 'for' portion was
longer than one line then the body needed the braces.

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

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index.h:263: void OnURLsVisited(const
URLVisitedDetails& details);
Yep, changed.

On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: Should be singular?

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index_types.cc (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.cc:43:
clean_matches.push_back(last_match);
Yeah, I like that. More 'algorithmic'.

On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: You can remove this and the +1 below if you remove the "=
> sorted_matches[0]" in the line above and add a NULL-constructor to TermMatch. 
> Maybe not a net win.  Your choice.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.cc:176: uni_string,
On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: This could go on previous line

Done.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.cc:177: break_on_space ?
base::i18n::BreakIterator::BREAK_SPACE
On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: I normally suggest placing the ':' on the end of the first line

Done.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index_types.h (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.h:42: // other terms.
Excellent point! Done.

On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: Might want to describe 2K clamp behavior

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.h:49: // WAS: SortAndDeoverlap
On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: Remove this line?

Done.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.h:52: // Extract and return the
offsets from |matches|.
On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: Extracts and returns

Done.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.h:55: // Replace the offsets in
|matches| with those given in |offsets|, deleting
On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: Replaces and returns

Done.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types.h:95: // resulting list will
contain strings broken at whitespace.
On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: This description seems incomplete given your example below, which
> apparently breaks after "http://" with this argument set to true.

Done.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index_types_unittest.cc (right):

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_types_unittest.cc:78: const size_t
expected_offsets[] = { 1, 3, 5, 7, 9, 12, 14, 17, 19, 21, 24 };
On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: If you place this above the ASSERT_EQ you can use arraysize() there and
> below.

Done.

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

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:175:
ASSERT_TRUE(actual.end() != actual_iter);
On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: If you can't use ASSERT_NE(), then I suggest reversing the order of these
> two values.

Done.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:181:
EXPECT_GT(actual_values.count(*set_iter), 0U);
On 2011/10/05 00:11:42, Peter Kasting wrote:
> This doesn't do a precise equivalence comparison.

Done.

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:636: // available_words_
will already be empty since we have freshly built the
Sure, just to be safe.

On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: Would it be better to just EXPECT_TRUE() this?

http://codereview.chromium.org/8120004/diff/3032/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:684:
ASSERT_FALSE(private_data.history_info_map_.end() == actual);
On 2011/10/05 00:11:42, Peter Kasting wrote:
> Nit: Same comment as earlier regarding variable order.

Done.

[Peter Kasting, 2011-10-07 17:50:36]

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/history_provider.h (right):

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/autocomplete...
chrome/browser/autocomplete/history_provider.h:58: // Find and remove the match
from the current collection of matches and
Nit: Finds and removes

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/hist...
File chrome/browser/history/history.cc (right):

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/hist...
chrome/browser/history/history.cc:177: InMemoryIndex()->ShutDown();
Nit: Shouldn't this be "Shutdown()" since it's one word?

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
File chrome/browser/history/in_memory_url_index.cc (right):

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index.cc:279: HistoryIDSet history_ids =
private_data.word_id_history_map_[word_id];
Why do you copy this out here, modify the copy, and then copy it back in?  Isn't
it safe to modify the set in place even if we're going to delete the last item
in it?

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index.cc:293: WordIDSet word_ids =
private_data.char_word_map_[uni_char];
Same comment.

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index.cc:431: // While the URL is
guaranteed to remain stable, the title may have change.
Nit: change -> changed

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
File chrome/browser/history/in_memory_url_index_types.cc (right):

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index_types.cc:25: location =
short_string.find(term, location + 1)) {
Nit: You can omit {} if you want

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
File chrome/browser/history/in_memory_url_index_types.h (right):

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index_types.h:29: : term_num(0),
Nit: You could probably fit all initializers on the above line

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index_types.h:126: typedef int WordID;
Nit: I wonder if this should be a size_t instead as that is really what the list
indexes are.

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index_types.h:176: // replacing a
potentially long and repeated string with a simple index.
Nit: This motivation sentence is kind of unclear to me.  Maybe I'm just too
ignorant of how the HQP code works, but to me this would be clearer in a more
expanded form, a la: "Without this list, we would need to x y z.  This would
mean we'd duplicate the words n times.  By using the list instead, we can a b c
and thereby save 62.37% of the memory."

For example, it's not obvious to me why you couldn't ditch the list entirely,
and keep the map of strings->IDs but just generate new IDs for new words using a
monotonically increasing counter.  If you're worried about overflow it could be
a 64-bit counter.

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index_types.h:186: // by future URL
updates.
Nit: You might want to motivate why this trick is useful and why you don't
simply compact the word_list_ on removing a word.  (I assume because that would
mean all the map entries would have to update their IDs, which would be
prohibitively expensive.)

[mrossetti, 2011-10-07 22:24:36]

All issues addressed except for reworking the word index which I propose doing
in the final protobuf to SQLite modifications. Also, discovered and removed an
unused variable.

I will keep an eye on the Win failure in OmniboxViewTest.DeleteItem and fix if
it comes up again.

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/history_provider.h (right):

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/autocomplete...
chrome/browser/autocomplete/history_provider.h:58: // Find and remove the match
from the current collection of matches and
On 2011/10/07 17:50:37, Peter Kasting wrote:
> Nit: Finds and removes

Done.

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/hist...
File chrome/browser/history/history.cc (right):

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/hist...
chrome/browser/history/history.cc:177: InMemoryIndex()->ShutDown();
On 2011/10/07 17:50:37, Peter Kasting wrote:
> Nit: Shouldn't this be "Shutdown()" since it's one word?

I was viewing it as the verb "shut down" vs. the noun "shutdown".

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
File chrome/browser/history/in_memory_url_index.cc (right):

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index.cc:279: HistoryIDSet history_ids =
private_data.word_id_history_map_[word_id];
Yeah, great catch! There used to be some more code in there but I didn't clean
things up when it was removed.

On 2011/10/07 17:50:37, Peter Kasting wrote:
> Why do you copy this out here, modify the copy, and then copy it back in? 
Isn't
> it safe to modify the set in place even if we're going to delete the last item
> in it?

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index.cc:293: WordIDSet word_ids =
private_data.char_word_map_[uni_char];
On 2011/10/07 17:50:37, Peter Kasting wrote:
> Same comment.

Done.

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index.cc:431: // While the URL is
guaranteed to remain stable, the title may have change.
On 2011/10/07 17:50:37, Peter Kasting wrote:
> Nit: change -> changed

Done.

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
File chrome/browser/history/in_memory_url_index_types.cc (right):

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index_types.cc:25: location =
short_string.find(term, location + 1)) {
On 2011/10/07 17:50:37, Peter Kasting wrote:
> Nit: You can omit {} if you want

Done.

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
File chrome/browser/history/in_memory_url_index_types.h (right):

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index_types.h:29: : term_num(0),
On 2011/10/07 17:50:37, Peter Kasting wrote:
> Nit: You could probably fit all initializers on the above line

Done.

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index_types.h:126: typedef int WordID;
On 2011/10/07 17:50:37, Peter Kasting wrote:
> Nit: I wonder if this should be a size_t instead as that is really what the
list
> indexes are.

Done.

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index_types.h:176: // replacing a
potentially long and repeated string with a simple index.
Yes, I was concerned about overflow but your observation is spot on and I want
to make the suggested change.

I'd like to postpone this change, though, because doing so would require adding
migration code to the protobuf since I'd need to add the new 'last_used_word_id'
to the protobuf cache description and determine that value when reading an older
version of the protobuf. I'd rather make this change when I switch to SQLite if
that's okay with you.

On 2011/10/07 17:50:37, Peter Kasting wrote:
> Nit: This motivation sentence is kind of unclear to me.  Maybe I'm just too
> ignorant of how the HQP code works, but to me this would be clearer in a more
> expanded form, a la: "Without this list, we would need to x y z.  This would
> mean we'd duplicate the words n times.  By using the list instead, we can a b
c
> and thereby save 62.37% of the memory."
> 
> For example, it's not obvious to me why you couldn't ditch the list entirely,
> and keep the map of strings->IDs but just generate new IDs for new words using
a
> monotonically increasing counter.  If you're worried about overflow it could
be
> a 64-bit counter.

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index_types.h:186: // by future URL
updates.
See above comment.

On 2011/10/07 17:50:37, Peter Kasting wrote:
> Nit: You might want to motivate why this trick is useful and why you don't
> simply compact the word_list_ on removing a word.  (I assume because that
would
> mean all the map entries would have to update their IDs, which would be
> prohibitively expensive.)

[Peter Kasting, 2011-10-07 23:01:28]

Rubber-stamp LGTM (I didn't re-review the latest patch)

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
File chrome/browser/history/in_memory_url_index_types.h (right):

http://codereview.chromium.org/8120004/diff/18001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index_types.h:176: // replacing a
potentially long and repeated string with a simple index.
On 2011/10/07 22:24:37, mrossetti wrote:
> I'd like to postpone this change, though, because doing so would require
adding
> migration code to the protobuf since I'd need to add the new
'last_used_word_id'
> to the protobuf cache description and determine that value when reading an
older
> version of the protobuf. I'd rather make this change when I switch to SQLite
if
> that's okay with you.

That's fine.

[mrossetti, 2011-10-13 03:46:12]

I need OWNERS reviews on a couple of files, please.

brettw or avi for chrome/browser/sync/profile_sync_service_typed_url_unittest.cc

atwilson or akalin for content/browser/cancelable_request.h

Thanks!

[mrossetti, 2011-10-13 13:57:20]

I'm TBRing this for brettw on cancelable_request.h, and for atwilson on
profile_sync_service_typed_url_unittest.cc.