Fix a small bug in bookmarks_bridge.cc

chrome/browser/android/bookmarks/bookmarks_bridge.cc

 // Copyright 2013 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/bookmarks/bookmarks_bridge.h"
 
 #include "base/android/jni_string.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
@@ skipping 66 matching lines @@
 static jboolean IsEditBookmarksEnabled(JNIEnv* env, jclass clazz) {
   return IsEditBookmarksEnabled();
 }
 
 void BookmarksBridge::GetBookmarksForFolder(JNIEnv* env,
                                             jobject obj,
                                             jobject j_folder_id_obj,
                                             jobject j_callback_obj,
                                             jobject j_result_obj) {
   DCHECK(bookmark_model_->loaded());
-  long folder_id = Java_BookmarkId_getId(env, j_folder_id_obj);
-  int type = Java_BookmarkId_getType(env, j_folder_id_obj);
-  const BookmarkNode* folder = GetFolderWithFallback(folder_id, type);
+  const BookmarkNode* folder = GetFolderWithFallback(j_folder_id_obj);
+  // Recreate the java bookmarkId object due to fallback.

[newt (away), 2013/11/08 01:54:06]

An idea: you could modify GetFolderWithFallback() to "return" the fallen back
Java BookmarkId as well as the BookmarkNode (using out parameters). Then you
could move lines 88-91 into GetFolderWithFallback() and call
Java_BookmarksBridge_getBookmarkId() only if fallback was required. e.g.:

    const BookmarkNode* GetFolderWithFallback(ScopedJavaLocalRef<jobject>
j_folder_id) {
        ...
        // if fallback:
        j_folder_id.Reset(Java_BookmarksBridge_getBookmarkId(env, folder->id())
    }

+  ScopedJavaLocalRef<jobject> folder_id_obj =
+      Java_BookmarksBridge_getBookmarkId(
+          env, folder->id(), GetBookmarkType(folder));
+  j_folder_id_obj = folder_id_obj.obj();
   // If this is the Mobile bookmarks folder then add the "Managed bookmarks"
   // folder first, so that it's the first entry.
   if (folder == bookmark_model_->mobile_node() &&
       managed_bookmarks_shim_->HasManagedBookmarks()) {
     ExtractBookmarkNodeInformation(
         managed_bookmarks_shim_->GetManagedBookmarksRoot(),
         j_result_obj);
   }
   // Get the folder contents
   for (int i = 0; i < folder->child_count(); ++i) {
     const BookmarkNode* node = folder->GetChild(i);
     ExtractBookmarkNodeInformation(node, j_result_obj);
   }
 
   Java_BookmarksCallback_onBookmarksAvailable(
       env, j_callback_obj, j_folder_id_obj, j_result_obj);
 }
 
 void BookmarksBridge::GetCurrentFolderHierarchy(JNIEnv* env,
                                                 jobject obj,
                                                 jobject j_folder_id_obj,
                                                 jobject j_callback_obj,
                                                 jobject j_result_obj) {
   DCHECK(bookmark_model_->loaded());
-  long folder_id = Java_BookmarkId_getId(env, j_folder_id_obj);
-  int type = Java_BookmarkId_getType(env, j_folder_id_obj);
-  const BookmarkNode* folder = GetFolderWithFallback(folder_id, type);
+  const BookmarkNode* folder = GetFolderWithFallback(j_folder_id_obj);
+  // Recreate the java bookmarkId object due to fallback.
+  ScopedJavaLocalRef<jobject> folder_id_obj =
+      Java_BookmarksBridge_getBookmarkId(
+          env, folder->id(), GetBookmarkType(folder));
+  j_folder_id_obj = folder_id_obj.obj();
   // Get the folder heirarchy
   const BookmarkNode* node = folder;
   while (node) {
     ExtractBookmarkNodeInformation(node, j_result_obj);
     node = GetParentNode(node);
   }
-
   Java_BookmarksCallback_onBookmarksFolderHierarchyAvailable(
       env, j_callback_obj, j_folder_id_obj, j_result_obj);
 }
 
 void BookmarksBridge::DeleteBookmark(JNIEnv* env,
                                      jobject obj,
                                      jobject j_bookmark_id_obj) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(bookmark_model_->loaded());
 
-  long bookmark_id = Java_BookmarkId_getId(env, j_bookmark_id_obj);
-  int type = Java_BookmarkId_getType(env, j_bookmark_id_obj);
-  const BookmarkNode* node = GetNodeByID(bookmark_id, type);
+  const BookmarkNode* node = GetNodeByID(j_bookmark_id_obj);
   if (!IsEditable(node)) {
     NOTREACHED();
     return;
   }
 
   const BookmarkNode* parent_node = GetParentNode(node);
   bookmark_model_->Remove(parent_node, parent_node->GetIndexOf(node));
 }
 
 ScopedJavaLocalRef<jobject> BookmarksBridge::CreateJavaBookmark(
@@ skipping 34 matching lines @@
   const BookmarkNode* node;
   if (type == kBookmarkTypeManaged) {
     node = managed_bookmarks_shim_->GetNodeByID(
         static_cast<int64>(node_id));
   } else {
     node = bookmark_model_->GetNodeByID(static_cast<int64>(node_id));
   }
   return node;
 }
 
