Fix a small bug in bookmarks_bridge.cc

chrome/browser/android/bookmarks/bookmarks_bridge.cc

Index: chrome/browser/android/bookmarks/bookmarks_bridge.cc
diff --git a/chrome/browser/android/bookmarks/bookmarks_bridge.cc b/chrome/browser/android/bookmarks/bookmarks_bridge.cc
index c355b497a1cf6273b1685f19aab782411da27172..9c018802fcf58cdeeec890817d1452a2221fa344 100644
--- a/chrome/browser/android/bookmarks/bookmarks_bridge.cc
+++ b/chrome/browser/android/bookmarks/bookmarks_bridge.cc
@@ -84,9 +84,12 @@ void BookmarksBridge::GetBookmarksForFolder(JNIEnv* env,
                                             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() &&
@@ -111,16 +114,18 @@ void BookmarksBridge::GetCurrentFolderHierarchy(JNIEnv* env,
                                                 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);
 }
@@ -131,9 +136,7 @@ void BookmarksBridge::DeleteBookmark(JNIEnv* env,
   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;
@@ -188,6 +191,13 @@ const BookmarkNode* BookmarksBridge::GetNodeByID(long 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);
@@ -196,6 +206,15 @@ const BookmarkNode* BookmarksBridge::GetFolderWithFallback(
   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 ||