[NTP Next] Create WebUI scaffolding #27780

zenparsing · 2025-02-24T18:00:36Z

Resolves

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:

petemill · 2025-02-24T18:07:10Z

browser/ui/webui/brave_new_tab_page_refresh/brave_new_tab_page.mojom

+// 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/.
+
+module brave_new_tab_page_refresh.mojom;


you don't really need the namespace here if you don't want it since the types in the other files in this dir don't have one

I feel like it might be asking for trouble to have names defined here put into the global namespace?

There is precedent in chromium. It's the same problem if we name something too generically in a .h file, isn't it? So as long as we name everything NewTabPageXYZ it should be fine. You could leave the line as module mojom; if you want it in a mojom:: global namespace, or you could omit it entirely for global namespace. Just a suggestion as I know you had an issue with the frequent repetition of namespaces in the c++.

My would be for brave_new_tab_page_refresh.mojom but it probably is worth being consistent (and the other files aren't in a namespace)

It's absolutely ok to keep the namespace here. I was just giving the option because I remember it was a complaint of having to write the namespace everywhere if the C++ doesn't have the same namespace.

zenparsing · 2025-02-24T19:10:22Z

browser/ui/webui/brave_new_tab_page_refresh/new_tab_page_handler.h

+#include "mojo/public/cpp/bindings/receiver.h"
+#include "mojo/public/cpp/bindings/remote.h"
+
+namespace brave_new_tab_page_refresh {


I forgot that this namespace was in here. However, if I remove the namespace and rename this class BraveNewTabPageHandler, then I get a conflict with the existing BraveNewTabPageHandler.

Then perhaps we can temporarily add the Refresh word in to the name? It's probably only going to be referenced outside of this class once or twice.

I have a slight preference for namespacing everything but upstream definitely doesn't, so I'm fine with Pete's suggestion - I'd rather we be consistent

petemill

I can't speak to whether the brave_chrome_browser_allow_circular_includes_from part is ok, but the rest seems inline with what we discussed

browser/resources/brave_new_tab_page_refresh/BUILD.gn

browser/resources/brave_new_tab_page_refresh/stories/index.tsx

brave-builds · 2025-02-24T19:25:54Z

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

fallaciousreasoning · 2025-02-24T19:28:49Z

browser/ui/webui/brave_new_tab_page_refresh/brave_new_tab_page.mojom

+// 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/.
+
+module brave_new_tab_page_refresh.mojom;


My would be for brave_new_tab_page_refresh.mojom but it probably is worth being consistent (and the other files aren't in a namespace)

fallaciousreasoning · 2025-02-24T19:31:08Z

browser/ui/webui/brave_new_tab_page_refresh/new_tab_page_handler.h

+#include "mojo/public/cpp/bindings/receiver.h"
+#include "mojo/public/cpp/bindings/remote.h"
+
+namespace brave_new_tab_page_refresh {


I have a slight preference for namespacing everything but upstream definitely doesn't, so I'm fine with Pete's suggestion - I'd rather we be consistent

bridiver · 2025-02-27T20:14:17Z

browser/about_flags.cc

@@ -535,6 +535,13 @@ const flags_ui::FeatureEntry::FeatureVariation kZCashFeatureVariations[] = {
          kOsDesktop,                                                          \
          FEATURE_VALUE_TYPE(features::kBraveNtpSearchWidget),                 \
      },                                                                       \
+      {                                                                        \
+          "brave-use-updated-ntp",                                             \
+          "Use the updated New Tab Page",                                      \


please use consistent naming (refresh instead updated). Also kBraveNewTabPageRefreshEnabled

bridiver · 2025-02-27T20:19:53Z

browser/sources.gni

@@ -633,3 +635,11 @@ if (is_android) {
    "//brave/browser/android:tab_features",
  ]
 }
+
+# TODO(https://github.com/brave/brave-browser/issues/43310):
+# brave_new_tab_page_ui.cc includes some headers (e.g. from


Ugh, another variation (new_tab vs new_tab_page). In any case this seems trivial to move new_tab_shows_options.h/new_tab_shows_options.cc to a separate target inside //brave/browser/BUILD.gn? Or is there something else as well?

I've updated the linked issue to include the current list of circular includes for the NTP next prototype. They are:

brave/browser/new_tab/new_tab_shows_options.h

chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.h

brave/browser/brave_vpn/brave_vpn_service_factory.h

There may be more (some features aren't yet implemented in the prototype). Unfortunately, I don't think it will be pragmatic to chase these down in the context of this project.

zenparsing requested review from a team as code owners February 24, 2025 18:00

github-actions bot assigned zenparsing Feb 24, 2025

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 Feb 24, 2025

petemill reviewed Feb 24, 2025

View reviewed changes

zenparsing commented Feb 24, 2025

View reviewed changes

petemill approved these changes Feb 24, 2025

View reviewed changes

browser/resources/brave_new_tab_page_refresh/BUILD.gn Outdated Show resolved Hide resolved

browser/resources/brave_new_tab_page_refresh/stories/index.tsx Outdated Show resolved Hide resolved

fallaciousreasoning approved these changes Feb 24, 2025

View reviewed changes

zenparsing requested a review from bridiver February 24, 2025 20:15

zenparsing force-pushed the ksmith-ntp-next-scaffolding branch 3 times, most recently from 0a4f0f6 to 0bb44e6 Compare February 27, 2025 03:22

bridiver reviewed Feb 27, 2025

View reviewed changes

zenparsing force-pushed the ksmith-ntp-next-scaffolding branch 2 times, most recently from b393945 to 14efb1b Compare February 28, 2025 13:55

[NTP Next] Create WebUI scaffolding

1447814

zenparsing force-pushed the ksmith-ntp-next-scaffolding branch from 14efb1b to 1447814 Compare February 28, 2025 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NTP Next] Create WebUI scaffolding #27780

[NTP Next] Create WebUI scaffolding #27780

zenparsing commented Feb 24, 2025

petemill Feb 24, 2025

zenparsing Feb 24, 2025

petemill Feb 24, 2025

fallaciousreasoning Feb 24, 2025

petemill Feb 24, 2025

zenparsing Feb 24, 2025

petemill Feb 24, 2025

fallaciousreasoning Feb 24, 2025

petemill left a comment

brave-builds commented Feb 24, 2025

fallaciousreasoning Feb 24, 2025

fallaciousreasoning Feb 24, 2025

bridiver Feb 27, 2025 •

edited

Loading

zenparsing Feb 28, 2025

bridiver Feb 27, 2025

zenparsing Feb 28, 2025

[NTP Next] Create WebUI scaffolding #27780

Are you sure you want to change the base?

[NTP Next] Create WebUI scaffolding #27780

Conversation

zenparsing commented Feb 24, 2025

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

petemill left a comment

Choose a reason for hiding this comment

brave-builds commented Feb 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Feb 27, 2025 • 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

bridiver Feb 27, 2025 •

edited

Loading