Move Ownership of IMUI to HistoryService.

Move Ownership of IMUI to HistoryService.

This change moves the InMemoryURLIndex (IMUI) from being owned by the InMemoryHistoryBackend to being owned by the HistoryService. The only dependency the IMUI has on the HistoryBackend is the need to access the history database for the one-time need to build the IMUI from scratch. Divorcing the IMUI from the HistoryBackend also pointed out that updates to the IMUI index related to URL visits and history deletes should be handled by notification, and this was implemented in this CL.

NOTE: Portions previously reviewed as: http://codereview.chromium.org/9030031/. That change involved both 1) moving the ownership of the IMUI from the InMemoryHistoryBackend, and 2) performing the caching file operations on the FILE thread. I've split this up into two CLs with this CL dealing with the move of ownership.

BUG=83659
TEST=Updated unit tests.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120707

[mrossetti, 2012-02-04 00:26:37]

As mentioned in the description, this is just the portion of the previous
refactoring CL that deals with moving ownership of the IMUI from the
InMemoryHistoryBackend and adding use of notifications for handling visit
events.

[mrossetti, 2012-02-04 00:38:37]

Just a couple of notes about changing std::vector<URLRow>'s to URLRows.

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

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/history/...
chrome/browser/history/history_backend.cc:764: void
HistoryBackend::AddPagesWithDetails(const std::vector<URLRow>& urls,
FYI, I will change all of the std::vector<URLRow>'s to URLRows in a separate CL.

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

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/history/...
chrome/browser/history/in_memory_history_backend.cc:137: for
(std::vector<URLRow>::const_iterator row = details.rows.begin();
Oops, missed this one. Will replace with URLRows on the next pass.

[Peter Kasting, 2012-02-04 02:20:31]

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_quick_provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:161: // DO WE
WANT THIS CHANGE?
?

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

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/history/...
chrome/browser/history/history.cc:253: #if 0
Shouldn't have #if 0s by commit time...

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/history/...
File chrome/browser/history/history_unittest.cc (left):

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/history/...
chrome/browser/history/history_unittest.cc:118: new HistoryBackend(history_dir_,
0, new BackendDelegate(this), NULL);
Nit: Either way is fine with me, honestly

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

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/history/...
chrome/browser/history/in_memory_history_backend.h:47: URLDatabase* db);
Nit: Can probably fit on previous line?

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

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/history/...
chrome/browser/history/in_memory_url_index.cc:195: &cache_reader_consumer_);
So much saner than the old threading model!  Yay!

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

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/history/...
chrome/browser/history/in_memory_url_index.h:99: // actually changed. For unit
testing only.
Nit: If this is the case maybe these function names should have "ForTesting" on
the ends?  Although it looks like maybe you're actually calling these
internally... in which case I wonder if this comment shouldn't be updated or
removed, or maybe these should be private and the relevant test made a
friend...?

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

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/history/...
chrome/browser/history/in_memory_url_index_unittest.cc:17: #include
"chrome/browser/history/history.h"
Nit: This goes above other two

[mrossetti, 2012-02-04 04:30:52]

Issues addressed.

Primarily, removed UpdateURL and DeleteURL functions only needed by unit tests.
The unit tests now use pass-through functions.

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_quick_provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:161: // DO WE
WANT THIS CHANGE?
On 2012/02/04 02:20:31, Peter Kasting wrote:
> ?

I apologize. I thought I'd gotten all the conditionals. :(

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

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/history/...
chrome/browser/history/history.cc:253: #if 0
On 2012/02/04 02:20:31, Peter Kasting wrote:
> Shouldn't have #if 0s by commit time...

No, again I'm embarrassed.

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

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/history/...
chrome/browser/history/in_memory_history_backend.cc:137: for
(std::vector<URLRow>::const_iterator row = details.rows.begin();
On 2012/02/04 00:38:38, mrossetti wrote:
> Oops, missed this one. Will replace with URLRows on the next pass.

Done.

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

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/history/...
chrome/browser/history/in_memory_history_backend.h:47: URLDatabase* db);
On 2012/02/04 02:20:31, Peter Kasting wrote:
> Nit: Can probably fit on previous line?

