Modularize chrome/browser/browser_features.h [chromium/src : main]

0 views
Skip to first unread message

Victor Vianna (Gerrit)

unread,
Mar 23, 2026, 7:16:38 AM (22 hours ago) Mar 23
to Lei Zhang, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, devtools...@chromium.org, Enterprise Policy Reviews, Andrew Rayskiy, prerendering-reviews, Rijubrata Bhaumik, Simon Hangl, aixba+wat...@chromium.org, ajayramamurthy...@google.com, akingsb+wat...@google.com, andysjl...@chromium.org, browser-comp...@chromium.org, chfreme...@chromium.org, chromium-a...@chromium.org, crisrael+wa...@google.com, dewitt...@chromium.org, dfried...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, druber...@chromium.org, estali...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, gavin...@chromium.org, hansberry+wa...@chromium.org, jackshira+wa...@google.com, japhet+...@chromium.org, julietlevesque...@google.com, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mattreyno...@chromium.org, mek+w...@chromium.org, mgiuca...@chromium.org, mickeybu...@chromium.org, nicolas...@chromium.org, npm+...@chromium.org, nwoked...@chromium.org, odejesu...@chromium.org, philli...@chromium.org, tburkar...@chromium.org, vakh+safe_br...@chromium.org, webap...@microsoft.com, xinghui...@chromium.org, ydago...@chromium.org, yigu+...@chromium.org, zackha...@chromium.org, zelin+watch-we...@chromium.org
Attention needed from Lei Zhang

Victor Vianna voted and added 2 comments

Votes added by Victor Vianna

Code-Review+1

2 comments

File chrome/browser/BUILD.gn
Line 118, Patchset 2 (Latest): sources = [ "browser_features.h" ]
Victor Vianna . unresolved

I would prefer if the .cc lived in the same target. The include below seems unused, can you delete it and add the .cc here? If it's still used, can you move the part that needs it to a different file?

https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/browser_features.cc;l=11-13;drc=4ddae623c1d0ea65c74646033e3b06935fbebf6d

Line 121, Patchset 2 (Latest): "//build:branding_buildflags",
"//build:chromeos_buildflags",
"//extensions/buildflags",
Victor Vianna . unresolved

I understand why //base should be in public_deps, but what about these?

Open in Gerrit

Related details

Attention is currently required from:
  • Lei Zhang
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Owners
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I68963b104a6272acab5730d85475b5539c29c8ea
Gerrit-Change-Number: 7690452
Gerrit-PatchSet: 2
Gerrit-Owner: Lei Zhang <the...@chromium.org>
Gerrit-Reviewer: Lei Zhang <the...@chromium.org>
Gerrit-Reviewer: Victor Vianna <victor...@google.com>
Gerrit-CC: Andrew Rayskiy <green...@google.com>
Gerrit-CC: Christian Biesinger <cbies...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: Rijubrata Bhaumik <rijubrat...@intel.com>
Gerrit-CC: Simon Hangl <sim...@google.com>
Gerrit-CC: prerendering-reviews <prerenderi...@chromium.org>
Gerrit-Attention: Lei Zhang <the...@chromium.org>
Gerrit-Comment-Date: Mon, 23 Mar 2026 11:16:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Lei Zhang (Gerrit)

unread,
Mar 23, 2026, 5:46:40 PM (12 hours ago) Mar 23
to Lei Zhang, Victor Vianna, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, devtools...@chromium.org, Enterprise Policy Reviews, Andrew Rayskiy, prerendering-reviews, Rijubrata Bhaumik, Simon Hangl, aixba+wat...@chromium.org, ajayramamurthy...@google.com, akingsb+wat...@google.com, andysjl...@chromium.org, browser-comp...@chromium.org, chfreme...@chromium.org, chromium-a...@chromium.org, crisrael+wa...@google.com, dewitt...@chromium.org, dfried...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, druber...@chromium.org, estali...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, gavin...@chromium.org, hansberry+wa...@chromium.org, jackshira+wa...@google.com, japhet+...@chromium.org, julietlevesque...@google.com, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mattreyno...@chromium.org, mek+w...@chromium.org, mgiuca...@chromium.org, mickeybu...@chromium.org, nicolas...@chromium.org, npm+...@chromium.org, nwoked...@chromium.org, odejesu...@chromium.org, philli...@chromium.org, tburkar...@chromium.org, vakh+safe_br...@chromium.org, webap...@microsoft.com, xinghui...@chromium.org, ydago...@chromium.org, yigu+...@chromium.org, zackha...@chromium.org, zelin+watch-we...@chromium.org
Attention needed from Lei Zhang and Victor Vianna

