Chromium Code Reviews| Index: net/docs/life-of-a-feature.md |
| diff --git a/net/docs/life-of-a-feature.md b/net/docs/life-of-a-feature.md |
| new file mode 100644 |
| index 0000000000000000000000000000000000000000..fc763ce44adc5b555457d93fbf38e91174ce9cd3 |
| --- /dev/null |
| +++ b/net/docs/life-of-a-feature.md |
| @@ -0,0 +1,255 @@ |
| +# Life of a Feature |
| + |
| +In the years since the Chromium browser was first open-sourced, the `//net` |
| +directory has expanded from being the basis of loading web content in the |
| +Chromium browser to accomodating a wide variety of networking needs, |
| +both in the Chromium browser and in other Google and third-party products |
| +and projects. |
| + |
| +This brings with it many new opportunities, such as the ability to |
| +introduce new protocols rapidly or push Web security forward, as well as |
| +new challenges, such as how to balance the needs of various `//net` |
| +consumers effectively. |
| + |
| +To make it easier to contribute new features or to change existing |
| +behaviours in `//net`, this document tries to capture the life of a |
| +feature in `//net`, from initial design to the eventual possibility of |
| +deprecation and removal. |
| + |
| +## Supported Projects |
| + |
| +When considering the introduction of a new `//net` feature or changing |
| +a `//net` behaviour, it's first necessary to understand where `//net` |
| +is used, how it is used, and what the various constraints and limits are. |
| + |
| +To understand a more comprehensive matrix of the supported platforms and |
| +constraints, see [Supported Projects](supported-projects.md). When |
| +examining a new feature request, or a change in behaviour, it's necessary |
| +to consider dimensions such as: |
| + |
| + * Does this feature apply to all supported projects, or is this something |
| + like a Browser-only feature? |
| + * Does this feature apply to all supported platforms, or is this something |
| + specific to a particular subset? |
| + * Is the feature a basic networking library feature, or is it specific to |
| + something in the Web Platform? |
| + * Will some projects wish to strip the feature in order to meet targets |
| + such as memory usage (RAM) or binary size? |
| + * Does it depend on Google services or Google-specific behaviours or |
| + features? |
| + * How will this feature be tested / experimented with? For example, |
| + __Field Trials (Finch)__ and __User Metrics (UMA)__ may not be available |
| + on a number of platforms. |
| + * How risky is the feature towards compatibility/stability? How will it |
| + be undone if there is a bug? |
| + * Are the power/memory/CPU/bandwidth requirements appropriate for the |
| + targetted projects and/or platforms? |
| + |
| +## Design and Layering |
| + |
| +Once the supported platforms and constraints are identified, it's necessary |
| +to determine how to actually design the feature to meet those constraints, |
| +in hopefully the easiest way possible both for implementation and consumption. |
| + |
| +### Designing for multiple platforms |
| + |
| +In general, `//net` features try to support all platforms with a common |
| +interface, and generally eschew OS-specific interfaces from being exposed as |
| +part of `//net`. |
| + |
| +Cross-platform code is generally done via declaring an interface named |
| +`foo.h`, which is common for all platforms, and then using the build-system to |
| +do compile-time switching between implementations in `foo_win.cc`, `foo_mac.cc`, |
| +`foo_android.cc`, etc. |
| + |
| +The goal is to ensure that consumers generally don't have to think about |
| +OS-specific considerations, and can instead code to the interface. |
| + |
| +### Designing for multiple products |
| + |
| +While premature abstraction can significantly harm readability, if it is |
| +anticipated that different products will have different implementation needs, |
| +or may wish to selectively disable the feature, it's often necessary to |
| +abstract that interface sufficiently in `//net` to allow for dependency |
| +injection. |
| + |
| +This is true whether discussing concrete classes and interfaces or something |
| +as simple a boolean configuration flag that different consumers wish to set |
| +differently. |
| + |
| +The two most common approaches in `//net` are injection and delegation. |
| + |
| +#### Injection |
| + |
| +Injection refers to the pattern of defining the interface or concrete |
| +configuration parameter (such as a boolean), along with the concrete |
| +implementation, but requiring the `//net` embedder to supply it (perhaps |
| +optionally). |
| + |
| +Examples of this pattern include things such as the `ProxyConfigService`, |
| +which has concrete implementations in `//net` for a variety of platforms' |
| +configuration of proxies, but which requires it be supplied as part of the |
| +`URLRequestContextGetter` building, if proxies are going to be supported. |
| + |
| +An example of injecting configuration flags can be seen in the |
| +`HttpNetworkSession::Params` structure, which is used to provide much of |
| +the initialization parameters for the HTTP layer. |
| + |
| +While the ideal form of injection is to pass ownership of the injected |
| +object, such as via a `std::unique_ptr<Foo>`, there are a number of places |
| +in `//net` in which ownership is shared / left to the embedder, and the |
| +injected object is passed around as a naked/raw pointer. |
| + |
| +#### Delegation |
| + |
| +Delegation refers to forcing the `//net` embedder to respond to specific |
| +delegated calls via a Delegate interface that it implements. In general, |
| +when using the delegated pattern, ownership of the delegate should be |
| +transferred to the thing consuming the delegate, so that the lifetime and |
|
Charlie Harrison
2017/02/16 18:33:11
Can you be more specific about the "thing consumin
|
| +threading semantics are always clear. |
| + |
| +The most notable example of the delegate pattern is `URLRequest::Delegate`. |
| + |
| +While the use of a `base::Callback` can also be considered a form of |
| +delegation, the `//net` layer tries to eschew any callbacks that can be |
|
Charlie Harrison
2017/02/16 18:33:11
might want to mention e.g. base::OnceCallback if y
Charlie Harrison
2017/02/16 18:33:11
might want to mention e.g. base::OnceCallback if y
|
| +called more than once, and instead favors defining class interfaces |
| +with concrete behavioural requirements in order to ensure the correct |
| +lifetimes of objects and to adjust over time. |
| + |
| +### Understanding the Layering |
| + |
| +A significant challenge many feature proposals face is understanding the |
| +layering in `//net` and what different portions of `//net` are allowed to |
| +know. |
| + |
| +#### Socket Pools |
| + |
| +The most common challenge feature proposals encounter is the awareness |
| +that the act of associating an actual request to make with a socket is |
| +done lazily, referred to as "late-binding". |
| + |
| +With late-bound sockets, a given `URLRequest` will not be assigned an actual |
| +transport connection until the request is ready to be sent. This allows for |
| +reprioritizing requests as they come in, to ensure that higher priority requests |
| +get preferential treatment, but it also means that features or data associated |
| +with a `URLRequest` generally don't participate in socket establishment or |
| +maintenance. |
| + |
| +For example, a feature that wanted to associate the low-level network socket |
| +with a `URLRequest` during connection establishment is not something that the |
| +`//net` design supports, since the `URLRequest` is kept unaware of how sockets |
| +are established by virtue of the socket pools and late binding. This allows for |
| +more flexibility when working to improve performance, such as the ability to |
| +coalesce multiple logical 'sockets' over a single HTTP/2 or QUIC stream, which |
| +may only have a single physical network socket involved. |
| + |
| +#### Making Additional Requests |
| + |
| +From time to time, `//net` feature proposals will involve needing to load |
| +a secondary resource as part of processing. For example, SDCH involves loading |
| +additional dictionaries that are advertised in a header, and other feature |
| +proposals have involved fetching a `/.well-known/` URI or reporting errors to |
| +a particular URL. |
| + |
| +This is particularly challenging, because often, these features are implemented |
| +deeper in the network stack, such as [`//net/cert`](../cert), [`//net/http`](../http), |
| +or [`//net/filter`](../filter), which [`//net/url_request`](../url_request) depends |
| +on. Because `//net/url_request` depends on these low-level directories, it would |
| +be a circular dependency to have these directories depend on `//net/url_request`, |
| +and circular dependencies are forbidden. |
| + |
| +The recommended solution to address this is to adopt the delegation or injection |
| +patterns. The lower-level directory will define some interface that represents the |
| +"I need this URL" request, and then elsewhere, in a directory allowed to depend |
| +on `//net/url_request`, an implementation of that interface/delegate that uses |
| +`//net/url_request` is implemented. |
| + |
| +### Specs: What Are They Good For |
| + |
| +As `//net` is used as the basis for a number of browsers, it's an important part |
| +of the design philosophy to ensure behaviours are well-specified, and that the |
| +implementation conforms to those specifications. This may be seen as burdensome |
| +when it's unclear whether or not a feature will 'take off,' but it's equally |
| +critical to ensure that the Chromium projects do not fork the Web Platform. |
| + |
| +#### Incubation Is Required |
| + |
| +`//net` respects Chromium's overall position of [incubation first](https://groups.google.com/a/chromium.org/d/msg/blink-dev/PJ_E04kcFb8/baiLN3DTBgAJ) standards development. |
| + |
| +With an incubation first approach, before introducing any new features that |
| +might be exposed over the wire to servers, whether they be explicit behaviours |
| +such as adding new headers to implicit behaviours such as |
| +[Happy Eyeballs](https://tools.ietf.org/html/rfc6555), should have some form |
| +of specification written. That specification should at least be incubation |
|
Charlie Harrison
2017/02/16 18:33:11
This sentence is a bit of a run-on. Suggest slight
|
| +track, although the expectation is that the specification SHOULD have a direct |
| +path towards an appropriate standards track, and moving towards that. Features |
| +which don't adhere to this pattern require a number of high-level approvals, to |
| +ensure that the Platform doesn't fragment. |
| + |
| +#### Introducing New Headers |
| + |
| +A common form of feature request is the introduction of new headers, either via |
| +the `//net` implementation directly, or through consuming `//net` interfaces |
| +and modifying headers on the fly. |
| + |
| +The introduction of any additional headers SHOULD have an incubated spec attached, |
| +ideally with cross-vendor interest. Particularly, headers which only apply to |
| +Google or Google services are very likely to be rejected outright. |
| + |
| +#### Making Additional Requests |
| + |
| +While it's necessary to provide abstraction around `//net/url_request` for |
| +any lower-level components that may need to make additional requests, for most |
| +features, that's not all that is necessary. Because `//net/url_request` only |
| +provides a basic HTTP fetching mechanism, it's insufficient for any Web Platform |
| +feature, because it doesn't consider the broader platform concerns such as |
| +interactions with CORS, Service Workers, cookie and authentication policies, or |
| +even basic interactions with optional features like Extensions or SafeBrowsing. |
| + |
| +To account for all of these things, any resource fetching that is to support a |
| +Web Platform feature (i.e. be exposed in any of the Browser projects), the |
|
Charlie Harrison
2017/02/16 18:33:11
The "i.e." confuses me, and I think it is due to t
|
| +feature should be explainable in terms of the |
| +[Fetch Living Standard](https://fetch.spec.whatwg.org/). The Fetch standard defines |
| +a JavaScript API for fetching resources, but more importantly, defines a common |
| +set of infrastructure and terminology that tries to define how all resource loads |
| +in the Web Platform happen - whether it be through the JavaScript API, through |
| +`XMLHttpRequest`, or the `src` attribute in HTML tags, for example. |
| + |
| +Thus, in addition to defining a class abstraction to "fetch resources," any feature |
| +proposal that needs to do so, and will be part of any of the browser projects, |
| +should be "explained in terms of Fetch". |
| + |
| +## Implementation |
| + |
| +In general, prior to implementing, try to get a review on net-dev@chromium.org |
| +for the general feedback and design review. |
| + |
| +In addition to the net-dev@chromium.org early review, `//net` requires that any |
| +browser-exposed behaviour should also adhere to the |
|
Charlie Harrison
2017/02/16 18:33:11
ditto, I think this should probably be "Web Platfo
|
| +[Blink Process](https://www.chromium.org/blink#new-features), which includes an |
| +"Intent to Implement" message to blink-dev@chromium.org |
| + |
| +For features that are unclear about their future, such as experiments or trials, |
| +it's also expected that the design planning will also account for how features |
| +will be removed cleanly. For features that radically affect the architecture of |
| +`//net`, expect a high bar of justification, since reversing those changes if |
| +it fails to pan out can cause significant disruption to productivity and |
| +stability. |
| + |
| +## Deprecation |
| + |
| +Plan for obsolence, hope for success. Similar to implementation, features that |
| +are to be removed should also go through the |
| +[Blink Process](https://www.chromium.org/blink#TOC-Web-Platform-Changes:-Process) |
| +for removing features. |
| + |
| +Note that due to the diversity of [Supported Projects](supported-projects.md), |
| +removing a feature while minimizing disruption can be just as complex as adding |
| +a feature. For example, relying solely on __User Metrics (UMA)__ to signal the |
| +safety of removing a feature may not consider all projects, and relying on |
| +__Field Trials (Finch)__ to assess risk or restore the 'legacy' behaviour may not |
| +work on all projects either. |
| + |
| +It's precisely because of these challenges that there's such a high bar for |
| +adding features, because they may represent multi-year committments to support, |
| +even when the feature itself is deprecated or targetted for removal. |