Done.

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

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/history/...
chrome/browser/history/in_memory_url_index.cc:195: &cache_reader_consumer_);
On 2012/02/04 02:20:31, Peter Kasting wrote:
> So much saner than the old threading model!  Yay!

:^)

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

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/history/...
chrome/browser/history/in_memory_url_index.h:99: // actually changed. For unit
testing only.
On 2012/02/04 02:20:31, Peter Kasting wrote:
> Nit: If this is the case maybe these function names should have "ForTesting"
on
> the ends?  Although it looks like maybe you're actually calling these
> internally... in which case I wonder if this comment shouldn't be updated or
> removed, or maybe these should be private and the relevant test made a
> friend...?

Ack! They weren't used anywhere else except the unit tests until I changed the
observer functions to call them.

So I've eliminated these functions, added passthrough functions to the unit test
class, and changed the observer functions back to directly calling the
private_data directly. A similar change was required in
history_quick_provider_unittest.c.

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

https://chromiumcodereview.appspot.com/9316109/diff/1/chrome/browser/history/...
chrome/browser/history/in_memory_url_index_unittest.cc:17: #include
"chrome/browser/history/history.h"
On 2012/02/04 02:20:31, Peter Kasting wrote:
> Nit: This goes above other two

Done.

[Peter Kasting, 2012-02-06 19:28:20]

LGTM

[Peter Kasting, 2012-02-07 00:12:22]

The qualifiers will make Linux gcc work.  This may be a gcc bug.

http://codereview.chromium.org/9316109/diff/14001/chrome/browser/history/in_m...
File chrome/browser/history/in_memory_url_index.h (right):

http://codereview.chromium.org/9316109/diff/14001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index.h:100: friend class
HistoryQuickProviderTest;
Nit: Add "::" before classname

http://codereview.chromium.org/9316109/diff/14001/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index.h:177: URLIndexPrivateData*
private_data() const { return private_data_.get(); }
Nit: This should not be const

http://codereview.chromium.org/9316109/diff/14001/chrome/browser/history/url_...
File chrome/browser/history/url_index_private_data.h (right):

http://codereview.chromium.org/9316109/diff/14001/chrome/browser/history/url_...
chrome/browser/history/url_index_private_data.h:35: friend class
HistoryQuickProviderTest;
Nit: Add "::" before classname

[mrossetti, 2012-02-07 01:10:13]

Sweet! Thanks for the assist! The linux compiler is much happier now.

https://chromiumcodereview.appspot.com/9316109/diff/14001/chrome/browser/hist...
File chrome/browser/history/in_memory_url_index.h (right):

https://chromiumcodereview.appspot.com/9316109/diff/14001/chrome/browser/hist...
chrome/browser/history/in_memory_url_index.h:100: friend class
HistoryQuickProviderTest;
On 2012/02/07 00:12:22, Peter Kasting wrote:
> Nit: Add "::" before classname

Ah!

https://chromiumcodereview.appspot.com/9316109/diff/14001/chrome/browser/hist...
chrome/browser/history/in_memory_url_index.h:177: URLIndexPrivateData*
private_data() const { return private_data_.get(); }
On 2012/02/07 00:12:22, Peter Kasting wrote:
> Nit: This should not be const

Done.

https://chromiumcodereview.appspot.com/9316109/diff/14001/chrome/browser/hist...
File chrome/browser/history/url_index_private_data.h (right):

https://chromiumcodereview.appspot.com/9316109/diff/14001/chrome/browser/hist...
chrome/browser/history/url_index_private_data.h:35: friend class
HistoryQuickProviderTest;
On 2012/02/07 00:12:22, Peter Kasting wrote:
> Nit: Add "::" before classname

Done.