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 HistoryService, where it truly belongs.
2. 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.
3. Correctly handle the adding and removing of page title words when a URL change is detected.
4. Other small cleanups.

BUG=96731, 92718
TEST=Unit tests updated.
TBR=atwilson (for profile_sync_service_typed_url_unittest.cc)
Previously reviewed as: http://codereview.chromium.org/8384024/
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111378

[mrossetti, 2011-11-08 04:44:21]

I've moved the InMemoryURLIndex up to the HistoryService as its owner. This was
done because registration for notifications had to be handled on the same thread
which was difficult and unnecessarily complex from the HistoryBackend. What I've
done, though, is almost identical to the initialization of the
InMemoryHistoryBackend, which is also managed by the HistoryService.

That's the primary change in this CL beyond the stuff which you have previously
reviewed but I thought you should take a look before I commit.

[mrossetti, 2011-11-09 16:03:47]

Peter,

The area of this CL in which you may have particular interest is in
history.h/.cc where I have transferred ownership of the InMemoryURLIndex to the
HistoryService. You have reviewed the rest of the changes in a previous CL.

Mike

On 2011/11/08 04:44:21, mrossetti wrote:
> I've moved the InMemoryURLIndex up to the HistoryService as its owner. This
was
> done because registration for notifications had to be handled on the same
thread
> which was difficult and unnecessarily complex from the HistoryBackend. What
I've
> done, though, is almost identical to the initialization of the
> InMemoryHistoryBackend, which is also managed by the HistoryService.
> 
> That's the primary change in this CL beyond the stuff which you have
previously
> reviewed but I thought you should take a look before I commit.

[Peter Kasting, 2011-11-14 21:17:35]

I'm sorry I've not gotten to this.  I was unexpectedly OOO several days last
week and am WebKit sheriff this week.  I will try to get to this as soon as I
can.

[mrossetti, 2011-11-14 21:32:30]

Thanks for the note Peter. It's no problem — I figured something came up.

I think http://codereview.chromium.org/8526010/ is more important in the
short term if you get a minute.

Let me know if there's anything I can do to help out — I know you're
carrying a lot.

Mike

On Mon, Nov 14, 2011 at 2:17 PM, <pkasting@chromium.org> wrote:

> I'm sorry I've not gotten to this.  I was unexpectedly OOO several days
> last
> week and am WebKit sheriff this week.  I will try to get to this as soon
> as I
> can.
>
>
http://codereview.chromium.**org/8451009/<http://codereview.chromium.org/8451...
>

[Peter Kasting, 2011-11-21 19:07:50]

LGTM, I didn't think about the implications of the ownership move so I'm
trusting it makes sense

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

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/hist...
chrome/browser/history/history.cc:160: new history::InMemoryURLIndex(profile,
history_dir_));
Nit: Use |profile_| for consistency.

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/hist...
chrome/browser/history/history.cc:162: if (profile) {
Nit: This conditional is unnecessary given that we DCHECK(profile_) above.

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/hist...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/hist...
chrome/browser/history/history.h:521: history::InMemoryURLIndex* InMemoryIndex()
const;
Nit: const mehtods should not return non-const pointers.  Make this method
non-const.

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/hist...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:213:
bookmark_service_(bookmark_service) {}
Nit: I normally prefer the braces on separate lines unless the entire
constructor definition exists on a single line.

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

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index.h:70: explicit
InMemoryURLIndex(Profile* profile,
Nit: "explicit" no longer needed

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index.h:195: // register to be notified
when the history database becomes available.
Nit: register -> registers

[mrossetti, 2011-11-21 21:53:25]

Thanks for the review Peter.

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

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/hist...
chrome/browser/history/history.cc:160: new history::InMemoryURLIndex(profile,
history_dir_));
On 2011/11/21 19:07:50, Peter Kasting wrote:
> Nit: Use |profile_| for consistency.

Done.

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/hist...
chrome/browser/history/history.cc:162: if (profile) {
On 2011/11/21 19:07:50, Peter Kasting wrote:
> Nit: This conditional is unnecessary given that we DCHECK(profile_) above.

Done.

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/hist...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/hist...
chrome/browser/history/history.h:521: history::InMemoryURLIndex* InMemoryIndex()
const;
On 2011/11/21 19:07:50, Peter Kasting wrote:
> Nit: const mehtods should not return non-const pointers.  Make this method
> non-const.

Done.

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/hist...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:213:
bookmark_service_(bookmark_service) {}
On 2011/11/21 19:07:50, Peter Kasting wrote:
> Nit: I normally prefer the braces on separate lines unless the entire
> constructor definition exists on a single line.

Done.

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

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index.h:70: explicit
InMemoryURLIndex(Profile* profile,
On 2011/11/21 19:07:50, Peter Kasting wrote:
> Nit: "explicit" no longer needed

Done.

http://codereview.chromium.org/8451009/diff/25002/chrome/browser/history/in_m...
chrome/browser/history/in_memory_url_index.h:195: // register to be notified
when the history database becomes available.
On 2011/11/21 19:07:50, Peter Kasting wrote:
> Nit: register -> registers

Done.