Add best practice: Use *.mojom-forward.h in headers
CS-070: prefer the auto-generated *.mojom-forward.h over the full
*.mojom.h bindings when a header only references mojom types as
pointers, references, or function parameters. Reduces compile times
and transitive dependencies.
Source: PR #35622 review comment by netzenbot.
Adds ARCH-072 to docs/best-practices/architecture.md. A KeyedServiceFactory
may only return nullptr based on session-stable attributes (profile type,
buildflags, base::Feature). Gating creation on user-toggleable prefs forces
restarts and was repeatedly flagged in PR reviews.
* Update best practices from upstream Chromium docs
Add three new rules sourced from upstream Chromium documentation:
- CS-070: Pointer/reference symbol positioning (T* not T *)
- CS-071: No Yoda conditions (foo == 0, not 0 == foo)
- TI-041: Feature flag combination testing with bitmask parameterization
Source URLs checked: Chromium C++ style guide, Chromium C++ testing
best practices, smart pointer guidelines, container guidelines,
componentization cookbook.
* Remove best practices covered by linting and formatting tools
CS-070 (pointer/reference positioning) is enforced by clang-format.
CS-071 (no Yoda conditions) is caught by clang-tidy.
Neither belongs in a human-facing best practices doc.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
* Docs updates
- New test suites doc (how to run tests)
- New WebUI Frontend doc (basic introduction to Brave's different stack)
- Patches points to best practices
- bug fix in claude code skills doc
- Adjust docs/README.md to prioritize the index
- Preflight skill addition of missing test suites
* [Rewards] Make gate3 URL environment-aware via CLI switches
* Address review: network audit list, public_deps, CHECK
- Add gate3 prod and dev endpoints to network audit allowed list
- Move //base to public_deps in :constants (BS-024)
- Replace DCHECK with CHECK in GetGate3URL (CS-025)
* Fix broken tests
* Remove gate3 endpoints from network audit list
* Add gate3 endpoints to TLS pin list
* Replace auto with explicit type
* Refactor GetGate3URL tests into table-driven loop
* Move kEnvGate3Switch to header and use named constants in tests
* Add explicit default for gate3_url in RewardsEngineOptions
* Fix MaybeWarnSwitchValue comment to describe both log paths
* Remove gate3 entries from brave_network_audit_allowed_lists.h
* Simplify GetGate3URL to delegate to GetServicesDomain
* Use std::optional<std::string> for gate3_url instead of empty sentinel
* Rename brave_domains/constants.h/cc to urls.h/cc
* Switch to GURL in GetGate3URL
* Clean up review nits from gate3 env switch
* Add comment in brave_domains/urls.h
ARCH-062 combined two distinct concerns under one heading, causing
the bot to demand CUJ InteractiveBrowserTest for non-UI code like
tab helpers and services. Split into:
- ARCH-062: CUJ tests required only for features with UI changes
- ARCH-062b: Avoid change detector tests (general testing guidance)
Clarify that non-UI components should use unit tests or browser tests
appropriate to the code rather than CUJ tests.
* Add --update-crate flag to update a single vendored tool crate
Adds a --update-crate=<name>@<version> option to tools/crates/update.py
that updates one crate in-place without running cargo vendor. It downloads
the tarball directly from crates.io, replaces the vendor directory, patches
Cargo.lock, and regenerates .cargo-checksum.json. This avoids collateral
version bumps from a full re-vendor and is useful for targeted security
advisory fixes.
Exposes the flag via `npm run update_brave_tools_crates -- --update-crate=`.
* Clarify best practice applicability and reduce upstream hallucinations
- ARCH-020: Clarify rule applies to bypassing access control, not
normal inheritance patterns (calling protected methods from subclasses)
- ARCH-062: Clarify CUJ test requirement applies to UI features, not
backend services or infrastructure
- Review rules: Add explicit instruction to verify upstream code claims
by reading actual files rather than relying on training data
* Update ARCH-020 friend class example to modern #define pattern
Replace outdated BRAVE_CLASS_NAME_H guidance with the current
chromium_src #define approach for adding friend declarations.
Self-contained tab features that don't need external retrieval can use
simple unique_ptr ownership in BraveTabFeatures. The Unowned User Data
pattern should only be used when external code needs to look up the
feature via a static From() method.
Clarify that CSA-053 only applies to temporary workarounds, not
permanent design decisions. Comments explaining why an alternative
API was not used due to a known limitation do not need TODO tracking
issues when the current code is the intended long-term solution.
The wiki content was moved into the docs folder, so the external
link to brave-browser/wiki/Patching-Chromium#making-methods-virtual
now just redirects. Use a local anchor instead.
Clarify that network auditor entries are only needed for
endpoints that do not require user opt-in. Add new best
practice (BS-056) requiring all new Brave endpoints be
added to the HSTS pin list.
* Support tab groups in tree tab strip
Integrate tab groups with the tree tab strip so that groups are represented
as a single tree node wrapping the group (not one tree node per tab). Tabs
inside a group remain direct children of the group collection.
Collection and delegate:
- BraveTreeTabStripCollectionDelegate: When adding a tab with new_group_id,
add to collection without wrapping in a tree node; the group wraps it.
MoveTabsRecursive: handle empty tab_indices (no-op when moving within same
group), and route move-out-of-group and move-into-group to new helpers.
- MoveTabsIntoGroup: unwrap tabs from tree nodes (or detach from other
groups), add to target group; when group is detached (new group), wrap
group in a TreeTabNode and attach at the correct tree position.
- MoveTabsOutOfGroup: move tabs from TabGroupTabCollection back into the
tree by wrapping each in a tree node at the destination index.
- BraveTabStripCollection: add PopDetachedGroupCollectionForDelegate and
GetTreeTabNodeIdForGroup; chromium_src TabStripCollection gains virtual
GetTreeTabNodeIdForGroup. BraveTabStripCollectionDelegate implements
GetTreeTabNodeIdForGroup.
TreeTabNodeTabCollection:
- BuildTreeTabs: wrap entire groups in one tree node (processed_groups set);
grouped tabs stay as direct children of the group.
- New constructor that wraps a TabGroupTabCollection (for creating a group
in tree mode). Single-tab constructor allows null for GetEmptyTreeTabNode
when a tab was moved into a group and the view still holds the old node id.
Model, controller, and UI:
- BraveTabStripModel::GetTreeTabNodeIdForGroup returns the tree node id for
a group (or nullptr if tree tabs off). Wired through controller and
BraveTabStripCollection.
- BraveBrowserTabStripController::GetTreeTabNode returns GetEmptyTreeTabNode()
when the node is null (e.g. tab just moved into group, before
TabGroupedStateChanged/AddTabToGroup updates the view).
- OnTreeTabChanged: handle tab index kNoTab and detached state during group
creation; avoid double-clearing when AddTabToGroup will clear the node id.
- BraveTabStrip::AddTabToGroup override sets the tab's tree_tab_node from
GetTreeTabNodeIdForGroup when adding to a group in tree mode.
Add upstream test failures best practices doc
Consolidates upstream-flake filter guidance from testing-isolation.md
(TI-031, TI-032, TI-041, TI-042, TI-043) and patches.md (PATCH-011)
into a dedicated testing-upstream-failures.md. Expands the flake-check
section with full script usage and the LUCI verdict table.
* Add split tab support for tree tabs
In order to support split tabs in tree tabs, mainly we need to reimplment
two methods - CreateSplit and Unsplit.
For CreateSplit, we need to wrap the split tabs in a tree node, and insert it
at the position.
For Unsplit, we need to extract the tabs back to the tree nodes.
- TabStripCollection: virtual CreateSplit, Unsplit, AddCollectionMapping,
RemoveCollectionMapping; AddTabCollectionAtPosition for delegate.
- BraveTabStripCollection(delegate): override CreateSplit/Unsplit and
AddCollectionMapping/RemoveCollectionMapping; PassKey overloads for
AddTabCollectionAtPosition and collection mapping.
- BraveTreeTabStripCollectionDelegate: implement CreateSplit (wrap split in
tree node, insert at position), Unsplit (extract tabs back to tree nodes),
AddCollectionMapping/RemoveCollectionMapping for TREE_NODE holding split/group;
MoveTabsRecursive same-position handling for split/group creation.
- Tab strip API: handle TREE_NODE in tab_converters (unpinned container).
- Docs: tree tabs + split tabs flow and MoveTabsRecursive behavior.
* Support contained tabs session restore and sync.
* Review fixes.
* Use struct as a return value instead of optional with out args.
* Add a unit test for view-source: scheme.
* Move TabRestore helper to components.
* Add containers-specific SerializedNavigationBuilder* tests.
* Guard PageState.top.url_string use.
* Add plaster files for few patches.
* Replace browser_tabrestore.cc #define patch with plaster rewrite.
* Replace content_serialized_navigation_builder.cc change with rewrite.
* Fix gn check.
* Add prefix_length check.
* Improve rewrite rules.
* Guard PageState changes with buildflag.
* Rename StoragePartitionKeyToUrlPrefix.
* Add buildflag check into SerializedNavigationEntry.
* Fix build issues with containers buildflag disabled.
* Fix iOS build.
* Make TreeTabModel::SetCollapsed/DoesBelongToCollapsedNode()
TreeTabModel:
- Cache closest collapsed ancestor per node for O(1) visibility checks.
- Add SetCollapsed(id, collapsed) as UI entry point; calls TreeTabNode::set_collapsed()
and updates cache for affected descendants.
- Add DoesBelongToCollapsedNode(id) for quick "under collapsed" queries.
- Update cache in AddTreeTabNode (new node's closest collapsed), RemoveTreeTabNode
(recompute nodes that had removed id as closest), and OnTreeTabNodeMoved (recompute
moved node and its descendants after reparent).
TreeTabNode:
- Add GetClosestCollapsedAncestorId(), IsUnderCollapsedAncestor(), CollectDescendantIds()
for cache maintenance and tree traversal.
TreeTabNodeTabCollection:
- Add on_move callback; BuildTreeTabs and constructor take on_move.
- OnReparented invokes on_move so TreeTabModel can refresh collapse cache.
- Change on_create signature from void(const TreeTabNode&) to void(TreeTabNode&) so
delegate can pass AddTreeTabNode (non-const).
BraveTreeTabStripCollectionDelegate:
- Wire on_move to TreeTabModel::OnTreeTabNodeMoved in BuildTreeTabs and in
single-node TreeTabNodeTabCollection creation sites.
Serg