Chromium Code Reviews| OLD | NEW |
|---|---|
| (Empty) | |
| 1 # Life of a Feature | |
| 2 | |
| 3 In the years since the Chromium browser was first open-sourced, the `//net` | |
| 4 directory has expanded from being the basis of loading web content in the | |
| 5 Chromium browser to accomodating a wide variety of networking needs, | |
| 6 both in the Chromium browser and in other Google and third-party products | |
| 7 and projects. | |
| 8 | |
| 9 This brings with it many new opportunities, such as the ability to | |
| 10 introduce new protocols rapidly or push Web security forward, as well as | |
| 11 new challenges, such as how to balance the needs of various `//net` | |
| 12 consumers effectively. | |
| 13 | |
| 14 To make it easier to contribute new features or to change existing | |
| 15 behaviours in `//net`, this document tries to capture the life of a | |
| 16 feature in `//net`, from initial design to the eventual possibility of | |
| 17 deprecation and removal. | |
| 18 | |
| 19 ## Supported Projects | |
| 20 | |
| 21 When considering the introduction of a new `//net` feature or changing | |
| 22 a `//net` behaviour, it's first necessary to understand where `//net` | |
| 23 is used, how it is used, and what the various constraints and limits are. | |
| 24 | |
| 25 To understand a more comprehensive matrix of the supported platforms and | |
| 26 constraints, see [Supported Projects](supported-projects.md). When | |
| 27 examining a new feature request, or a change in behaviour, it's necessary | |
| 28 to consider dimensions such as: | |
| 29 | |
| 30 * Does this feature apply to all supported projects, or is this something | |
| 31 like a Browser-only feature? | |
| 32 * Does this feature apply to all supported platforms, or is this something | |
| 33 specific to a particular subset? | |
| 34 * Is the feature a basic networking library feature, or is it specific to | |
| 35 something in the Web Platform? | |
| 36 * Will some projects wish to strip the feature in order to meet targets | |
| 37 such as memory usage (RAM) or binary size? | |
| 38 * Does it depend on Google services or Google-specific behaviours or | |
| 39 features? | |
| 40 * How will this feature be tested / experimented with? For example, | |
| 41 __Field Trials (Finch)__ and __User Metrics (UMA)__ may not be available | |
| 42 on a number of platforms. | |
| 43 * How risky is the feature towards compatibility/stability? How will it | |
| 44 be undone if there is a bug? | |
| 45 * Are the power/memory/CPU/bandwidth requirements appropriate for the | |
| 46 targetted projects and/or platforms? | |
| 47 | |
| 48 ## Design and Layering | |
| 49 | |
| 50 Once the supported platforms and constraints are identified, it's necessary | |
| 51 to determine how to actually design the feature to meet those constraints, | |
| 52 in hopefully the easiest way possible both for implementation and consumption. | |
| 53 | |
| 54 ### Designing for multiple platforms | |
| 55 | |
| 56 In general, `//net` features try to support all platforms with a common | |
| 57 interface, and generally eschew OS-specific interfaces from being exposed as | |
| 58 part of `//net`. | |
| 59 | |
| 60 Cross-platform code is generally done via declaring an interface named | |
| 61 `foo.h`, which is common for all platforms, and then using the build-system to | |
| 62 do compile-time switching between implementations in `foo_win.cc`, `foo_mac.cc`, | |
| 63 `foo_android.cc`, etc. | |
| 64 | |
| 65 The goal is to ensure that consumers generally don't have to think about | |
| 66 OS-specific considerations, and can instead code to the interface. | |
| 67 | |
| 68 ### Designing for multiple products | |
| 69 | |
| 70 While premature abstraction can significantly harm readability, if it is | |
| 71 anticipated that different products will have different implementation needs, | |
| 72 or may wish to selectively disable the feature, it's often necessary to | |
| 73 abstract that interface sufficiently in `//net` to allow for dependency | |
| 74 injection. | |
| 75 | |
| 76 This is true whether discussing concrete classes and interfaces or something | |
| 77 as simple a boolean configuration flag that different consumers wish to set | |
| 78 differently. | |
| 79 | |
| 80 The two most common approaches in `//net` are injection and delegation. | |
| 81 | |
| 82 #### Injection | |
| 83 | |
| 84 Injection refers to the pattern of defining the interface or concrete | |
| 85 configuration parameter (such as a boolean), along with the concrete | |
| 86 implementation, but requiring the `//net` embedder to supply it (perhaps | |
| 87 optionally). | |
| 88 | |
| 89 Examples of this pattern include things such as the `ProxyConfigService`, | |
| 90 which has concrete implementations in `//net` for a variety of platforms' | |
| 91 configuration of proxies, but which requires it be supplied as part of the | |
| 92 `URLRequestContextGetter` building, if proxies are going to be supported. | |
| 93 | |
| 94 An example of injecting configuration flags can be seen in the | |
| 95 `HttpNetworkSession::Params` structure, which is used to provide much of | |
| 96 the initialization parameters for the HTTP layer. | |
| 97 | |
| 98 While the ideal form of injection is to pass ownership of the injected | |
| 99 object, such as via a `std::unique_ptr<Foo>`, there are a number of places | |
| 100 in `//net` in which ownership is shared / left to the embedder, and the | |
| 101 injected object is passed around as a naked/raw pointer. | |
| 102 | |
| 103 #### Delegation | |
| 104 | |
| 105 Delegation refers to forcing the `//net` embedder to respond to specific | |
| 106 delegated calls via a Delegate interface that it implements. In general, | |
| 107 when using the delegated pattern, ownership of the delegate should be | |
| 108 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
| |
| 109 threading semantics are always clear. | |
| 110 | |
| 111 The most notable example of the delegate pattern is `URLRequest::Delegate`. | |
| 112 | |
| 113 While the use of a `base::Callback` can also be considered a form of | |
| 114 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
| |
| 115 called more than once, and instead favors defining class interfaces | |
| 116 with concrete behavioural requirements in order to ensure the correct | |
| 117 lifetimes of objects and to adjust over time. | |
| 118 | |
| 119 ### Understanding the Layering | |
| 120 | |
| 121 A significant challenge many feature proposals face is understanding the | |
| 122 layering in `//net` and what different portions of `//net` are allowed to | |
| 123 know. | |
| 124 | |
| 125 #### Socket Pools | |
| 126 | |
| 127 The most common challenge feature proposals encounter is the awareness | |
| 128 that the act of associating an actual request to make with a socket is | |
| 129 done lazily, referred to as "late-binding". | |
| 130 | |
| 131 With late-bound sockets, a given `URLRequest` will not be assigned an actual | |
| 132 transport connection until the request is ready to be sent. This allows for | |
| 133 reprioritizing requests as they come in, to ensure that higher priority requests | |
| 134 get preferential treatment, but it also means that features or data associated | |
| 135 with a `URLRequest` generally don't participate in socket establishment or | |
| 136 maintenance. | |
| 137 | |
| 138 For example, a feature that wanted to associate the low-level network socket | |
| 139 with a `URLRequest` during connection establishment is not something that the | |
| 140 `//net` design supports, since the `URLRequest` is kept unaware of how sockets | |
| 141 are established by virtue of the socket pools and late binding. This allows for | |
| 142 more flexibility when working to improve performance, such as the ability to | |
| 143 coalesce multiple logical 'sockets' over a single HTTP/2 or QUIC stream, which | |
| 144 may only have a single physical network socket involved. | |
| 145 | |
| 146 #### Making Additional Requests | |
| 147 | |
| 148 From time to time, `//net` feature proposals will involve needing to load | |
| 149 a secondary resource as part of processing. For example, SDCH involves loading | |
| 150 additional dictionaries that are advertised in a header, and other feature | |
| 151 proposals have involved fetching a `/.well-known/` URI or reporting errors to | |
| 152 a particular URL. | |
| 153 | |
| 154 This is particularly challenging, because often, these features are implemented | |
| 155 deeper in the network stack, such as [`//net/cert`](../cert), [`//net/http`](../ http), | |
| 156 or [`//net/filter`](../filter), which [`//net/url_request`](../url_request) depe nds | |
| 157 on. Because `//net/url_request` depends on these low-level directories, it would | |
| 158 be a circular dependency to have these directories depend on `//net/url_request` , | |
| 159 and circular dependencies are forbidden. | |
| 160 | |
| 161 The recommended solution to address this is to adopt the delegation or injection | |
| 162 patterns. The lower-level directory will define some interface that represents t he | |
| 163 "I need this URL" request, and then elsewhere, in a directory allowed to depend | |
| 164 on `//net/url_request`, an implementation of that interface/delegate that uses | |
| 165 `//net/url_request` is implemented. | |
| 166 | |
| 167 ### Specs: What Are They Good For | |
| 168 | |
| 169 As `//net` is used as the basis for a number of browsers, it's an important part | |
| 170 of the design philosophy to ensure behaviours are well-specified, and that the | |
| 171 implementation conforms to those specifications. This may be seen as burdensome | |
| 172 when it's unclear whether or not a feature will 'take off,' but it's equally | |
| 173 critical to ensure that the Chromium projects do not fork the Web Platform. | |
| 174 | |
| 175 #### Incubation Is Required | |
| 176 | |
| 177 `//net` respects Chromium's overall position of [incubation first](https://group s.google.com/a/chromium.org/d/msg/blink-dev/PJ_E04kcFb8/baiLN3DTBgAJ) standards development. | |
| 178 | |
| 179 With an incubation first approach, before introducing any new features that | |
| 180 might be exposed over the wire to servers, whether they be explicit behaviours | |
| 181 such as adding new headers to implicit behaviours such as | |
| 182 [Happy Eyeballs](https://tools.ietf.org/html/rfc6555), should have some form | |
| 183 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
| |
| 184 track, although the expectation is that the specification SHOULD have a direct | |
| 185 path towards an appropriate standards track, and moving towards that. Features | |
| 186 which don't adhere to this pattern require a number of high-level approvals, to | |
| 187 ensure that the Platform doesn't fragment. | |
| 188 | |
| 189 #### Introducing New Headers | |
| 190 | |
| 191 A common form of feature request is the introduction of new headers, either via | |
| 192 the `//net` implementation directly, or through consuming `//net` interfaces | |
| 193 and modifying headers on the fly. | |
| 194 | |
| 195 The introduction of any additional headers SHOULD have an incubated spec attache d, | |
| 196 ideally with cross-vendor interest. Particularly, headers which only apply to | |
| 197 Google or Google services are very likely to be rejected outright. | |
| 198 | |
| 199 #### Making Additional Requests | |
| 200 | |
| 201 While it's necessary to provide abstraction around `//net/url_request` for | |
| 202 any lower-level components that may need to make additional requests, for most | |
| 203 features, that's not all that is necessary. Because `//net/url_request` only | |
| 204 provides a basic HTTP fetching mechanism, it's insufficient for any Web Platform | |
| 205 feature, because it doesn't consider the broader platform concerns such as | |
| 206 interactions with CORS, Service Workers, cookie and authentication policies, or | |
| 207 even basic interactions with optional features like Extensions or SafeBrowsing. | |
| 208 | |
| 209 To account for all of these things, any resource fetching that is to support a | |
| 210 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
| |
| 211 feature should be explainable in terms of the | |
| 212 [Fetch Living Standard](https://fetch.spec.whatwg.org/). The Fetch standard defi nes | |
| 213 a JavaScript API for fetching resources, but more importantly, defines a common | |
| 214 set of infrastructure and terminology that tries to define how all resource load s | |
| 215 in the Web Platform happen - whether it be through the JavaScript API, through | |
| 216 `XMLHttpRequest`, or the `src` attribute in HTML tags, for example. | |
| 217 | |
| 218 Thus, in addition to defining a class abstraction to "fetch resources," any feat ure | |
| 219 proposal that needs to do so, and will be part of any of the browser projects, | |
| 220 should be "explained in terms of Fetch". | |
| 221 | |
| 222 ## Implementation | |
| 223 | |
| 224 In general, prior to implementing, try to get a review on net-dev@chromium.org | |
| 225 for the general feedback and design review. | |
| 226 | |
| 227 In addition to the net-dev@chromium.org early review, `//net` requires that any | |
| 228 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
| |
| 229 [Blink Process](https://www.chromium.org/blink#new-features), which includes an | |
| 230 "Intent to Implement" message to blink-dev@chromium.org | |
| 231 | |
| 232 For features that are unclear about their future, such as experiments or trials, | |
| 233 it's also expected that the design planning will also account for how features | |
| 234 will be removed cleanly. For features that radically affect the architecture of | |
| 235 `//net`, expect a high bar of justification, since reversing those changes if | |
| 236 it fails to pan out can cause significant disruption to productivity and | |
| 237 stability. | |
| 238 | |
| 239 ## Deprecation | |
| 240 | |
| 241 Plan for obsolence, hope for success. Similar to implementation, features that | |
| 242 are to be removed should also go through the | |
| 243 [Blink Process](https://www.chromium.org/blink#TOC-Web-Platform-Changes:-Process ) | |
| 244 for removing features. | |
| 245 | |
| 246 Note that due to the diversity of [Supported Projects](supported-projects.md), | |
| 247 removing a feature while minimizing disruption can be just as complex as adding | |
| 248 a feature. For example, relying solely on __User Metrics (UMA)__ to signal the | |
| 249 safety of removing a feature may not consider all projects, and relying on | |
| 250 __Field Trials (Finch)__ to assess risk or restore the 'legacy' behaviour may no t | |
| 251 work on all projects either. | |
| 252 | |
| 253 It's precisely because of these challenges that there's such a high bar for | |
| 254 adding features, because they may represent multi-year committments to support, | |
| 255 even when the feature itself is deprecated or targetted for removal. | |
| OLD | NEW |