[net] Add documentation for feature process & supported platforms

net/docs/life-of-a-feature.md

+# 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,

[xunjieli, 2017/02/16 21:11:25]

nit: s/accomodating/accommodating

+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`

[xunjieli, 2017/02/16 21:11:25]

optional nit: s/behaviour/behavior  assuming there is a guideline to adhere to
American English.

(I see usage of "favor" instead of "favour" below.)

+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? 

[xunjieli, 2017/02/16 21:11:25]

optional nit: s/targetted/targeted if there is a guideline on preferring
American English over British English

+
+## 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.

[Randy Smith (Not in Mondays), 2017/02/16 21:07:23]

I think an abstract example or two would be useful here.

[Ryan Sleevi, 2017/02/16 23:29:33]

I'm going to leave this for you to fill in, as I'm not quite sure what you have
in mind here.

[Randy Smith (Not in Mondays), 2017/02/17 18:01:09]

That's fine; happy to do that in a follow-on CL.  Mostly I just want some
concrete, simple example of a product that's used dependency injection, with an
explanation (of less than a full sentence) as to why that was better done
outside of /net than inside.

I feel like we often describe issues in the abstract, and often people who
aren't grounded in the details don't follow and feel like we're hand-waving or
obfuscating, so I want to bias towards simple examples of the general cases we
raise.

[Ryan Sleevi, 2017/02/17 18:07:17]

Do you feel like those examples belong in this doc? Versus, say code-patterns?

At least understanding this perspective seems important to figure out what next
steps - and whether this doc is meeting the original set of concerns you had,
and whether this doc will end up encompassing too much?

[Randy Smith (Not in Mondays), 2017/02/17 18:27:25]

In general, what I want in this doc is to have enough example backup s.t.
someone reading only this doc without other context a) is likely to understand
what it's saying, and b) has some sense as to why we're not just being
arbitrary.  I don't actually think that'll take a lot more text than what's
already in the doc.

In specific, this strikes me as being a pretty low priority case (because there
are examples in the next two sections) and I wouldn't mourn particularly long if
there was no example here. 
There are places in the doc I feel strongly about adding a bit extra context,
and places I don't.  This is a "would be nice" place from my perspective; the
others matter more to me.  I hope I'm (in aggregate) answering your question.

[Ryan Sleevi, 2017/02/17 22:44:54]

I see. I think I had a very different mental image for this doc, and was
interested in secondary passes on how to pare-down this document and restructure
it to reduce, rather than add.

That is, my hope was that this document would largely focus on principles -
which would largely spell out (seemingly by fiat) - the general concern with
introducing new features and the common pitfall that energetic proposals face -
but that the "how" would be left to other documents.

Even the discussion of lifetime feels misplaced in this document (beyond a few
sentences), with much of the "why" and "how" best left for secondary documents
(that still need to be done).

Similarly, the inclusion of "Injection" and "Delegation" in this document were
mostly intended as a placeholder to separate these out into a more fleshed out
design explaining how our APIs best serve other consumers.

That is, mentally I imagined this document being "There are many consumers (see
this doc for who) and we need to satisfy their needs (see this doc for how),
which is why features are more challenging to introduce"

+
+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

[Randy Smith (Not in Mondays), 2017/02/16 21:07:23]

Suggestion: "it" -> "an instantiation of the implementation or parameter class".

+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.

[Randy Smith (Not in Mondays), 2017/02/16 21:07:23]

Hmmm.  I'd imagine we'd want to move away from this over time.  Maybe eliminate
"While" and everything after the ,?

+
+#### 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

[xunjieli, 2017/02/16 21:11:25]

nit: should it be "delegate pattern"/"delegation pattern" rather than "delegated
pattern"?

+transferred, so that the lifetime and threading semantics are clear and
+unambiguous.
+
+That is, for a given class `Foo`, which has a `Foo::Delegate` interface
+defined to allow embedders to alter behaviour, prefer a constructor that
+is
+```
+explicit Foo(std::unique_ptr<Delegate> delegate);
+```
+so that it is clear that the lifetime of `delegate` is determined by
+`Foo`.
+
+The most notable example of the delegate pattern is `URLRequest::Delegate`.

[Randy Smith (Not in Mondays), 2017/02/16 21:07:23]

The most notable example violates the "ownership is transferred" pattern?  This
seems like a moderately large problem, at least from a documentation standpoint.

[Ryan Sleevi, 2017/02/16 23:29:33]

I'm not sure I agree. Does the expansion about "legacy vs preferred" help
clarify this for you?

[Randy Smith (Not in Mondays), 2017/02/17 18:01:09]

Can you give me a more direct pointer to the legacy vs. preferred discussion?  I
don't see it in the most recent "Delegation" section.

To expand on my comment: I think giving strong direction about transferring
ownership and then providing a canonical example that doesn't transfer ownership
will make people less likely to take the direction.  Not mentioning the conflict
in the documentation, IMO, will exacerbate this tendency. 

Having said all that, this isn't a blocker for this document, mostly because I
don't see how to fix it.  URLRequest::Delegate *is* the primary delegate
pattern, and it doesn't transfer ownership since the delegate is usually the
owner of the URLRequest.   I see two ways to address this problem, neither
ideal, so I'll toss them out as suggestions and leave up to you whether/which to
take:

* Document the "Delegate is guaranteed to outlive object using delegate"
pattern, and link that explanation to the URLRequest::Delegate description.

* Acknowledge that the URLRequest::Delegate pattern doesn't match what you're
saying, and put in a sentence as to why it shouldn't be the model for other
delegates ("Do as I say, not as I do" :-}).

[Ryan Sleevi, 2017/02/17 18:07:17]

I see the point of confusion. It's in the bit immediately proceeding, under
Injection.

To accommodate your concerns, it seems the options are:
- Repeat that admonition in Delegation
- Move it to its own section
- Say nothing

Saying nothing seems the worst of all outcomes, move it to its own section seems
wrong because I'm not sure it justifies itself as a top-level document, so that
leaves repeating the admonition.
Right, which Injection covers. I'm curious if you have thoughts on how to share
that guidance here

[Randy Smith (Not in Mondays), 2017/02/17 18:27:25]

I'm torn here, just because in the process of this discussion I've become
semi-convinced that the URLRequest::Delegate pattern is actually a valid one,
just one that's not likely to be repeated in the future.  (I'd be curious if you
agree with this evaluation).

I'm good with repeating the admonition, though I'd sorta like it to be a form
that acknowledges why URLRequest::Delegate is special.  Maybe 

"(URLRequest::Delegate does not follow the ownership transfer model because the
URLRequest::Delegate is usually the owner of the URLRequest and hence guaranteed
to outlive the URLRequest.  Most other delegates are retained for the system
lifetime, so linking their lifetimes to the network stack through ownership is
the easiest way to avoid destruction races.)"

This is reflective of my sense that the single biggest issue we have with code
reviews is getting contributors to take lifetime issues as seriously as we do,
so I don't consider extra words giving the context for why we care about
lifetime issues wasted.  Even at a document at this high a level.
Injection covers the "what"; it doesn't cover the "why".  I think some "why" is
a reasonable and useful feature of this doc.

[Ryan Sleevi, 2017/02/17 22:44:54]

I actually don't, but I also recognize how significant a task it would be to fix
it, and I'm not that strongly convinced it's that bad as to sign up for that
task :)
Fair enough, I've tried to expand on this point while leaving the subject of our
disagreement out ;)

+
+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
+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. When `//net` takes a
+callback (e.g. `net::CompletionCallback`), the intended pattern is to
+signal the asynchronous completion of a single method, invoking that
+callback at most once before deallocating it. For more discussion
+of these patterns, see [Code Patterns](code-patterns.md).
+
+### 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

