Conversation
EliHini
reviewed
Sep 15, 2026
EliHini
left a comment
Contributor
There was a problem hiding this comment.
Looking good. Just one question. With Top site enabled, I don't see any tiles or top sites. Where should be looking to verify this?
kylehickinson
requested changes
Sep 15, 2026
kylehickinson
left a comment
Collaborator
There was a problem hiding this comment.
As discussed in DM:
- Reuse/refactor favorites section provider, alter what it shows inside that provider rather than mess with the section providers. Just return 0 items when no top sites/favs are visible similar to stats/news. This should significantly simplify changes and avoid weird collection view section updates.
- Settings UI: No need to have both a switch and picker, just have "none"/"hidden"/some other language be an option
- All of these changes should be behind a feature flag since you're introducing user facing changes for a feature that isn't implemented yet. If a user picks "top sites" like Eli saw, nothing shows up at all. We shouldn't let users see this option yet
netzenbot
reviewed
Sep 16, 2026
Contributor
|
Chromium major version is behind target branch (153.0.8010.37 vs 154.0.8037.41). Please rebase. |
nuo-xu
force-pushed
the
topsites/new-tab-page-settings
branch
from
September 16, 2026 21:05
fde379a to
b56e48d
Compare
1. Gate NTP Top Sites settings behind a feature flag. 2. Move Favorites/Most Visited/None gating into FavoritesSectionProvider and FavoritesOverflowSectionProvider (return 0 items when hidden) instead of rebuilding NewTabPageViewController's section list on every pref change, matching how Stats/News hide themselves. 3. Collapse the settings UI to one control instead of a switch plus a picker: a plain Favorites toggle when feature flag is off, or a single Top Sites picker (Favorites/Most Visited/None) when kTopsitesEnabled is on. The flag defaults to disabled since Most Visited tiles aren't implemented yet — picking it currently falls back to showing Favorites, called out in the PR description.
nuo-xu
force-pushed
the
topsites/new-tab-page-settings
branch
from
September 17, 2026 02:56
b56e48d to
070c3fb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
isTopsitesHiddenflag and return 0 items when hidden, the same pattern Stats/News already use.kTopsitesEnabledfeature flag (disabled by default, exposed at brave://flags as "Enable Top Sites"), since this introduces user-facing options for a mode that isn't implemented yet.Note: Most Visited is not implemented yet: while the flag is on, picking "Frequently visited" currently behaves identically to "Favorites" (shows the existing Favorites tiles). Only "None" actually hides the section. The real Most Visited tile source lands in a follow-up PR.
Test:
(upgrade from a build older than v1.97.25 to a build include this PR's changes)
kTopsitesEnabledat its default (disabled):unaffected by these changes.