[Tracing] Modernize string handling and header hygiene [chromium/src : main]
2 views
Skip to first unread message
Andrew Paseltiner (Gerrit)
Mar 17, 2026, 1:19:02 PMMar 17
to Charlie Reis, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Charlie Reis
New activity on the change![]( "Open in Gerrit")
Related details
Attention is currently required from:
- Charlie Reis
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Reis (Gerrit)
Mar 18, 2026, 2:36:41 PMMar 18
to Andrew Paseltiner, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Andrew Paseltiner
Charlie Reis added 3 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 2 (Latest):
Charlie Reis . resolved
Thanks. String changes look good, but Gemini noted a problem on the forward declaration.
Commit Message
Line 20, Patchset 2 (Latest):
Change-Id: I39d938185c3810502e3050d91b251fd87578bd89Charlie Reis . unresolved
Please associate changes with a bug whenever possible, which is useful for context and for grouping together related CLs.
File content/browser/android/tracing_controller_android.h
Line 49, Patchset 2 (Latest):
static base::FilePath GenerateTracingFilePath(std::string_view basename);Charlie Reis . unresolved
Gemini noted a concern with this return type:
Return-by-Value with Forward Declaration
Issue: You cannot return a class by value if the compiler only sees a forward
declaration. The compiler needs the full definition of base::FilePath to
determine the return value's size and layout at the call site for any file
that includes this header.
Remediation: Revert the removal of #include "base/files/file_path.h" in
tracing_controller_android.h. Forward declarations are only suitable for
pointers or references.
It appears the compiler didn't complain, but Gemini confirmed it's still an issue:
1. Self-Contained Headers
Chromium follows the Google C++ Style Guide
(https://google.github.io/styleguide/cppguide.html#Self_contained_Headers),
which requires headers to be "self-contained." This means any file including
tracing_controller_android.h should be able to use its public API without
having to manually include additional dependencies.
Because GenerateTracingFilePath returns a base::FilePath by value, any caller
of this method will immediately encounter a compilation error unless they also
include base/files/file_path.h. By removing the include from the header, you
are offloading the "cost" of the include to every single call site, which
often results in more maintenance overhead and less readable code.
2. When to Forward Declare
The general rule in Chromium (and modern C++) is:
* Forward declare when the type is only used as a pointer or reference.
* Include when the type is used by value (as a member variable or a return
value), because the compiler needs to know the object's size and layout to
generate code at the call site.
Analysis of this Specific Case
In your patch, you actually had to add #include "base/files/file_path.h" to
startup_tracing_controller.cc precisely because the forward declaration in the
header was insufficient for the call site.
Related details
Attention is currently required from:
- Andrew Paseltiner
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Paseltiner (Gerrit)
Mar 18, 2026, 2:38:40 PMMar 18
to Charlie Reis, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Charlie Reis
Andrew Paseltiner added 1 comment![]( "Open in Gerrit")
File content/browser/android/tracing_controller_android.h
Attention is currently required from:
- Charlie Reis
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Reis (Gerrit)
Mar 18, 2026, 3:40:33 PMMar 18
to Andrew Paseltiner, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Andrew Paseltiner
Charlie Reis voted and added 2 comments![]( "Open in Gerrit")
Votes added by Charlie Reis
| Code-Review | +1 |
2 comments
Patchset-level comments
Charlie Reis . resolved
LGTM, ideally once a bug number is added.
File content/browser/android/tracing_controller_android.h
Line 49, Patchset 2 (Latest):
static base::FilePath GenerateTracingFilePath(std::string_view basename);Charlie Reis . resolved
Charlie Reis
Conflicting style guides are fun. Given "You can and should use forward declarations for most types passed or returned by value, reference, or pointer" from that link, this seems to be an approved case.
Related details
Attention is currently required from:
- Andrew Paseltiner
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Paseltiner (Gerrit)
Mar 19, 2026, 9:33:22 AMMar 19
to Charlie Reis, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Andrew Paseltiner voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Paseltiner
| Commit-Queue | +2 |
1 comment
Commit Message
Please associate changes with a bug whenever possible, which is useful for context and for grouping together related CLs.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Paseltiner (Gerrit)
Mar 19, 2026, 10:30:09 AMMar 19
to Charlie Reis, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Andrew Paseltiner voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Mar 19, 2026, 12:25:06 PMMar 19
to Andrew Paseltiner, Charlie Reis, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Commit message:
[Tracing] Modernize string handling and header hygiene
- Update TracingControllerAndroid::GenerateTracingFilePath to take a
std::string_view instead of const std::string&.
- Improve header hygiene in tracing_controller_android.h by using a
forward declaration for base::FilePath and moving the include to the
implementation.
- Update StartupTracingController to use std::string_view for basename
handling and std::move for property setters.
These changes reduce unnecessary string copies and improve compilation
times by pruning header dependencies.
Bug: 494114608
Change-Id: I39d938185c3810502e3050d91b251fd87578bd89
Reviewed-by: Charlie Reis <cr...@chromium.org>
Commit-Queue: Andrew Paseltiner <apase...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1602030}
Files:
- M content/browser/android/tracing_controller_android.cc
- M content/browser/android/tracing_controller_android.h
- M content/browser/tracing/startup_tracing_controller.cc
Change size: S
Delta: 3 files changed, 19 insertions(+), 8 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Charlie Reis
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages