diff options
| -rw-r--r-- | chrome/browser/autocomplete/history_contents_provider.cc | 72 | ||||
| -rw-r--r-- | chrome/browser/autocomplete/history_contents_provider.h | 40 | ||||
| -rw-r--r-- | chrome/browser/autocomplete/history_contents_provider_unittest.cc | 78 | ||||
| -rw-r--r-- | chrome/test/testing_profile.cc | 46 | ||||
| -rw-r--r-- | chrome/test/testing_profile.h | 7 |
5 files changed, 179 insertions, 64 deletions
diff --git a/chrome/browser/autocomplete/history_contents_provider.cc b/chrome/browser/autocomplete/history_contents_provider.cc index f6ba682..2d03db9 100644 --- a/chrome/browser/autocomplete/history_contents_provider.cc +++ b/chrome/browser/autocomplete/history_contents_provider.cc @@ -4,8 +4,8 @@ #include "chrome/browser/autocomplete/history_contents_provider.h" +#include "base/histogram.h" #include "base/string_util.h" -#include "chrome/browser/bookmark_bar_model.h" #include "chrome/browser/history/query_parser.h" #include "chrome/browser/profile.h" #include "net/base/net_util.h" @@ -43,9 +43,9 @@ void HistoryContentsProvider::Start(const AutocompleteInput& input, matches_.clear(); if (input.text().empty() || (input.type() == AutocompleteInput::INVALID) || - // The history service must exist. - (!history_service_ && - (!profile_ || !profile_->GetHistoryService(Profile::EXPLICIT_ACCESS)))) { + // The history service or bookmark bar model must exist. + !(profile_->GetHistoryService(Profile::EXPLICIT_ACCESS) || + profile_->GetBookmarkBarModel())) { Stop(); return; } @@ -76,19 +76,32 @@ void HistoryContentsProvider::Start(const AutocompleteInput& input, ConvertResults(); return; } else if (!done_) { - // We're still running the previous query. If we're allowed to keep running - // it, do so, and when it finishes, its results will get marked up for this - // new input. In synchronous_only mode, just cancel. - if (synchronous_only) - Stop(); + // We're still running the previous query on the HistoryService. If we're + // allowed to keep running it, do so, and when it finishes, its results will + // get marked up for this new input. In synchronous_only mode, cancel the + // history query. + if (synchronous_only) { + done_ = true; + request_consumer_.CancelAllRequests(); + } + ConvertResults(); return; } - // TODO(sky): re-enable providing suggestions from the user supplied title of - // bookmarks. + if (results_.size() != 0) { + // Clear the results. We swap in an empty one as the easy way to clear it. + history::QueryResults empty_results; + results_.Swap(&empty_results); + } + + // Querying bookmarks is synchronous, so we always do it. + QueryBookmarks(input); + + // Convert the bookmark results. + ConvertResults(); if (!synchronous_only) { - HistoryService* history = history_service_ ? history_service_ : + HistoryService* history = profile_->GetHistoryService(Profile::EXPLICIT_ACCESS); if (history) { done_ = false; @@ -111,17 +124,14 @@ void HistoryContentsProvider::Stop() { history::QueryResults empty_results; results_.Swap(&empty_results); have_results_ = false; - - db_match_count_ = 0; } void HistoryContentsProvider::QueryComplete(HistoryService::Handle handle, history::QueryResults* results) { - results_.Swap(results); + results_.AppendResultsBySwapping(results, true); have_results_ = true; ConvertResults(); - db_match_count_ = static_cast<int>(results_.size()); done_ = true; if (listener_) listener_->OnProviderUpdate(!matches_.empty()); @@ -181,7 +191,8 @@ AutocompleteMatch HistoryContentsProvider::ResultToMatch( ACMatchClassification(0, ACMatchClassification::URL)); match.description = result.title(); match.starred = - (profile_ && profile_->GetBookmarkBarModel()->IsBookmarked(result.url())); + (profile_->GetBookmarkBarModel() && + profile_->GetBookmarkBarModel()->IsBookmarked(result.url())); ClassifyDescription(result, &match); return match; @@ -216,7 +227,8 @@ int HistoryContentsProvider::CalculateRelevance( const history::URLResult& result) { bool in_title = !!result.title_match_positions().size(); bool is_starred = - (profile_ && profile_->GetBookmarkBarModel()->IsBookmarked(result.url())); + (profile_->GetBookmarkBarModel() && + profile_->GetBookmarkBarModel()->IsBookmarked(result.url())); switch (input_type_) { case AutocompleteInput::UNKNOWN: @@ -245,3 +257,27 @@ int HistoryContentsProvider::CalculateRelevance( } } +void HistoryContentsProvider::QueryBookmarks(const AutocompleteInput& input) { + BookmarkBarModel* bookmark_model = profile_->GetBookmarkBarModel(); + if (!bookmark_model) + return; + + DCHECK(results_.size() == 0); // When we get here the results should be + // empty. + + TimeTicks start_time = TimeTicks::Now(); + std::vector<BookmarkBarModel::TitleMatch> matches; + bookmark_model->GetBookmarksMatchingText(input.text(), kMaxMatchCount, + &matches); + for (size_t i = 0; i < matches.size(); ++i) + AddBookmarkTitleMatchToResults(matches[i]); + UMA_HISTOGRAM_TIMES(L"Omnibox.QueryBookmarksTime", + TimeTicks::Now() - start_time); +} + +void HistoryContentsProvider::AddBookmarkTitleMatchToResults( + const BookmarkBarModel::TitleMatch& match) { + history::URLResult url_result(match.node->GetURL(), match.match_positions); + url_result.set_title(match.node->GetTitle()); + results_.AppendURLBySwapping(&url_result); +} diff --git a/chrome/browser/autocomplete/history_contents_provider.h b/chrome/browser/autocomplete/history_contents_provider.h index 85665bb..b2451ee 100644 --- a/chrome/browser/autocomplete/history_contents_provider.h +++ b/chrome/browser/autocomplete/history_contents_provider.h @@ -6,29 +6,24 @@ #define CHROME_BROWSER_AUTOCOMPLETE_HISTORY_CONTENTS_PROVIDER_H__ #include "chrome/browser/autocomplete/autocomplete.h" +#include "chrome/browser/bookmark_bar_model.h" #include "chrome/browser/history/history.h" // HistoryContentsProvider is an AutocompleteProvider that provides results from -// the contents (body and/or title) of previously visited pages. Results are -// obtained asynchronously from the history service. +// the contents (body and/or title) of previously visited pages. +// HistoryContentsProvider gets results from two sources: +// . HistoryService: this provides results for matches in the body/title of +// previously viewed pages. This is asynchronous. +// . BookmarkBarModel: provides results for matches in the titles of bookmarks. +// This is synchronous. class HistoryContentsProvider : public AutocompleteProvider { public: HistoryContentsProvider(ACProviderListener* listener, Profile* profile) : AutocompleteProvider(listener, profile, "HistoryContents"), - history_service_(NULL), have_results_(false) { DCHECK(profile); } -#ifdef UNIT_TEST - HistoryContentsProvider(ACProviderListener* listener, - HistoryService* history_service) - : AutocompleteProvider(listener, NULL, "HistoryContents"), - history_service_(history_service), - have_results_(false) { - } -#endif - // As necessary asks the history service for the relevant results. When // done SetResults is invoked. virtual void Start(const AutocompleteInput& input, @@ -39,8 +34,8 @@ class HistoryContentsProvider : public AutocompleteProvider { // Returns the total number of matches available in the database, up to // kMaxMatchCount, whichever is smaller. - // Return value is only valid if done() returns true. - size_t db_match_count() const { return db_match_count_; } + // Return value is incomplete if done() returns false. + size_t db_match_count() const { return results_.size(); } // The maximum match count we'll report. If the db_match_count is greater // than this, it will be clamped to this result. @@ -67,11 +62,16 @@ class HistoryContentsProvider : public AutocompleteProvider { // chart in autocomplete.h for the list of values this returns. int CalculateRelevance(const history::URLResult& result); - CancelableRequestConsumerT<int, 0> request_consumer_; + // Queries the bookmarks for any bookmarks whose title matches input. All + // matches are added directly to results_. + void QueryBookmarks(const AutocompleteInput& input); - // This is only non-null for testing, otherwise the HistoryService from the - // Profile is used. - HistoryService* history_service_; + // Converts a BookmarkBarModel::TitleMatch to a QueryResult and adds it + // to results_. + void AddBookmarkTitleMatchToResults( + const BookmarkBarModel::TitleMatch& match); + + CancelableRequestConsumerT<int, 0> request_consumer_; // The number of times we're returned each different type of result. These are // used by CalculateRelevance. Initialized in Start. @@ -93,11 +93,7 @@ class HistoryContentsProvider : public AutocompleteProvider { // Current query string. std::wstring query_; - // Total number of matches available in the database. - int db_match_count_; - DISALLOW_EVIL_CONSTRUCTORS(HistoryContentsProvider); }; #endif // CHROME_BROWSER_AUTOCOMPLETE_HISTORY_CONTENTS_PROVIDER_H__ - diff --git a/chrome/browser/autocomplete/history_contents_provider_unittest.cc b/chrome/browser/autocomplete/history_contents_provider_unittest.cc index 2e1a646..5796f4d 100644 --- a/chrome/browser/autocomplete/history_contents_provider_unittest.cc +++ b/chrome/browser/autocomplete/history_contents_provider_unittest.cc @@ -8,6 +8,7 @@ #include "chrome/browser/autocomplete/autocomplete.h" #include "chrome/browser/autocomplete/history_contents_provider.h" #include "chrome/browser/history/history.h" +#include "chrome/test/testing_profile.h" #include "testing/gtest/include/gtest/gtest.h" namespace { @@ -45,16 +46,18 @@ class HistoryContentsProviderTest : public testing::Test, 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() { - PathService::Get(base::DIR_TEMP, &history_dir_); - file_util::AppendToPath(&history_dir_, L"HistoryContentProviderTest"); - file_util::Delete(history_dir_, true); // Normally won't exist. - file_util::CreateDirectoryW(history_dir_); + profile_.reset(new TestingProfile()); + profile_->CreateHistoryService(false); - history_service_ = new HistoryService; - history_service_->Init(history_dir_, NULL); + HistoryService* history_service = + profile_->GetHistoryService(Profile::EXPLICIT_ACCESS); // Populate history. for (int i = 0; i < arraysize(test_entries); i++) { @@ -67,28 +70,18 @@ class HistoryContentsProviderTest : public testing::Test, // is "right now" or it will nondeterministically appear in the results. Time t = Time::Now() - TimeDelta::FromDays(arraysize(test_entries) + i); - history_service_->AddPage(url, t, id_scope, i, GURL(), + history_service->AddPage(url, t, id_scope, i, GURL(), PageTransition::LINK, HistoryService::RedirectList()); - history_service_->SetPageTitle(url, test_entries[i].title); - history_service_->SetPageContents(url, test_entries[i].body); + history_service->SetPageTitle(url, test_entries[i].title); + history_service->SetPageContents(url, test_entries[i].body); } - provider_ = new HistoryContentsProvider(this, history_service_); + provider_ = new HistoryContentsProvider(this, profile_.get()); } virtual void TearDown() { - history_service_->SetOnBackendDestroyTask(new MessageLoop::QuitTask); - history_service_->Cleanup(); provider_ = NULL; - history_service_ = NULL; - - // Wait for history thread to complete (the QuitTask will cause it to exit - // on destruction). Note: if this never terminates, somebody is probably - // leaking a reference to the history backend, so it never calls our - // destroy task. - MessageLoop::current()->Run(); - - file_util::Delete(history_dir_, true); + profile_.reset(NULL); } // ACProviderListener @@ -98,11 +91,11 @@ class HistoryContentsProviderTest : public testing::Test, } MessageLoopForUI message_loop_; - + std::wstring history_dir_; + scoped_ptr<TestingProfile> profile_; scoped_refptr<HistoryContentsProvider> provider_; - scoped_refptr<HistoryService> history_service_; }; } // namespace @@ -154,3 +147,42 @@ TEST_F(HistoryContentsProviderTest, MinimalChanges) { const ACMatches& m3 = matches(); EXPECT_EQ(2, m3.size()); } + +// Tests that the BookmarkBarModel is queried correctly. +TEST_F(HistoryContentsProviderTest, Bookmarks) { + profile()->CreateBookmarkBarModel(false); + profile()->BlockUntilBookmarkModelLoaded(); + + // Add a bookmark. + GURL bookmark_url("http://www.google.com/4"); + profile()->GetBookmarkBarModel()->SetURLStarred(bookmark_url, L"bar", true); + + AutocompleteInput input(L"bar", std::wstring(), true); + + // Ask for synchronous. This should only get the bookmark. + RunQuery(input, false, true); + const ACMatches& m1 = matches(); + ASSERT_EQ(1, m1.size()); + EXPECT_EQ(bookmark_url.spec(), WideToUTF8(m1[0].destination_url)); + EXPECT_EQ(L"bar", m1[0].description); + EXPECT_TRUE(m1[0].starred); + + // Ask for async. We should get the bookmark immediately. + provider()->Start(input, false, false); + const ACMatches& m2 = matches(); + ASSERT_EQ(1, m2.size()); + EXPECT_EQ(bookmark_url.spec(), WideToUTF8(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. + const ACMatches& m3 = matches(); + ASSERT_EQ(2, m3.size()); + if (bookmark_url.spec() == WideToUTF8(m3[0].destination_url)) { + EXPECT_EQ(L"http://www.google.com/3", m3[1].destination_url); + } else { + EXPECT_EQ(bookmark_url.spec(), WideToUTF8(m3[1].destination_url)); + EXPECT_EQ(L"http://www.google.com/3", m3[0].destination_url); + } +} diff --git a/chrome/test/testing_profile.cc b/chrome/test/testing_profile.cc index c91c372..ac7021d 100644 --- a/chrome/test/testing_profile.cc +++ b/chrome/test/testing_profile.cc @@ -7,6 +7,40 @@ #include "chrome/browser/history/history_backend.h" #include "chrome/common/chrome_constants.h" +namespace { + +// BookmarkLoadObserver is used when blocking until the BookmarkBarModel +// finishes loading. As soon as the BookmarkBarModel finishes loading the +// message loop is quit. +class BookmarkLoadObserver : public BookmarkBarModelObserver { + public: + BookmarkLoadObserver() {} + virtual void Loaded(BookmarkBarModel* model) { + MessageLoop::current()->Quit(); + } + + virtual void BookmarkNodeMoved(BookmarkBarModel* model, + BookmarkBarNode* old_parent, + int old_index, + BookmarkBarNode* new_parent, + int new_index) {} + virtual void BookmarkNodeAdded(BookmarkBarModel* model, + BookmarkBarNode* parent, + int index) {} + virtual void BookmarkNodeRemoved(BookmarkBarModel* model, + BookmarkBarNode* parent, + int index) {} + virtual void BookmarkNodeChanged(BookmarkBarModel* model, + BookmarkBarNode* node) {} + virtual void BookmarkNodeFavIconLoaded(BookmarkBarModel* model, + BookmarkBarNode* node) {} + + private: + DISALLOW_COPY_AND_ASSIGN(BookmarkLoadObserver); +}; + +} // namespace + TestingProfile::TestingProfile() : start_time_(Time::Now()), has_history_service_(false) { PathService::Get(base::DIR_TEMP, &path_); @@ -75,7 +109,17 @@ void TestingProfile::CreateBookmarkBarModel(bool delete_file) { bookmark_bar_model_->Load(); } +void TestingProfile::BlockUntilBookmarkModelLoaded() { + DCHECK(bookmark_bar_model_.get()); + if (bookmark_bar_model_->IsLoaded()) + return; + BookmarkLoadObserver observer; + bookmark_bar_model_->AddObserver(&observer); + MessageLoop::current()->Run(); + bookmark_bar_model_->RemoveObserver(&observer); + DCHECK(bookmark_bar_model_->IsLoaded()); +} + void TestingProfile::CreateTemplateURLModel() { template_url_model_.reset(new TemplateURLModel(this)); } - diff --git a/chrome/test/testing_profile.h b/chrome/test/testing_profile.h index 3d8e04c..4ff9dbb 100644 --- a/chrome/test/testing_profile.h +++ b/chrome/test/testing_profile.h @@ -31,8 +31,15 @@ class TestingProfile : public Profile { // the model is created. As TestingProfile deletes the directory containing // the files used by HistoryService, the boolean only matters if you're // recreating the BookmarkBarModel. + // + // NOTE: this does not block until the bookmarks are loaded. For that use + // BlockUntilBookmarkModelLoaded. void CreateBookmarkBarModel(bool delete_file); + // Blocks until the BookmarkBarModel finishes loaded. This is NOT invoked + // from CreateBookmarkBarModel. + void BlockUntilBookmarkModelLoaded(); + // Creates a TemplateURLModel. If not invoked the TemplateURLModel is NULL. void CreateTemplateURLModel(); |