Add local search API to Enhanced Bookmark Bridge

chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc

 // Copyright 2014 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 "chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h"
 
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h"
 #include "chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service_factory.h"
 #include "chrome/browser/enhanced_bookmarks/enhanced_bookmark_model_factory.h"
 #include "chrome/browser/profiles/profile_android.h"
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/pref_names.h"
+#include "components/bookmarks/browser/bookmark_match.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_utils.h"
 #include "components/bookmarks/common/android/bookmark_id.h"
 #include "components/bookmarks/common/android/bookmark_type.h"
 #include "components/enhanced_bookmarks/enhanced_bookmark_model.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "jni/EnhancedBookmarksBridge_jni.h"
 
 using base::android::AttachCurrentThread;
@@ skipping 178 matching lines @@
   if (!results.get())
     return ScopedJavaLocalRef<jobject>();
 
   for (const BookmarkNode* node : *results) {
     Java_EnhancedBookmarksBridge_addToBookmarkIdList(env, j_list.obj(),
                                                      node->id(), node->type());
   }
   return j_list;
 }
 
+ScopedJavaLocalRef<jobject> EnhancedBookmarksBridge::SearchLocalResults(
+    JNIEnv* env,
+    jobject obj,
+    jstring j_query) {

[Kibeom Kim (inactive), 2014/10/31 09:13:51]

This function doesn't use any enhanced bookmark feature so it should go
BookmarksBridge.

I think EnhancedBookmarksBridge should be a place for enhanced bookmarks
features, not a collection of what we use in our enhanced bookmark dialog. If
so, we should move BookmarksBridge#getOtherFolderId() EnhancedBookmarksBridge
because we don't call that in our old bookmark UI. But then it just introduces
unnecessary code dependency.

[Ian Wen, 2014/10/31 17:46:14]

Done.

+  DCHECK(enhanced_bookmark_model_->loaded());
+
+  ScopedJavaLocalRef<jobject> j_list =
+            Java_EnhancedBookmarksBridge_createBookmarkIdList(env);
+  std::vector<bookmarks::BookmarkMatch> results;
+  const size_t kMaxBookmarkMatches = 1000;

[Kibeom Kim (inactive), 2014/10/31 09:13:51]

This is quite arbitrary. Let's make it a function argument or
std::numeric_limits<size_t>::max() or BookmarkModel's bookmark count if
available.

[Ian Wen, 2014/10/31 17:46:14]

Basically we have 3 options here.
1. A number that is too large for the user to browse.(1000 here)
2. Total number of children of root node.
3. Maximum number allowed by size_t.

Option 2 and 3 are okay in theory, but option 1 might be a more practical choice
here. Having 1000 result or 10000 looks exactly the same to a mobile user
(scroll bar is already at minimum size, and user is definitely not going to
browse these 1000 results to find something). Also this kind of ensures that
even if user search for something really broad terms like "com", it still does
not kill the performance.

Correct me if I am wrong.

[Kibeom Kim (inactive), 2014/10/31 18:04:42]

Yes I agree that it's good to do such optimization, but that should be up to UI
code, not built into the API, so that's why I suggested making as an argument so
that UI code can decide what it requires.

If we're going to trim the list, I think we should show something like "..." at
the end of the list to let user know there is actually more.

Or better, Maybe something like:

1. For search, request 100 bookmarks.
2. If user scrolls down below half, request 500 bookmarks
3. repeat 2 and increase the number.

+  enhanced_bookmark_model_->bookmark_model()->GetBookmarksMatching(

[lpromero, 2014/10/31 16:27:36]

As per email thread on mobile-stars, this search function will not return
anything when ANY term in the query is less than 3 characters and does not match
exactly (ie is equal, not a substring) of a term in the titles.

Do we really want to use this? The resulting UX feels really weird.

On iOS, I will put a flag for this search and let people see what they think of
both.

[Ian Wen, 2014/10/31 17:46:14]

The function we used can be modified if we really like to have result for only
one/two letters. I am okay with both.

+      base::android::ConvertJavaStringToUTF16(env, j_query),
+      kMaxBookmarkMatches, &results);
+  for (const bookmarks::BookmarkMatch& match : results) {
+    const BookmarkNode* node = match.node;
+    Java_EnhancedBookmarksBridge_addToBookmarkIdList(env, j_list.obj(),
+                                                     node->id(), node->type());
+  }
+  return j_list;
+}
+
 void EnhancedBookmarksBridge::OnChange(BookmarkServerService* service) {
   DCHECK(enhanced_bookmark_model_->loaded());
   JNIEnv* env = AttachCurrentThread();
 
   ScopedJavaLocalRef<jobject> obj = weak_java_ref_.get(env);
   if (obj.is_null())
     return;
 
   if (service == cluster_service_) {
     Java_EnhancedBookmarksBridge_onFiltersChanged(env, obj.obj());
@@ skipping 15 matching lines @@
   return reinterpret_cast<jlong>(new EnhancedBookmarksBridge(
         env, obj, ProfileAndroid::FromProfileAndroid(j_profile)));
 }
 
 bool RegisterEnhancedBookmarksBridge(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 }  // namespace android
 }  // namespace enhanced_bookmarks