[NTP Next] Add updated NTP with background support #26408

zenparsing · 2024-11-06T19:57:29Z

Resolves brave/brave-browser#42298

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

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, for instance:

Test Plan:

brave-builds · 2024-11-06T21:08:48Z

A Storybook has been deployed to preview UI for the latest push

components/brave_new_tab/resources/lib/scoped_css.ts

components/brave_new_tab/resources/lib/app_model.ts

components/resources/brave_components_strings.grd

bridiver · 2024-11-22T23:08:43Z

components/brave_new_tab/common/BUILD.gn

+# License, v. 2.0. If a copy of the MPL was not distributed with this file,
+# You can obtain one at https://mozilla.org/MPL/2.0/.
+
+import("//mojo/public/tools/bindings/mojom.gni")


you don't need a common directory here since this is browser process only, just put everything in the root. Also there is inconsistent naming here, sometimes brave_new_tab and sometimes new_tab_page in other places. brave_new_tab_page is probably best I think

also not really sure that it makes sense to put any of this code in components unless. I would just put this code in brave/browser/ui/webui/brave_new_tab_page and put the resources in brave/browser/resources/brave_new_tab_page to match upstream

I've moved all of the code out of components as suggested - thanks! Naming is TBD.

I've updated the folder names, file names, and namespace name. I think naming is in a good place now.

I've moved the resource code back into components/brave_new_tab, as the suggested change resulted in naming conflicts with the existing NTP. Follow-up issue created here: brave/brave-browser#42563

browser/ui/webui/brave_new_tab/new_tab_page_handler.h

ui/webui/resources/BUILD.gn

browser/resources/brave_new_tab_page/lib/store.ts

browser/resources/brave_new_tab_page/webui/debouncer.ts

bridiver · 2024-12-01T20:45:52Z

browser/ui/webui/brave_new_tab/new_tab_page_ui.cc

+
+}  // namespace
+
+NewTabPageUI::NewTabPageUI(content::WebUI* web_ui)


Maybe you haven't got back to this yet, but as discussed this should be new_tab_page/brave_new_tab_* with no namespace. The existing file/directory is already correct (for some reason I thought it was different) so the flag should be internal to it as is normally the case for this kind of thing.

Follow up issue for folders/namespacing: brave/brave-browser#42563

github-actions · 2024-12-02T12:30:55Z

Chromium major version is behind target branch (131.0.6778.85 vs 132.0.6834.15). Please rebase.

fallaciousreasoning

This is looking great @zenparsing - awesome work 😄

Sorry its taken so long to review!

fallaciousreasoning · 2024-12-08T21:36:58Z

components/brave_new_tab/new_tab_page.mojom

