Add always show bookmark bar on NTP option #2869

simonhong · 2019-07-05T06:13:43Z

Fix brave/brave-browser#4782

Some user might want to show bookmark on NTP
when show bookmark bar option is false.

Like below, only NTP has bookmark bar when this option is on.

Submitter Checklist:

Test Plan:

yarn test brave_browser_tests --filter=BookmarkTabHelperBrowserTest.AlwaysShowBookmarkBarOnNTPTest

Launch browser with clean profile
Check Always show bookmarks bar on New Tab page is off by default
Check Show bookmarks bar is off
Load NTP
Check bookmark bar on NTP is hidden regardless of Always show bookmarks bar on New Tab page
Add any bookmark and turn Always show bookmarks bar on New Tab page on
Check bookmark bar on NTP is visible after active tab changing to trigger bookmark bar visibility
Turn off Always show bookmarks bar on New Tab page and check bookmark bar on NTP is hidden
Turn on Show bookmarks bar and check bookmark bar on NTP is always visible regardless of Always show bookmarks bar on New Tab page status

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.

simonhong · 2019-07-05T06:16:59Z

Need to add test cases and not ready for review.

emerick · 2019-07-05T11:24:16Z

app/brave_generated_resources.grd

@@ -222,6 +222,9 @@ By installing this extension, you are agreeing to the Google Widevine Terms of U
      <message name="IDS_SETTINGS_HIDE_BRAVE_REWARDS_BUTTON_DESC" desc="The description for settings switch controlling the visibility of Brave Rewards button">
        Hides the Brave Rewards button in the location bar when Brave Rewards is not enabled
      </message>
+      <message name="IDS_SETTINGS_ALWAYS_SHOW_BOOKMARK_BAR_ON_NTP" desc="The label for settings switch controlling the visibility of bookmarks bar on NTP">
+        Always show bookmarks bar on NTP


Is this a user-visible string? If so, I don't think the average user would understand what NTP means - probably better to spell it out instead of using an acronym.

Agreed. @rebron Can you check this sentence?

Can you have it read: "Always show bookmarks bar on New Tab page."

simonhong · 2019-07-05T12:32:32Z

@emerick This PR is ready to review. test case is also added. PTAL.

emerick

LGTM

simonhong

Rebased on C76. PTAL @bridiver

simonhong · 2019-07-12T01:03:51Z

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

 --- a/chrome/browser/ui/bookmarks/bookmark_tab_helper.cc
 +++ b/chrome/browser/ui/bookmarks/bookmark_tab_helper.cc
 @@ -67,6 +67,7 @@ bool BookmarkTabHelper::ShouldShowBookmarkBar() const {
       !prefs->GetBoolean(bookmarks::prefs::kShowBookmarkBar))
     return false;

-+  return false;
+  ReturnFalseIfBookmarkBarShouldHide(profile);


This patch is inevitable because we have to return in the middle of existing logic.

there is already a NTP check below. If we need to change the behavior of that check we can do a chromium_src override of search::NavEntryIsInstantNTP

I guess I'm not sure if the override will work because of the || and I'm not sure what the real url is for a custom NTP, but let's discuss in dm

another option here is to override BookmarkTabHelper in browser.cc and replace with our own tab helper that proxies to the original instead of subclassing it (no virtual method issues and only needs to implement the subset of methods used in browser.cc)

BraveBookmarkTabHelper is introduced to proxy BookmarkTabHelper.

simonhong · 2019-07-12T01:12:04Z

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

+                           base::Unretained(this),
+                           BOOKMARK_BAR_STATE_CHANGE_PREF_CHANGE));
+
+  RegisterAlwaysShowBookmarkBarOnNTPChange


There are some reason why I made a patch instead of subclassing Browser class.

Browser is created by static method Browser::Create(). So, we also have to change it to return BraveBrowser. This also causes patching to browser.cc.

To subclass, making a patch for browser.h is inevitable for overriding non-virtual method.
So, direct inserting it here is smallest patch.

