-
Notifications
You must be signed in to change notification settings - Fork 891
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NTP Next] Add updated NTP with background support #26408
base: master
Are you sure you want to change the base?
Conversation
A Storybook has been deployed to preview UI for the latest push |
a4056cc
to
9a7f7aa
Compare
03bf3b2
to
e9bef8c
Compare
# 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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved all of the code out of components
as suggested - thanks! Naming is TBD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the folder names, file names, and namespace name. I think naming is in a good place now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
15d255c
to
1a2f01c
Compare
070de76
to
c71be60
Compare
|
||
} // namespace | ||
|
||
NewTabPageUI::NewTabPageUI(content::WebUI* web_ui) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow up issue for folders/namespacing: brave/brave-browser#42563
Chromium major version is behind target branch (131.0.6778.85 vs 132.0.6834.15). Please rebase. |
c71be60
to
fc65e01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great @zenparsing - awesome work 😄
Sorry its taken so long to review!
interface NewTabPage { | ||
|
||
// Called when a background-related profile preference has been updated. | ||
OnBackgroundPrefsUpdated(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/resources/components/settings/background_panel.tsx
Outdated
Show resolved
Hide resolved
components/brave_new_tab/resources/components/settings/background_panel.tsx
Outdated
Show resolved
Hide resolved
components/brave_new_tab/resources/components/settings/background_panel.tsx
Show resolved
Hide resolved
364c32b
to
e3f3e20
Compare
53a5151
to
4af2b2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @zenparsing, the PR is looking great - I'm approving
Do you mind updating & resolving (with whatever you decide) these comment threads:
- [NTP Next] Add updated NTP with background support #26408 (comment)
- [NTP Next] Add updated NTP with background support #26408 (comment)
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 😄
62b662e
to
ba30e79
Compare
ba30e79
to
a5f16d0
Compare
[puLL-Merge] - brave/brave-core@26408 DescriptionThis PR introduces a new version of the New Tab Page (NTP) for Brave Browser. It includes a redesigned UI with customizable background options, including Brave backgrounds, custom user backgrounds, solid colors, and gradient backgrounds. The PR also adds support for sponsored images and implements a settings panel for managing these background options. Possible Issues
Security Hotspots
ChangesChanges
sequenceDiagram
participant User
participant NTP as New Tab Page
participant Handler as NewTabPageHandler
participant Model as NewTabModel
participant BG as BackgroundAdapter
User->>NTP: Open New Tab
NTP->>Handler: Initialize
Handler->>Model: Create NewTabModel
Model->>BG: Get background info
BG-->>Model: Return background data
Model-->>NTP: Update UI with background
NTP->>User: Display New Tab Page
User->>NTP: Interact with settings
NTP->>Handler: Update preferences
Handler->>Model: Update state
Model->>BG: Apply changes
BG-->>Model: Confirm changes
Model-->>NTP: Reflect updated state
NTP->>User: Show updated New Tab Page
|
Resolves brave/brave-browser#42298
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: