Make HistoryContentsProvider results deletable

Make HistoryContentsProvider results deletable

This change should allow you to press Shift+Delete to remove selected results from the Omnibox auto-complete drop-down that have been collected from the contents of pages from the user's history.

BUG=14748
TEST=trybots and deleting history-contents results from the omnibox auto-complete drop-down.

[msw, 2010-12-14 03:54:03]

Should this also remove results collected from bookmark titles
(AddBookmarkTitleMatchToResults)?

Should I be deleting any other corresponding data (result, MatchReference,
etc.)?

Please help me ensure this is the intended behavior.

[msw, 2010-12-15 21:54:26]

Brett, can you review this CL for me? (evanm suggested you!)
This appears to be the correct behavior after further investigation.
This only modifies the history contents, not bookmark status.

On 2010/12/14 03:54:03, msw wrote:
> Should this also remove results collected from bookmark titles
> (AddBookmarkTitleMatchToResults)?
> 
> Should I be deleting any other corresponding data (result, MatchReference,
> etc.)?
> 
> Please help me ensure this is the intended behavior.

[msw, 2010-12-15 21:56:08]

Ready for review, re-running trybots now.

[brettw, 2010-12-15 22:24:11]

http://codereview.chromium.org/5774004/diff/3001/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/5774004/diff/3001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_contents_provider.cc:164: if
(!history_service || !selected_url.is_valid()) {
This code is copied and pasted from the HistoryURLProvider. This is sort of
unfortunate. I wonder if we can have a static function in the HistoryURLProvider
(can you think of a better place? I'm not totally thrilled with it) for

static void DeleteMatchFromHistory(const AutocompleteMatch& match, ACMatches*
matches)

[msw, 2010-12-16 01:37:23]

On 2010/12/15 22:24:11, brettw wrote:
>
http://codereview.chromium.org/5774004/diff/3001/chrome/browser/autocomplete/...
> File chrome/browser/autocomplete/history_contents_provider.cc (right):
> 
>
http://codereview.chromium.org/5774004/diff/3001/chrome/browser/autocomplete/...
> chrome/browser/autocomplete/history_contents_provider.cc:164: if
> (!history_service || !selected_url.is_valid()) {
> This code is copied and pasted from the HistoryURLProvider. This is sort of
> unfortunate. I wonder if we can have a static function in the
HistoryURLProvider
> (can you think of a better place? I'm not totally thrilled with it) for
> 
> static void DeleteMatchFromHistory(const AutocompleteMatch& match, ACMatches*
> matches)

I moved DeleteMatch into HistoryProvider, and used that as
HistoryContentsProvider's base class instead. I also updated some includes. Let
me know what you think.

[msw, 2010-12-16 01:38:25]

Please review my updated CL that addresses your comment.

http://codereview.chromium.org/5774004/diff/3001/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/5774004/diff/3001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_contents_provider.cc:164: if
(!history_service || !selected_url.is_valid()) {
On 2010/12/15 22:24:11, brettw wrote:
> This code is copied and pasted from the HistoryURLProvider. This is sort of
> unfortunate. I wonder if we can have a static function in the
HistoryURLProvider
> (can you think of a better place? I'm not totally thrilled with it) for
> 
> static void DeleteMatchFromHistory(const AutocompleteMatch& match, ACMatches*
> matches)

How about using base class HistoryProvider?

[brettw, 2010-12-16 17:55:14]

LGTM

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

http://codereview.chromium.org/5774004/diff/10001/chrome/browser/autocomplete...
chrome/browser/autocomplete/history_provider.cc:31:
profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
This should be indented 2 more spaces, like it was in the original.

[Peter Kasting, 2010-12-17 19:32:01]

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

http://codereview.chromium.org/5774004/diff/10001/chrome/browser/autocomplete...
chrome/browser/autocomplete/history_provider.h:34: virtual void
DeleteMatch(const AutocompleteMatch& match) OVERRIDE;
This causes the HistoryQuickProvider to inherit this as well.  That's probably
problematic as I believe deleting matches there, while desirable, will require
additional bookkeeping.  You should probably scan that file or talk to mrossetti
so you can write an override in that class that calls this function and also
performs the additional work.