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

chrome/browser/autocomplete/history_contents_provider_unittest.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete.h"
 #include "chrome/browser/autocomplete/autocomplete_match.h"
@@ skipping 29 matching lines @@
   void RunQuery(const AutocompleteInput& input,
                 bool minimal_changes) {
     provider_->Start(input, minimal_changes);
 
     // When we're waiting for asynchronous messages, we have to spin the message
     // loop. This will be exited in the OnProviderUpdate function when complete.
     if (!input.synchronous_only())
       MessageLoop::current()->Run();
   }
 
+  void DeleteMatch(const AutocompleteMatch& match) {
+    MessageLoop::current()->PostTask(FROM_HERE,

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

nit: This is on the same message loop, so why not just call
provider()->DeleteMatch(match) directly?

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

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]

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]

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.

+        NewRunnableMethod(provider(), &HistoryProvider::DeleteMatch, match));
+    // Run the message loop to execute the task.
+    MessageLoop::current()->Run();
+  }
+
   const ACMatches& matches() const { return provider_->matches(); }
 
   TestingProfile* profile() const { return profile_.get(); }
 
   HistoryContentsProvider* provider() const { return provider_.get(); }
 
  private:
   // testing::Test
   virtual void SetUp() {
     profile_.reset(new TestingProfile());
@@ skipping 117 matching lines @@
   AutocompleteInput async_input(L"bar", std::wstring(), true, false, true,
                                 false);
   provider()->Start(async_input, false);
   const ACMatches& m2 = matches();
   ASSERT_EQ(1U, m2.size());
   EXPECT_EQ(bookmark_url, m2[0].destination_url);
 
   // Run the message loop (needed for async history results).
   MessageLoop::current()->Run();
 
-  // We should two urls now, bookmark_url and http://www.google.com/3.
+  // We should have two urls now, bookmark_url and http://www.google.com/3.
   const ACMatches& m3 = matches();
   ASSERT_EQ(2U, m3.size());
   if (bookmark_url == m3[0].destination_url) {
     EXPECT_EQ("http://www.google.com/3", m3[1].destination_url.spec());
   } else {
     EXPECT_EQ(bookmark_url, m3[1].destination_url);
     EXPECT_EQ("http://www.google.com/3", m3[0].destination_url.spec());
   }
 }
 
+// Tests that history is deleted properly.
+TEST_F(HistoryContentsProviderTest, DeleteMatch) {
+  AutocompleteInput input(L"bar", std::wstring(), true, false, true, false);
+  RunQuery(input, false);
+
+  // Query; the result should be the third page.
+  const ACMatches& m = matches();
+  ASSERT_EQ(1U, m.size());
+  EXPECT_EQ(test_entries[2].url, m[0].destination_url.spec());
+
+  // Now delete the match and ensure it was removed.
+  DeleteMatch(m[0]);
+  EXPECT_EQ(0U, matches().size());
+}
+
+// Tests deleting starred results from history, not affecting bookmarks/matches.
+TEST_F(HistoryContentsProviderTest, DeleteStarredMatch) {
+  profile()->CreateBookmarkModel(false);
+  profile()->BlockUntilBookmarkModelLoaded();
+
+  // Bookmark a history item.
+  GURL bookmark_url(test_entries[2].url);
+  profile()->GetBookmarkModel()->SetURLStarred(bookmark_url,
+                                               ASCIIToUTF16("bar"), true);
+
+  // Get the match to delete its history
+  AutocompleteInput input(L"bar", std::wstring(), true, false, true, false);
+  RunQuery(input, false);
+  const ACMatches& m = matches();
+  ASSERT_EQ(1U, m.size());
+
+  // Now delete the match and ensure it was *not* removed.
+  DeleteMatch(m[0]);
+  EXPECT_EQ(1U, matches().size());
+
+  // Run a query that would only match history (but the history is deleted)
+  AutocompleteInput you_input(L"you", std::wstring(), true, false, true, false);
+  RunQuery(you_input, false);
+  EXPECT_EQ(0U, matches().size());
+
+  // Run a query that matches the bookmark
+  RunQuery(input, false);
+  EXPECT_EQ(1U, matches().size());
+}
+
 }  // namespace