Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1211003003:
net: Add Life of a URLRequest documentation. (Closed)
Descriptionnet: Add Life of a URLRequest documentation.
BUG=none
Committed: https://crrev.com/2ac8cb50a0c9d284b57db8f3de71a71d4a45c6e8
Cr-Commit-Position: refs/heads/master@{#338732}
Patch Set 1 #Patch Set 2 : Self-review #Patch Set 3 : Use single spaces after periods #
Total comments: 19
Patch Set 4 : Response to rdsmith's comments #Patch Set 5 : Spellcheck #
Total comments: 45
Patch Set 6 : Response to (most) comments #Patch Set 7 : Oops, forgot to save #
Total comments: 67
Patch Set 8 : Response to comments (And fix linebreak type) #Patch Set 9 : Inline overview, minor fixes #Patch Set 10 : Clarify QUIC section #
Total comments: 4
Patch Set 11 : Response to Randy's comments #
Total comments: 4
Patch Set 12 : Response to Helen's comments #Messages
Total messages: 33 (6 generated)
mmenke@chromium.org changed reviewers: + rdsmith@chromium.org
Chris suggested I land this basically as-is, since it's clear I'm not going to get around to make pretty pictures anytime soon. This is basically what I sent you as before, with an additional section or two, some cleanup, and a pretty detailed bulleted summary of the main section up top (Not sure if it adds much, currently).
So I'm torn. I believe that (at least in this case) any documentation is better than no documentation. So if you (at any point) ask me for a stamp for whatever you've got written, I'll give it to you. Having said that, I really feel like this suffers from the "Wall of Text" problem. It's a whole lot of information that isn't very broken down into digestible lumps, and doesn't have motivation, which would help for both understanding and against despair :-}. I've made some suggestions as to how to improve that (without pictures), but I don't know how much time you're willing to devote to this, so I haven't gone whole hog. I also had the feeling, as I read through the "Additional Topics" section, that it was both too detailed and not detailed enough, which suggests to me we're trying to put too much into this one document. Maybe simplifying this down with stub external docs to be filled in by content owners (yes, most of whom will still be you :-}) might make sense? https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:1: # Life of a URLRequest Could you add this doc (and maybe the other docs in this directory, if you are so moved) into net/net.gypi:net_docs_sources so that HTML gets automatically created in people's output directories? Arguably we don't need to do this (because source view auto-translates) but it does make reviewing easier. I also believe in consistency--i.e. we should do it for all docs or none. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:31: * The main URLRequestContext is mostly created in ProfileIOData, though it These sub-items aren't showing up as indented/second level list in the HTML. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:76: ## Overview I'd go for a (short!) paragraph before you dive into the bulleted list giving an even more abstract description for a request. Maybe something like: "A request for data is normally dispatched from the renderer to the browser process. There a URLRequest is created to represent it. An job specific to the protocol (e.g. HTTP) is attached to the request. That job first checks the cache then establishes a network connection object to actually fetch the data. That connection object interacts with network socket pools to potentially re-use sockets; the socket pools create and connect a socket if there is no appropriate existing socket. Once that socket exists, the HTTP request is dispatched, the response read and parsed, and the result returned back up the stack and sent over to the renderer. Of course, it's never quite that simple :-}." And then dive into the details. That'll give the reader a bit more understanding of the motivation for the different levels, and some sense of not being completely lost as they wander through those levels. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:82: * ResourceDispatcher sends an IPC to the ResourceDispatcherHost in the browser I'd add a line with some visual distinction after this one to call out the transition to the browser process. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:85: * ResourceDispatcherHost creates a ResourceLoader and ResourceHandler chain to In this line and the next one, I'd refer to the "URLRequest" rather than the "Request" to make clear the top-level concept versus lower level object distinction. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:89: ### Request is Issued I'd put some verbiage here indicating that what's below is the common case, but there may be variations at each level, usually at the "asks the X to create a Y, which is a Z" that sometimes it's not a Z but some other subclass of Y. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:89: ### Request is Issued This section is intimidating in the number of bullet items it has just hammering at the reader. (At least, it's intimidating to me, and I've repeatedly created this same bulleted list for my own education.) I'd break this into sub-sections, maybe URLRequest->HttpNetworkTransaction, HTTPNetworkTransaction->TransportClientSocketPool, TransportClientSocketPool->HttpStreamFactoryImpl::Job/HttpBasicStream. This might be done with nesting, to make unwinding easier, but if there's a clear way to have it in separate sections, that's somewhat better. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:91: * The URLRequest asks the URLRequestJobFactory to create a URLRequest[Http]Job. Especially if you're targeting this document at folks new to the network stack, I think "URLRequest[Http]Job" is too abbreviated a way of saying "A URLRequestJob. Usually, this will be the URLRequestJob subclass, URLRequestHttpJob", so I'd spell that out. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:135: ## Details I'd suggest breaking this section out into subsections; I think it'll be a lot more readable. Maybe "Process Architecture" for RD/RDH, "URLRequest Creation and Plumbing" for URLRequestContext->ResourceLoader/Handler, "Http Transactions" for the cache and network transactions, and maybe "Socket pools", "Http Parsing and Data Processing" for the rest. It would be really useful if this section was broken out in the same way as the above section (you don't need the labels on the above section, but if the reader has already bucketed the concepts in a certain way, they'll read this section more easily). https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:309: cache independently of the response that was compressed with it. nit: I'd leave out "from the cache" since SDCH implementation is, at least currently, partially memory based around SDCH-specific storage.
On 2015/07/08 20:36:25, rdsmith wrote: > So I'm torn. I believe that (at least in this case) any documentation is better > than no documentation. So if you (at any point) ask me for a stamp for whatever > you've got written, I'll give it to you. > > Having said that, I really feel like this suffers from the "Wall of Text" > problem. It's a whole lot of information that isn't very broken down into > digestible lumps, and doesn't have motivation, which would help for both > understanding and against despair :-}. I've made some suggestions as to how to > improve that (without pictures), but I don't know how much time you're willing > to devote to this, so I haven't gone whole hog. > > I also had the feeling, as I read through the "Additional Topics" section, that > it was both too detailed and not detailed enough, which suggests to me we're > trying to put too much into this one document. Maybe simplifying this down with > stub external docs to be filled in by content owners (yes, most of whom will > still be you :-}) might make sense? > > https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... > File net/docs/life-of-a-url-request.md (right): > > https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... > net/docs/life-of-a-url-request.md:1: # Life of a URLRequest > Could you add this doc (and maybe the other docs in this directory, if you are > so moved) into net/net.gypi:net_docs_sources so that HTML gets automatically > created in people's output directories? Arguably we don't need to do this > (because source view auto-translates) but it does make reviewing easier. I also > believe in consistency--i.e. we should do it for all docs or none. > > https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... > net/docs/life-of-a-url-request.md:31: * The main URLRequestContext is mostly > created in ProfileIOData, though it > These sub-items aren't showing up as indented/second level list in the HTML. > > https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... > net/docs/life-of-a-url-request.md:76: ## Overview > I'd go for a (short!) paragraph before you dive into the bulleted list giving an > even more abstract description for a request. Maybe something like: > > "A request for data is normally dispatched from the renderer to the browser > process. There a URLRequest is created to represent it. An job specific to the > protocol (e.g. HTTP) is attached to the request. That job first checks the > cache then establishes a network connection object to actually fetch the data. > That connection object interacts with network socket pools to potentially re-use > sockets; the socket pools create and connect a socket if there is no appropriate > existing socket. Once that socket exists, the HTTP request is dispatched, the > response read and parsed, and the result returned back up the stack and sent > over to the renderer. > > Of course, it's never quite that simple :-}." > > And then dive into the details. That'll give the reader a bit more > understanding of the motivation for the different levels, and some sense of not > being completely lost as they wander through those levels. > > https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... > net/docs/life-of-a-url-request.md:82: * ResourceDispatcher sends an IPC to the > ResourceDispatcherHost in the browser > I'd add a line with some visual distinction after this one to call out the > transition to the browser process. > > https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... > net/docs/life-of-a-url-request.md:85: * ResourceDispatcherHost creates a > ResourceLoader and ResourceHandler chain to > In this line and the next one, I'd refer to the "URLRequest" rather than the > "Request" to make clear the top-level concept versus lower level object > distinction. > > https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... > net/docs/life-of-a-url-request.md:89: ### Request is Issued > I'd put some verbiage here indicating that what's below is the common case, but > there may be variations at each level, usually at the "asks the X to create a Y, > which is a Z" that sometimes it's not a Z but some other subclass of Y. > > https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... > net/docs/life-of-a-url-request.md:89: ### Request is Issued > This section is intimidating in the number of bullet items it has just hammering > at the reader. (At least, it's intimidating to me, and I've repeatedly created > this same bulleted list for my own education.) I'd break this into > sub-sections, maybe URLRequest->HttpNetworkTransaction, > HTTPNetworkTransaction->TransportClientSocketPool, > TransportClientSocketPool->HttpStreamFactoryImpl::Job/HttpBasicStream. This > might be done with nesting, to make unwinding easier, but if there's a clear way > to have it in separate sections, that's somewhat better. > > https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... > net/docs/life-of-a-url-request.md:91: * The URLRequest asks the > URLRequestJobFactory to create a URLRequest[Http]Job. > Especially if you're targeting this document at folks new to the network stack, > I think "URLRequest[Http]Job" is too abbreviated a way of saying "A > URLRequestJob. Usually, this will be the URLRequestJob subclass, > URLRequestHttpJob", so I'd spell that out. > > https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... > net/docs/life-of-a-url-request.md:135: ## Details > I'd suggest breaking this section out into subsections; I think it'll be a lot > more readable. Maybe "Process Architecture" for RD/RDH, "URLRequest Creation > and Plumbing" for URLRequestContext->ResourceLoader/Handler, "Http Transactions" > for the cache and network transactions, and maybe "Socket pools", "Http Parsing > and Data Processing" for the rest. It would be really useful if this section > was broken out in the same way as the above section (you don't need the labels > on the above section, but if the reader has already bucketed the concepts in a > certain way, they'll read this section more easily). > > https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... > net/docs/life-of-a-url-request.md:309: cache independently of the response that > was compressed with it. > nit: I'd leave out "from the cache" since SDCH implementation is, at least > currently, partially memory based around SDCH-specific storage. Follow-up: I think it'd also be useful to have this reviewed by someone new to the network stack, who won't be coming at it from a state of (admittedly imperfect) knowledge as I am.
Thanks for the feedback! On 2015/07/08 20:36:25, rdsmith wrote: > So I'm torn. I believe that (at least in this case) any documentation is better > than no documentation. So if you (at any point) ask me for a stamp for whatever > you've got written, I'll give it to you. > > Having said that, I really feel like this suffers from the "Wall of Text" > problem. It's a whole lot of information that isn't very broken down into > digestible lumps, and doesn't have motivation, which would help for both > understanding and against despair :-}. I've made some suggestions as to how to > improve that (without pictures), but I don't know how much time you're willing > to devote to this, so I haven't gone whole hog. You're certainly right about this. I find myself unable to assign a priority that puts making pretty pictures above anything I've been working on this year, however. It feels like a waste of my work time, and I'm certainly not going to spend my own time figuring out how to make them. I'd probably find it less painful to write code to make the pictures than to use an existing program to make them (Particularly as I could handle additions / subtractions without having to move everything around manually again), which really seems like it's not something I should be doing during work hours. > I also had the feeling, as I read through the "Additional Topics" section, that > it was both too detailed and not detailed enough, which suggests to me we're > trying to put too much into this one document. Maybe simplifying this down with > stub external docs to be filled in by content owners (yes, most of whom will > still be you :-}) might make sense? The stubs are targeted at two purposes: 1) Provide information on "this exists, and here's where it plugs in". So people get basic info without wading through yet more walls of text. 2) Providing basic information useful when reading net-internals logs. Knowing how and where SPDY plugs in at a high level makes reading logs a lot easier, without having to go through a SPDY specific doc. 3) Point out things that one should generally know while on triage duty, without doing a deep dive into any area. If we ever have well formatted, more specific info on these modules, maybe I'll be happy enough with the other docs to just have pointers, or even get rid of the sections entirely. I'm happy to remove the Uploads, Cookies, and ResourceScheduler sections - the first two are pretty straight forward (At the level of detail I'm going into) and as long as I mention the ResourceScheduler's existence in the priority section, I'm happy with that. Sdch + cache lifetime issues are perhaps less central, maybe could remove that. Could maybe remove the cache section as well, but I'm reluctant to remove any of the others: * How SPDY/QUIC plug in is important, and completely non-obvious. * Don't run into many issues with cancellation, but it happens a lot, and people should be at least a little familiar with it. * Everyone should know where redirects plug in. * SocketPools are weird and scary things, and it's non-obvious, but pretty important, how they're linked together. * Where proxy logic is. * I could get rid of the ResourceScheduler section, but just what priority does - it's kind of peppered through the code, and non-obvious, from a top-level perspective, just how it matters. Admittedly, there are a lot of things network developers should know in depth that I don't go into (DoLoop pattern, passing error codes / bytes read around, etc), but those are things people should know in depth, which isn't what I wanted to include in this doc. On 2015/07/08 21:16:32, rdsmith wrote: > Follow-up: I think it'd also be useful to have this reviewed by someone new to > the network stack, who won't be coming at it from a state of (admittedly > imperfect) knowledge as I am. I'll add Helen, once I've responded to your comments. I almost added her earlier, but decided I'd at least wait to get your initial comments.
On 2015/07/08 21:46:42, mmenke wrote: > Thanks for the feedback! > > On 2015/07/08 20:36:25, rdsmith wrote: > > So I'm torn. I believe that (at least in this case) any documentation is > better > > than no documentation. So if you (at any point) ask me for a stamp for > whatever > > you've got written, I'll give it to you. > > > > Having said that, I really feel like this suffers from the "Wall of Text" > > problem. It's a whole lot of information that isn't very broken down into > > digestible lumps, and doesn't have motivation, which would help for both > > understanding and against despair :-}. I've made some suggestions as to how > to > > improve that (without pictures), but I don't know how much time you're willing > > to devote to this, so I haven't gone whole hog. > > You're certainly right about this. I find myself unable to assign a priority > that puts making pretty pictures above anything I've been working on this year, > however. It feels like a waste of my work time, and I'm certainly not going to > spend my own time figuring out how to make them. I'd probably find it less > painful to write code to make the pictures than to use an existing program to > make them (Particularly as I could handle additions / subtractions without > having to move everything around manually again), which really seems like it's > not something I should be doing during work hours. And by "waste of my time", I mean I'm currently assigned 3 crashers, working on fixing net-export, recently did a deep dive into fixing URLRequest lifetime issues, and also did a major breaking API change in Cronet (Which is the kind of thing you want to get out sooner rather than later, to minimize people you break). I also generally do 8-15 codereviews a week. I don't feel like I can prioritize making pictures above any of those. Feature work, sure, I could put pictures to help new team members above that, but I really haven't been doing much of that lately.
On 2015/07/08 21:53:51, mmenke wrote: > On 2015/07/08 21:46:42, mmenke wrote: > > Thanks for the feedback! > > > > On 2015/07/08 20:36:25, rdsmith wrote: > > > So I'm torn. I believe that (at least in this case) any documentation is > > better > > > than no documentation. So if you (at any point) ask me for a stamp for > > whatever > > > you've got written, I'll give it to you. > > > > > > Having said that, I really feel like this suffers from the "Wall of Text" > > > problem. It's a whole lot of information that isn't very broken down into > > > digestible lumps, and doesn't have motivation, which would help for both > > > understanding and against despair :-}. I've made some suggestions as to how > > to > > > improve that (without pictures), but I don't know how much time you're > willing > > > to devote to this, so I haven't gone whole hog. > > > > You're certainly right about this. I find myself unable to assign a priority > > that puts making pretty pictures above anything I've been working on this > year, > > however. It feels like a waste of my work time, and I'm certainly not going > to > > spend my own time figuring out how to make them. I'd probably find it less > > painful to write code to make the pictures than to use an existing program to > > make them (Particularly as I could handle additions / subtractions without > > having to move everything around manually again), which really seems like it's > > not something I should be doing during work hours. > > And by "waste of my time", I mean I'm currently assigned 3 crashers, working on > fixing net-export, recently did a deep dive into fixing URLRequest lifetime > issues, and also did a major breaking API change in Cronet (Which is the kind of > thing you want to get out sooner rather than later, to minimize people you > break). I also generally do 8-15 codereviews a week. I don't feel like I can > prioritize making pictures above any of those. > > Feature work, sure, I could put pictures to help new team members above that, > but I really haven't been doing much of that lately. So to be clear, I wasn't asking about pictures. I took the attitude of "Ok, if I want pictures I'll have to make them myself, and that's probably fair" and should have been explicit about it. I was more asking what level of textual review and suggestions you wanted/had the bandwidth to incorporate. Unfortunately, I'm unfortunately still not sure on that, and it obviously affects how detailed a review I do. Let me offer you a couple of options: * I can stamp as is (and maybe come back and hack it up myself and ask you to review). * I can drive towards some breaking apart into sections with higher level overviews that I think will make what you've got here more easily understandable. * I can do a deep drive towards making this as textually good a document as I can manage from a reviewer point of view, which would probably involve splitting it out into separate documents with cross-links. I'm sorta assuming from what you say the last one is off the table; what's your preference between the first two? And to be clear: I am *happy* to accept your priorities as given, and very glad you're doing this documentation at all. I'm just trying to understand how demanding a review I should give, given that I'm willing to stamp as written, but that I'd ideally like something a lot more organized and broken down. Thanks/sorry ....
On 2015/07/08 22:11:20, rdsmith wrote: > On 2015/07/08 21:53:51, mmenke wrote: > > On 2015/07/08 21:46:42, mmenke wrote: > > > Thanks for the feedback! > > > > > > On 2015/07/08 20:36:25, rdsmith wrote: > > > > So I'm torn. I believe that (at least in this case) any documentation is > > > better > > > > than no documentation. So if you (at any point) ask me for a stamp for > > > whatever > > > > you've got written, I'll give it to you. > > > > > > > > Having said that, I really feel like this suffers from the "Wall of Text" > > > > problem. It's a whole lot of information that isn't very broken down into > > > > digestible lumps, and doesn't have motivation, which would help for both > > > > understanding and against despair :-}. I've made some suggestions as to > how > > > to > > > > improve that (without pictures), but I don't know how much time you're > > willing > > > > to devote to this, so I haven't gone whole hog. > > > > > > You're certainly right about this. I find myself unable to assign a > priority > > > that puts making pretty pictures above anything I've been working on this > > year, > > > however. It feels like a waste of my work time, and I'm certainly not going > > to > > > spend my own time figuring out how to make them. I'd probably find it less > > > painful to write code to make the pictures than to use an existing program > to > > > make them (Particularly as I could handle additions / subtractions without > > > having to move everything around manually again), which really seems like > it's > > > not something I should be doing during work hours. > > > > And by "waste of my time", I mean I'm currently assigned 3 crashers, working > on > > fixing net-export, recently did a deep dive into fixing URLRequest lifetime > > issues, and also did a major breaking API change in Cronet (Which is the kind > of > > thing you want to get out sooner rather than later, to minimize people you > > break). I also generally do 8-15 codereviews a week. I don't feel like I can > > prioritize making pictures above any of those. > > > > Feature work, sure, I could put pictures to help new team members above that, > > but I really haven't been doing much of that lately. > > So to be clear, I wasn't asking about pictures. I took the attitude of "Ok, if > I want pictures I'll have to make them myself, and that's probably fair" and > should have been explicit about it. I was more asking what level of textual > review and suggestions you wanted/had the bandwidth to incorporate. > > Unfortunately, I'm unfortunately still not sure on that, and it obviously > affects how detailed a review I do. Let me offer you a couple of options: > * I can stamp as is (and maybe come back and hack it up myself and ask you to > review). > * I can drive towards some breaking apart into sections with higher level > overviews that I think will make what you've got here more easily > understandable. > * I can do a deep drive towards making this as textually good a document as I > can manage from a reviewer point of view, which would probably involve splitting > it out into separate documents with cross-links. > I'm sorta assuming from what you say the last one is off the table; what's your > preference between the first two? I'm happy with either of the first two options - the third would be great, but yea, it's a matter of time. How about go with the second option? I'm all for making this as useful as we can. I would like to land this sometime in the next week or two, though, just to get it out there, so if you don't have the time yourself, I can live with 1. Happy to do followup review passes with you if we go that route, though in my experience, that often doesn't work as well. > And to be clear: I am *happy* to accept your priorities as given, and very glad > you're doing this documentation at all. I'm just trying to understand how > demanding a review I should give, given that I'm willing to stamp as written, > but that I'd ideally like something a lot more organized and broken down. > > Thanks/sorry .... You have nothing to apologize for - even if I had correctly understood your comment about time, you'd have nothing to apologize for. I certainly wish I had time to do more complete job of this.
On 2015/07/08 22:21:10, mmenke wrote: > On 2015/07/08 22:11:20, rdsmith wrote: > > On 2015/07/08 21:53:51, mmenke wrote: > > > On 2015/07/08 21:46:42, mmenke wrote: > > > > Thanks for the feedback! > > > > > > > > On 2015/07/08 20:36:25, rdsmith wrote: > > > > > So I'm torn. I believe that (at least in this case) any documentation > is > > > > better > > > > > than no documentation. So if you (at any point) ask me for a stamp for > > > > whatever > > > > > you've got written, I'll give it to you. > > > > > > > > > > Having said that, I really feel like this suffers from the "Wall of > Text" > > > > > problem. It's a whole lot of information that isn't very broken down > into > > > > > digestible lumps, and doesn't have motivation, which would help for both > > > > > understanding and against despair :-}. I've made some suggestions as to > > how > > > > to > > > > > improve that (without pictures), but I don't know how much time you're > > > willing > > > > > to devote to this, so I haven't gone whole hog. > > > > > > > > You're certainly right about this. I find myself unable to assign a > > priority > > > > that puts making pretty pictures above anything I've been working on this > > > year, > > > > however. It feels like a waste of my work time, and I'm certainly not > going > > > to > > > > spend my own time figuring out how to make them. I'd probably find it > less > > > > painful to write code to make the pictures than to use an existing program > > to > > > > make them (Particularly as I could handle additions / subtractions without > > > > having to move everything around manually again), which really seems like > > it's > > > > not something I should be doing during work hours. > > > > > > And by "waste of my time", I mean I'm currently assigned 3 crashers, working > > on > > > fixing net-export, recently did a deep dive into fixing URLRequest lifetime > > > issues, and also did a major breaking API change in Cronet (Which is the > kind > > of > > > thing you want to get out sooner rather than later, to minimize people you > > > break). I also generally do 8-15 codereviews a week. I don't feel like I > can > > > prioritize making pictures above any of those. > > > > > > Feature work, sure, I could put pictures to help new team members above > that, > > > but I really haven't been doing much of that lately. > > > > So to be clear, I wasn't asking about pictures. I took the attitude of "Ok, > if > > I want pictures I'll have to make them myself, and that's probably fair" and > > should have been explicit about it. I was more asking what level of textual > > review and suggestions you wanted/had the bandwidth to incorporate. > > > > Unfortunately, I'm unfortunately still not sure on that, and it obviously > > affects how detailed a review I do. Let me offer you a couple of options: > > * I can stamp as is (and maybe come back and hack it up myself and ask you to > > review). > > * I can drive towards some breaking apart into sections with higher level > > overviews that I think will make what you've got here more easily > > understandable. > > * I can do a deep drive towards making this as textually good a document as I > > can manage from a reviewer point of view, which would probably involve > splitting > > it out into separate documents with cross-links. > > I'm sorta assuming from what you say the last one is off the table; what's > your > > preference between the first two? > > I'm happy with either of the first two options - the third would be great, but > yea, it's a matter of time. How about go with the second option? I'm all for > making this as useful as we can. I would like to land this sometime in the next > week or two, though, just to get it out there, so if you don't have the time > yourself, I can live with 1. Happy to do followup review passes with you if we > go that route, though in my experience, that often doesn't work as well. Sounds good. I think my first round feedback (which I think duplicates notes above, just more pithily) is then: * Include a single paragraph very high level description of what happens during a URL request that gestures at motivations (I included a suggestion to give you an idea of what I was looking for). * Create some finer granularity within the "Request is issued" bullet point list and the "Details" section, with the groupings in those two sections matching each other as much as possible. > > > And to be clear: I am *happy* to accept your priorities as given, and very > glad > > you're doing this documentation at all. I'm just trying to understand how > > demanding a review I should give, given that I'm willing to stamp as written, > > but that I'd ideally like something a lot more organized and broken down. > > > > Thanks/sorry .... > > You have nothing to apologize for - even if I had correctly understood your > comment about time, you'd have nothing to apologize for. I certainly wish I had > time to do more complete job of this. No worries at all--happy to take what you can give. I may try and pick up where you leave off (though that's not a promise--my todo list isn't as bad as yours, but I'm already pretty scattered in my focus).
Patchset #4 (id:60001) has been deleted
I've removed 3 sections (Cookies, ResourceScheduler, and Uploads), and inlined brief mentions of where they plug in, since I don't think all the details I had fit under "things a network stack team member should know", just where they hook up to. Could do the same for proxies, but I still think proxies+socket pools need a mention (Though I suppose that's more because of how weird it is and unclear from the code, rather than because it's important information for everyone). Open to more pruning. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:1: # Life of a URLRequest On 2015/07/08 20:36:25, rdsmith wrote: > Could you add this doc (and maybe the other docs in this directory, if you are > so moved) into net/net.gypi:net_docs_sources so that HTML gets automatically > created in people's output directories? Arguably we don't need to do this > (because source view auto-translates) but it does make reviewing easier. I also > believe in consistency--i.e. we should do it for all docs or none. Done. I was unaware of that target...and suspect almost everyone else is, too, which may be a problem. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:31: * The main URLRequestContext is mostly created in ProfileIOData, though it On 2015/07/08 20:36:25, rdsmith wrote: > These sub-items aren't showing up as indented/second level list in the HTML. Fixed. Weird, they're fine when I paste them into snippets (See earlier comment about not knowing about net_docs target). Can't we just standardize on one version of markdown for all of Google? :( Looks like this version of markdown needs 4 space indent (I meant to use two, but when I switched to 1 space between sentences, accidentally reduced these to 1 space, too). https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:82: * ResourceDispatcher sends an IPC to the ResourceDispatcherHost in the browser On 2015/07/08 20:36:24, rdsmith wrote: > I'd add a line with some visual distinction after this one to call out the > transition to the browser process. I split up the section. Let me know if you think I should do more. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:85: * ResourceDispatcherHost creates a ResourceLoader and ResourceHandler chain to On 2015/07/08 20:36:24, rdsmith wrote: > In this line and the next one, I'd refer to the "URLRequest" rather than the > "Request" to make clear the top-level concept versus lower level object > distinction. Done. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:89: ### Request is Issued On 2015/07/08 20:36:25, rdsmith wrote: > I'd put some verbiage here indicating that what's below is the common case, but > there may be variations at each level, usually at the "asks the X to create a Y, > which is a Z" that sometimes it's not a Z but some other subclass of Y. I don't want to hammer everywhere that this is a "simple" URLRequest. I've rearranged things slightly, tell me what you think. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:91: * The URLRequest asks the URLRequestJobFactory to create a URLRequest[Http]Job. On 2015/07/08 20:36:24, rdsmith wrote: > Especially if you're targeting this document at folks new to the network stack, > I think "URLRequest[Http]Job" is too abbreviated a way of saying "A > URLRequestJob. Usually, this will be the URLRequestJob subclass, > URLRequestHttpJob", so I'd spell that out. Done. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:135: ## Details On 2015/07/08 20:36:24, rdsmith wrote: > I'd suggest breaking this section out into subsections; I think it'll be a lot > more readable. Maybe "Process Architecture" for RD/RDH, "URLRequest Creation > and Plumbing" for URLRequestContext->ResourceLoader/Handler, "Http Transactions" > for the cache and network transactions, and maybe "Socket pools", "Http Parsing > and Data Processing" for the rest. It would be really useful if this section > was broken out in the same way as the above section (you don't need the labels > on the above section, but if the reader has already bucketed the concepts in a > certain way, they'll read this section more easily). I've split this into the same sections, with the same names, as the first part, which I think is a big improvment. Open to ideas for how to better split things. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:309: cache independently of the response that was compressed with it. On 2015/07/08 20:36:25, rdsmith wrote: > nit: I'd leave out "from the cache" since SDCH implementation is, at least > currently, partially memory based around SDCH-specific storage. Reworded it a bit.
mmenke@chromium.org changed reviewers: + xunjieli@chromium.org
[+xunjieli]: Helen, mind taking a look at this? I'd appreciate comments on areas that are confusing, or could use more (Or less) detail. I don't expect you (Or Randy, for that matter) to fully review it for accuracy. Hope that after this is landed, others will continue to improve it, as they use it and notice its deficiencies.
On 2015/07/09 19:41:37, mmenke wrote: > [+xunjieli]: Helen, mind taking a look at this? I'd appreciate comments on > areas that are confusing, or could use more (Or less) detail. I don't expect > you (Or Randy, for that matter) to fully review it for accuracy. Hope that > after this is landed, others will continue to improve it, as they use it and > notice its deficiencies. Definitely! But i will need some time to read it thoroughly. will get back to you tomorrow :)
On 2015/07/09 20:39:58, xunjieli wrote: > On 2015/07/09 19:41:37, mmenke wrote: > > [+xunjieli]: Helen, mind taking a look at this? I'd appreciate comments on > > areas that are confusing, or could use more (Or less) detail. I don't expect > > you (Or Randy, for that matter) to fully review it for accuracy. Hope that > > after this is landed, others will continue to improve it, as they use it and > > notice its deficiencies. > > Definitely! But i will need some time to read it thoroughly. will get back to > you tomorrow :) Thanks! Take your time, there's no rush. I'd like to land it late next week or early the following one.
This will be incredibly helpful to anyone new in the team!! I really appreciate the work that has gone into the making of this doc. I left a few comments. I wonder whether combining subsections of Overview with those of Details will make the structure clearer and easier to understand (Explained in my comment below). https://codereview.chromium.org/1211003003/diff/100001/content/browser/loader... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1211003003/diff/100001/content/browser/loader... content/browser/loader/resource_loader.cc:259: return; I don't understand why this block is almost identical as the one added on line 267, and why it is in this CL. The indentation does not look right. Should it be in a separate CL? https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:42: The "HttpNetworkSession" is another major network stack object. It has nit: I think the quotes can be removed, since quotes aren't used in any other places when referring to class names. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:56: two main interfaces: The URLRequest::Delegate interface and the NetworkDelegate nit: lowercase "The" after the colon. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:59: The URLRequest::Delegate interface consists of small set callbacks needed to let [a] small set [of] callbacks? https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:65: as an assortment of other methods. The NetworkDelegate is optional, the nit: Add a conjunction in "The NetworkDelegate is optional, [and/but] the URLRequest::Delegate is not." https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:86: ### Request Starts in a Child Process I find it a bit confusing that this doc mentioned "renderer process" and "child process", as if they are interchangeable. Though later in the doc (Line 173), I see that "child process" isn't necessarily a "renderer process". Could you clarify a bit on what kinds of "child process" there are? I am assuming they all use ResourceDispatcher & IPC to communicate with RDH. Is this right? Reading http://dev.chromium.org/developers/design-documents/multi-process-architecture does not enlighten me in this aspect, so I'd really appreciate a sentence or two on whether the "child process" referred in this doc also includes other processes like plugin processes. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:88: * ResourceDispatcher to creates an IPCResourceLoaderBridge. nit: remove "to". https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:96: * ResourceDispatcherHost creates a ResourceLoader and ResourceHandler chain to nit: and [a] ResourceHandler chain. (It reads as if it is a chain comprised of a ResourceLoader and a ResourceHandler, but I think you meant a ResourceLoader and many handlers) https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:132: through the ResourceHandler stack. nit: I would replace "stack" with "chain" so there is one fewer variation. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:133: * They're then sent by the AsyncResourceHandler to the ResourceDispatcher. I got confused by the sudden mention of AsyncResourceHandler here when I read the doc first time, because I did not immediately associate AsyncResourceHandler with the chain of ResourceHandlers. You mentioned later in the doc that AsyncResourceHandler is the end of the chain. I would move that sentence here. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:141: * AsyncResourceHandler informs the ResourceDispatcher of each read. How does AsyncResourceHandler inform the ResourceDispatcher, which is in the child process? Should we mention this is through IPC? https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:145: ### URLRequest is Destroyed nit: s/Destroyed/destroyed. I think we should stick with lowercase except first word or change all the words for the section titles to have uppercase first letter. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:147: * When complete, the RDH deletes the ResourceLoader, which deletes the Does the chain of ResourceHandlers also get deleted? https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:152: # Details I thought the life of a url is ended, because I didn't realize that the sections above are **sub** sections of "Overview", and the real details begin here. Maybe add some hints here by numbering the sections? For example, Overview can be numbered 1, and subsections numbered from 1.1 to 1.7; Details can be numbered 2, and subsections numbered from 2.1 to 2.7. Or maybe combine the subsections of Overview with those of Details into one subsection? since they exactly correspond? Maybe something like: # Request starts in a Child Process [Indent] Bullet points from subsection in Overview [no Indent] Paragraph from subsection in Details. When I was reading Overview subsections, I had many questions which are answered in Details subsections, so I think it will help to reduce cognitive load if the contents are together. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:174: Browser-initiated don't go through the RDH, with some exceptions. nit: s/"Browser-initiated"/"requests initiated by the browser process" or something similar to make it clearer that we are talking about requests and browser process here. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:214: ClientSocketPoolBase<TransportSocketParams> it wraps, which sends it on to its I am not 100% sure on what the three "it" refer to in this sentence. The first "it" is probably the request. Not clear whether second "it" refers to... I think the second "it" is the pool, but I probably got it wrong. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:234: response headers, and then passes them up through both Transaction classes nit: I'd spell out what these two Transaction classes are. I am assuming it is the HttpNetworkTransaction and HttpCache::Transaction. But if I didn't pay much attention, I probably would be lost here. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:268: ResourceLoader, which deletes the URLRequest. nit: I'd mention what happens to the chain of ResourceHandlers. Do they also get deleted by RDH? https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:303: A request can be cancelled by the renderer process Blink or by any of the Why is the "renderer process" called Blink? I thought Blink is the rendering engine. Can we use them interchangeably? https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:304: ResourceHandlers through the ResourceLoader. When the cancellation message nit: maybe add the word "chain" here? e.g. any ResourceHandler on the chain of ResourceHandlers? A little bit verbose, but it makes the concept a bit clearer. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:342: Encoding values to Filter::Factory, which creates a (possibly empty) chain of nit: I think there is a space between the dash and Encoding when the html is generated. Maybe move "Content-" down? https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:387: Sockets in the higher layer pool must have their own distinct group name in the This sentence is a bit hard to understand. Consider rephrasing a bit? Sockets in higher layer pools can be used in a lower layer pool, right? Maybe it is clearer to just say that the sockets in different layer pools cannot share the same group name. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:458: sockets for higher priority requests. I don't understand this sentence. Do you instead mean that idle sockets will be assigned to **high** priority requests in preference to creating new sockets?
I've responded to all your comments but the one about combining the overview with details - want to play with that on Monday, and see how things look. https://codereview.chromium.org/1211003003/diff/100001/content/browser/loader... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1211003003/diff/100001/content/browser/loader... content/browser/loader/resource_loader.cc:259: return; On 2015/07/10 20:32:25, xunjieli wrote: > I don't understand why this block is almost identical as the one added on line > 267, and why it is in this CL. The indentation does not look right. Should it be > in a separate CL? Hrm...This shouldn't be in this CL. Was investigating another issue, and forgot to remove the test code before committing. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:42: The "HttpNetworkSession" is another major network stack object. It has On 2015/07/10 20:32:26, xunjieli wrote: > nit: I think the quotes can be removed, since quotes aren't used in any other > places when referring to class names. Done. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:56: two main interfaces: The URLRequest::Delegate interface and the NetworkDelegate On 2015/07/10 20:32:26, xunjieli wrote: > nit: lowercase "The" after the colon. Done. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:59: The URLRequest::Delegate interface consists of small set callbacks needed to let On 2015/07/10 20:32:26, xunjieli wrote: > [a] small set [of] callbacks? Done. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:65: as an assortment of other methods. The NetworkDelegate is optional, the On 2015/07/10 20:32:26, xunjieli wrote: > nit: Add a conjunction in "The NetworkDelegate is optional, [and/but] the > URLRequest::Delegate is not." Done. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:86: ### Request Starts in a Child Process On 2015/07/10 20:32:26, xunjieli wrote: > I find it a bit confusing that this doc mentioned "renderer process" and "child > process", as if they are interchangeable. Though later in the doc (Line 173), I > see that "child process" isn't necessarily a "renderer process". > > Could you clarify a bit on what kinds of "child process" there are? I am > assuming they all use ResourceDispatcher & IPC to communicate with RDH. Is this > right? That's right. > Reading > http://dev.chromium.org/developers/design-documents/multi-process-architecture > does not enlighten me in this aspect, so I'd really appreciate a sentence or two > on whether the "child process" referred in this doc also includes other > processes like plugin processes. I added a paragraph in the details section about the child processes, and switch to "child" rather than "renderer" in most places, to be clearer. Don't want to go too much into processes, but since this is targeted at new Chromites, think it makes sense to touch on them a little. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:88: * ResourceDispatcher to creates an IPCResourceLoaderBridge. On 2015/07/10 20:32:26, xunjieli wrote: > nit: remove "to". Done. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:96: * ResourceDispatcherHost creates a ResourceLoader and ResourceHandler chain to On 2015/07/10 20:32:26, xunjieli wrote: > nit: and [a] ResourceHandler chain. > (It reads as if it is a chain comprised of a ResourceLoader and a > ResourceHandler, but I think you meant a ResourceLoader and many handlers) Done, and rephrased to hopefully be clearer. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:132: through the ResourceHandler stack. On 2015/07/10 20:32:26, xunjieli wrote: > nit: I would replace "stack" with "chain" so there is one fewer variation. Done. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:133: * They're then sent by the AsyncResourceHandler to the ResourceDispatcher. On 2015/07/10 20:32:26, xunjieli wrote: > I got confused by the sudden mention of AsyncResourceHandler here when I read > the doc first time, because I did not immediately associate AsyncResourceHandler > with the chain of ResourceHandlers. You mentioned later in the doc that > AsyncResourceHandler is the end of the chain. I would move that sentence here. > Clarified that the AsyncResourceHandler is the last in the chain. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:141: * AsyncResourceHandler informs the ResourceDispatcher of each read. On 2015/07/10 20:32:26, xunjieli wrote: > How does AsyncResourceHandler inform the ResourceDispatcher, which is in the > child process? Should we mention this is through IPC? Done. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:145: ### URLRequest is Destroyed On 2015/07/10 20:32:26, xunjieli wrote: > nit: s/Destroyed/destroyed. > I think we should stick with lowercase except first word or change all the words > for the section titles to have uppercase first letter. Done to all the "###" titles (Hrm, I was really inconsistent there). https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:147: * When complete, the RDH deletes the ResourceLoader, which deletes the On 2015/07/10 20:32:26, xunjieli wrote: > Does the chain of ResourceHandlers also get deleted? Done. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:174: Browser-initiated don't go through the RDH, with some exceptions. On 2015/07/10 20:32:26, xunjieli wrote: > nit: s/"Browser-initiated"/"requests initiated by the browser process" or > something similar to make it clearer that we are talking about requests and > browser process here. Done. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:214: ClientSocketPoolBase<TransportSocketParams> it wraps, which sends it on to its On 2015/07/10 20:32:26, xunjieli wrote: > I am not 100% sure on what the three "it" refer to in this sentence. > The first "it" is probably the request. Not clear whether second > "it" refers to... I think the second "it" is the pool, but I probably got it > wrong. Good point. Removed almost all the its. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:234: response headers, and then passes them up through both Transaction classes On 2015/07/10 20:32:26, xunjieli wrote: > nit: I'd spell out what these two Transaction classes are. I am assuming it is > the HttpNetworkTransaction and HttpCache::Transaction. But if I didn't pay much > attention, I probably would be lost here. Done. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:268: ResourceLoader, which deletes the URLRequest. On 2015/07/10 20:32:27, xunjieli wrote: > nit: I'd mention what happens to the chain of ResourceHandlers. Do they also get > deleted by RDH? Done. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:303: A request can be cancelled by the renderer process Blink or by any of the On 2015/07/10 20:32:26, xunjieli wrote: > Why is the "renderer process" called Blink? I thought Blink is the rendering > engine. Can we use them interchangeably? Removed the Blink (Was supposed to be "Blink in the renderer process", but it can be cancelled by a lot of other things. Blink is a lot more than rendering engine (Handles sending requests for subresources, for instance)...But not going to get into that here. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:304: ResourceHandlers through the ResourceLoader. When the cancellation message On 2015/07/10 20:32:26, xunjieli wrote: > nit: maybe add the word "chain" here? > e.g. any ResourceHandler on the chain of ResourceHandlers? A little bit verbose, > but it makes the concept a bit clearer. Done. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:342: Encoding values to Filter::Factory, which creates a (possibly empty) chain of On 2015/07/10 20:32:26, xunjieli wrote: > nit: I think there is a space between the dash and Encoding when the html is > generated. Maybe move "Content-" down? Done. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:387: Sockets in the higher layer pool must have their own distinct group name in the On 2015/07/10 20:32:26, xunjieli wrote: > This sentence is a bit hard to understand. Consider rephrasing a bit? > > Sockets in higher layer pools can be used in a lower layer pool, right? Maybe it > is clearer to just say that the sockets in different layer pools cannot share > the same group name. Tried to make it a bit clearer. https://codereview.chromium.org/1211003003/diff/100001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:458: sockets for higher priority requests. On 2015/07/10 20:32:26, xunjieli wrote: > I don't understand this sentence. > Do you instead mean that idle sockets will be assigned to **high** priority > requests in preference to creating new sockets? No...If we have no free socket slots, and a high priority request to www.foo.com, and a low priority request to www.bar.com, and then a connection to www.bar.com becomes idle, we'll service the low priority request, instead of closing it to service the high priority request. I've rephrased it a bit. The detail may be too much information for the level of detail this document should go into.
And thanks for all the great feedback!
This is wonderful. I feel like my view of the network stack settled into place in reading this--not a lot was new to me, but even the things that I understood "fit" better, and I did learn new things reading through it. Thank you. General call out: When I say "suggestion" (or "Random idea", which is intended to have even less pressure behind it :-}), it's completely up to you--I'm good with either the current phrasing or my variation. https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1211003003/diff/40001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:1: # Life of a URLRequest On 2015/07/09 19:38:28, mmenke wrote: > On 2015/07/08 20:36:25, rdsmith wrote: > > Could you add this doc (and maybe the other docs in this directory, if you are > > so moved) into net/net.gypi:net_docs_sources so that HTML gets automatically > > created in people's output directories? Arguably we don't need to do this > > (because source view auto-translates) but it does make reviewing easier. I > also > > believe in consistency--i.e. we should do it for all docs or none. > > Done. I was unaware of that target...and suspect almost everyone else is, too, > which may be a problem. Agreed; not sure what to do about it, though. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:1: # Life of a URLRequest Suggestion: There's enough in this doc that a table of contents might be useful. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:12: through how a basic request issued by Blink works its way through the network I'd suggest scrubbing mention of "Blink" from this document; I think it's almost always more accurate and less distracting detail to just say "the renderer" or something like that. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:16: # Anatomy of the Network Stack From my perspective, *the* most important object in the network stack is the URLRequest, and URLRequestContext and HttpNetworkSession are supporting infrastructure (admittedly important supporting infrastructure :-}). So I find the lack of mention in it in the Anatomy of the network stack section a bit weird; maybe just mention it as the primary consumer interface to the network stack, created by a call to URLRequestContext::CreateRequest()? https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:23: contexts that the network stack team owns: Random idea: I think the breakdown of the different URLRequestContexts below could be in an appendix or not included if that's needed. I don't think that it's a problem to have it here, but I'm not sure it adds much. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:43: pointers to the network stack objects that more directly deal with sockets, and nit: owning of non-owning pointers? Construction/need for construction? (Just looking at parallelism with URLRequestContext.) https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:61: the URLRequest. This mention adds to the weirdness of not having yet mentioned URLRequest. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:66: URLRequest::Delegate is not. Worthwhile indicating how the NetworkDelegate is specified? (By setter on the URLRequestContext, it looks like.) https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:74: cache, and then creates a network connection, if necessary, to actually fetch nit, suggestion: "network connection" -> "network connection object" to make parallelism and linkage with next sentence clearer. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:84: request, the response is uncompressed, and no matching entry in the cache. I like this method of indicating the conditional areas without putting a lot of text energy into them. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:84: request, the response is uncompressed, and no matching entry in the cache. nit, suggestion: For completeness, this sentence should also include "there are no idle sockets already connected to the server int he socket pool". https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:103: usually a URLRequestHttpJob for HTTP/HTTPS requests. nit, suggestion: Given last sentence of overview, change this clause to "in this case a URLRequestHttpJob"? https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:132: through the ResourceHandler chain. nit: ", *and* through the ResourceHandler chain". Optional suggestion: "and down". https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:142: * AsyncResourceHandler informs the ResourceDispatcher of each read using IPCs. nit, suggestion: "using cross-process IPCs" (redundant, but it may be worthwhile calling out exactly when the process crossings occur). https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:191: then creates a ResourceLoader (Which is the URLRequest::Delegate), passes nit: lower-case "w" on "Which". https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:335: The URLRequestHttpJob then checks if the URLRequest if the request should be "checks if"? https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:354: embedder. You haven't really used the language of "embedder" before, and I'd suggest avoiding it now. Maybe " ... to the ResourceLoader and down the ResourceHandler chain"? https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:358: aren't compressed in the cache, either. This behavior can also create problems nit, suggestion: Remove "also"; I don't think this document has identified other problems with caching uncompressed data. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:371: socket, and return it to the socket pool when done. All connections with the I don't understand what you mean by ", once connected socket,"? https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:384: ## Socket Pool Layering nit, suggestion: Subsection of Socket Pools? https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:389: sections for examples. There are a couple additional complexities here. I'd include a parenthetical example right here--I think (e.g.) knowing that one case of this is SSL sockets being layered on top of transport sockets would help a lot in getting the general concept. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:402: ## SSL nit, suggestion: Subsection of Socket Pool Layering? https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:426: ## Proxy Socket Pools Suggestion: I'd combine this section with the previous one; I ended the last section with my pre-existing confusion about proxies intact, and ended this section with it being a little bit resolved. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:435: ## SPDY nit, suggestion: Subsection of SSL? Alternatively, combine QUIC & SPDY under "Alternate protocols"? I'm fully aware that if you take all my nesting suggestions the result will be a cascading nested mess :-}. So take them in sum as wanting a bit more clearer sense of the connection between the called out sections, but not being absolutely certain what the best way to do that is. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:437: Once an SSL connection is established, the HttpStreamFactoryImpl::Job checks if This suggests that SSL connection establishment has important mysteries not referred to above (i.e. that it can end up occurring over SPDY, or, if I read things right, QUIC). That might be worthwhile referring to parenthetically in SSL, above, and then expanding on how it actually happens here (and maybe in QUIC below--I don't end up after reading these sections with a clear sense of how QUIC/SPDY fit into connection establishment, even at a high level. Of course, given that it was in my OKRs to document exactly that this quarter, you might consider that my failure, not the documentations :-}.) https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:453: HttpStreamFactoryImpl::Job will be created for SPDY, and will be raced against Did you mean QUIC in this sentence? https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:457: TODO(mmenke): Discuss SPDY/QUIC proxies? FWIW, I don't think that's necessary in this document, though proxies having their own document would be a great and wonderful thing.
https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:23: contexts that the network stack team owns: On 2015/07/13 15:39:31, rdsmith wrote: > Random idea: I think the breakdown of the different URLRequestContexts below > could be in an appendix or not included if that's needed. I don't think that > it's a problem to have it here, but I'm not sure it adds much. I will vote to keep this breakdown of URLRequestContexts. I think it does a good job of clarifying who the consumers are. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:59: The URLRequest::Delegate interface consists of small set of callbacks needed to nit: Do we need an article here? s/"small set"/"a small set" https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:93: ### ResourceDispatcherHost sets up the request in the Browser Process nit: browser process should be lowercase. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:398: pool, have their own distinct group name. This is needed so that, for instance, nit: The second part of the first sentence does not have a subject. Consider revising? https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:399: SSL and HTTP connections won't be group together in the TcpClientSocketPool, nit: grouped.
Patchset #9 (id:170001) has been deleted
I've inlined the overview, per Helen's suggestion...Looks almost as long as the full description in most sections, though. Wonder if it should just be removed? I'm not sure, if you guys think it's useful, happy to keep it. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:1: # Life of a URLRequest On 2015/07/13 15:39:30, rdsmith wrote: > Suggestion: There's enough in this doc that a table of contents might be useful. It doesn't look to me like markdown has a way to automatically generate a table of contents, and I'm pretty reluctant to do it manually, since it seems the sort of thing destined to become outdated. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:12: through how a basic request issued by Blink works its way through the network On 2015/07/13 15:39:30, rdsmith wrote: > I'd suggest scrubbing mention of "Blink" from this document; I think it's almost > always more accurate and less distracting detail to just say "the renderer" or > something like that. Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:16: # Anatomy of the Network Stack On 2015/07/13 15:39:31, rdsmith wrote: > From my perspective, *the* most important object in the network stack is the > URLRequest, and URLRequestContext and HttpNetworkSession are supporting > infrastructure (admittedly important supporting infrastructure :-}). So I find > the lack of mention in it in the Anatomy of the network stack section a bit > weird; maybe just mention it as the primary consumer interface to the network > stack, created by a call to URLRequestContext::CreateRequest()? I've added a paragraph about it (After I discuss the contexts - I think the term "network stack" more closely corresponds to the URLRequestContext than a URLRequest, so it makes sense as the first thing to mention). I'm not sure exactly what to say about the URLRequest. Suggestions welcome. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:23: contexts that the network stack team owns: On 2015/07/13 15:39:31, rdsmith wrote: > Random idea: I think the breakdown of the different URLRequestContexts below > could be in an appendix or not included if that's needed. I don't think that > it's a problem to have it here, but I'm not sure it adds much. Acknowledged. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:23: contexts that the network stack team owns: On 2015/07/13 18:23:57, xunjieli wrote: > On 2015/07/13 15:39:31, rdsmith wrote: > > Random idea: I think the breakdown of the different URLRequestContexts below > > could be in an appendix or not included if that's needed. I don't think that > > it's a problem to have it here, but I'm not sure it adds much. > > I will vote to keep this breakdown of URLRequestContexts. I think it does a good > job of clarifying who the consumers are. I'll keep it as-is for now, though happy to talk further about it. I don't feel strongly about it, just inertia winning out. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:43: pointers to the network stack objects that more directly deal with sockets, and On 2015/07/13 15:39:31, rdsmith wrote: > nit: owning of non-owning pointers? Construction/need for construction? (Just > looking at parallelism with URLRequestContext.) I've updated it to be clearer here (Owns what I called the main objects before, most everything else it has a non-owning pointer to). https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:59: The URLRequest::Delegate interface consists of small set of callbacks needed to On 2015/07/13 18:23:57, xunjieli wrote: > nit: Do we need an article here? s/"small set"/"a small set" Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:61: the URLRequest. On 2015/07/13 15:39:30, rdsmith wrote: > This mention adds to the weirdness of not having yet mentioned URLRequest. Acknowledged. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:66: URLRequest::Delegate is not. On 2015/07/13 15:39:31, rdsmith wrote: > Worthwhile indicating how the NetworkDelegate is specified? (By setter on the > URLRequestContext, it looks like.) Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:74: cache, and then creates a network connection, if necessary, to actually fetch On 2015/07/13 15:39:30, rdsmith wrote: > nit, suggestion: "network connection" -> "network connection object" to make > parallelism and linkage with next sentence clearer. Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:84: request, the response is uncompressed, and no matching entry in the cache. On 2015/07/13 15:39:31, rdsmith wrote: > nit, suggestion: For completeness, this sentence should also include "there are > no idle sockets already connected to the server int he socket pool". Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:84: request, the response is uncompressed, and no matching entry in the cache. On 2015/07/13 15:39:30, rdsmith wrote: > I like this method of indicating the conditional areas without putting a lot of > text energy into them. Acknowledged. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:93: ### ResourceDispatcherHost sets up the request in the Browser Process On 2015/07/13 18:23:57, xunjieli wrote: > nit: browser process should be lowercase. Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:103: usually a URLRequestHttpJob for HTTP/HTTPS requests. On 2015/07/13 15:39:30, rdsmith wrote: > nit, suggestion: Given last sentence of overview, change this clause to "in this > case a URLRequestHttpJob"? Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:132: through the ResourceHandler chain. On 2015/07/13 15:39:30, rdsmith wrote: > nit: ", *and* through the ResourceHandler chain". Optional suggestion: "and > down". Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:142: * AsyncResourceHandler informs the ResourceDispatcher of each read using IPCs. On 2015/07/13 15:39:30, rdsmith wrote: > nit, suggestion: "using cross-process IPCs" (redundant, but it may be worthwhile > calling out exactly when the process crossings occur). Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:191: then creates a ResourceLoader (Which is the URLRequest::Delegate), passes On 2015/07/13 15:39:30, rdsmith wrote: > nit: lower-case "w" on "Which". Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:335: The URLRequestHttpJob then checks if the URLRequest if the request should be On 2015/07/13 15:39:31, rdsmith wrote: > "checks if"? Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:354: embedder. On 2015/07/13 15:39:31, rdsmith wrote: > You haven't really used the language of "embedder" before, and I'd suggest > avoiding it now. Maybe " ... to the ResourceLoader and down the ResourceHandler > chain"? Done (Used "URLRequest::Delegate" instead) https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:358: aren't compressed in the cache, either. This behavior can also create problems On 2015/07/13 15:39:31, rdsmith wrote: > nit, suggestion: Remove "also"; I don't think this document has identified other > problems with caching uncompressed data. Done (Though the fact the cache isn't compressed does seem like a waste of space to me). https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:371: socket, and return it to the socket pool when done. All connections with the On 2015/07/13 15:39:31, rdsmith wrote: > I don't understand what you mean by ", once connected socket,"? Oops...Should be "once connected". Reworded slightly. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:384: ## Socket Pool Layering On 2015/07/13 15:39:31, rdsmith wrote: > nit, suggestion: Subsection of Socket Pools? Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:389: sections for examples. There are a couple additional complexities here. On 2015/07/13 15:39:31, rdsmith wrote: > I'd include a parenthetical example right here--I think (e.g.) knowing that one > case of this is SSL sockets being layered on top of transport sockets would help > a lot in getting the general concept. Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:398: pool, have their own distinct group name. This is needed so that, for instance, On 2015/07/13 18:23:57, xunjieli wrote: > nit: The second part of the first sentence does not have a subject. Consider > revising? Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:399: SSL and HTTP connections won't be group together in the TcpClientSocketPool, On 2015/07/13 18:23:57, xunjieli wrote: > nit: grouped. Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:402: ## SSL On 2015/07/13 15:39:30, rdsmith wrote: > nit, suggestion: Subsection of Socket Pool Layering? Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:426: ## Proxy Socket Pools On 2015/07/13 15:39:30, rdsmith wrote: > Suggestion: I'd combine this section with the previous one; I ended the last > section with my pre-existing confusion about proxies intact, and ended this > section with it being a little bit resolved. I've merged the sections, and flipped the order (Mention socket pools, and then directing a request to them. Think it flows a bit better) https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:435: ## SPDY On 2015/07/13 15:39:31, rdsmith wrote: > nit, suggestion: Subsection of SSL? Alternatively, combine QUIC & SPDY under > "Alternate protocols"? > > I'm fully aware that if you take all my nesting suggestions the result will be a > cascading nested mess :-}. So take them in sum as wanting a bit more clearer > sense of the connection between the called out sections, but not being > absolutely certain what the best way to do that is. I've combined QUIC and SPDY into a section (And switched from SPDY to HTTP/2, in most places). Think SPDY makes more sense with QUIC, so the two can be compared, thought SPDY negotiation is a part of SSL negotiation. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:437: Once an SSL connection is established, the HttpStreamFactoryImpl::Job checks if On 2015/07/13 15:39:31, rdsmith wrote: > This suggests that SSL connection establishment has important mysteries not > referred to above (i.e. that it can end up occurring over SPDY, or, if I read > things right, QUIC). That might be worthwhile referring to parenthetically in > SSL, above, and then expanding on how it actually happens here (and maybe in > QUIC below--I don't end up after reading these sections with a clear sense of > how QUIC/SPDY fit into connection establishment, even at a high level. Of > course, given that it was in my OKRs to document exactly that this quarter, you > might consider that my failure, not the documentations :-}.) I don't think you're not reading it quite right - SPDY negotation is part of SSL negotiation, so we have to pull the information if SPDY was negotiated from out of the SSL socket. I've tried to clarify both SSL and QUIC discussion. Please do tell me if you still find it confusing. I don't want to go into too much detail here, but I think understanding at least the basics of how this stuff work is generally useful when triaging bugs (Which I consider one of the two main goals of this doc - getting an overview of the stack, and learning just enough to be able to help direct bugs to the right bucket). In the case of proxies, we actually can have an SSL on top of SPDY, but I don't think that's what you're referring to. Actually, we could have SPDY (server) on top of SSL (server) on top of SPDY (proxy) on top of SSL (proxy). Those are kind of vaguely hinted at in the proxy section, but going into the details is probably a bit too much for this document, and you suggested not going into detail on SPDY/QUIC proxies. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:453: HttpStreamFactoryImpl::Job will be created for SPDY, and will be raced against On 2015/07/13 15:39:31, rdsmith wrote: > Did you mean QUIC in this sentence? Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:457: TODO(mmenke): Discuss SPDY/QUIC proxies? On 2015/07/13 15:39:31, rdsmith wrote: > FWIW, I don't think that's necessary in this document, though proxies having > their own document would be a great and wonderful thing. Removed.
I like it. LGTM modulo nits below. Thanks very much for doing this--I think this'll be a very useful long-term document for us to have. (It still has the wall of text feeling to it-I want diagrams or a table of contents just to break things up a bit. But it is accessible with a bit of focus, and it's *really* useful once you engage in that way. So definitely, let's get it into the tree; if we want to we can work on improvements in the future.) https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:1: # Life of a URLRequest On 2015/07/13 19:57:23, mmenke wrote: > On 2015/07/13 15:39:30, rdsmith wrote: > > Suggestion: There's enough in this doc that a table of contents might be > useful. > > It doesn't look to me like markdown has a way to automatically generate a table > of contents, and I'm pretty reluctant to do it manually, since it seems the sort > of thing destined to become outdated. Yeah, if there's no way to do it automatically, it's a bad idea. Oh, well. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:16: # Anatomy of the Network Stack On 2015/07/13 19:57:24, mmenke wrote: > On 2015/07/13 15:39:31, rdsmith wrote: > > From my perspective, *the* most important object in the network stack is the > > URLRequest, and URLRequestContext and HttpNetworkSession are supporting > > infrastructure (admittedly important supporting infrastructure :-}). So I > find > > the lack of mention in it in the Anatomy of the network stack section a bit > > weird; maybe just mention it as the primary consumer interface to the network > > stack, created by a call to URLRequestContext::CreateRequest()? > > I've added a paragraph about it (After I discuss the contexts - I think the term > "network stack" more closely corresponds to the URLRequestContext than a > URLRequest, so it makes sense as the first thing to mention). > > I'm not sure exactly what to say about the URLRequest. Suggestions welcome. What you have looks good to me, though I'd also add a line about "The URLRequest is the primary interface used by consumers of the network stack." or something like that. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:358: aren't compressed in the cache, either. This behavior can also create problems On 2015/07/13 19:57:24, mmenke wrote: > On 2015/07/13 15:39:31, rdsmith wrote: > > nit, suggestion: Remove "also"; I don't think this document has identified > other > > problems with caching uncompressed data. > > Done (Though the fact the cache isn't compressed does seem like a waste of space > to me). It totally is, and if you want to modify this section to add that as a problem and keep the "also" I'm happy with that change too :-}. https://codereview.chromium.org/1211003003/diff/210001/net/docs/life-of-a-url... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1211003003/diff/210001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:137: ResourceDispatchers in all child processes, not just renderer process. nit: "renderer process*es*" https://codereview.chromium.org/1211003003/diff/210001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:351: The URLRequestHttpJob then checks with the URLRequest if the request should be nit: Should this be "... if the redirect should be followed."?
Thanks for the feedback! https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:16: # Anatomy of the Network Stack On 2015/07/14 16:35:50, rdsmith wrote: > On 2015/07/13 19:57:24, mmenke wrote: > > On 2015/07/13 15:39:31, rdsmith wrote: > > > From my perspective, *the* most important object in the network stack is the > > > URLRequest, and URLRequestContext and HttpNetworkSession are supporting > > > infrastructure (admittedly important supporting infrastructure :-}). So I > > find > > > the lack of mention in it in the Anatomy of the network stack section a bit > > > weird; maybe just mention it as the primary consumer interface to the > network > > > stack, created by a call to URLRequestContext::CreateRequest()? > > > > I've added a paragraph about it (After I discuss the contexts - I think the > term > > "network stack" more closely corresponds to the URLRequestContext than a > > URLRequest, so it makes sense as the first thing to mention). > > > > I'm not sure exactly what to say about the URLRequest. Suggestions welcome. > > What you have looks good to me, though I'd also add a line about "The URLRequest > is the primary interface used by consumers of the network stack." or something > like that. Done. https://codereview.chromium.org/1211003003/diff/130003/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:358: aren't compressed in the cache, either. This behavior can also create problems On 2015/07/14 16:35:50, rdsmith wrote: > On 2015/07/13 19:57:24, mmenke wrote: > > On 2015/07/13 15:39:31, rdsmith wrote: > > > nit, suggestion: Remove "also"; I don't think this document has identified > > other > > > problems with caching uncompressed data. > > > > Done (Though the fact the cache isn't compressed does seem like a waste of > space > > to me). > > It totally is, and if you want to modify this section to add that as a problem > and keep the "also" I'm happy with that change too :-}. Left it as-is. https://codereview.chromium.org/1211003003/diff/210001/net/docs/life-of-a-url... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1211003003/diff/210001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:137: ResourceDispatchers in all child processes, not just renderer process. On 2015/07/14 16:35:50, rdsmith wrote: > nit: "renderer process*es*" Done. https://codereview.chromium.org/1211003003/diff/210001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:351: The URLRequestHttpJob then checks with the URLRequest if the request should be On 2015/07/14 16:35:50, rdsmith wrote: > nit: Should this be "... if the redirect should be followed."? Done.
I think the content is very clear now. I still need time to fully digest all, but I think it will be a great reference when I start to explore more in net/. I printed the doc in html. A h2 and a h3 don't really look very different to me. Will numbering make the structure clearer so it's easier to scan the doc quickly? e.g. ## 1 Socket Pools ### 1.1 Socket Pool Layering ### 1.2 SSL ## 2 Proxies https://codereview.chromium.org/1211003003/diff/230001/net/docs/life-of-a-url... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1211003003/diff/230001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:51: error occurs, its canceled, or a final response is received, with a (possibly nit: it's https://codereview.chromium.org/1211003003/diff/230001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:419: instance, SSL and HTTP connections won't grouped together in the nit: be grouped.
On 2015/07/14 19:22:32, xunjieli wrote: > I think the content is very clear now. I still need time to fully digest all, > but I think it will be a great reference when I start to explore more in net/. > > I printed the doc in html. A h2 and a h3 don't really look very different to me. > Will numbering make the structure clearer so it's easier to scan the doc > quickly? > > e.g. > > ## 1 Socket Pools > > ### 1.1 Socket Pool Layering > ### 1.2 SSL > > ## 2 Proxies This is another cases of a feature that would have to be kept up to date manually, and I'm worried about adding additional maintenance overhead and another potential source of bugs. Inserting a new section would require renumbering every section after it manually. https://codereview.chromium.org/1211003003/diff/230001/net/docs/life-of-a-url... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1211003003/diff/230001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:51: error occurs, its canceled, or a final response is received, with a (possibly On 2015/07/14 19:22:32, xunjieli wrote: > nit: it's Done. https://codereview.chromium.org/1211003003/diff/230001/net/docs/life-of-a-url... net/docs/life-of-a-url-request.md:419: instance, SSL and HTTP connections won't grouped together in the On 2015/07/14 19:22:32, xunjieli wrote: > nit: be grouped. Done.
On 2015/07/14 19:29:44, mmenke wrote: > On 2015/07/14 19:22:32, xunjieli wrote: > > I think the content is very clear now. I still need time to fully digest all, > > but I think it will be a great reference when I start to explore more in net/. > > > > > I printed the doc in html. A h2 and a h3 don't really look very different to > me. > > Will numbering make the structure clearer so it's easier to scan the doc > > quickly? > > > > e.g. > > > > ## 1 Socket Pools > > > > ### 1.1 Socket Pool Layering > > ### 1.2 SSL > > > > ## 2 Proxies > > This is another cases of a feature that would have to be kept up to date > manually, and I'm worried about adding additional maintenance overhead and > another potential source of bugs. Inserting a new section would require > renumbering every section after it manually. Compared to the table of contents, numbering the sections is less likely to be out of date, since it's easier to spot during the review cycle. But I think I can live without it. Thanks for the work! LGTM.
On 2015/07/14 19:36:51, xunjieli wrote: > On 2015/07/14 19:29:44, mmenke wrote: > > On 2015/07/14 19:22:32, xunjieli wrote: > > > I think the content is very clear now. I still need time to fully digest > all, > > > but I think it will be a great reference when I start to explore more in > net/. > > > > > > > > I printed the doc in html. A h2 and a h3 don't really look very different to > > me. > > > Will numbering make the structure clearer so it's easier to scan the doc > > > quickly? > > > > > > e.g. > > > > > > ## 1 Socket Pools > > > > > > ### 1.1 Socket Pool Layering > > > ### 1.2 SSL > > > > > > ## 2 Proxies > > > > This is another cases of a feature that would have to be kept up to date > > manually, and I'm worried about adding additional maintenance overhead and > > another potential source of bugs. Inserting a new section would require > > renumbering every section after it manually. > > Compared to the table of contents, numbering the sections is less likely to be > out of date, since it's easier to spot during the review cycle. But I think I > can live without it. Thanks for the work! LGTM. Definitely true, though it's less localized, the the issue is more not correctly updating the labels, or missing one...Or deciding not to try and improve the doc due to overhead. I'll land as-is, for now. If others ask for it, can reconsider. Thanks for all the feedback!
The CQ bit was checked by mmenke@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/1211003003/#ps250001 (title: "Response to Helen's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211003003/250001
Message was sent while issue was closed.
Committed patchset #12 (id:250001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/2ac8cb50a0c9d284b57db8f3de71a71d4a45c6e8 Cr-Commit-Position: refs/heads/master@{#338732} |
