[Experiment] Remove Bounce Tracking Mitigations from Cobalt#11536
[Experiment] Remove Bounce Tracking Mitigations from Cobalt#11536linxinan-yt wants to merge 5 commits into
Conversation
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
There was a problem hiding this comment.
Code Review
This pull request introduces conditional compilation guards to gate various Privacy Sandbox APIs (such as Fenced Frames, WebID, BTM, and Trust Tokens) behind the ENABLE_PRIVACY_SANDBOX_APIS build flag across the codebase. The feedback highlights two critical issues in render_frame_host_impl.cc: first, a maintainability anti-pattern where preprocessor directives span across function boundaries, which breaks code formatting and IDE tooling; second, a compilation error when the APIs are disabled due to RevokeNetworkForNonceCallback referencing the incomplete type FencedFrameProperties. It is recommended to isolate each function with its own preprocessor guards and completely compile out RevokeNetworkForNonceCallback when the build flag is disabled.
| } | ||
|
|
||
| void RenderFrameHostImpl::DisableUntrustedNetworkInFencedFrame( | ||
| DisableUntrustedNetworkInFencedFrameCallback callback) { |
There was a problem hiding this comment.
Maintainability Anti-Pattern: Preprocessor Directives Spanning Across Function Boundaries
Spanning preprocessor directives (#if / #else / #endif) across function boundaries (starting inside DisableUntrustedNetworkInFencedFrame and ending inside RevokeNetworkForNonceCallback) is a highly discouraged practice.
This "brace-matching" trick:
- Breaks Tooling: Automatic code formatters like
clang-formatwill fail or produce highly distorted formatting due to mismatched braces inside preprocessor blocks. - Breaks IDEs: IDE outline views, code navigation, refactoring tools, and brace-matching features will be completely broken.
- Reduces Readability: It is extremely confusing for future maintainers to follow the control flow and structure of the code.
Recommended Solution:
Each function should be self-contained with its own preprocessor guards.
- Wrap
DisableUntrustedNetworkInFencedFramewith its own#if/#else/#endifblock, running the callback in the#elseblock. - Wrap
RevokeNetworkForNonceCallbackentirely in#if BUILDFLAG(ENABLE_PRIVACY_SANDBOX_APIS)(both its declaration inrender_frame_host_impl.hand its definition inrender_frame_host_impl.cc). SinceDisableUntrustedNetworkInFencedFramewon't call it when the APIs are disabled, it can be completely compiled out.
| #else | ||
| std::move(callback).Run(); | ||
| #endif // BUILDFLAG(ENABLE_PRIVACY_SANDBOX_APIS) |
There was a problem hiding this comment.
Compilation Error: Incomplete Type FencedFrameProperties
When ENABLE_PRIVACY_SANDBOX_APIS is false, FencedFrameProperties is not defined because the fenced_frame/* directory is excluded from the build.
Because FencedFrameProperties is undefined, the signature of RevokeNetworkForNonceCallback in render_frame_host_impl.h (which uses std::optional<FencedFrameProperties>) will fail to compile, as std::optional requires a complete type.
Recommended Solution:
Completely wrap both the declaration of RevokeNetworkForNonceCallback in render_frame_host_impl.h and its definition in render_frame_host_impl.cc with #if BUILDFLAG(ENABLE_PRIVACY_SANDBOX_APIS). Since the calling function DisableUntrustedNetworkInFencedFrame is already guarded, RevokeNetworkForNonceCallback is completely unused when the APIs are disabled and can be safely compiled out.
Bug: 505811196