while this may be the smallest patch for this particular change, it is not very extensible. I think a better patch is a define in the header file which for now will just add a friend class. The friend class can have its own pref registrar and call UpdateBookmarkBarState using BrowserList (matching against the relevant profile). @iefremov already has a pref related BCKS in his network service PR that might make sense to reuse for this since a BCKS would make the most sense for this imo.

BookmarkPrefsService is introduced.

bsclifton · 2019-07-17T22:27:50Z

@bridiver can you check this one out? 😄

simonhong · 2019-07-23T00:34:45Z

@bridiver kindly ping :)

simonhong · 2019-08-19T12:44:15Z

@bridiver come back from PTO.
I think I addressed all your comments. PTAL.

bridiver · 2019-08-20T20:43:51Z

patches/chrome-browser-ui-browser.h.patch

+   friend class BrowserTest;
+   friend class FullscreenControllerInteractiveTest;
+   friend class FullscreenControllerTest;
+  friend class BookmarkPrefsService;


Most patches should be done as defines now to make them extensible. The convention would be BRAVE_BROWSER_H and placed at the end of public:. To add private entries (like this one) just use private: inside the define

Some user might want to show bookmark on NTP when they choose to hide bookmark bar.

…ption Add always show bookmark bar on NTP option

simonhong added this to the 0.69.x - Nightly milestone Jul 5, 2019

simonhong requested a review from bridiver as a code owner July 5, 2019 06:13

simonhong self-assigned this Jul 5, 2019

simonhong added the CI/skip Do not run CI builds (except noplatform) label Jul 5, 2019

simonhong requested a review from emerick July 5, 2019 06:14

simonhong force-pushed the add_always_show_bookmark_on_ntp_option branch from 377f9bb to c430ec3 Compare July 5, 2019 06:20

simonhong closed this Jul 5, 2019

simonhong reopened this Jul 5, 2019

emerick reviewed Jul 5, 2019

View reviewed changes

simonhong force-pushed the add_always_show_bookmark_on_ntp_option branch from c430ec3 to 0225e94 Compare July 5, 2019 12:28

simonhong removed the CI/skip Do not run CI builds (except noplatform) label Jul 5, 2019

simonhong force-pushed the add_always_show_bookmark_on_ntp_option branch from 0225e94 to 3920262 Compare July 5, 2019 12:32

simonhong force-pushed the add_always_show_bookmark_on_ntp_option branch 2 times, most recently from 744dc12 to cfc0497 Compare July 5, 2019 12:35

emerick previously approved these changes Jul 5, 2019

View reviewed changes

simonhong dismissed emerick’s stale review via afea0c9 July 11, 2019 23:40

simonhong force-pushed the add_always_show_bookmark_on_ntp_option branch 2 times, most recently from afea0c9 to 644387d Compare July 12, 2019 00:59

simonhong commented Jul 12, 2019

View reviewed changes

simonhong force-pushed the add_always_show_bookmark_on_ntp_option branch 2 times, most recently from 5262b6a to afb373d Compare July 16, 2019 00:47

simonhong modified the milestones: 0.69.x - Dev, 0.70.x - Nightly Jul 23, 2019

simonhong force-pushed the add_always_show_bookmark_on_ntp_option branch 2 times, most recently from a1902f4 to 785392b Compare July 31, 2019 01:59

rebron mentioned this pull request Aug 6, 2019

Can't hide Bookmarks bar on New Tab page brave/brave-browser#1306

Closed

simonhong added the CI/skip Do not run CI builds (except noplatform) label Aug 19, 2019

simonhong force-pushed the add_always_show_bookmark_on_ntp_option branch 2 times, most recently from 05b6db4 to 35fe8ac Compare August 19, 2019 07:59

simonhong removed the CI/skip Do not run CI builds (except noplatform) label Aug 19, 2019

simonhong force-pushed the add_always_show_bookmark_on_ntp_option branch 2 times, most recently from fb50527 to 976078a Compare August 19, 2019 12:36

simonhong force-pushed the add_always_show_bookmark_on_ntp_option branch 2 times, most recently from 5ff0e95 to 9db7fd9 Compare August 20, 2019 00:43

