[Android] Change enhanced bookmark top level definitions.

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_array.h"
 #include "base/android/jni_string.h"
 #include "base/containers/stack_container.h"
 #include "base/i18n/string_compare.h"
@@ skipping 193 matching lines @@
   }
 }
 
 void BookmarksBridge::GetTopLevelFolderParentIDs(JNIEnv* env,
                                                  jobject obj,
                                                  jobject j_result_obj) {
   Java_BookmarksBridge_addToBookmarkIdList(
       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()));
 }
 
 void BookmarksBridge::GetTopLevelFolderIDs(JNIEnv* env,
                                            jobject obj,
                                            jboolean get_special,
                                            jboolean get_normal,
                                            jobject j_result_obj) {
   DCHECK(IsLoaded());
   std::vector<const BookmarkNode*> top_level_folders;
 
   if (get_special) {
     if (client_->managed_node() &&
         client_->managed_node()->child_count() > 0) {
       top_level_folders.push_back(client_->managed_node());
     }
     if (partner_bookmarks_shim_->HasPartnerBookmarks()) {
       top_level_folders.push_back(
           partner_bookmarks_shim_->GetPartnerBookmarksRoot());
     }
   }
   std::size_t special_count = top_level_folders.size();
 
   if (get_normal) {
     DCHECK_EQ(bookmark_model_->root_node()->child_count(), 4);
 
     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) {
       const BookmarkNode* node = other_node->GetChild(i);
       if (node->is_folder()) {
         top_level_folders.push_back(node);
       }
     }
 
     scoped_ptr<icu::Collator> collator = GetICUCollator();
     std::stable_sort(top_level_folders.begin() + special_count,
                      top_level_folders.end(),
                      BookmarkTitleComparer(this, collator.get()));
   }
 
   for (std::vector<const BookmarkNode*>::const_iterator it =
       top_level_folders.begin(); it != top_level_folders.end(); ++it) {
     Java_BookmarksBridge_addToBookmarkIdList(env,
                                              j_result_obj,
                                              (*it)->id(),
                                              GetBookmarkType(*it));
   }
 }
 
 void BookmarksBridge::GetUncategorizedBookmarkIDs(JNIEnv* env,
                                                   jobject obj,
                                                   jobject j_result_obj) {
   const BookmarkNode* mobile_node = bookmark_model_->mobile_node();

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

Is this method used anymore? Shouldn't you remove bookmarks under Mobile
Bookmarks from here?

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

Yes I should have removed mobile_node here. Done.

   for (int i = 0; i < mobile_node->child_count(); ++i) {
     const BookmarkNode* node = mobile_node->GetChild(i);
     if (!node->is_folder()) {
       Java_BookmarksBridge_addToBookmarkIdList(env,
                                                j_result_obj,
                                                node->id(),
                                                GetBookmarkType(node));
     }
   }
 
@@ skipping 20 matching lines @@
   const BookmarkNode* other = bookmark_model_->other_node();
 
   scoped_ptr<icu::Collator> collator = GetICUCollator();
 
   // Vector to temporarily contain all child bookmarks at same level for sorting
   std::vector<const BookmarkNode*> bookmarkList;
   // Stack for Depth-First Search of bookmark model. It stores nodes and their
   // 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()));
 
   // Push all sorted top folders in stack and give them depth of 0.
   // Note the order to push folders to stack should be opposite to the order in
   // output.
   for (std::vector<const BookmarkNode*>::reverse_iterator it =
            bookmarkList.rbegin();
        it != bookmarkList.rend();
@@ skipping 708 matching lines @@
   BookmarkModelChanged();
 }
 
 void BookmarksBridge::PartnerShimLoaded(PartnerBookmarksShim* shim) {
   NotifyIfDoneLoading();
 }
 
 void BookmarksBridge::ShimBeingDeleted(PartnerBookmarksShim* shim) {
   partner_bookmarks_shim_ = NULL;
 }