Hide other bookmarks #3620

darkdh · 2019-10-07T23:57:11Z

Need brave/brave-browser#6344 for rename completely work

This PR make "Other Bookmarks" invisible and prevent user from adding bookmarks into "Other Bookmarks". Existing bookmarks in "Other Bookmarks" will be moved under the same name folder under "Bookmarks"(Bookmarks Bar).
We also rename "Bookmarks Bar" to "Bookmarks" in Bookmarks Manager, [Always] Show Bookmarks Bar in mac menu, menu and setting page

Submitter Checklist:

Test Plan:

Bookmarks Bar to Bookmarks

Check chrome://bookmarks/
Check main menu on Mac
Check menu on all platforms
Check chrome://settings/?search=bookmarks

Hide Other Bookmarks with sync disabled

Open old Brave in fresh profile
Put more than one bookmarks into "Other Bookmarks" and one bookmark into "Bookmarks Bar"
Check these places that it contains "Other Bookmarks"
3.1 Bookmarks Bar
3.2 Bookmarks Manager
3.3 Main menu on Mac
3.4 Menu on all platforms
3.5 Bookmarks Icon
3.6 Edit Bookmark Dialog
Close the old Brave and launch new Brave with the same profile
All existing bookmarks in "Other Bookmarks" will be moved to the same name folder at the end of "Bookmarks Bar"
Check all places in step 3, user should have no way to see "Other Bookmarks" or add any bookmarks into it

Hide Other Bookmarks with sync enabled

Open old Brave in two desktop devices (A, B)
Put more than one bookmarks into "Other Bookmarks" and one bookmark into "Bookmarks Bar"
Establish sync chain between device A and B
Wait until both devices displaying same bookmarks
Close the old Brave and launch new Brave with the same profile on device A
All existing bookmarks in "Other Bookmarks" will be moved to the same name folder at the end of "Bookmarks Bar" on device A
Check all places in step 3 of previous section, user should have no way to see "Other Bookmarks" or add any bookmarks into it on device A
Wait for a while, device B should receive sync update and check step 6 on device B
Relaunch device B with new Brave and check step 7 on device B

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

chromium_src/components/bookmarks/browser/bookmark_model_unittest.cc

AlexeyBarabash

looks good, STR passes

bridiver · 2019-10-15T15:59:58Z

chromium_src/components/bookmarks/browser/bookmark_node.cc

+
+// other_node should remain hidden
+#define BRAVE_IS_VISIBLE    \
+  if (type() == OTHER_NODE) \


can't we use ChromeBookmarkClient::IsPermanentNodeVisible? That's how MOBILE is displayed on android/ios and hidden on desktop

meaning subclass ChromeBookmarkClient if we haven't already

wait, you already have BraveBookmarkClient::IsPermanentNodeVisible so why do we need this?

the patch was originally return visible_ to get rid of !children().empty().
Then I pulled upstream unit test to run and expect it to be just invisible without override TestBookmarkClient

The reason that other node is not always empty listed here
https://github.com/brave/brave-core/pull/3620/files#r335092116

removed in 3f388ed

bridiver · 2019-10-15T16:01:32Z

chromium_src/components/bookmarks/browser/bookmark_model.cc