simonhong requested a review from bridiver August 20, 2019 00:45

simonhong force-pushed the add_always_show_bookmark_on_ntp_option branch 3 times, most recently from 2370122 to 0b9171b Compare August 20, 2019 03:51

bridiver reviewed Aug 20, 2019

View reviewed changes

simonhong force-pushed the add_always_show_bookmark_on_ntp_option branch from 0b9171b to b1ec3f3 Compare August 21, 2019 00:20

simonhong requested a review from bridiver August 21, 2019 00:22

simonhong force-pushed the add_always_show_bookmark_on_ntp_option branch from b1ec3f3 to b351cab Compare August 21, 2019 00:42

Add always show bookmark bar on NTP option

1033a4b

Some user might want to show bookmark on NTP when they choose to hide bookmark bar.

simonhong force-pushed the add_always_show_bookmark_on_ntp_option branch from b351cab to 1033a4b Compare August 21, 2019 02:56

bridiver approved these changes Aug 23, 2019

View reviewed changes

simonhong merged commit f5e6114 into master Aug 23, 2019

simonhong deleted the add_always_show_bookmark_on_ntp_option branch August 23, 2019 01:31

simonhong added a commit that referenced this pull request Aug 23, 2019

Merge pull request #2869 from brave/add_always_show_bookmark_on_ntp_o…

d23d88b

…ption Add always show bookmark bar on NTP option

simonhong mentioned this pull request Aug 23, 2019

Add always show bookmark bar on NTP option (Uplift 2869 to dev) #3243

Merged

4 tasks

simonhong added a commit that referenced this pull request Aug 23, 2019

Merge pull request #2869 from brave/add_always_show_bookmark_on_ntp_o…

fc6e25b

…ption Add always show bookmark bar on NTP option

This was referenced Aug 23, 2019

Add always show bookmark bar on NTP option (Uplift 2869 to beta) #3244

Merged

Make bookmarks bar visible on New Tabs Page by default brave/brave-browser#5781

Closed

simonhong added a commit that referenced this pull request Aug 24, 2019

Merge pull request #2869 from brave/add_always_show_bookmark_on_ntp_o…

eb946db

…ption Add always show bookmark bar on NTP option

simonhong mentioned this pull request Aug 24, 2019

Add always show bookmark bar on NTP option (Uplift 2869 to release) #3250

Closed

4 tasks

rebron mentioned this pull request Aug 27, 2019

Add setting to 'Always show bookmarks bar on New Tab Page' brave/brave-browser#4782

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add always show bookmark bar on NTP option #2869

Add always show bookmark bar on NTP option #2869

simonhong commented Jul 5, 2019 •

edited

Loading

simonhong commented Jul 5, 2019 •

edited

Loading

emerick Jul 5, 2019

simonhong Jul 5, 2019

rebron Jul 5, 2019

simonhong commented Jul 5, 2019

emerick left a comment

simonhong left a comment

simonhong Jul 12, 2019

bridiver Aug 7, 2019

bridiver Aug 7, 2019

bridiver Aug 7, 2019 •

edited

Loading

simonhong Aug 19, 2019

simonhong Jul 12, 2019

bridiver Aug 7, 2019

simonhong Aug 19, 2019

bsclifton commented Jul 17, 2019

simonhong commented Jul 23, 2019

simonhong commented Aug 19, 2019

bridiver Aug 20, 2019

simonhong Aug 21, 2019

Add always show bookmark bar on NTP option #2869

Add always show bookmark bar on NTP option #2869

Conversation

simonhong commented Jul 5, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

simonhong commented Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Jul 5, 2019

emerick left a comment

Choose a reason for hiding this comment

simonhong 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

bridiver Aug 7, 2019 • edited Loading

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

bsclifton commented Jul 17, 2019

simonhong commented Jul 23, 2019

simonhong commented Aug 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Jul 5, 2019 •

edited

Loading

simonhong commented Jul 5, 2019 •

edited

Loading

bridiver Aug 7, 2019 •

edited

Loading