+const BookmarkNode* BookmarksBridge::GetNodeByID(jobject j_node_id_obj) {

[newt (away), 2013/11/08 01:54:06]

I'm not sure how much work AttachCurrentThread() is, but it's probably worth
passing in to GetNodeByID() and GetFolderWithFallback() the JNIEnv since we
already know it.

+  JNIEnv* env = AttachCurrentThread();
+  long node_id = Java_BookmarkId_getId(env, j_node_id_obj);
+  int type = Java_BookmarkId_getType(env, j_node_id_obj);
+  return GetNodeByID(node_id, type);
+}
+
 const BookmarkNode* BookmarksBridge::GetFolderWithFallback(
     long folder_id, int type) {
   const BookmarkNode* folder = GetNodeByID(folder_id, type);
   if (!folder || folder->type() == BookmarkNode::URL)
     folder = bookmark_model_->mobile_node();
   return folder;
 }
 
+const BookmarkNode* BookmarksBridge::GetFolderWithFallback(

[Ted C, 2013/11/08 01:28:27]

I wouldn't add new methods for this or GetNodeByID.  I think keep the awareness
of the bookmarkID java object isolated to only the JNI methods and use the
previous versions internally.

[Ramya, 2013/11/11 18:34:16]

Done.

+    jobject j_folder_id_obj) {
+  JNIEnv* env = AttachCurrentThread();
+  long folder_id = Java_BookmarkId_getId(env, j_folder_id_obj);
+  int type = Java_BookmarkId_getType(env, j_folder_id_obj);
+  const BookmarkNode* folder = GetFolderWithFallback(folder_id, type);
+  return folder;
+}
+
 bool BookmarksBridge::IsEditable(const BookmarkNode* node) const {
   return node &&
       (node->type() == BookmarkNode::FOLDER ||
           node->type() == BookmarkNode::URL) &&
       !managed_bookmarks_shim_->IsManagedBookmark(node) &&
       IsEditBookmarksEnabled();
 }
 
 const BookmarkNode* BookmarksBridge::GetParentNode(const BookmarkNode* node) {
   DCHECK(bookmark_model_->loaded());
@@ skipping 122 matching lines @@
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jobject> obj = weak_java_ref_.get(env);
   if (obj.is_null())
     return;
   Java_BookmarksBridge_extensiveBookmarkChangesEnded(env, obj.obj());
 }
 
 void BookmarksBridge::OnManagedBookmarksChanged() {
   BookmarkModelChanged();
 }