Add New tab page options to settings #6879

simonhong · 2020-10-16T13:01:18Z

With this, user can select which page should be opened in NTP.

dark mode blank page:

Resolves brave/brave-browser#2999

Submitter Checklist:

Test Plan:

npm run test brave_unit_tests -- --filter=*BraveNewTabTest*

Choose Dashboard...
Create NTP by new tab button, shortcut(ex, CMD+T or new tab from app menu)
Check brave NTP is opened
Choose HomePage
If newtab is homepage is on, create NTP is brave NTP
If newtab is homepage is off, configured page is opened
Choose blankpage
Check about:blank is loaded for NTP

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.

mkarolin · 2020-10-16T16:37:11Z

This setting seems to also apply to Private and Tor windows. Is this expected?

simonhong · 2020-10-19T00:01:32Z

@mkarolin It's not intentional. @rebron should we only apply this behavior to normal window?

bsclifton · 2020-10-19T05:46:42Z

@simonhong I think we only need to apply this to regular window - Private and Private w/ Tor windows both have the informational page that we would like to show

simonhong · 2020-10-19T07:31:18Z

This option is only effective for normal window.

simonhong · 2020-10-19T09:22:23Z

@rebron @bsclifton , one more question - Should New Window also follow this option?
With current PR, only new tab created by new tab command(cmd + T or click new tab button) uses this option.
and new window is launched with one NTP.

bsclifton · 2020-10-21T20:39:06Z

Curious what you think @karenkliu @rebron

To me, it makes more sense to put New Tab shows at the top (above the Customize your Brave Dashboard)

Unrelated to this PR, I think that Brave Rewards, Binance, Gemini, Crypto.com, etc should all be under a Widgets parent item, similar to how Background images has sub-items
cc: @ryanml

bsclifton · 2020-10-21T20:42:51Z

@rebron @bsclifton , one more question - Should New Window also follow this option?
With current PR, only new tab created by new tab command(cmd + T or click new tab button) uses this option.
and new window is launched with one NTP.

Yes - this new setting should be checked/respected when opening a new window too 😄

Should we hide the Homepage option if the Show home button preference is disabled? I think we should consider hiding it. When it's disabled, it defaults to brave://newtab - so user might think preference is not being respected. User needs to enable home button in order to actually set the home page

rebron · 2020-10-21T20:48:48Z

@karenkliu and I chatted about this yesterday. Placement looks good.

We should hide the homepage option until a user enables the homepage button. Otherwise, it won't really make sense.

bsclifton · 2020-10-21T21:35:36Z