+interface NewTabPage {
+
+  // Called when a background-related profile preference has been updated.
+  OnBackgroundPrefsUpdated();


Hey do you think it would be useful to pass the change in here? At the moment, when you change something there are quite a few mojom round trips:

Send a message to update the pref

Receive a notification it changed from the pref handler

Send a new message to the tab handler asking about the new values

Receive the result

You're right - this can be optimized. The easiest next step would be to provide the pref name (as an enum value perhaps?) and let the front end decide what to update. I think we should leave this optimization for later, since it will take some thinking through (and it's hard to tell whether it's worth it).

Okay, happy to leave this for now. One thing to keep in mind is that it does somewhat simplify the mojom interface, as you no longer need to provide getters for any of the values (you just fire the change listener when its added). I've found the pattern makes it a bit easier to reason about where stuff comes from.

If you didn't want to add a separate listener for each different channel, you could just add all the fields into the callback

OnBackgroundPrefsUpdates(Thing1 thing1, Thing2 thing2, ..., ThingN thingN)

and you can simplify some of your initialization logic (instead, we just add ourselves as a listener and are notified of the current state + any changes that happen later).

I see that we've been moving some mojo interfaces toward a push-only API. I think that the most flexible setup is a hybrid where data is pulled and update notifications are pushed. It does require more round-trips and possibly more interface methods, however. I'll keep this in mind as we move through the project - the hybrid approach is not yet set-in-stone.

components/brave_new_tab/new_tab_page.mojom

components/brave_new_tab/resources/new_tab_page.tsx

components/brave_new_tab/resources/brave_new_tab_strings.grdp

components/brave_new_tab/resources/components/settings/background_panel.tsx

browser/ui/webui/brave_new_tab/update_observer.cc

browser/ui/webui/brave_new_tab/custom_image_chooser.cc

browser/ui/webui/brave_new_tab/new_tab_page_ui.cc

fallaciousreasoning

Hey @zenparsing, the PR is looking great - I'm approving

Do you mind updating & resolving (with whatever you decide) these comment threads:

Thanks for all the work you've put into this! I'm going to be on PTO for a few weeks so probably best to reach out to @petemill if anything comes up 😄

github-actions · 2025-01-02T17:16:13Z

[puLL-Merge] - brave/brave-core@26408

Description

This PR implements a new version of the New Tab Page (NTP) for the Brave browser. It introduces a more modular and React-based approach to the NTP UI, with improved background image handling, settings management, and overall user experience. The changes include new components for background selection, settings panels, and a redesigned layout.

Changes

Changes

browser/about_flags.cc:
- Added a new feature flag for the updated NTP.
browser/brave_content_browser_client.cc:
- Registered the new NTP UI interface binder.
browser/brave_profile_prefs.cc:
- Updated preference registration for the new NTP.
browser/extensions/api/settings_private/brave_prefs_util.cc:
- Updated preference handling for the new NTP.
browser/new_tab/:
- Modified existing NTP-related files to work with the new implementation.
browser/ui/webui/brave_new_tab/:
- Added new files for the updated NTP UI implementation, including handlers, adapters, and UI components.
browser/ui/webui/brave_web_ui_controller_factory.cc:
- Updated to use the new NTP UI when the feature flag is enabled.
components/brave_new_tab/:
- Added new files for the NTP model, features, and preferences.
components/brave_new_tab/resources/:
- Added new React components, styles, and utilities for the updated NTP UI.
components/brave_new_tab_ui/:
- Modified existing files to accommodate the new NTP implementation.
Various other files:
- Updated build configurations, resource packs, and string definitions to support the new NTP implementation.

sequenceDiagram
    participant User
    participant BraveNewTabUI
    participant NewTabPageHandler
    participant BackgroundAdapter
    participant NewTabModel
    participant BackgroundService

    User->>BraveNewTabUI: Open New Tab
    BraveNewTabUI->>NewTabPageHandler: Initialize
    NewTabPageHandler->>BackgroundAdapter: Get background data
    BackgroundAdapter->>BackgroundService: Fetch backgrounds
    BackgroundService-->>BackgroundAdapter: Return backgrounds
    BackgroundAdapter-->>NewTabPageHandler: Background data
    NewTabPageHandler->>NewTabModel: Update state
    NewTabModel-->>BraveNewTabUI: Render updated UI
    BraveNewTabUI-->>User: Display New Tab Page

    User->>BraveNewTabUI: Change background
    BraveNewTabUI->>NewTabPageHandler: Select background
    NewTabPageHandler->>BackgroundAdapter: Update background
    BackgroundAdapter->>BackgroundService: Save preference
    BackgroundService-->>BackgroundAdapter: Confirm update
    BackgroundAdapter-->>NewTabPageHandler: Update complete
    NewTabPageHandler->>NewTabModel: Update state
    NewTabModel-->>BraveNewTabUI: Render updated UI
    BraveNewTabUI-->>User: Display updated background

Possible Issues

The new implementation relies heavily on React and modern web technologies, which may impact performance on older devices.
The feature flag approach may lead to maintenance of two separate NTP implementations for a period of time.

Security Hotspots

browser/ui/webui/brave_new_tab/background_adapter.cc:
- Ensure proper sanitization of user-supplied background image URLs to prevent potential XSS attacks.
components/brave_new_tab/resources/lib/url_sanitizer.ts:
- Verify that the URL sanitization logic is robust and covers all potential edge cases.

github-actions bot assigned zenparsing Nov 6, 2024

github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) labels Nov 6, 2024