[xunjieli, 2017/02/16 21:11:25]

nit: s/wanted/wants since the rest of the paragraph is in the present tense.

+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.

[Randy Smith (Not in Mondays), 2017/02/16 21:07:22]

I feel like this needs to be fleshed out some; this paragraph doesn't really
sound like it's breaking the dependency.

In the SDCH case the notification was "Saw this header", and the embedder
(through its instantiation of the SdchOwner class) could deal with that however
it wanted; //net had no dependency on the results of its notification.  This is
a special case having to do with SDCH, but I think it reflects a general truth:
Conceptually, the dependency needs to be expressed in a fashion that can be cut
(e.g. in a race with shutdown).  That'll be feature specific, but I think we
should at least gesture at the conceptual pattern that the feature needs to try
and implement, and how that'll map to the implementation.

[Ryan Sleevi, 2017/02/16 23:29:33]

I'm not sure I really understand this feedback, or how to make it actionable. Do
you have alternative suggestions?

I'm not sure I'd agree that breaking circular dependencies is feature-specific;
it's perhaps the most common flaw in the feature proposals I've seen?

[Randy Smith (Not in Mondays), 2017/02/17 18:01:09]

Let's talk about this in our VC today.  Not blocking for landing this CL, but I
think adding in the fleshing out I'm requesting would aid the reader
(potentially a lot) in making this recommendation useful.

+
+### Understanding the Lifetimes
+
+Understanding the object lifetime and dependency graph can be one of the largest
+challenges to contributing and maintaining `//net`. As a consequence, features
+which require introducing more complexity to the lifetimes of objects generally
+have a greater challenge to acceptance.
+
+When designing features, features which model object lifetimes as a hierarchal

[xunjieli, 2017/02/16 21:11:25]

nit: consider revising the subject of this sentence.
"When designing features, features..." seems to imply that features are
designing features.

[xunjieli, 2017/02/16 21:11:25]

I slightly prefer "hierachical" over "hierarchal." Google says both have the
same meaning. Though the former has more usage.