Lei Zhang added 2 comments

File chrome/browser/BUILD.gn
Line 118, Patchset 2 (Latest): sources = [ "browser_features.h" ]
Victor Vianna . unresolved

I would prefer if the .cc lived in the same target. The include below seems unused, can you delete it and add the .cc here? If it's still used, can you move the part that needs it to a different file?

https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/browser_features.cc;l=11-13;drc=4ddae623c1d0ea65c74646033e3b06935fbebf6d

Lei Zhang

I do too, but that requires more work. I can follow-up in another CL.

Line 121, Patchset 2 (Latest): "//build:branding_buildflags",
"//build:chromeos_buildflags",
"//extensions/buildflags",
Victor Vianna . resolved

I understand why //base should be in public_deps, but what about these?

Lei Zhang

In chrome/browser/browser_features.h, there are a bunch of includes, such as extensions/buildflags/buildflags.h.

Open in Gerrit

Related details

Attention is currently required from:
  • Lei Zhang
  • Victor Vianna
Gerrit-Attention: Victor Vianna <victor...@google.com>
Gerrit-Comment-Date: Mon, 23 Mar 2026 21:46:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Victor Vianna <victor...@google.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Victor Vianna (Gerrit)

unread,
Mar 23, 2026, 5:50:01 PM (12 hours ago) Mar 23
to Lei Zhang, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, devtools...@chromium.org, Enterprise Policy Reviews, Andrew Rayskiy, prerendering-reviews, Rijubrata Bhaumik, Simon Hangl, aixba+wat...@chromium.org, ajayramamurthy...@google.com, akingsb+wat...@google.com, andysjl...@chromium.org, browser-comp...@chromium.org, chfreme...@chromium.org, chromium-a...@chromium.org, crisrael+wa...@google.com, dewitt...@chromium.org, dfried...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, druber...@chromium.org, estali...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, gavin...@chromium.org, hansberry+wa...@chromium.org, jackshira+wa...@google.com, japhet+...@chromium.org, julietlevesque...@google.com, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mattreyno...@chromium.org, mek+w...@chromium.org, mgiuca...@chromium.org, mickeybu...@chromium.org, nicolas...@chromium.org, npm+...@chromium.org, nwoked...@chromium.org, odejesu...@chromium.org, philli...@chromium.org, tburkar...@chromium.org, vakh+safe_br...@chromium.org, webap...@microsoft.com, xinghui...@chromium.org, ydago...@chromium.org, yigu+...@chromium.org, zackha...@chromium.org, zelin+watch-we...@chromium.org
Attention needed from Lei Zhang

Victor Vianna added 1 comment

File chrome/browser/BUILD.gn
Line 118, Patchset 2 (Latest): sources = [ "browser_features.h" ]
Victor Vianna . unresolved

I would prefer if the .cc lived in the same target. The include below seems unused, can you delete it and add the .cc here? If it's still used, can you move the part that needs it to a different file?

https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/browser_features.cc;l=11-13;drc=4ddae623c1d0ea65c74646033e3b06935fbebf6d

Lei Zhang

I do too, but that requires more work. I can follow-up in another CL.

Victor Vianna

Sure, but if the include I linked to above is indeed unused, you can also do it in this CL without resetting the +1

Open in Gerrit

Related details

Attention is currently required from:
  • Lei Zhang
Gerrit-Comment-Date: Mon, 23 Mar 2026 21:49:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Lei Zhang <the...@chromium.org>
Comment-In-Reply-To: Victor Vianna <victor...@google.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Victor Vianna (Gerrit)

unread,
Mar 23, 2026, 5:51:06 PM (12 hours ago) Mar 23
to Lei Zhang, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, devtools...@chromium.org, Enterprise Policy Reviews, Andrew Rayskiy, prerendering-reviews, Rijubrata Bhaumik, Simon Hangl, aixba+wat...@chromium.org, ajayramamurthy...@google.com, akingsb+wat...@google.com, andysjl...@chromium.org, browser-comp...@chromium.org, chfreme...@chromium.org, chromium-a...@chromium.org, crisrael+wa...@google.com, dewitt...@chromium.org, dfried...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, druber...@chromium.org, estali...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, gavin...@chromium.org, hansberry+wa...@chromium.org, jackshira+wa...@google.com, japhet+...@chromium.org, julietlevesque...@google.com, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mattreyno...@chromium.org, mek+w...@chromium.org, mgiuca...@chromium.org, mickeybu...@chromium.org, nicolas...@chromium.org, npm+...@chromium.org, nwoked...@chromium.org, odejesu...@chromium.org, philli...@chromium.org, tburkar...@chromium.org, vakh+safe_br...@chromium.org, webap...@microsoft.com, xinghui...@chromium.org, ydago...@chromium.org, yigu+...@chromium.org, zackha...@chromium.org, zelin+watch-we...@chromium.org
Attention needed from Lei Zhang

