Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2700673002:
[net] Add documentation for feature process & supported platforms (Closed)
|
Created:
3 years, 10 months ago by Ryan Sleevi Modified:
3 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[net] Add documentation for feature process & supported platforms
As //net is used in a variety of "Not Chrome (the Browser)"
cases, add documentation to //net/docs to help enumerate the
various supported projects using //net and their capabilities
as it relates to feature design/support.
In addition, document a bit more about how to a //net feature
lives, including common design patterns that tend to trip up
folks proposing features for //net. This is just an initial
draft to be iterated and refined on.
BUG=none
TEST=none
Review-Url: https://codereview.chromium.org/2700673002
Cr-Commit-Position: refs/heads/master@{#451795}
Committed: https://chromium.googlesource.com/chromium/src/+/7390e5594fb6f8468359b519580dacb29fdf269b
Patch Set 1 #
Total comments: 19
Patch Set 2 : Feedback #
Total comments: 37
Patch Set 3 : More feedback #Patch Set 4 : Feedback #
Total comments: 1
Patch Set 5 : Feedback #Messages
Total messages: 24 (5 generated)
rsleevi@chromium.org changed reviewers: + csharrison@chromium.org, mef@chromium.org, rdsmith@chromium.org
Randy, Charlie, Misha: Could y'all give this a once-over for consistency? We can always expand, but I wanted to make sure I got the general principles right. https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects.md File net/docs/supported-projects.md (right): https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects... net/docs/supported-projects.md:99: ## Cronet +mef: Could you review this for its capabilities and expand if necessary?
https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects.md File net/docs/supported-projects.md (right): https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects... net/docs/supported-projects.md:99: ## Cronet On 2017/02/16 00:58:30, Ryan Sleevi wrote: > +mef: Could you review this for its capabilities and expand if necessary? Could you elaborate on 'capabilities'? https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects... net/docs/supported-projects.md:112: * **Release Frequency**: Varies Builds follow Chrome release schedule with daily Canary, Dev, Beta and Stable channel builds matching Chrome for Android and iOS. There are no official release availability for general consumers. https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects... net/docs/supported-projects.md:114: * **Field Trials (Finch)**: No There are 'experimental options' that consumers can configure based on their own experiment framework. https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects... net/docs/supported-projects.md:117: * __Yes__: First-party (Google) projects Proto-serialized UMA data is exposed via getGlobalMetricsDeltas() and First-party apps have established pipeline to feed that into UMA server. This doesn't preclude other consumers from pulling same data for separate processing.
https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects.md File net/docs/supported-projects.md (right): https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects... net/docs/supported-projects.md:99: ## Cronet On 2017/02/16 18:00:00, mef wrote: > On 2017/02/16 00:58:30, Ryan Sleevi wrote: > > +mef: Could you review this for its capabilities and expand if necessary? > > Could you elaborate on 'capabilities'? The points below and making sure I got them right https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects... net/docs/supported-projects.md:112: * **Release Frequency**: Varies On 2017/02/16 18:00:00, mef wrote: > Builds follow Chrome release schedule with daily Canary, Dev, Beta and Stable > channel builds matching Chrome for Android and iOS. > There are no official release availability for general consumers. Well, my understanding is that a build being available doesn't mean it's integrated into a consuming product for potentially quite some time, is that a fair statement? If so, the "When does this thing X that I landed here end up affecting products" is ... an "it depends"? https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects... net/docs/supported-projects.md:114: * **Field Trials (Finch)**: No On 2017/02/16 18:00:00, mef wrote: > There are 'experimental options' that consumers can configure based on their own > experiment framework. Do those options use base::FieldTrial? My understanding is that no, it was a series of explicit bools at the top-level interface. https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects... net/docs/supported-projects.md:117: * __Yes__: First-party (Google) projects On 2017/02/16 18:00:00, mef wrote: > Proto-serialized UMA data is exposed via getGlobalMetricsDeltas() and > First-party apps have established pipeline to feed that into UMA server. This > doesn't preclude other consumers from pulling same data for separate processing. No, but it does preclude other consumers from sending those to Google's UMA dashboard, correct?
https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects.md File net/docs/supported-projects.md (right): https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects... net/docs/supported-projects.md:99: ## Cronet On 2017/02/16 18:03:55, Ryan Sleevi wrote: > On 2017/02/16 18:00:00, mef wrote: > > On 2017/02/16 00:58:30, Ryan Sleevi wrote: > > > +mef: Could you review this for its capabilities and expand if necessary? > > > > Could you elaborate on 'capabilities'? > > The points below and making sure I got them right Acknowledged. https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects... net/docs/supported-projects.md:112: * **Release Frequency**: Varies On 2017/02/16 18:03:55, Ryan Sleevi wrote: > On 2017/02/16 18:00:00, mef wrote: > > Builds follow Chrome release schedule with daily Canary, Dev, Beta and Stable > > channel builds matching Chrome for Android and iOS. > > There are no official release availability for general consumers. > > Well, my understanding is that a build being available doesn't mean it's > integrated into a consuming product for potentially quite some time, is that a > fair statement? If so, the "When does this thing X that I landed here end up > affecting products" is ... an "it depends"? Correct. We generally make Dev channel builds available to internal consumers on a weekly basis, but after that individual products follow their own release schedule which 'Varies'. https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects... net/docs/supported-projects.md:114: * **Field Trials (Finch)**: No On 2017/02/16 18:03:55, Ryan Sleevi wrote: > On 2017/02/16 18:00:00, mef wrote: > > There are 'experimental options' that consumers can configure based on their > own > > experiment framework. > > Do those options use base::FieldTrial? My understanding is that no, it was a > series of explicit bools at the top-level interface. That's correct, those don't use base::FieldTrial. https://codereview.chromium.org/2700673002/diff/1/net/docs/supported-projects... net/docs/supported-projects.md:117: * __Yes__: First-party (Google) projects On 2017/02/16 18:03:55, Ryan Sleevi wrote: > On 2017/02/16 18:00:00, mef wrote: > > Proto-serialized UMA data is exposed via getGlobalMetricsDeltas() and > > First-party apps have established pipeline to feed that into UMA server. This > > doesn't preclude other consumers from pulling same data for separate > processing. > > No, but it does preclude other consumers from sending those to Google's UMA > dashboard, correct? I don't know, I was under impression that UMA is limited to first party. To be clear, first party apps don't use the Chrome UMA mechanism to send data to Google neither.
On 2017/02/16 18:11:13, mef wrote: > I don't know, I was under impression that UMA is limited to first party. > To be clear, first party apps don't use the Chrome UMA mechanism to send data to > Google neither. OK, then it perhaps might be better to clarify here that "It varies" Certainly, you can't use Chrome UMA to measure the compat risk of a change / feature for Cronet consumers, but even with the Cronet case, you may have to look across a variety of products and dashboards rather than the main UMA dashboard - is that a fair statement?
Some initial comments. Many thanks for writing this up! https://codereview.chromium.org/2700673002/diff/1/net/docs/life-of-a-feature.md File net/docs/life-of-a-feature.md (right): https://codereview.chromium.org/2700673002/diff/1/net/docs/life-of-a-feature.... net/docs/life-of-a-feature.md:108: transferred to the thing consuming the delegate, so that the lifetime and Can you be more specific about the "thing consuming the delegate"? https://codereview.chromium.org/2700673002/diff/1/net/docs/life-of-a-feature.... net/docs/life-of-a-feature.md:114: delegation, the `//net` layer tries to eschew any callbacks that can be might want to mention e.g. base::OnceCallback if you are advocating that (I know it is relatively new) https://codereview.chromium.org/2700673002/diff/1/net/docs/life-of-a-feature.... net/docs/life-of-a-feature.md:114: delegation, the `//net` layer tries to eschew any callbacks that can be might want to mention e.g. base::OnceCallback if you are advocating that (I know it is relatively new) https://codereview.chromium.org/2700673002/diff/1/net/docs/life-of-a-feature.... net/docs/life-of-a-feature.md:183: of specification written. That specification should at least be incubation This sentence is a bit of a run-on. Suggest slight rewording and cutting out the last clause. "That specification should at least be on an incubation track, and the expectation is that the the specification should have a direct path towards an appropriate standards track." https://codereview.chromium.org/2700673002/diff/1/net/docs/life-of-a-feature.... net/docs/life-of-a-feature.md:210: Web Platform feature (i.e. be exposed in any of the Browser projects), the The "i.e." confuses me, and I think it is due to the word "exposed". Can you clarify this a bit more, I think it's a very important point. You may want to explicitly *include* examples of things which don't need to be explained in terms of Fetch. https://codereview.chromium.org/2700673002/diff/1/net/docs/life-of-a-feature.... net/docs/life-of-a-feature.md:228: browser-exposed behaviour should also adhere to the ditto, I think this should probably be "Web Platform exposed".
On 2017/02/16 18:14:51, Ryan Sleevi wrote: > On 2017/02/16 18:11:13, mef wrote: > > I don't know, I was under impression that UMA is limited to first party. > > To be clear, first party apps don't use the Chrome UMA mechanism to send data > to > > Google neither. > > OK, then it perhaps might be better to clarify here that "It varies" sgtm > > Certainly, you can't use Chrome UMA to measure the compat risk of a change / > feature for Cronet consumers, but even with the Cronet case, you may have to > look across a variety of products and dashboards rather than the main UMA > dashboard - is that a fair statement? Yes, absolutely.
Thanks for the feedback so far. Another set of updates, including a brief discussion of lifetimes. I'm trying to figure out what stuff should be punted into code-patterns, and that probably needs some love and expansion, but given how frequently it trips up design docs, I included a small blurb about lifetime. Charlie: I didn't end up explicitly mentioning base::OnceCallback, since //net really doesn't use it (and in general prefers net::CompletionCallback). I tried to expand on the "Web Platform" stuff, but my hope is that once Randy's doc is landed, more of that nuance about 'explain in terms of fetch' can be shifted into that document.
These comments were made before reading other folks comments, and specifically your most recent comment about code-patterns and the API document. You should read the "go into more detail about X" as meaning "We need to go into more detail about X *somewhere*, and I'm not actually saying it has to be in this document". https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... File net/docs/life-of-a-feature.md (right): https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:74: injection. I think an abstract example or two would be useful here. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:86: implementation, but requiring the `//net` embedder to supply it (perhaps Suggestion: "it" -> "an instantiation of the implementation or parameter class". https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:101: injected object is passed around as a naked/raw pointer. Hmmm. I'd imagine we'd want to move away from this over time. Maybe eliminate "While" and everything after the ,? https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:120: The most notable example of the delegate pattern is `URLRequest::Delegate`. The most notable example violates the "ownership is transferred" pattern? This seems like a moderately large problem, at least from a documentation standpoint. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:178: `//net/url_request` is implemented. 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. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:189: the use of either reference counting or weak pointers to maintain ownership. 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. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:265: of Fetch". 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. https://codereview.chromium.org/2700673002/diff/20001/net/docs/supported-proj... File net/docs/supported-projects.md (right): https://codereview.chromium.org/2700673002/diff/20001/net/docs/supported-proj... net/docs/supported-projects.md:89: [content_shell](../../content/shell) test framework. Opera?
drive-by -- Please ignore the nits if not applicable. It's a great read. Thanks for writing these up. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... File net/docs/life-of-a-feature.md (right): https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:5: Chromium browser to accomodating a wide variety of networking needs, nit: s/accomodating/accommodating https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:22: a `//net` behaviour, it's first necessary to understand where `//net` optional nit: s/behaviour/behavior assuming there is a guideline to adhere to American English. (I see usage of "favor" instead of "favour" below.) https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:46: targetted projects and/or platforms? optional nit: s/targetted/targeted if there is a guideline on preferring American English over British English https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:107: when using the delegated pattern, ownership of the delegate should be nit: should it be "delegate pattern"/"delegation pattern" rather than "delegated pattern"? https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:151: For example, a feature that wanted to associate the low-level network socket nit: s/wanted/wants since the rest of the paragraph is in the present tense. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:187: When designing features, features which model object lifetimes as a hierarchal nit: consider revising the subject of this sentence. "When designing features, features..." seems to imply that features are designing features. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:187: When designing features, features which model object lifetimes as a hierarchal I slightly prefer "hierachical" over "hierarchal." Google says both have the same meaning. Though the former has more usage. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:216: such as adding new headers to implicit behaviours such as nit: s/to/or whether explicit behaviors [or] implicit behaviors? https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:221: this pattern, or which are not making progress towars a standards track, will nit: s/towars/towards https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:222: require a high-level approvals, to ensure that the Platform doesn't fragment. nit: remove 'a' https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:258: web exposed (directly or indirectly) was fetched in a consistent and nit: s/was/is https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:281: it fails to pan out can cause significant disruption to productivity and nit: they fail to pan out assuming "it" means "those changes" https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:299: adding features, because they may represent multi-year committments to support, nit: s/committments/commitments
Thanks for the great feedback all! Randy, I'm not sure how to make a few of your comments actionable, and I'm not sure if they're blockers (meaning you think this is bad if we submit) or whether they're suggestions for future improvements. I want to try to get the basics down, and then allow everyone to expand as they see appropriate to cover that. I also think a lot of the "How" belongs in code-patterns, but capturing the high-level idea of "Here's all the ways in which it'll feel like you're getting speed-bumped by //net OWNERs" early, to point people to and guide them through things, is still a good thing. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... File net/docs/life-of-a-feature.md (right): https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:74: injection. On 2017/02/16 21:07:23, Randy Smith (Not in Mondays) wrote: > I think an abstract example or two would be useful here. I'm going to leave this for you to fill in, as I'm not quite sure what you have in mind here. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:120: The most notable example of the delegate pattern is `URLRequest::Delegate`. On 2017/02/16 21:07:23, Randy Smith (Not in Mondays) wrote: > The most notable example violates the "ownership is transferred" pattern? This > seems like a moderately large problem, at least from a documentation standpoint. I'm not sure I agree. Does the expansion about "legacy vs preferred" help clarify this for you? https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:178: `//net/url_request` is implemented. On 2017/02/16 21:07:22, Randy Smith (Not in Mondays) wrote: > 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. 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? https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:189: the use of either reference counting or weak pointers to maintain ownership. On 2017/02/16 21:07:23, Randy Smith (Not in Mondays) wrote: > 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. 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. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:265: of Fetch". On 2017/02/16 21:07:22, Randy Smith (Not in Mondays) wrote: > 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. 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 :)
My apologies, Ryan--you're quite right that my comments would have been more useful with wording and indication of whether or not I felt like they were blocking. Hopefully I've remedied those lacks below. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... File net/docs/life-of-a-feature.md (right): https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:74: injection. On 2017/02/16 23:29:33, Ryan Sleevi wrote: > On 2017/02/16 21:07:23, Randy Smith (Not in Mondays) wrote: > > I think an abstract example or two would be useful here. > > I'm going to leave this for you to fill in, as I'm not quite sure what you have > in mind here. 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. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:120: The most notable example of the delegate pattern is `URLRequest::Delegate`. On 2017/02/16 23:29:33, Ryan Sleevi wrote: > On 2017/02/16 21:07:23, Randy Smith (Not in Mondays) wrote: > > The most notable example violates the "ownership is transferred" pattern? > This > > seems like a moderately large problem, at least from a documentation > standpoint. > > I'm not sure I agree. Does the expansion about "legacy vs preferred" help > clarify this for you? 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" :-}). https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:178: `//net/url_request` is implemented. On 2017/02/16 23:29:33, Ryan Sleevi wrote: > On 2017/02/16 21:07:22, Randy Smith (Not in Mondays) wrote: > > 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. > > 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? 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. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:189: the use of either reference counting or weak pointers to maintain ownership. On 2017/02/16 23:29:33, Ryan Sleevi wrote: > On 2017/02/16 21:07:23, Randy Smith (Not in Mondays) wrote: > > 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. > > I'm also not sure how to interpret this comment, and whether you see it as a > blocker for this document. Not a blocker. This document is already solidly in the "Better off with it than without it" category. > 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. 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." https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:265: of Fetch". On 2017/02/16 23:29:33, Ryan Sleevi wrote: > On 2017/02/16 21:07:22, Randy Smith (Not in Mondays) wrote: > > 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. > > 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 :) 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.
A few last comments to see if we can sort out the lifetime remarks, and then I'll go ahead and land. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... File net/docs/life-of-a-feature.md (right): https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:74: injection. On 2017/02/17 18:01:09, Randy Smith (Not in Mondays) wrote: > On 2017/02/16 23:29:33, Ryan Sleevi wrote: > > On 2017/02/16 21:07:23, Randy Smith (Not in Mondays) wrote: > > > I think an abstract example or two would be useful here. > > > > I'm going to leave this for you to fill in, as I'm not quite sure what you > have > > in mind here. > > 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. 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? https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:120: The most notable example of the delegate pattern is `URLRequest::Delegate`. On 2017/02/17 18:01:09, Randy Smith (Not in Mondays) wrote: > 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. 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. > * 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" :-}). Right, which Injection covers. I'm curious if you have thoughts on how to share that guidance here
https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... File net/docs/life-of-a-feature.md (right): https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:74: injection. On 2017/02/17 18:07:17, Ryan Sleevi wrote: > On 2017/02/17 18:01:09, Randy Smith (Not in Mondays) wrote: > > On 2017/02/16 23:29:33, Ryan Sleevi wrote: > > > On 2017/02/16 21:07:23, Randy Smith (Not in Mondays) wrote: > > > > I think an abstract example or two would be useful here. > > > > > > I'm going to leave this for you to fill in, as I'm not quite sure what you > > have > > > in mind here. > > > > 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. > > Do you feel like those examples belong in this doc? Versus, say code-patterns? 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. > 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? 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. https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:120: The most notable example of the delegate pattern is `URLRequest::Delegate`. On 2017/02/17 18:07:17, Ryan Sleevi wrote: > On 2017/02/17 18:01:09, Randy Smith (Not in Mondays) wrote: > > 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. > > 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. 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. > > > * 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" :-}). > > Right, which Injection covers. I'm curious if you have thoughts on how to share > that guidance here Injection covers the "what"; it doesn't cover the "why". I think some "why" is a reasonable and useful feature of this doc.
New version published. Randy and I also met over VC (and these comments were written before that VC, and largely covered on the VC, so Randy, you can ignore), but we spent time discussing the intended audience for this doc and the relative goals. To that end, we agreed that there's likely several documents that need to be produced/expanded that touch on the concerns: - Guidance for those inside and outside of //net of adding features/API surface to //net (which this doc tries to be) - Documentation about code patterns and practices (prescriptive) - Documentation about the collective knowledge about why the first two documents need to exist; in particular, "why this pattern" and "why should I care" that contextualizes the various requirements/guidelines - Documentation for those *consuming* //net APIs and wishing to surface those details (either as part of the Web Platform, to Google services, etc). Namely, what are the parts of the API that assume "Chromium stable" (e.g. subject to change without notice, so long as we //net owners can touch the whole codebase) and what are "Web Platform stable" (e.g. load-bearing parts of the Web platform that affect external behaviour in a way we need to be careful of) - Documentation for those *consuming* //net APIs and wanting to match specific code patterns to specific design requirements. This aligns moreso with the doc Randy sent to net-dev@ In the short-term, we agreed to land this doc in its present form. I agreed to try to expand more of the code patterns to provide more structure around that, both in implementation as well as things like layering and lifetimes. I'll also try to write up a doc about the collective "why" for those various bits of guidance, so that more of the communal knowledge is documented and agreed on. Put differently: - This document spells out what hoops to jump (in general) - Code Patterns spells out how to jump through the hoops (in implementation) - [Code History?] spells out WHY we jump through hoops - [API stability?] spells out when and where new hoops can be introduced - Randy's document spells out the types of hoop courses you may want to jump and how to train for them https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... File net/docs/life-of-a-feature.md (right): https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:74: injection. On 2017/02/17 18:27:25, Randy Smith (Not in Mondays) wrote: > 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. 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" https://codereview.chromium.org/2700673002/diff/20001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:120: The most notable example of the delegate pattern is `URLRequest::Delegate`. On 2017/02/17 18:27:25, Randy Smith (Not in Mondays) wrote: > 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 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 :) > 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. Fair enough, I've tried to expand on this point while leaving the subject of our disagreement out ;)
LGTM modulo nit. Thank you for Friday's conversation and all the effort you're putting into clarifying our patterns. https://codereview.chromium.org/2700673002/diff/60001/net/docs/life-of-a-feat... File net/docs/life-of-a-feature.md (right): https://codereview.chromium.org/2700673002/diff/60001/net/docs/life-of-a-feat... net/docs/life-of-a-feature.md:213: defined sequencing that cannot be ordered. To be ensure these requirements nit: I don't understand what "defined sequencing that cannot be ordered" means.
On 2017/02/21 17:01:04, Randy Smith (Not in Mondays) wrote: > LGTM modulo nit. Thank you for Friday's conversation and all the effort you're > putting into clarifying our patterns. > > https://codereview.chromium.org/2700673002/diff/60001/net/docs/life-of-a-feat... > File net/docs/life-of-a-feature.md (right): > > https://codereview.chromium.org/2700673002/diff/60001/net/docs/life-of-a-feat... > net/docs/life-of-a-feature.md:213: defined sequencing that cannot be ordered. To > be ensure these requirements > nit: I don't understand what "defined sequencing that cannot be ordered" means. Thanks! That was a typo to indicate "reordered" rather than "ordered" (e.g. to compare against TaskScheduler and its resequencing of events)
The CQ bit was checked by rsleevi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/2700673002/#ps80001 (title: "Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487697839875900,
"parent_rev": "31b2815487e8c7edcc7ae481cc3494887110ace3", "commit_rev":
"7390e5594fb6f8468359b519580dacb29fdf269b"}
Message was sent while issue was closed.
Description was changed from ========== [net] Add documentation for feature process & supported platforms As //net is used in a variety of "Not Chrome (the Browser)" cases, add documentation to //net/docs to help enumerate the various supported projects using //net and their capabilities as it relates to feature design/support. In addition, document a bit more about how to a //net feature lives, including common design patterns that tend to trip up folks proposing features for //net. This is just an initial draft to be iterated and refined on. BUG=none TEST=none ========== to ========== [net] Add documentation for feature process & supported platforms As //net is used in a variety of "Not Chrome (the Browser)" cases, add documentation to //net/docs to help enumerate the various supported projects using //net and their capabilities as it relates to feature design/support. In addition, document a bit more about how to a //net feature lives, including common design patterns that tend to trip up folks proposing features for //net. This is just an initial draft to be iterated and refined on. BUG=none TEST=none Review-Url: https://codereview.chromium.org/2700673002 Cr-Commit-Position: refs/heads/master@{#451795} Committed: https://chromium.googlesource.com/chromium/src/+/7390e5594fb6f8468359b519580d... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/7390e5594fb6f8468359b519580d... |
