Make partner bookmarks appear in search

Bug: 814417
Change-Id: I9b9083cbbb10e8735579e64ee4a594c0afe69547
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1726636
Commit-Queue: Theresa  <twellington@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Theresa  <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688214}

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkBridge.java

@@ -268,6 +268,15 @@ public class BookmarkBridge {
         nativeLoadEmptyPartnerBookmarkShimForTesting(mNativeBookmarkBridge);
     }
 
+    /**
+     * Load a fake partner bookmark shim for testing. To see (or edit) the titles and URLs of the
+     * partner bookmarks, go to bookmark_bridge.cc.
+     */
+    @VisibleForTesting
+    public void loadFakePartnerBookmarkShimForTesting() {
+        nativeLoadFakePartnerBookmarkShimForTesting(mNativeBookmarkBridge);
+    }
+
     /**
      * Add an observer to bookmark model changes.
      * @param observer The observer to be added.
@@ -939,6 +948,7 @@ public class BookmarkBridge {
     private native void nativeStartGroupingUndos(long nativeBookmarkBridge);
     private native void nativeEndGroupingUndos(long nativeBookmarkBridge);
     private native void nativeLoadEmptyPartnerBookmarkShimForTesting(long nativeBookmarkBridge);
+    private native void nativeLoadFakePartnerBookmarkShimForTesting(long nativeBookmarkBridge);
     private native void nativeSearchBookmarks(long nativeBookmarkBridge,
             List<BookmarkId> bookmarkMatches, String query, int maxNumber);
     private native long nativeInit(Profile profile);

chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkBridgeTest.java

@@ -52,7 +52,7 @@ public class BookmarkBridgeTest {
         TestThreadUtils.runOnUiThreadBlocking(() -> {
             Profile profile = Profile.getLastUsedProfile();
             mBookmarkBridge = new BookmarkBridge(profile);
-            mBookmarkBridge.loadEmptyPartnerBookmarkShimForTesting();
+            mBookmarkBridge.loadFakePartnerBookmarkShimForTesting();
         });
 
         BookmarkTestUtil.waitForBookmarkModelLoaded();
@@ -266,4 +266,52 @@ public class BookmarkBridgeTest {
         }
         return newOrder;
     }
+
+    @Test
+    @SmallTest
+    @UiThreadTest
+    @Feature({"Bookmark"})
+    public void testSearchPartner() {
+        List<BookmarkId> expectedSearchResults = new ArrayList<>();
+        expectedSearchResults.add(new BookmarkId(
+                1, 1)); // Partner bookmark with ID 1: "Partner Bookmark A", http://a.com
+        expectedSearchResults.add(new BookmarkId(
+                2, 1)); // Partner bookmark with ID 2: "Partner Bookmark B", http://b.com
+        List<BookmarkId> searchResults = mBookmarkBridge.searchBookmarks("pArTnER BookMARK", 100);
+        Assert.assertEquals("Expected search results would yield partner bookmark with "
+                        + "case-insensitive title match",
+                expectedSearchResults, searchResults);
+    }
+
+    @Test
+    @SmallTest
+    @UiThreadTest
+    @Feature({"Bookmark"})
+    public void testSearchFolder() {
+        List<BookmarkId> expectedSearchResults = new ArrayList<>();
+        expectedSearchResults.add(mBookmarkBridge.addFolder(mMobileNode, 0, "FooBar"));
+        List<BookmarkId> searchResults = mBookmarkBridge.searchBookmarks("oba", 100);
+        Assert.assertEquals("Expected search results would yield case-insensitive match of "
+                        + "part of title",
+                expectedSearchResults, searchResults);
+    }
+
+    @Test
+    @SmallTest
+    @UiThreadTest
+    @Feature({"Bookmark"})
+    public void testSearch_MaxResults() {
+        List<BookmarkId> expectedSearchResults = new ArrayList<>();
+        expectedSearchResults.add(mBookmarkBridge.addFolder(mMobileNode, 0, "FooBar"));
+        expectedSearchResults.add(mBookmarkBridge.addFolder(mMobileNode, 1, "BazQuux"));
+        expectedSearchResults.add(new BookmarkId(
+                1, 1)); // Partner bookmark with ID 1: "Partner Bookmark A", http://a.com
+
+        List<BookmarkId> searchResults = mBookmarkBridge.searchBookmarks("a", 3);
+        Assert.assertEquals(
+                "Expected search results size to be 3 (maximum size)", 3, searchResults.size());
+        Assert.assertEquals("Expected that user (non-partner) bookmarks would get priority "
+                        + "over partner bookmarks",
+                expectedSearchResults, searchResults);
+    }
 }

chrome/browser/android/bookmarks/bookmark_bridge.cc

@@ -15,7 +15,10 @@
 #include "base/containers/adapters.h"
 #include "base/containers/stack.h"
 #include "base/containers/stack_container.h"
+#include "base/guid.h"
 #include "base/i18n/string_compare.h"
+#include "base/strings/string16.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/android/chrome_jni_headers/BookmarkBridge_jni.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/bookmarks/managed_bookmark_service_factory.h"
@@ -159,6 +162,29 @@ void BookmarkBridge::LoadEmptyPartnerBookmarkShimForTesting(
   DCHECK(partner_bookmarks_shim_->IsLoaded());
 }
 
+// Loads a fake partner bookmarks shim for testing.
+// This is used in BookmarkBridgeTest.java.
+void BookmarkBridge::LoadFakePartnerBookmarkShimForTesting(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj) {
+  if (partner_bookmarks_shim_->IsLoaded())
+    return;
+  std::unique_ptr<BookmarkPermanentNode> root_partner_node =
+      std::make_unique<BookmarkPermanentNode>(0, BookmarkNode::FOLDER);
+  BookmarkNode* partner_bookmark_a =
+      root_partner_node->Add(std::make_unique<BookmarkNode>(
+          1, base::GenerateGUID(), GURL("http://www.a.com")));
+  partner_bookmark_a->SetTitle(base::ASCIIToUTF16("Partner Bookmark A"));
+  BookmarkNode* partner_bookmark_b =
+      root_partner_node->Add(std::make_unique<BookmarkNode>(
+          2, base::GenerateGUID(), GURL("http://www.b.com")));
+  partner_bookmark_b->SetTitle(base::ASCIIToUTF16("Partner Bookmark B"));
+  partner_bookmarks_shim_->SetPartnerBookmarksRoot(
+      std::move(root_partner_node));
+  PartnerBookmarksShim::DisablePartnerBookmarksEditing();
+  DCHECK(partner_bookmarks_shim_->IsLoaded());
+}
+
 ScopedJavaLocalRef<jobject> BookmarkBridge::GetBookmarkByID(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
@@ -594,6 +620,12 @@ void BookmarkBridge::SearchBookmarks(JNIEnv* env,
 
   GetBookmarksMatchingProperties(bookmark_model_, query, max_results, &results);
 
+  if (partner_bookmarks_shim_->HasPartnerBookmarks() &&
+      IsReachable(partner_bookmarks_shim_->GetPartnerBookmarksRoot())) {
+    partner_bookmarks_shim_->GetPartnerBookmarksMatchingProperties(
+        query, max_results, &results);
+  }
+  DCHECK((int)results.size() <= max_results);
   for (const bookmarks::BookmarkNode* match : results) {
     // If this bookmark is a partner bookmark
     if (partner_bookmarks_shim_->IsPartnerBookmark(match) &&

chrome/browser/android/bookmarks/bookmark_bridge.h

@@ -8,7 +8,10 @@
 #include "base/android/jni_android.h"
 #include "base/android/jni_weak_ref.h"
 #include "base/compiler_specific.h"
+#include "base/guid.h"
 #include "base/macros.h"
+#include "base/strings/string16.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/android/bookmarks/partner_bookmarks_shim.h"
 #include "components/bookmarks/browser/base_bookmark_model_observer.h"
 #include "components/bookmarks/common/android/bookmark_id.h"
@@ -42,6 +45,12 @@ class BookmarkBridge : public bookmarks::BaseBookmarkModelObserver,
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj);
 
+  // Loads a fake partner bookmarks shim for testing.
+  // This is used in BookmarkBridgeTest.java.
+  void LoadFakePartnerBookmarkShimForTesting(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& obj);
+
   base::android::ScopedJavaLocalRef<jobject> GetBookmarkByID(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,

chrome/browser/android/bookmarks/partner_bookmarks_shim.cc

@@ -7,16 +7,23 @@
 #include <tuple>
 #include <utility>
 
+#include "base/i18n/case_conversion.h"
+#include "base/i18n/string_search.h"
 #include "base/lazy_instance.h"
 #include "base/memory/ptr_util.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "components/bookmarks/browser/bookmark_model.h"
+#include "components/bookmarks/browser/bookmark_utils.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
+#include "components/url_formatter/url_formatter.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
+#include "net/base/escape.h"
+#include "ui/base/models/tree_node_iterator.h"
 
 using bookmarks::BookmarkNode;
 using content::BrowserThread;
@@ -290,3 +297,30 @@ void PartnerBookmarksShim::SaveNodeMapping() {
   }
   prefs_->Set(prefs::kPartnerBookmarkMappings, list);
 }
+
+void PartnerBookmarksShim::GetPartnerBookmarksMatchingProperties(
+    const bookmarks::QueryFields& query,
+    size_t max_count,
+    std::vector<const BookmarkNode*>* nodes) {
+  DCHECK(nodes->size() <= max_count);
+
+  std::vector<base::string16> query_words =
+      bookmarks::ParseBookmarkQuery(query);
+  if (query_words.empty())
+    return;
+  ui::TreeNodeIterator<const BookmarkNode> iterator(GetPartnerBookmarksRoot());
+  // The check that size < max_count is necessary because we will search for
+  // user (non-partner) bookmarks before calling this function
+  while (iterator.has_next() && nodes->size() < max_count) {
+    const BookmarkNode* node = iterator.Next();
+    // Make sure we don't include the "Partner Bookmarks" folder
+    if (node == GetPartnerBookmarksRoot())
+      continue;
+    if (!query_words.empty() && !bookmarks::DoesBookmarkContainWords(
+                                    GetTitle(node), node->url(), query_words))
+      continue;
+    if (query.title && GetTitle(node) != *query.title)
+      continue;
+    nodes->push_back(node);
+  }
+}

chrome/browser/android/bookmarks/partner_bookmarks_shim.h

@@ -10,10 +10,17 @@
 #include <memory>
 
 #include "base/android/jni_weak_ref.h"
+#include "base/i18n/case_conversion.h"
+#include "base/i18n/string_search.h"
 #include "base/macros.h"
 #include "base/strings/string16.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/supports_user_data.h"
 #include "components/bookmarks/browser/bookmark_model.h"
+#include "components/bookmarks/browser/bookmark_utils.h"
+#include "components/url_formatter/url_formatter.h"
+#include "net/base/escape.h"
+#include "ui/base/models/tree_node_iterator.h"
 #include "url/gurl.h"
 
 class PrefService;
@@ -116,6 +123,12 @@ class PartnerBookmarksShim : public base::SupportsUserData::Data {
   };
   typedef std::map<NodeRenamingMapKey, base::string16> NodeRenamingMap;
 
+  // PartnerBookmarksShim version of BookmarkUtils methods
+  void GetPartnerBookmarksMatchingProperties(
+      const bookmarks::QueryFields& query,
+      size_t max_count,
+      std::vector<const bookmarks::BookmarkNode*>* nodes);
+
   // For testing: clears an instance of the shim in a given |browser_context|.
   static void ClearInBrowserContextForTesting(
       content::BrowserContext* browser_context);

chrome/browser/android/bookmarks/partner_bookmarks_shim_unittest.cc

@@ -409,3 +409,78 @@ TEST_F(PartnerBookmarksShimTest, DisableEditing) {
   EXPECT_EQ(base::ASCIIToUTF16("a"), shim->GetTitle(partner_bookmark1));
   EXPECT_TRUE(shim->IsReachable(partner_bookmark2));
 }
+
+TEST_F(PartnerBookmarksShimTest, GetPartnerBookmarksMatchingProperties) {
+  std::unique_ptr<BookmarkPermanentNode> root_partner_node =
+      std::make_unique<BookmarkPermanentNode>(0, BookmarkNode::FOLDER);
+  BookmarkNode* partner_folder1 = root_partner_node->Add(
+      std::make_unique<BookmarkNode>(1, base::GenerateGUID(), GURL()));
+  partner_folder1->SetTitle(base::ASCIIToUTF16("Folder1"));
+
+  BookmarkNode* partner_folder2 = partner_folder1->Add(
+      std::make_unique<BookmarkNode>(2, base::GenerateGUID(), GURL()));
+  partner_folder2->SetTitle(base::ASCIIToUTF16("Folder2"));
+
+  BookmarkNode* partner_bookmark1 =
+      partner_folder1->Add(std::make_unique<BookmarkNode>(
+          3, base::GenerateGUID(), GURL("http://www.ugtdat.com")));
+  partner_bookmark1->SetTitle(base::ASCIIToUTF16("wx"));
+
+  BookmarkNode* partner_bookmark2 =
+      partner_folder2->Add(std::make_unique<BookmarkNode>(
+          4, base::GenerateGUID(), GURL("http://argbhl.com")));
+  partner_bookmark2->SetTitle(base::ASCIIToUTF16("wx yz"));
+
+  PartnerBookmarksShim* shim = partner_bookmarks_shim();
+  ASSERT_FALSE(shim->IsLoaded());
+  shim->SetPartnerBookmarksRoot(std::move(root_partner_node));
+  ASSERT_TRUE(shim->IsLoaded());
+
+  // Ensure that search returns case-insensitive matches for title only.
+  std::vector<const BookmarkNode*> nodes;
+  bookmarks::QueryFields query;
+  query.word_phrase_query.reset(new base::string16(base::ASCIIToUTF16("WX")));
+  shim->GetPartnerBookmarksMatchingProperties(query, 100, &nodes);
+  ASSERT_EQ(2u, nodes.size());
+  ASSERT_EQ(partner_bookmark1, nodes[1]);
+  ASSERT_EQ(partner_bookmark2, nodes[0]);
+
+  // Ensure that every word in the search must have a match.
+  nodes.clear();
+  query.word_phrase_query.reset(new base::string16(base::ASCIIToUTF16("WX Y")));
+  shim->GetPartnerBookmarksMatchingProperties(query, 100, &nodes);
+  ASSERT_EQ(1u, nodes.size());
+  ASSERT_EQ(partner_bookmark2, nodes[0]);
+
+  // Ensure that search returns matches for URL only.
+  nodes.clear();
+  query.word_phrase_query.reset(
+      new base::string16(base::ASCIIToUTF16("dat.com")));
+  shim->GetPartnerBookmarksMatchingProperties(query, 100, &nodes);
+  ASSERT_EQ(1u, nodes.size());
+  ASSERT_EQ(partner_bookmark1, nodes[0]);
+
+  // Ensure that folders appear in search results, and that max_count is
+  // effective.
+  nodes.clear();
+  query.word_phrase_query.reset(
+      new base::string16(base::ASCIIToUTF16("folder")));
+
+  shim->GetPartnerBookmarksMatchingProperties(query, 100, &nodes);
+  ASSERT_EQ(2u, nodes.size());
+
+  nodes.clear();
+  shim->GetPartnerBookmarksMatchingProperties(query, 1, &nodes);
+  ASSERT_EQ(1u, nodes.size());
+
+  nodes.clear();
+  shim->GetPartnerBookmarksMatchingProperties(query, 0, &nodes);
+  ASSERT_EQ(0u, nodes.size());
+
+  // Test a scenario with no search results.
+  nodes.clear();
+  query.word_phrase_query.reset(
+      new base::string16(base::ASCIIToUTF16("foo.com")));
+  shim->GetPartnerBookmarksMatchingProperties(query, 100, &nodes);
+  ASSERT_EQ(0u, nodes.size());
+}

components/bookmarks/browser/bookmark_utils.cc

@@ -95,18 +95,6 @@ bool DoesBookmarkTextContainWords(const base::string16& text,
   return true;
 }
 
-// Returns true if |node|s title or url contains the strings in |words|.
-bool DoesBookmarkContainWords(const BookmarkNode* node,
-                              const std::vector<base::string16>& words) {
-  return DoesBookmarkTextContainWords(node->GetTitle(), words) ||
-         DoesBookmarkTextContainWords(base::UTF8ToUTF16(node->url().spec()),
-                                      words) ||
-         DoesBookmarkTextContainWords(
-             url_formatter::FormatUrl(
-                 node->url(), url_formatter::kFormatUrlOmitNothing,
-                 net::UnescapeRule::NORMAL, nullptr, nullptr, nullptr),
-             words);
-}
 
 // This is used with a tree iterator to skip subtrees which are not visible.
 bool PruneInvisibleFolders(const BookmarkNode* node) {
@@ -196,7 +184,8 @@ void GetBookmarksMatchingPropertiesImpl(
   while (iterator.has_next()) {
     const BookmarkNode* node = iterator.Next();
     if ((!query_words.empty() &&
-         !DoesBookmarkContainWords(node, query_words)) ||
+         !DoesBookmarkContainWords(node->GetTitle(), node->url(),
+                                   query_words)) ||
         model->is_permanent_node(node)) {
       continue;
     }
@@ -404,15 +393,9 @@ void GetBookmarksMatchingProperties(BookmarkModel* model,
                                     const QueryFields& query,
                                     size_t max_count,
                                     std::vector<const BookmarkNode*>* nodes) {
-  std::vector<base::string16> query_words;
-  query_parser::QueryParser parser;
-  if (query.word_phrase_query) {
-    parser.ParseQueryWords(base::i18n::ToLower(*query.word_phrase_query),
-                           query_parser::MatchingAlgorithm::DEFAULT,
-                           &query_words);
-    if (query_words.empty())
-      return;
-  }
+  std::vector<base::string16> query_words = ParseBookmarkQuery(query);
+  if (query.word_phrase_query && query_words.empty())
+    return;
 
   if (query.url) {
     // Shortcut into the BookmarkModel if searching for URL.
@@ -431,6 +414,32 @@ void GetBookmarksMatchingProperties(BookmarkModel* model,
   }
 }
 
+// Parses the provided query and returns a vector of query words.
+std::vector<base::string16> ParseBookmarkQuery(
+    const bookmarks::QueryFields& query) {
+  std::vector<base::string16> query_words;
+  query_parser::QueryParser parser;
+  if (query.word_phrase_query) {
+    parser.ParseQueryWords(base::i18n::ToLower(*query.word_phrase_query),
+                           query_parser::MatchingAlgorithm::DEFAULT,
+                           &query_words);
+  }
+  return query_words;
+}
+
+// Returns true if |node|s title or url contains the strings in |words|.
+bool DoesBookmarkContainWords(const base::string16& title,
+                              const GURL& url,
+                              const std::vector<base::string16>& words) {
+  return DoesBookmarkTextContainWords(title, words) ||
+         DoesBookmarkTextContainWords(base::UTF8ToUTF16(url.spec()), words) ||
+         DoesBookmarkTextContainWords(
+             url_formatter::FormatUrl(url, url_formatter::kFormatUrlOmitNothing,
+                                      net::UnescapeRule::NORMAL, nullptr,
+                                      nullptr, nullptr),
+             words);
+}
+
 void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterBooleanPref(
       prefs::kShowBookmarkBar,

components/bookmarks/browser/bookmark_utils.h

@@ -90,6 +90,16 @@ void GetBookmarksMatchingProperties(BookmarkModel* model,
                                     size_t max_count,
                                     std::vector<const BookmarkNode*>* nodes);
 
+// Parses the provided query and returns a vector of query words.
+std::vector<base::string16> ParseBookmarkQuery(
+    const bookmarks::QueryFields& query);
+
+// Returns true iff |title| or |url| contains each string in |words|. This is
+// used when searching for bookmarks.
+bool DoesBookmarkContainWords(const base::string16& title,
+                              const GURL& url,
+                              const std::vector<base::string16>& words);
+
 // Register user preferences for Bookmarks Bar.
 void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);