Victor Vianna added 1 comment

File chrome/browser/BUILD.gn
Line 118, Patchset 2 (Latest): sources = [ "browser_features.h" ]
Victor Vianna . unresolved

I would prefer if the .cc lived in the same target. The include below seems unused, can you delete it and add the .cc here? If it's still used, can you move the part that needs it to a different file?

https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/browser_features.cc;l=11-13;drc=4ddae623c1d0ea65c74646033e3b06935fbebf6d

Lei Zhang

I do too, but that requires more work. I can follow-up in another CL.

Victor Vianna

Sure, but if the include I linked to above is indeed unused, you can also do it in this CL without resetting the +1

Victor Vianna

you can also do it in this CL without resetting the +1

Nah, actually not, browser_features.cc is not touched by the CL

Gerrit-Comment-Date: Mon, 23 Mar 2026 21:50:40 +0000
satisfied_requirement
unsatisfied_requirement
open
diffy

Lei Zhang (Gerrit)

unread,
Mar 23, 2026, 7:31:38 PM (10 hours ago) Mar 23
to Lei Zhang, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, devtools...@chromium.org, Enterprise Policy Reviews, Andrew Rayskiy, prerendering-reviews, Rijubrata Bhaumik, Simon Hangl, aixba+wat...@chromium.org, ajayramamurthy...@google.com, akingsb+wat...@google.com, andysjl...@chromium.org, browser-comp...@chromium.org, chfreme...@chromium.org, chromium-a...@chromium.org, crisrael+wa...@google.com, dewitt...@chromium.org, dfried...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, druber...@chromium.org, estali...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, gavin...@chromium.org, hansberry+wa...@chromium.org, jackshira+wa...@google.com, japhet+...@chromium.org, julietlevesque...@google.com, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mattreyno...@chromium.org, mek+w...@chromium.org, mgiuca...@chromium.org, mickeybu...@chromium.org, nicolas...@chromium.org, npm+...@chromium.org, nwoked...@chromium.org, odejesu...@chromium.org, philli...@chromium.org, tburkar...@chromium.org, vakh+safe_br...@chromium.org, webap...@microsoft.com, xinghui...@chromium.org, ydago...@chromium.org, yigu+...@chromium.org, zackha...@chromium.org, zelin+watch-we...@chromium.org

Lei Zhang added 1 comment

File chrome/browser/BUILD.gn
Line 118, Patchset 2: sources = [ "browser_features.h" ]
Victor Vianna . resolved

I would prefer if the .cc lived in the same target. The include below seems unused, can you delete it and add the .cc here? If it's still used, can you move the part that needs it to a different file?

https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/browser_features.cc;l=11-13;drc=4ddae623c1d0ea65c74646033e3b06935fbebf6d

Lei Zhang

I do too, but that requires more work. I can follow-up in another CL.

Victor Vianna

Sure, but if the include I linked to above is indeed unused, you can also do it in this CL without resetting the +1

Victor Vianna

you can also do it in this CL without resetting the +1

Nah, actually not, browser_features.cc is not touched by the CL

Lei Zhang

This CL already touches a lot of files, so I appended a CL. Thanks for pointing out the unused file that tripped me up.

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I68963b104a6272acab5730d85475b5539c29c8ea
    Gerrit-Change-Number: 7690452
    Gerrit-PatchSet: 3
    Gerrit-Owner: Lei Zhang <the...@chromium.org>
    Gerrit-Reviewer: Lei Zhang <the...@chromium.org>
    Gerrit-Reviewer: Victor Vianna <victor...@google.com>
    Gerrit-CC: Andrew Rayskiy <green...@google.com>
    Gerrit-CC: Christian Biesinger <cbies...@chromium.org>
    Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
    Gerrit-CC: Rijubrata Bhaumik <rijubrat...@intel.com>
    Gerrit-CC: Simon Hangl <sim...@google.com>
    Gerrit-CC: prerendering-reviews <prerenderi...@chromium.org>
    Gerrit-Comment-Date: Mon, 23 Mar 2026 23:31:25 +0000
    satisfied_requirement
    open
    diffy
    Reply all
    Reply to author
    Forward
    0 new messages