Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1329)

Unified Diff: net/docs/life-of-a-feature.md

Issue 2700673002: [net] Add documentation for feature process & supported platforms (Closed)
Patch Set: Created 3 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | net/docs/supported-projects.md » ('j') | net/docs/supported-projects.md » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
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.
« no previous file with comments | « no previous file | net/docs/supported-projects.md » ('j') | net/docs/supported-projects.md » ('J')

Powered by Google App Engine
This is Rietveld 408576698