Make starred History*Provider results stay in the autocomplete dropdown, update tests

Make starred History*Provider results stay in the autocomplete dropdown, update tests.

Don't delete bookmarked AutoCompleteMatches from the dropdown list, remove their history data and hide their backing store instead. Expose HistoryProvider::DeleteMatch() and add delete tests.

BUG=67822
TEST=New unit tests + Check that starred History*Provider results aren't removed from the omnibox autocomplete dropdown set of matches.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=70648

[msw, 2010-12-23 13:42:29]

This behavior is better, but we should investigate the reappearance of page
titles for starred and what-you-typed matches.

[Paweł Hajdan Jr., 2011-01-03 08:51:12]

Drive-by with a minor test comment. No need to wait for another review by me.

http://codereview.chromium.org/6078005/diff/1/chrome/browser/autocomplete/his...
File chrome/browser/autocomplete/history_contents_provider_unittest.cc (right):

http://codereview.chromium.org/6078005/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider_unittest.cc:51:
MessageLoop::current()->PostTask(FROM_HERE,
nit: This is on the same message loop, so why not just call
provider()->DeleteMatch(match) directly?

[msw, 2011-01-04 07:57:27]

Thanks for the drive-by CR!
See my comment, and please do get back to me!

http://codereview.chromium.org/6078005/diff/1/chrome/browser/autocomplete/his...
File chrome/browser/autocomplete/history_contents_provider_unittest.cc (right):

http://codereview.chromium.org/6078005/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider_unittest.cc:51:
MessageLoop::current()->PostTask(FROM_HERE,
On 2011/01/03 08:51:12, Paweł Hajdan Jr. wrote:
> nit: This is on the same message loop, so why not just call
> provider()->DeleteMatch(match) directly?

That was my initial strategy, but then |state_| is null and triggers a
NOTREACHED() in the partial callstack:
MessageLoop::Quit()
HistoryContentsProvider::OnProviderUpdate()
HistoryProvider::DeleteMatch()

I believe OnProviderUpdate() assumes it's called in a message loop, but that's
not true for a direct DeleteMatch() call in this test run. I thought my
workaround would be adequate, but please enlighten me or let me know if I should
dig deeper! Here's the full unit_tests.exe main thread callstack with the
workaround HistoryContentsProviderTest::DeleteMatch():

MessageLoop::Quit()
`anonymous namespace'::HistoryContentsProviderTest::OnProviderUpdate(bool
updated_matches=true)
HistoryProvider::DeleteMatch(const AutocompleteMatch & match={...})
DispatchToMethod<HistoryContentsProvider,void (__thiscall
HistoryProvider::*)(AutocompleteMatch const
&),AutocompleteMatch>(HistoryContentsProvider * obj=0x010a19a0, void (const
AutocompleteMatch &)* method=0x020909b0, const Tuple1<AutocompleteMatch> &
arg={...})
RunnableMethod<HistoryContentsProvider,void (__thiscall
HistoryProvider::*)(AutocompleteMatch const &),Tuple1<AutocompleteMatch>
>::Run()
MessageLoop::RunTask(Task * task=0x0109f140)
MessageLoop::DeferOrRunPendingTask(const MessageLoop::PendingTask &
pending_task={...})
MessageLoop::DoWork()
base::MessagePumpForUI::DoRunLoop()
base::MessagePumpWin::RunWithDispatcher(base::MessagePump::Delegate *
delegate=0x010a3a10, base::MessagePumpWin::Dispatcher * dispatcher=0x00000000)
base::MessagePumpWin::Run(base::MessagePump::Delegate * delegate=0x010a3a10)
MessageLoop::RunInternal()
MessageLoop::RunHandler()
MessageLoop::Run()
`anonymous namespace'::HistoryContentsProviderTest::DeleteMatch(const
AutocompleteMatch & match={...})
`anonymous namespace'::HistoryContentsProviderTest_DeleteMatch_Test::TestBody()
testing::HandleExceptionsInMethodIfSupported<testing::Test,void>(testing::Test *
object=0x010a3a00, void (void)* method=0x03199890, const char *
location=0x06465ab0)
testing::Test::Run()
testing::TestInfo::Run()
testing::TestCase::Run()
testing::internal::UnitTestImpl::RunAllTests()
testing::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>(testing::internal::UnitTestImpl
* object=0x010a4000, bool (void)* method=0x03181b60, const char *
location=0x06466310)
testing::UnitTest::Run()
base::TestSuite::Run()
main(int argc=3, char * * argv=0x01094000)
__tmainCRTStartup()

[Paweł Hajdan Jr., 2011-01-04 16:00:13]

http://codereview.chromium.org/6078005/diff/1/chrome/browser/autocomplete/his...
File chrome/browser/autocomplete/history_contents_provider_unittest.cc (right):

http://codereview.chromium.org/6078005/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider_unittest.cc:51:
MessageLoop::current()->PostTask(FROM_HERE,
On 2011/01/04 07:57:27, msw wrote:
> On 2011/01/03 08:51:12, Paweł Hajdan Jr. wrote:
> > nit: This is on the same message loop, so why not just call
> > provider()->DeleteMatch(match) directly?
> 
> That was my initial strategy, but then |state_| is null and triggers a
> NOTREACHED() in the partial callstack:
> MessageLoop::Quit()
> HistoryContentsProvider::OnProviderUpdate()
> HistoryProvider::DeleteMatch()
> 
> I believe OnProviderUpdate() assumes it's called in a message loop, but that's
> not true for a direct DeleteMatch() call in this test run.

Right, calling Quit on a stopped message loop is a bit weird.

But then, OnProviderUpdate is a part of this test, so it can be change not to
quit the loop if we don't want that. It might be indeed more complicated than
just using PostTask here, so it's up to you and Peter what to do. I'm fine with
the current state of this patch. It may be worth to comment why PostTask is
being used, and remove the "Run the message loop" comment above
MessageLoop::current()->Run().

[Peter Kasting, 2011-01-04 22:16:01]

LGTM

http://codereview.chromium.org/6078005/diff/1/chrome/browser/autocomplete/his...
File chrome/browser/autocomplete/history_contents_provider_unittest.cc (right):

http://codereview.chromium.org/6078005/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider_unittest.cc:51:
MessageLoop::current()->PostTask(FROM_HERE,
On 2011/01/04 07:57:27, msw wrote:
> On 2011/01/03 08:51:12, Paweł Hajdan Jr. wrote:
> > nit: This is on the same message loop, so why not just call
> > provider()->DeleteMatch(match) directly?
> 
> That was my initial strategy, but then |state_| is null and triggers a
> NOTREACHED() in the partial callstack

There are three ways to fix.  One way is what you did, preferably with an added
comment explaining why you need to call Run().  The second is to add a flag to
HistoryContentsProviderTest that says whether OnProviderUpdate() actually needs
to Quit().  The third is to add a running() function to MessageLoop and check
that directly from OnProviderUpdate().

The third route seems cleanest but I'm not sure why we don't have this already. 
Maybe it's intentional.  You could ask Darin.

http://codereview.chromium.org/6078005/diff/1/chrome/browser/autocomplete/his...
File chrome/browser/autocomplete/history_provider.cc (right):

http://codereview.chromium.org/6078005/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_provider.cc:25: void
HistoryProvider::DeleteMatch(const AutocompleteMatch& match) {
Nit: Move above constructor

[msw, 2011-01-06 19:06:31]

Okay, I've taken a different approach after doing a bit of debugging and talking
to Darin. Now, I'm running DeleteMatch() directly (not posting a task) and then
posting a QuitTask in OnProviderUpdate() to quit the message loop, if it's
running.

Side note: I thought it'd be simple to replace Run() calls with RunAllPending(),
and then OnProviderUpdate() shouldn't need to quit a loop at all. Unfortunately,
that prevents the history thread from spinning up to complete its message loop
tasks and communicate with the main thread message loop, etc.

[Peter Kasting, 2011-01-06 19:10:38]

LGTM

http://codereview.chromium.org/6078005/diff/9001/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_contents_provider_unittest.cc (right):

http://codereview.chromium.org/6078005/diff/9001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_contents_provider_unittest.cc:93: // We must
quit the message loop (if running) to return control to the test.
Nit: I'd probably add "Note that calling Quit() directly will checkfail if the
loop isn't running, so we post a task, which is safe for either case."

[msw, 2011-01-06 20:24:56]

Added extra Quit() comment... will land after lunch.

http://codereview.chromium.org/6078005/diff/9001/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_contents_provider_unittest.cc (right):

http://codereview.chromium.org/6078005/diff/9001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_contents_provider_unittest.cc:93: // We must
quit the message loop (if running) to return control to the test.
On 2011/01/06 19:10:39, Peter Kasting wrote:
> Nit: I'd probably add "Note that calling Quit() directly will checkfail if the
> loop isn't running, so we post a task, which is safe for either case."

Done.