+
+// Move bookmarks under "Other Bookmarks" permanent node to a same name folder
+// at the end of "Bookmark Bar" permanent node
+void BookmarkModel::MigrateOtherNode() {


are any of these calls private? Can't we just pass the BookmarkModel in to a method somewhere else?

addressed in 122272a

bridiver · 2019-10-15T16:03:17Z

patches/chrome-browser-ui-bookmarks-recently_used_folders_combo_model.cc.patch

+
+   // And put the bookmark bar and other nodes at the end of the list.
+   items_.push_back(Item(model->bookmark_bar_node(), Item::TYPE_NODE));
+  BRAVE_RECENTLY_USED_FOLDERS_COMBO_MODEL


can we subclass this with an override in BookmarkBubbleView and then call
RemoveNode(model->other_node()); in the constructor?

addressed in 456a86f

bridiver · 2019-10-15T16:04:07Z

patches/chrome-browser-ui-cocoa-bookmarks-bookmark_menu_bridge.mm.patch

+   // If the "Other Bookmarks" folder has any content, make a submenu for it and
+   // fill it in.
+-  if (!model->other_node()->children().empty()) {
+  BRAVE_BUILD_ROOT_MENU


why is this needed? The other node will always be empty now, right?

not always, chromium sync uses it as a temporary place to put foster children
https://cs.chromium.org/chromium/src/components/sync_bookmarks/bookmark_change_processor.cc?l=619&rcl=74cefac725837520ab3d553eb2fa531e5ee02f63

addressed in 3f388ed

bridiver · 2019-10-15T16:07:33Z

patches/chrome-browser-ui-views-bookmarks-bookmark_bar_view.cc.patch

+   bool has_other_children = !model_->other_node()->children().empty();
+   bool update_other =
+       has_other_children != other_bookmarks_button_->GetVisible();
+  BRAVE_UPDATE_OTHER_AND_MANAGED_BUTTONS_VISIBILITY


is this needed since has_other_children is false so it will set the visibility of the button to false here? https://github.com/brave/brave-core/pull/3620/files#diff-b3cabb3a3f40d70b6d97898b3f71b98fR19 so this is also maybe not needed? https://github.com/brave/brave-core/pull/3620/files#diff-b3cabb3a3f40d70b6d97898b3f71b98fR9

has_other_children will not always be true https://github.com/brave/brave-core/pull/3620/files#r335092116
and
https://github.com/brave/brave-core/pull/3620/files#diff-b3cabb3a3f40d70b6d97898b3f71b98fR9
is to prevent it from drawing separator (BookmarkBarView::UpdateBookmarksSeparatorVisibility)

no, I'm saying has_other_children will always be false so other_bookmarks_button_->SetVisible(has_other_children); will always be set to false

addressed in 3f388ed

… bookmarks under it to the same name folder at the end of "Bookmarks Bar" permanent node

and persistently hide other_node

(ignore the temporary visibility by adding foster parent in chromium sync)

…grateOtherNode

bridiver · 2019-10-16T15:59:10Z

chromium_src/components/bookmarks/browser/bookmark_model_unittest.cc

+
+ private:
+  // BookmarkClient:
+  bool IsPermanentNodeVisible(const BookmarkPermanentNode* node) override {


It's not clear to me what we're doing here. Changing this in a test-only bookmark client doesn't seem to really validate anything useful for us in the test?

addressed in 0f076bb

…delLoaded to avoid duplication

#3620

brave#3620

darkdh mentioned this pull request Oct 7, 2019

Rename "Bookmarks Bar/bar" to "Bookmarks" (brave-browser) brave/brave-browser#6344

Merged

28 tasks

darkdh force-pushed the hide-other-bookmarks branch 3 times, most recently from e2f0a49 to a4cfdee Compare October 9, 2019 23:12

darkdh marked this pull request as ready for review October 9, 2019 23:12

darkdh requested a review from bridiver as a code owner October 9, 2019 23:12

darkdh force-pushed the hide-other-bookmarks branch 2 times, most recently from 31be5b5 to 1e0da64 Compare October 9, 2019 23:45

darkdh self-assigned this Oct 9, 2019

darkdh requested review from AlexeyBarabash, bsclifton and petemill October 10, 2019 01:01

AlexeyBarabash reviewed Oct 10, 2019

View reviewed changes

chromium_src/components/bookmarks/browser/bookmark_model_unittest.cc Outdated Show resolved Hide resolved

AlexeyBarabash reviewed Oct 10, 2019

View reviewed changes

chromium_src/components/bookmarks/browser/bookmark_model_unittest.cc Outdated Show resolved Hide resolved

AlexeyBarabash previously approved these changes Oct 10, 2019

View reviewed changes

bridiver reviewed Oct 15, 2019

View reviewed changes

darkdh dismissed AlexeyBarabash’s stale review via 3f388ed October 15, 2019 23:20

darkdh added 8 commits October 15, 2019 20:31

Make "Other bookmarks" permanent node invisible to users and move all…

724d63c

… bookmarks under it to the same name folder at the end of "Bookmarks Bar" permanent node

Rename "Bookmarks Bar/bar" to "Bookmarks"

ddbd1f9

Decouple BookmarkModelObserver from BraveProfileSyncServiceImpl

d6c2a8b

and persistently hide other_node

Add BookmarkModelTest

5fa8776

Subclass RecentlyUsedFoldersComboModel

6fed841

Remove checking other_node visibility disregarding children count

f892c5d

(ignore the temporary visibility by adding foster parent in chromium sync)

Move BookmarkModel::MigrateOtherNode to a standalone function BraveMi…

4dab70c

…grateOtherNode

78.0.3904.50 update

48f3202

darkdh force-pushed the hide-other-bookmarks branch from 122272a to 48f3202 Compare October 16, 2019 06:00

bridiver reviewed Oct 16, 2019

View reviewed changes

darkdh added 2 commits October 16, 2019 11:15

Add BraveBookmarkClientTest browser test

0f076bb

Calling parent method in BraveBookmarkModelLoadedObserver::BookmarkMo…

1cd5cb7

…delLoaded to avoid duplication

darkdh force-pushed the hide-other-bookmarks branch from b821950 to 1cd5cb7 Compare October 16, 2019 20:14

bridiver approved these changes Oct 16, 2019

View reviewed changes

darkdh merged commit ccafc94 into master Oct 16, 2019

darkdh deleted the hide-other-bookmarks branch October 16, 2019 22:08

darkdh added this to the 0.73.x - Nightly milestone Oct 16, 2019

btlechowski mentioned this pull request Nov 27, 2019

Desktop Bookmark Platform Alignment brave/brave-browser#5158

Closed

darkdh added a commit that referenced this pull request Jan 28, 2020

Unhide other bookmarks node

1b51d3d

#3620

darkdh mentioned this pull request Jan 28, 2020

Restore other bookmarks #4404

Merged

32 tasks

darkdh added a commit that referenced this pull request Feb 8, 2020

Unhide other bookmarks node

09800ec

#3620

darkdh added a commit that referenced this pull request Feb 20, 2020

Unhide other bookmarks node

eaf5dd9

#3620

jonathanKingston pushed a commit to jonathanKingston/brave-core that referenced this pull request Mar 11, 2020

Unhide other bookmarks node

07444f6

brave#3620

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide other bookmarks #3620

Hide other bookmarks #3620

darkdh commented Oct 7, 2019 •

edited

Loading

AlexeyBarabash left a comment

bridiver Oct 15, 2019

bridiver Oct 15, 2019

bridiver Oct 15, 2019

darkdh Oct 15, 2019

darkdh Oct 15, 2019

darkdh Oct 16, 2019

bridiver Oct 15, 2019

darkdh Oct 16, 2019

bridiver Oct 15, 2019

darkdh Oct 15, 2019

bridiver Oct 15, 2019

darkdh Oct 15, 2019

darkdh Oct 15, 2019

bridiver Oct 15, 2019

darkdh Oct 15, 2019

bridiver Oct 15, 2019

darkdh Oct 15, 2019 •

edited

Loading

bridiver Oct 16, 2019

darkdh Oct 16, 2019

Hide other bookmarks #3620

Hide other bookmarks #3620

Conversation

darkdh commented Oct 7, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Bookmarks Bar to Bookmarks

Hide Other Bookmarks with sync disabled

Hide Other Bookmarks with sync enabled

Reviewer Checklist:

After-merge Checklist:

AlexeyBarabash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh commented Oct 7, 2019 •

edited

Loading

darkdh Oct 15, 2019 •

edited

Loading