From fa5dfaff1d203ab3fb692b6cd2bf0bdb67b1059c Mon Sep 17 00:00:00 2001 From: "brettw@chromium.org" Date: Tue, 2 Jun 2009 22:12:06 +0000 Subject: Support for searching bookmarks for IDN. The main part of this change is DoesBookmarkContainWords() in bookmark_utils.cc. It tries to match a query words to IDN and %-decoded strings as well as ASCII URL. BUG=3991 TEST=add a unit test, check for IDN in the search box of Bookmark Manager. Checked in for tkent@google.com. Original review: http://codereview.chromium.org/113815 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@17435 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/bookmarks/bookmark_table_model.cc | 18 +++++--- chrome/browser/bookmarks/bookmark_table_model.h | 5 ++- .../bookmarks/bookmark_table_model_unittest.cc | 14 ++++--- chrome/browser/bookmarks/bookmark_utils.cc | 18 +++++--- chrome/browser/bookmarks/bookmark_utils.h | 23 +++++++---- .../browser/bookmarks/bookmark_utils_unittest.cc | 48 +++++++++++++++++++--- 6 files changed, 94 insertions(+), 32 deletions(-) (limited to 'chrome/browser/bookmarks') diff --git a/chrome/browser/bookmarks/bookmark_table_model.cc b/chrome/browser/bookmarks/bookmark_table_model.cc index 53e712e..44978ce 100644 --- a/chrome/browser/bookmarks/bookmark_table_model.cc +++ b/chrome/browser/bookmarks/bookmark_table_model.cc @@ -225,18 +225,22 @@ class RecentlyBookmarkedTableModel : public VectorBackedBookmarkTableModel { class BookmarkSearchTableModel : public VectorBackedBookmarkTableModel { public: BookmarkSearchTableModel(BookmarkModel* model, - const std::wstring& search_text) + const std::wstring& search_text, + const std::wstring& languages) : VectorBackedBookmarkTableModel(model), - search_text_(search_text) { + search_text_(search_text), + languages_(languages) { bookmark_utils::GetBookmarksContainingText( - model, search_text, std::numeric_limits::max(), &nodes()); + model, search_text, std::numeric_limits::max(), + languages, &nodes()); } virtual void BookmarkNodeAdded(BookmarkModel* model, BookmarkNode* parent, int index) { BookmarkNode* node = parent->GetChild(index); - if (bookmark_utils::DoesBookmarkContainText(node, search_text_)) { + if (bookmark_utils::DoesBookmarkContainText( + node, search_text_, languages_)) { nodes().push_back(node); if (observer()) observer()->OnItemsAdded(static_cast(nodes().size() - 1), 1); @@ -258,6 +262,7 @@ class BookmarkSearchTableModel : public VectorBackedBookmarkTableModel { private: const std::wstring search_text_; + const std::wstring languages_; DISALLOW_COPY_AND_ASSIGN(BookmarkSearchTableModel); }; @@ -281,8 +286,9 @@ BookmarkTableModel* BookmarkTableModel::CreateBookmarkTableModelForFolder( // static BookmarkTableModel* BookmarkTableModel::CreateSearchTableModel( BookmarkModel* model, - const std::wstring& text) { - return new BookmarkSearchTableModel(model, text); + const std::wstring& text, + const std::wstring& languages) { + return new BookmarkSearchTableModel(model, text, languages); } BookmarkTableModel::BookmarkTableModel(BookmarkModel* model) diff --git a/chrome/browser/bookmarks/bookmark_table_model.h b/chrome/browser/bookmarks/bookmark_table_model.h index 7832f35..db7051f 100644 --- a/chrome/browser/bookmarks/bookmark_table_model.h +++ b/chrome/browser/bookmarks/bookmark_table_model.h @@ -24,13 +24,16 @@ class BookmarkTableModel : public views::TableModel, public: // Methods for creating the various BookmarkTableModels. Ownership passes // to the caller. + // |languages| is the kAcceptLanguages value of the user preference. It is + // used to decode IDN. static BookmarkTableModel* CreateRecentlyBookmarkedModel( BookmarkModel* model); static BookmarkTableModel* CreateBookmarkTableModelForFolder( BookmarkModel* model, BookmarkNode* node); static BookmarkTableModel* CreateSearchTableModel( BookmarkModel* model, - const std::wstring& text); + const std::wstring& text, + const std::wstring& languages); explicit BookmarkTableModel(BookmarkModel* model); virtual ~BookmarkTableModel(); diff --git a/chrome/browser/bookmarks/bookmark_table_model_unittest.cc b/chrome/browser/bookmarks/bookmark_table_model_unittest.cc index 768e3bc..7add6a9 100644 --- a/chrome/browser/bookmarks/bookmark_table_model_unittest.cc +++ b/chrome/browser/bookmarks/bookmark_table_model_unittest.cc @@ -33,7 +33,7 @@ class BookmarkTableModelTest : public testing::Test, item_changed_count_(0), added_count_(0), removed_count_(0) { - } + } virtual void SetUp() { profile_.reset(new TestingProfile()); @@ -248,7 +248,8 @@ TEST_F(BookmarkTableModelTest, ChangeRecentlyBookmarked) { // Verifies search finds the correct bookmarks. TEST_F(BookmarkTableModelTest, Search) { - SetModel(BookmarkTableModel::CreateSearchTableModel(bookmark_model(), L"c")); + SetModel(BookmarkTableModel::CreateSearchTableModel( + bookmark_model(), L"c", std::wstring())); ASSERT_EQ(1, model_->RowCount()); EXPECT_TRUE(bookmark_model()->other_node()->GetChild(2) == model_->GetNodeForRow(0)); @@ -259,7 +260,8 @@ TEST_F(BookmarkTableModelTest, Search) { // Verifies adding an item to search notifies observers. TEST_F(BookmarkTableModelTest, AddToSearch) { - SetModel(BookmarkTableModel::CreateSearchTableModel(bookmark_model(), L"c")); + SetModel(BookmarkTableModel::CreateSearchTableModel( + bookmark_model(), L"c", std::wstring())); BookmarkNode* new_node = bookmark_model()->AddURL(bookmark_model()->other_node(), 0, L"c", url1_); // Should have gotten notification of the add. @@ -281,7 +283,8 @@ TEST_F(BookmarkTableModelTest, AddToSearch) { // Verifies removing an item updates search. TEST_F(BookmarkTableModelTest, RemoveFromSearch) { - SetModel(BookmarkTableModel::CreateSearchTableModel(bookmark_model(), L"c")); + SetModel(BookmarkTableModel::CreateSearchTableModel( + bookmark_model(), L"c", std::wstring())); bookmark_model()->Remove(bookmark_model()->other_node(), 2); // Should have gotten notification of the remove. VerifyAndClearOberserverCounts(0, 0, 0, 1); @@ -298,7 +301,8 @@ TEST_F(BookmarkTableModelTest, RemoveFromSearch) { // Verifies changing an item in search notifies observer. TEST_F(BookmarkTableModelTest, ChangeSearch) { - SetModel(BookmarkTableModel::CreateSearchTableModel(bookmark_model(), L"c")); + SetModel(BookmarkTableModel::CreateSearchTableModel(bookmark_model(), + L"c", std::wstring())); bookmark_model()->SetTitle(bookmark_model()->other_node()->GetChild(2), L"new"); // Should have gotten notification of the change. diff --git a/chrome/browser/bookmarks/bookmark_utils.cc b/chrome/browser/bookmarks/bookmark_utils.cc index 61be84d..964df01 100644 --- a/chrome/browser/bookmarks/bookmark_utils.cc +++ b/chrome/browser/bookmarks/bookmark_utils.cc @@ -29,6 +29,7 @@ #include "chrome/common/pref_service.h" #include "grit/chromium_strings.h" #include "grit/generated_resources.h" +#include "net/base/net_util.h" #include "views/event.h" using base::Time; @@ -177,12 +178,16 @@ bool DoesBookmarkTextContainWords(const std::wstring& text, } // Returns true if |node|s title or url contains the strings in |words|. +// |languages| argument is user's accept-language setting to decode IDN. bool DoesBookmarkContainWords(BookmarkNode* node, - const std::vector& words) { + const std::vector& words, + const std::wstring& languages) { return DoesBookmarkTextContainWords( l10n_util::ToLower(node->GetTitle()), words) || - DoesBookmarkTextContainWords(UTF8ToWide(node->GetURL().spec()), words); + DoesBookmarkTextContainWords(UTF8ToWide(node->GetURL().spec()), words) || + DoesBookmarkTextContainWords(net::FormatUrl( + node->GetURL(), languages, false, true, NULL, NULL), words); } } // namespace @@ -478,6 +483,7 @@ bool MoreRecentlyAdded(BookmarkNode* n1, BookmarkNode* n2) { void GetBookmarksContainingText(BookmarkModel* model, const std::wstring& text, size_t max_count, + const std::wstring& languages, std::vector* nodes) { std::vector words; QueryParser parser; @@ -488,7 +494,7 @@ void GetBookmarksContainingText(BookmarkModel* model, TreeNodeIterator iterator(model->root_node()); while (iterator.has_next()) { BookmarkNode* node = iterator.Next(); - if (node->is_url() && DoesBookmarkContainWords(node, words)) { + if (node->is_url() && DoesBookmarkContainWords(node, words, languages)) { nodes->push_back(node); if (nodes->size() == max_count) return; @@ -496,14 +502,16 @@ void GetBookmarksContainingText(BookmarkModel* model, } } -bool DoesBookmarkContainText(BookmarkNode* node, const std::wstring& text) { +bool DoesBookmarkContainText(BookmarkNode* node, + const std::wstring& text, + const std::wstring& languages) { std::vector words; QueryParser parser; parser.ExtractQueryWords(l10n_util::ToLower(text), &words); if (words.empty()) return false; - return (node->is_url() && DoesBookmarkContainWords(node, words)); + return (node->is_url() && DoesBookmarkContainWords(node, words, languages)); } void ApplyEditsWithNoGroupChange(BookmarkModel* model, diff --git a/chrome/browser/bookmarks/bookmark_utils.h b/chrome/browser/bookmarks/bookmark_utils.h index 057183b..e161a81 100644 --- a/chrome/browser/bookmarks/bookmark_utils.h +++ b/chrome/browser/bookmarks/bookmark_utils.h @@ -39,17 +39,17 @@ int BookmarkDragOperation(BookmarkNode* node); // |parent| is the parent node the drop is to occur on and |index| the index the // drop is over. int BookmarkDropOperation(Profile* profile, - const views::DropTargetEvent& event, - const BookmarkDragData& data, - BookmarkNode* parent, - int index); + const views::DropTargetEvent& event, + const BookmarkDragData& data, + BookmarkNode* parent, + int index); // Performs a drop of bookmark data onto |parent_node| at |index|. Returns the // type of drop the resulted. int PerformBookmarkDrop(Profile* profile, - const BookmarkDragData& data, - BookmarkNode* parent_node, - int index); + const BookmarkDragData& data, + BookmarkNode* parent_node, + int index); // Returns true if the bookmark data can be dropped on |drop_parent| at // |index|. A drop from a separate profile is always allowed, where as @@ -127,14 +127,19 @@ struct TitleMatch { bool MoreRecentlyAdded(BookmarkNode* n1, BookmarkNode* n2); // Returns up to |max_count| bookmarks from |model| whose url or title contains -// the text |text|. +// the text |text|. |languages| is user's accept-language setting to decode +// IDN. void GetBookmarksContainingText(BookmarkModel* model, const std::wstring& text, size_t max_count, + const std::wstring& languages, std::vector* nodes); // Returns true if |node|'s url or title contains the string |text|. -bool DoesBookmarkContainText(BookmarkNode* node, const std::wstring& text); +// |languages| is user's accept-language setting to decode IDN. +bool DoesBookmarkContainText(BookmarkNode* node, + const std::wstring& text, + const std::wstring& languages); // Modifies a bookmark node (assuming that there's no magic that needs to be // done regarding moving from one folder to another). diff --git a/chrome/browser/bookmarks/bookmark_utils_unittest.cc b/chrome/browser/bookmarks/bookmark_utils_unittest.cc index a97a7ec..2bd612f 100644 --- a/chrome/browser/bookmarks/bookmark_utils_unittest.cc +++ b/chrome/browser/bookmarks/bookmark_utils_unittest.cc @@ -21,21 +21,57 @@ TEST_F(BookmarkUtilsTest, GetBookmarksContainingText) { model.AddGroup(model.other_node(), 0, L"foo"); std::vector nodes; - bookmark_utils::GetBookmarksContainingText(&model, L"foo", 100, &nodes); + bookmark_utils::GetBookmarksContainingText( + &model, L"foo", 100, std::wstring(), &nodes); ASSERT_EQ(1U, nodes.size()); EXPECT_TRUE(nodes[0] == n1); - EXPECT_TRUE(bookmark_utils::DoesBookmarkContainText(n1, L"foo")); + EXPECT_TRUE(bookmark_utils::DoesBookmarkContainText( + n1, L"foo", std::wstring())); nodes.clear(); - bookmark_utils::GetBookmarksContainingText(&model, L"cnn", 100, &nodes); + bookmark_utils::GetBookmarksContainingText( + &model, L"cnn", 100, std::wstring(), &nodes); ASSERT_EQ(1U, nodes.size()); EXPECT_TRUE(nodes[0] == n2); - EXPECT_TRUE(bookmark_utils::DoesBookmarkContainText(n2, L"cnn")); + EXPECT_TRUE(bookmark_utils::DoesBookmarkContainText( + n2, L"cnn", std::wstring())); nodes.clear(); - bookmark_utils::GetBookmarksContainingText(&model, L"foo bar", 100, &nodes); + bookmark_utils::GetBookmarksContainingText( + &model, L"foo bar", 100, std::wstring(), &nodes); ASSERT_EQ(1U, nodes.size()); EXPECT_TRUE(nodes[0] == n1); - EXPECT_TRUE(bookmark_utils::DoesBookmarkContainText(n1, L"foo bar")); + EXPECT_TRUE(bookmark_utils::DoesBookmarkContainText( + n1, L"foo bar", std::wstring())); nodes.clear(); } + +TEST_F(BookmarkUtilsTest, DoesBookmarkContainText) { + BookmarkModel model(NULL); + BookmarkNode* node = model.AddURL(model.other_node(), 0, L"foo bar", + GURL("http://www.google.com")); + // Matches to the title. + ASSERT_TRUE(bookmark_utils::DoesBookmarkContainText( + node, L"ar", std::wstring())); + // Matches to the URL + ASSERT_TRUE(bookmark_utils::DoesBookmarkContainText( + node, L"www", std::wstring())); + // No match. + ASSERT_FALSE(bookmark_utils::DoesBookmarkContainText( + node, L"cnn", std::wstring())); + + // Tests for a Japanese IDN. + const wchar_t* kDecodedIdn = L"\x30B0\x30FC\x30B0\x30EB"; + node = model.AddURL(model.other_node(), 0, L"foo bar", + GURL("http://xn--qcka1pmc.jp")); + // Unicode query doesn't match if languages have no "ja". + ASSERT_FALSE(bookmark_utils::DoesBookmarkContainText( + node, kDecodedIdn, L"en")); + // Unicode query matches if languages have "ja". + ASSERT_TRUE(bookmark_utils::DoesBookmarkContainText( + node, kDecodedIdn, L"ja")); + // Punycode query also matches as ever. + ASSERT_TRUE(bookmark_utils::DoesBookmarkContainText( + node, L"qcka1pmc", L"ja")); +} + -- cgit v1.1