[Android] Change enhanced bookmark top level definitions.

[Android] Change enhanced bookmark folder structure.

- Top level folders are now all the sub folders of
  Bookmark Bar, Mobile, and Others nodes.
- We no longer use Uncategorized.

BUG=453024
Committed: https://crrev.com/a01c664cd6802d64d212f68d1c9392a4340ca835
Cr-Commit-Position: refs/heads/master@{#314013}

[Kibeom Kim (inactive), 2015-01-24 00:07:59]

kkimlabs@chromium.org changed reviewers:
+ ianwen@chromium.org, newt@chromium.org, tedchoc@chromium.org

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

https://codereview.chromium.org/869193008/diff/1/chrome/browser/android/bookm...
File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right):

https://codereview.chromium.org/869193008/diff/1/chrome/browser/android/bookm...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:271: const BookmarkNode*
mobile_node = bookmark_model_->mobile_node();
Is this method used anymore? Shouldn't you remove bookmarks under Mobile
Bookmarks from here?

https://codereview.chromium.org/869193008/diff/1/chrome/browser/android/bookm...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:322:
bookmarkList.push_back(desktop);
Does this affect the order in which bookmark folders are listed? If so, seems
like we should put mobile first (no pun intended).

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

https://codereview.chromium.org/869193008/diff/1/chrome/browser/android/bookm...
File chrome/browser/android/bookmarks/bookmarks_bridge.cc (left):

https://codereview.chromium.org/869193008/diff/1/chrome/browser/android/bookm...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:321: for (int i = 0; i <
mobile->child_count(); ++i) {
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?

https://codereview.chromium.org/869193008/diff/1/chrome/browser/android/bookm...
File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right):

https://codereview.chromium.org/869193008/diff/1/chrome/browser/android/bookm...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:243:
top_level_folders.push_back(bookmark_model_->mobile_node());
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]

https://codereview.chromium.org/869193008/diff/1/chrome/browser/android/bookm...
File chrome/browser/android/bookmarks/bookmarks_bridge.cc (left):

https://codereview.chromium.org/869193008/diff/1/chrome/browser/android/bookm...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:321: for (int i = 0; i <
mobile->child_count(); ++i) {
On 2015/01/24 02:03:43, Ian Wen wrote:
> 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?

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.

https://codereview.chromium.org/869193008/diff/1/chrome/browser/android/bookm...
File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right):

https://codereview.chromium.org/869193008/diff/1/chrome/browser/android/bookm...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:243:
top_level_folders.push_back(bookmark_model_->mobile_node());
On 2015/01/24 02:03:43, Ian Wen wrote:
> 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?

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.

https://codereview.chromium.org/869193008/diff/1/chrome/browser/android/bookm...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:271: const BookmarkNode*
mobile_node = bookmark_model_->mobile_node();
On 2015/01/24 00:40:00, newt wrote:
> Is this method used anymore? Shouldn't you remove bookmarks under Mobile
> Bookmarks from here?

Yes I should have removed mobile_node here. Done.

https://codereview.chromium.org/869193008/diff/1/chrome/browser/android/bookm...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:322:
bookmarkList.push_back(desktop);
On 2015/01/24 00:40:00, newt wrote:
> Does this affect the order in which bookmark folders are listed? If so, seems
> like we should put mobile first (no pun intended).

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.

[Kibeom Kim (inactive), 2015-01-28 19:15:57]

PTAL.

Proposal changed (the doc linked at the bug) so this CL is also changed. I think
it will be easier to review by diff against base, not patch set 1 because it's
somewhat different.

[Ian Wen, 2015-01-28 19:57:09]

https://codereview.chromium.org/869193008/diff/80001/chrome/browser/android/b...
File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right):

https://codereview.chromium.org/869193008/diff/80001/chrome/browser/android/b...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:293:
bookmarkList.push_back(bookmark_model_->bookmark_bar_node());
I wouldn't add them among other second-tier folders. Do you think showing these
three as special folders would be more consistent? Ideally the folder selecting
list should be similar to what we show in the drawer.

[Kibeom Kim (inactive), 2015-01-28 20:01:57]

https://codereview.chromium.org/869193008/diff/80001/chrome/browser/android/b...
File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right):

https://codereview.chromium.org/869193008/diff/80001/chrome/browser/android/b...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:293:
bookmarkList.push_back(bookmark_model_->bookmark_bar_node());
On 2015/01/28 19:57:08, Ian Wen wrote:
> I wouldn't add them among other second-tier folders. Do you think showing
these
> three as special folders would be more consistent? Ideally the folder
selecting
> list should be similar to what we show in the drawer.

Only these three nodes will be the top level. Second-tier folders are shown as
sub-folders of them. (Also mentioned in the doc, the 4th bullet)

[Ian Wen, 2015-01-28 20:08:19]

Ok. I saw you show special icons for those three folders. Then I suppose they
are obvious enough. lgtm.

[newt (away), 2015-01-28 21:43:04]

lgtm

[Kibeom Kim (inactive), 2015-01-30 20:33:06]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2015-01-30 20:33:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/869193008/120001

[commit-bot: I haz the power, 2015-01-30 21:21:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-30 21:21:05]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Kibeom Kim (inactive), 2015-01-30 21:55:34]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2015-01-30 21:56:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/869193008/140001

[commit-bot: I haz the power, 2015-01-30 22:40:07]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-01-30 22:41:46]

Patchset 8 (id:??) landed as
https://crrev.com/a01c664cd6802d64d212f68d1c9392a4340ca835
Cr-Commit-Position: refs/heads/master@{#314013}