This page documents responsibilities of //net code reviewers. For the general guidelines, see Code Reviews.
See: The general flag guarding guidelines.
Changes that introduce new features or non-trivial behavior should be controlled by a feature flag defined in net/base/features.h. This allows us to disable the feature or revert to the previous behavior if serious issues arise, without requiring an emergency fix or reverting CLs.
The term “non-trivial behavior changes” is ambiguous, and it is difficult to define it clearly. Whether to add a flag should be determined on a case-by-case basis. For example, when changing the interpretation of an HTTP header, it would be necessary to add a flag.
Code whose behavior is influenced by the feature flag should be tested both with and without the flag enabled, using INSTANTIATE_TEST_SUITE_P.
Code in //net is also used in Cronet which runs the code in very different configurations to Chrome. This creates a risk that a configuration that is untested in Chrome could cause breakage in an Android app. As one way to mitigate this risk, //net has higher code coverage requirements than most of Chrome.
Our aim is to have 100% unit test coverage of non-trivial reachable code paths. Look for coverage gaps in Gerrit amd request additional tests if appropriate while respecting the code author's time.
See also: Code Coverage in Chromium
As in the rest of Chrome, std::string should be used for UTF-8 text and std::vector<uint8_t> should be used for binary data. net::IOBuffer can also be used for binary data, but use the newer spanified interfaces and not the legacy interfaces based on char* pointers.
A large amount of legacy code in //net uses std::string for binary data. Try to avoid spreading this usage any further.
See: Launching Features.
If the changes affect web API behavior or could have potential impacts for Web developers, make sure to ask the authors to follow the guidelines outlined on the “Launching Features” page.
TLS Encrypted ClientHello is an example that needed to follow the guidelines, even though it didn't involve explicit changes in web exposed APIs.
We do not implement standards just because they exist. If a CL implements a standard that is not implemented in other browsers, make sure there is an explainer that clearly describes the user benefit.
We implement standards from the WHATWG, W3C and IETF. Where they contradict, we attempt to align with other browsers.
New HTTP headers should be using structured fields, or have a good reason not to.
Try to verify that the implementation matches the standard.
We deviate from standards for privacy, security and iteroperability reasons. Be careful of CLs that improve standards compliance at the cost of other factors.
//net has access to sensitive user data. Every parser should have a fuzzer.
Parsers should operate on std::string_view or base::span<uint8_t>.
Most of the code in //net is user-visible performance critical. Look out for common performance pitfalls
const std::string& parameters. Unless all current and future callers already have a string, this will result in silent string copies. Usually std::string_view is a better choice.std::vector::push_back() without reserve() is a common source of unnecessary copies and allocations. Use base::ToVector() where appropriate.Because of Cronet's strict size restrictions, the bar for adding new dependencies to //net is very high.
Do not add a dependency on mojo.
//net is an implementation of the lower levels of the Fetch specification, very roughly corresponding to HTTP-network-or-cache fetch. It should not know or care about higher-level concepts like windows, frames or CORS. This rule is frequently bent for pragmatic reasons, but be cautious of CLs that talk about higher-level Web Platform features.
Another way to answer the question “does it belong in //net?” is to ask “Would an Android application using Cronet need this?”