chromium_src/chrome/browser/ui/browser_commands.cc


 namespace chrome {

+// Whole logic is copied from upstream chrome::NewTab().


Maybe also note as part of this comment:

// the only change is `new_tab_url` instead of `Gurl()`

bsclifton

Code changes look really good! Notes left about how this should work on new window also

@karenkliu may have some comments too 😄

karenkliu · 2020-10-21T21:56:59Z

@simonhong Hi Simon, after going through the details again, these are some updates to the design:

Remove "Customize Brave Dashboard" label from the New Tab Page section because with the addition of this dropdown the controls no longer apply to just the dashboard
The New tab page shows... dropdown should go at the top of the New Tab Page section

The widgets go at the bottom of the New Tab Page section, after Top Sites
The options are Dashboard, Homepage, and Blank page
If the user selects Blank page, then the user will get a blank white page in light theme and #17171F in dark theme
If user selects Homepage or Blank page, then the Dashboard options will not be shown:

simonhong · 2020-10-22T07:41:54Z

@bsclifton @rebron What should we do in this scenario?
At first, user turned homepage button on and New Tab Page is set as Homepage.
Then, user turned off homepage button. How should we change New Tab page shows option?

IMO, making Homepage available always seems fine because Homepage url prefs are persisted regardless of homepage button show option. WDYT?

mkarolin · 2020-10-22T13:48:08Z

Agree with @simonhong that it would be more beneficial to have Homepage drop-down option visible not based on the Home Button setting, but rather on whether the homepage URL has been set. However, setting the Home URL is not well-discoverable in the settings right now.

Why not always have the Homepage drop-down visible and, if selected, show to the user the entry field to set the URL (which would be tied to the same pref as the one under Home Button)?

bsclifton · 2020-10-22T18:31:13Z

I'm OK with always showing Homepage given above argument. WDYT @rebron? I could go either way 😛 Would defer to you

simonhong · 2020-10-23T04:50:46Z

@karenkliu When user select Homepage, user can choose New Tab page as a homepage button option.
Or user can set empty string as a homepage url. In these cases, dashboard options should be visible.

petemill

Can remove the different resources for light and dark. I suggest we just use chrome://newtab and serve blank.html from there, but maybe there's a reason you created chrome://blank

petemill · 2020-10-27T18:30:26Z

components/brave_blank_page/resources/brave_blank_page.html

+  <title>Blank page</title>
+</head>
+
+<body style="background-color:#FFFFFF;">


We can put a <style> element here which varies the background-color based on dark / light theme. Similar to on the dashboard NTP: https://github.com/brave/brave-core/blob/master/components/brave_new_tab_ui/brave_new_tab.html

@media (prefers-color-scheme: dark) { body { background-color: #333639; } }

Then we don't need both blank_page.html and dark_blank_page.html.

petemill · 2020-10-27T18:35:03Z

browser/ui/webui/brave_blank_page_ui.h

+class BraveBlankPageUI : public content::WebUIController {
+ public:
+  BraveBlankPageUI(content::WebUI* web_ui,
+                   const std::string& host);
+  ~BraveBlankPageUI() override;
+
+  BraveBlankPageUI(const BraveBlankPageUI&) = delete;
+  BraveBlankPageUI& operator=(BraveBlankPageUI&) = delete;
+};


Instead of creating a whole new WebUI route and class, I was thinking we could just use the regular chrome://newtab route and class, and simply change the .html resource that is rendered, depending on the value of kNewTabPageShowsOptions. I can't think of a reason that we would need to have separate routes or both the dashboard route and the blank route available at the same time, regardless of the preference value. Do you have a different opinion?

I thought new tab and blank are different webui. So, I created new route.
But, I don't have strong opinon about this. I'll follow your suggestion.

@petemill Fixed. PTAL again!

I realized that your approach is better because blank page can also act like NTP. - search suggestion in location bar.

petemill · 2020-10-27T18:35:33Z

browser/ui/webui/brave_blank_page_ui.cc

+  const bool is_dark_mode =
+      dark_mode::GetActiveBraveDarkModeType() ==
+      dark_mode::BraveDarkModeType::BRAVE_DARK_MODE_TYPE_DARK;


See comment about no need to vary on dark mode.

petemill · 2020-10-27T18:35:56Z

browser/new_tab/new_tab_shows_options.cc

  } else if (option == brave::NewTabPageShowsOptions::BLANKPAGE) {
-    return GURL(url::kAboutBlankURL);
+    return GURL(kBraveBlankPageURL);


see comment about not needing a separate route as far as I can tell

mkarolin

chromium_src overrides LGTM

petemill

Looks great to me apart from 1 html syntax error to fix

components/brave_blank_page/resources/brave_blank_new_tab.html

With this, user can select which page should be opened in NTP.

Also renamed dashboard with images to dashboard.

Instead of url rewriting, navigation throttle is used. When new tab url is requested, throttle cancels it if new tab options url should be used. With url rewriting, new tab options url loading is fine. However, tab title is not updated well if blank url is loaded.

This blank page will have different bg color for theme type. And new tab shows blank page options will use this page.

Blank page will be configured by BraveNewTabUI.

also fix some var name after c87 rebasing.

Fixed requested changes.

simonhong · 2020-10-30T03:56:17Z

Merged because only unrelated rewards browser test are failed.

simonhong self-assigned this Oct 16, 2020

simonhong force-pushed the new_tab_shows_options branch from 9efd0e4 to dd2f61e Compare October 16, 2020 13:03

simonhong marked this pull request as ready for review October 16, 2020 15:46

simonhong requested a review from a team as a code owner October 16, 2020 15:46

simonhong force-pushed the new_tab_shows_options branch from dd2f61e to c809b7b Compare October 16, 2020 15:47

simonhong requested review from bsclifton, petemill and rebron October 16, 2020 15:48

simonhong force-pushed the new_tab_shows_options branch from c809b7b to 6ecc6fe Compare October 19, 2020 03:18

simonhong force-pushed the new_tab_shows_options branch from 6ecc6fe to 391c5de Compare October 19, 2020 07:29

simonhong force-pushed the new_tab_shows_options branch from 391c5de to 83f36e6 Compare October 19, 2020 08:57

simonhong added this to the 1.18.x - Nightly milestone Oct 21, 2020

simonhong force-pushed the new_tab_shows_options branch from 83f36e6 to 58e0092 Compare October 21, 2020 02:08

bsclifton force-pushed the new_tab_shows_options branch from 58e0092 to 1755ed9 Compare October 21, 2020 19:14

bsclifton reviewed Oct 21, 2020

View reviewed changes

simonhong added the CI/skip Do not run CI builds (except noplatform) label Oct 23, 2020

petemill requested changes Oct 27, 2020

View reviewed changes

simonhong force-pushed the new_tab_shows_options branch from 343a4c2 to da0dfe9 Compare October 28, 2020 01:34

simonhong requested a review from petemill October 28, 2020 01:35

simonhong force-pushed the new_tab_shows_options branch 2 times, most recently from 81c3903 to 2a5be52 Compare October 28, 2020 01:51

mkarolin approved these changes Oct 28, 2020

View reviewed changes

petemill previously requested changes Oct 28, 2020

View reviewed changes

components/brave_blank_page/resources/brave_blank_new_tab.html Show resolved Hide resolved

simonhong force-pushed the new_tab_shows_options branch from 2a5be52 to dcb29c8 Compare October 28, 2020 23:27

simonhong requested a review from petemill October 28, 2020 23:28

simonhong force-pushed the new_tab_shows_options branch from dcb29c8 to 903c4c0 Compare October 28, 2020 23:51

simonhong added 13 commits October 30, 2020 08:24

Add New tab page options to settings

0bff4d2

With this, user can select which page should be opened in NTP.

Fetch NewTabShows options list dynamically

8acf9d1

Also renamed dashboard with images to dashboard.

Re-order items in New Tab Page settings

e70fcb1

Hide dashboard customization option when new tab doesn't show dashboard

4d80de1

Make new empty window follow our ntp page option

e893581

Use URL Rewriting instead of patching/overriding

07a3e7e

Introduce brave blank page (brave://blank/)

5410d9a

This blank page will have different bg color for theme type. And new tab shows blank page options will use this page.

Add new tab options throttle only for non-android platform

fb01ee4

Fix unit test failure

22ff246

Use one blank tab html file by using media query

acd70ba

Makes blank page uses NTP WebUI route

1dd9e8e

Blank page will be configured by BraveNewTabUI.

Fix gn_check error and html tag error

0713614

also fix some var name after c87 rebasing.

simonhong force-pushed the new_tab_shows_options branch from 903c4c0 to 0713614 Compare October 29, 2020 23:51

Fix build failure after rebasing C87

727f1e6

simonhong merged commit 93bda64 into master Oct 30, 2020

simonhong deleted the new_tab_shows_options branch October 30, 2020 03:56

simonhong mentioned this pull request Oct 30, 2020

Hide NTP dashboard settings when blankpage option is used #6995

Merged

33 tasks


		namespace chrome {

		// Whole logic is copied from upstream chrome::NewTab().

Add New tab page options to settings #6879

Add New tab page options to settings #6879

Uh oh!

Conversation

simonhong commented Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

Uh oh!

mkarolin commented Oct 16, 2020

Uh oh!

simonhong commented Oct 19, 2020

Uh oh!

bsclifton commented Oct 19, 2020

Uh oh!

simonhong commented Oct 19, 2020

Uh oh!

simonhong commented Oct 19, 2020

Uh oh!

bsclifton commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsclifton commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rebron commented Oct 21, 2020

Uh oh!

bsclifton Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsclifton left a comment

Choose a reason for hiding this comment

Uh oh!

karenkliu commented Oct 21, 2020

Uh oh!

simonhong commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkarolin commented Oct 22, 2020

Uh oh!

bsclifton commented Oct 22, 2020

Uh oh!

simonhong commented Oct 23, 2020

Uh oh!

petemill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkarolin left a comment

Choose a reason for hiding this comment

Uh oh!

petemill left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simonhong commented Oct 30, 2020

Uh oh!

Uh oh!

simonhong commented Oct 16, 2020 •

edited

Loading

bsclifton commented Oct 21, 2020 •

edited

Loading

bsclifton commented Oct 21, 2020 •

edited

Loading

bsclifton Oct 21, 2020 •

edited

Loading

simonhong commented Oct 22, 2020 •

edited

Loading