fallaciousreasoning reviewed Nov 6, 2024

View reviewed changes

components/brave_new_tab/resources/lib/scoped_css.ts Show resolved Hide resolved

components/brave_new_tab/resources/lib/app_model.ts Outdated Show resolved Hide resolved

zenparsing force-pushed the ksmith-ntp-update branch 9 times, most recently from a4056cc to 9a7f7aa Compare November 13, 2024 16:23

zenparsing force-pushed the ksmith-ntp-update branch 7 times, most recently from 03bf3b2 to e9bef8c Compare November 21, 2024 17:42

zenparsing marked this pull request as ready for review November 21, 2024 20:05

zenparsing requested review from a team and bridiver as code owners November 21, 2024 20:05

mkarolin reviewed Nov 21, 2024

View reviewed changes

components/resources/brave_components_strings.grd Outdated Show resolved Hide resolved

zenparsing requested a review from petemill November 22, 2024 01:45

bridiver reviewed Nov 22, 2024

View reviewed changes

browser/ui/webui/brave_new_tab/new_tab_page_handler.h Outdated Show resolved Hide resolved

bridiver reviewed Nov 22, 2024

View reviewed changes

ui/webui/resources/BUILD.gn Show resolved Hide resolved

kdenhartog unassigned goodov Nov 25, 2024

zenparsing force-pushed the ksmith-ntp-update branch from 15d255c to 1a2f01c Compare November 25, 2024 22:31

petemill reviewed Nov 26, 2024

View reviewed changes

browser/resources/brave_new_tab_page/lib/store.ts Outdated Show resolved Hide resolved

petemill reviewed Nov 26, 2024

View reviewed changes

browser/resources/brave_new_tab_page/webui/debouncer.ts Outdated Show resolved Hide resolved

zenparsing force-pushed the ksmith-ntp-update branch 7 times, most recently from 070de76 to c71be60 Compare November 30, 2024 17:37

bridiver reviewed Dec 1, 2024

View reviewed changes

github-actions bot added the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Dec 2, 2024

zenparsing force-pushed the ksmith-ntp-update branch from c71be60 to fc65e01 Compare December 2, 2024 12:42

github-actions bot removed the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Dec 2, 2024

fallaciousreasoning reviewed Dec 8, 2024

View reviewed changes

zenparsing force-pushed the ksmith-ntp-update branch 2 times, most recently from 364c32b to e3f3e20 Compare December 10, 2024 23:10

fallaciousreasoning reviewed Dec 11, 2024

View reviewed changes

browser/ui/webui/brave_new_tab/new_tab_page_ui.cc Show resolved Hide resolved

zenparsing force-pushed the ksmith-ntp-update branch 3 times, most recently from 53a5151 to 4af2b2b Compare December 12, 2024 14:00

fallaciousreasoning approved these changes Dec 12, 2024

View reviewed changes

zenparsing force-pushed the ksmith-ntp-update branch 3 times, most recently from ba30e79 to a5f16d0 Compare December 20, 2024 17:42

[NTP Next] Add updated NTP with background support

ba6a952

zenparsing force-pushed the ksmith-ntp-update branch from a5f16d0 to ba6a952 Compare January 2, 2025 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NTP Next] Add updated NTP with background support #26408

[NTP Next] Add updated NTP with background support #26408

Changes


		} // namespace

		NewTabPageUI::NewTabPageUI(content::WebUI* web_ui)

[NTP Next] Add updated NTP with background support #26408

Are you sure you want to change the base?

[NTP Next] Add updated NTP with background support #26408

Conversation

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

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

Description

Changes

Possible Issues

Security Hotspots