Make bookmark toggle on NTP whether bookmark is empty or not

[simonhong]

Fix brave/brave-browser#1306

Submitter Checklist:

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Verified that these changes build without errors on
  • Windows
  • macOS
  • Linux
• Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security) on
  • Windows
  • macOS
  • Linux
• Verified that all lint errors/warnings are resolved (npm run lint)
• Ran git rebase master (if needed).
• Ran git rebase -i to squash commits (if needed).
• Tagged reviewers and labelled the pull request as needed.
• Request a security/privacy review as needed.
• Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone
• Public documentation has been updated as necessary. For instance:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protection-Mode
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Custom-Headers
  • https://github.com/brave/brave-browser/wiki/Web-compatibility-issues-with-tracking-protection
  • https://github.com/brave/brave-browser/wiki/QA-Guide

Test Plan:

yarn test brave_browser_tests --filter=BookmarkTabHelper*

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.

[emerick]

Was expecting to see something like return !IsNTP(web_contents()) here. Is it safe to just return false here without further checks?

[simonhong]

@emerick I think this method is used for preventing bookmark bar visible or hidden opposite to the kShowBookmarkBar prefs value.

When kShowBookmarkBar is set to hidden, this method only returns true when active tab is NTP and bookmark bar isn't empty if all above conditions aren't met. So, we can hide bookmark bar on NTP by returning false here in this case.

When kShowBookmarkBar is set to visible, this method isn't called except fullscreen condition.
According to the comment of Browser::ShouldHideUIForFullscreen(), browser controls UI can be visible on MacOS fullscreen mode by sliding down. When Browser::ShouldHideUIForFullscreen() returns true, this method will be called and we should also return false to hide bookmark bar.

So, I think returning false in here is safe.

[emerick]

OK, got it.

[simonhong]

@emerick Thanks for quick review!

[emerick]

LGTM

[bsclifton]

Nice work! 😄👍