[Android] Change enhanced bookmark top level definitions.

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 128caa9fef6c686eb99b1cd5c1344d92f8240650..55606508a965eb0d778fb1b0e8710e32a912a3dc 100644
--- a/chrome/browser/android/bookmarks/bookmarks_bridge.cc
+++ b/chrome/browser/android/bookmarks/bookmarks_bridge.cc
@@ -211,9 +211,6 @@ void BookmarksBridge::GetTopLevelFolderParentIDs(JNIEnv* env,
       env, j_result_obj, bookmark_model_->root_node()->id(),
       GetBookmarkType(bookmark_model_->root_node()));
   Java_BookmarksBridge_addToBookmarkIdList(
-      env, j_result_obj, bookmark_model_->mobile_node()->id(),
-      GetBookmarkType(bookmark_model_->mobile_node()));
-  Java_BookmarksBridge_addToBookmarkIdList(
       env, j_result_obj, bookmark_model_->other_node()->id(),
       GetBookmarkType(bookmark_model_->other_node()));
 }
@@ -243,13 +240,7 @@ void BookmarksBridge::GetTopLevelFolderIDs(JNIEnv* env,
 
     top_level_folders.push_back(bookmark_model_->bookmark_bar_node());
 
-    const BookmarkNode* mobile_node = bookmark_model_->mobile_node();
-    for (int i = 0; i < mobile_node->child_count(); ++i) {
-      const BookmarkNode* node = mobile_node->GetChild(i);
-      if (node->is_folder()) {
-        top_level_folders.push_back(node);
-      }
-    }
+    top_level_folders.push_back(bookmark_model_->mobile_node());

[Ian Wen, 2015/01/24 02:03:43]

I thought we decided to make mobile a special root folder that sits besides all
bookmarks, and is the folder that the new user would be navigate to in their
first time star experience. Are we now making it a normal folder in the midst of
other folders?

[Kibeom Kim (inactive), 2015/01/24 02:48:07]

I thought so too, but realized that we are also treating "Bookmarks bar"
(desktop) in this way. In fact I don't remember which was the latest decision,
but even if we need to change, I think it's good to be another CL as it includes
desktop node and also downstream fix.

 
     const BookmarkNode* other_node = bookmark_model_->other_node();
     for (int i = 0; i < other_node->child_count(); ++i) {
@@ -318,17 +309,19 @@ void BookmarksBridge::GetAllFoldersWithDepths(JNIEnv* env,
   // heights.
   std::stack<std::pair<const BookmarkNode*, int> > stk;
 
-  for (int i = 0; i < mobile->child_count(); ++i) {

[Ian Wen, 2015/01/24 02:03:43]

According to the mock and to the old bookmark manager, this list should return
all subfolders that are either in mobile node or in other. Doesn't this change
break this logic?

Also this change seems to break the folder choosing UI. Currently if user does
nothing, the NONE entry will be selected, and the parent folder will be MOBILE.
If we have another mobile folder lying in the underneath list, isn't this a bit
confusing?

[Kibeom Kim (inactive), 2015/01/24 02:48:07]

Yeah right, so I was planning to change NONE to OTHERS in the downstream CL but
seems I forgot that. I will update the downstream CL.

btw, I think the top level folders shown on the drawer-list and folder-choosing
dialog should be consistent.

-    const BookmarkNode* child = mobile->GetChild(i);
-    if (child->is_folder() && client_->CanBeEditedByUser(child))
-      bookmarkList.push_back(child);
-  }
+  // TODO(kkimlabs): Share the top level folder logic with
+  //                 |GetTopLevelFolderIDs|. There is no reason to have two
+  //                 places doing the same work.
+
   for (int i = 0; i < other->child_count(); ++i) {
     const BookmarkNode* child = other->GetChild(i);
     if (child->is_folder() && client_->CanBeEditedByUser(child))
       bookmarkList.push_back(child);
   }
+
   bookmarkList.push_back(desktop);

[newt (away), 2015/01/24 00:40:00]

Does this affect the order in which bookmark folders are listed? If so, seems
like we should put mobile first (no pun intended).

[Kibeom Kim (inactive), 2015/01/24 02:48:07]

Yeah we should put mobile first (pun intended).

I didn't bother ordering since we sort at #325 anyways, but I guess if we have
some same name folders, we'd want to put the special nodes first. Done.

+  bookmarkList.push_back(mobile);
+
   std::stable_sort(bookmarkList.begin(),
                    bookmarkList.end(),
                    BookmarkTitleComparer(this, collator.get()));