+DAG are much more likely to be reviewed and accepted than features which rely on
+the use of either reference counting or weak pointers to maintain ownership.

[Randy Smith (Not in Mondays), 2017/02/16 21:07:23]

I think that we need to writeup, somewhere, a justification for this bias.  It
probably doesn't need to be anything too complex (lots of asynchronous I/O
floating around, can't delay shutdown, need to be sure that shutdown doesn't
result in use-after-free, thus care a *lot* about clear lifetimes), but I think
we'll get a lot more with a kind word ^W^W explanation and a gun ^W directive
than we would with the directive alone.

[Ryan Sleevi, 2017/02/16 23:29:33]

I'm also not sure how to interpret this comment, and whether you see it as a
blocker for this document.

It sounds like you're wanting more expansion on lifetime issues, which I agree
would be good, but that likely belongs in the code-patterns document to fully
capture these.

[Randy Smith (Not in Mondays), 2017/02/17 18:01:09]

Not a blocker.  This document is already solidly in the "Better off with it than
without it" category.
Yes and no.  For this issue, I want a bit more justification right here, so the
hypothetical future reader doesn't just have the reaction that we're being
restrictive and annoying for its own sake.  Let me toss a sample addendum to
this paragraph at you, which you can use, modify, or ignore as you prefer, and
let's also talk about it in our VC:

"The network stack has much more asynchronous code than most of Chromium, which
leads to a greater vulnerability to use-after-free errors.  This has resulted in
a much stronger bias within the network stack for precise, well defined lifetime
semantics, which are most easily generated by hierarchical ownership
relationships."

+
+In addition to preferring explicit lifetimes, such as through judicious use of
+`std::unique_ptr<>` to indicate ownership transfer of dependencies, many
+features in `//net` also expect that if a `base::Callback` is involved (which
+includes `net::CompletionCallback`), then it's possible that invoking the
+callback may result in the deletion of the current (calling) object. As
+further expanded upon in [Code Patterns](code-patterns.md), features and
+changes should be designed such that any callback invocation is the last
+bit of code executed, and that the callback is accessed via the stack (such
+as through the use of either `base::ResetAndReturn(callback_).Run()` or
+`std::move(callback_).Run()`.
+
+### 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

[xunjieli, 2017/02/16 21:11:25]

nit: s/to/or

whether explicit behaviors [or] implicit behaviors?

+[Happy Eyeballs](https://tools.ietf.org/html/rfc6555), should have some form
+of specification written. That specification should at least be on an
+incubation track, and the expectation is that the specification should have a
+direct path to an appropriate standards track. Features which don't adhere to
+this pattern, or which are not making progress towars a standards track, will

[xunjieli, 2017/02/16 21:11:25]

nit: s/towars/towards

+require a high-level approvals, to ensure that the Platform doesn't fragment.

[xunjieli, 2017/02/16 21:11:25]

nit: remove 'a'

+
+#### 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 feature of the Web Platform, whether because the resource will be directly
+exposed to web content (for example, an image load or prefetch) or because it
+is in response to invoking a Web Platform API (for example, invoking the
+credential management API), the feature's resource fetching 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.
+
+This also includes any resource fetching that wishes to use the same socket
+pools or caches as the Web Platform, to ensure that every resource that is
+web exposed (directly or indirectly) was fetched in a consistent and

[xunjieli, 2017/02/16 21:11:25]

nit: s/was/is

+well-documented way, thus minimizing platform fragmentation and security
+issues.
+
+There are exceptions to this, however, but they're generally few and far
+between. In general, any feature that needs to define an abstraction to
+allow it to "fetch resources," likely needs to also be "explained in terms
+of Fetch".

[Randy Smith (Not in Mondays), 2017/02/16 21:07:22]

I think this needs to have some implementation backup about how to implement
things once you've explained them in terms of fetch.  My (general, vague,
nervous) feeling is that if it's explainable in terms of fetch it almost has to
be done from the renderer to take advantage of the large start of things that
implement much of the fetch semantics.  If that's not true, or partially true, I
think we need to be very clear about what one does once one has explained a
feature in terms of fetch.

Obviously, the discussion on the //net API doc is very relevant here.

[Ryan Sleevi, 2017/02/16 23:29:33]

While I agree, I think that belongs in something like code-patterns. My point in
highlighting here is that this is a necessary condition to progressing if doing
something with such a dependency, not that it's sufficient :)

[Randy Smith (Not in Mondays), 2017/02/17 18:01:09]

Yep, I'm completely with you.  We need to flesh this out, but right here is
probably not the right place.  Non-blocking, will continue to iterate elsewhere.

+
+## 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
+[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

[xunjieli, 2017/02/16 21:11:25]

nit: they fail to pan out
assuming "it" means "those changes"

+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,

[xunjieli, 2017/02/16 21:11:25]

nit: s/committments/commitments

+even when the feature itself is deprecated or targetted for removal.