Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1859793002:
Include class relationship diagrams in network stack documentation. (Closed)
|
Created:
4 years, 8 months ago by Randy Smith (Not in Mondays) Modified:
4 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInclude class relationship diagrams in network stack documentation.
This CL includes class relationship diagrams for most of the
classes mentioned in life_of_a_url_request.md and a sketch of the
object relationships inside socket pools. It also removes the net_docs
target (which wasn't really being used) and adds information for debugging
markdown changes and updating SVG files from dot source.
BUG=None
R=eroman@chromium.org
R=mmenke@chromium.org
Committed: https://crrev.com/fb2fd16bec430431971d14658ef3800b23f0ab3f
Cr-Commit-Position: refs/heads/master@{#385934}
Committed: https://crrev.com/6bc403b64d692b7510525254acecfd40abef1d3b
Cr-Commit-Position: refs/heads/master@{#386113}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Initial response to comments. #
Total comments: 6
Patch Set 3 : Incorporated comments. #Patch Set 4 : Cleaned up and regularized .dot files. #
Total comments: 27
Patch Set 5 : Incorporated comments from Matt's detailed review. #Patch Set 6 : Incorporated final diagram comments. #Patch Set 7 : Put net_docs.py back for Cronet. #
Messages
Total messages: 38 (9 generated)
Eric: I'd like you to do a first pass review on this, since you were the person who primarily encouraged me in adding the diagrams to the network stack docs (that's how I remember it, anyway :-}). Could you take a look at this, specifically focussing on whether you think these diagrams are useful for helping people understand what's going on in the network stack, and giving your opinion about the document organization (location of diagrams, relationship to surrounding text, whether they should be inline, whether they should be inline but shrunk and people can click on them to get an expanded version). Matt: This is a cc FYI. I'll be asking you for a detailed review, but after Eric's done a first pass. I want to clean up and comment the .dot source; right now it's a mess whose only virtue is it produces pretty pictures. We may not have a .dot style guide, but I'd still like it better than that before landing. So no comments on the source files, yet :-}, just on the diagrams and text. Thanks!
https://codereview.chromium.org/1859793002/diff/1/net/docs/README.txt File net/docs/README.txt (right): https://codereview.chromium.org/1859793002/diff/1/net/docs/README.txt#newcode4 net/docs/README.txt:4: PYTHONPATH=../../third_party python -m markdown <input>.md > output.html This is not platform independent.
https://codereview.chromium.org/1859793002/diff/1/net/docs/README.txt File net/docs/README.txt (right): https://codereview.chromium.org/1859793002/diff/1/net/docs/README.txt#newcode4 net/docs/README.txt:4: PYTHONPATH=../../third_party python -m markdown <input>.md > output.html On 2016/04/04 21:19:26, mmenke wrote: > This is not platform independent. I was going to ask you about that in the detailed review :-}. I presume it's ok to just put a Windows version in here too? I think it would be something like: set PYTHONPATH=..\..\third_party python -m markdown -f <output>.html <input>.md That an ok plan? (I haven't tested the above, but I expect it would work and I'll make sure it's tested before landing.)
haven't reviewed the build file changes yet, some quick comments on the docs. https://codereview.chromium.org/1859793002/diff/1/net/docs/life-of-a-url-requ... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1859793002/diff/1/net/docs/life-of-a-url-requ... net/docs/life-of-a-url-request.md:442: shown diagramatically for the lowest layer socket pool spelling: diagrammatically as an aside, I initially read that as "dramatically" and was getting all excited for what followed. but then i realized my error and the magic moment was gone. https://codereview.chromium.org/1859793002/diff/1/net/docs/life-of-a-url-requ... net/docs/life-of-a-url-request.md:453: When socket pools are initialized, they in term initialize their should it be "in term" or "in turn"? Asking because I genuinely don't know, but am more familiar with "in turn". https://codereview.chromium.org/1859793002/diff/1/net/docs/life-of-a-url-requ... net/docs/life-of-a-url-request.md:455: should create connect jobs. That object must drive from "must drive" ? must derive? https://codereview.chromium.org/1859793002/diff/1/net/docs/life-of-a-url-requ... net/docs/life-of-a-url-request.md:464: API to the initialization of the socket pool. and for the reader's head to explode :) https://codereview.chromium.org/1859793002/diff/1/net/docs/pools.dot File net/docs/pools.dot (right): https://codereview.chromium.org/1859793002/diff/1/net/docs/pools.dot#newcode1 net/docs/pools.dot:1: digraph SocketPools { does this need a liscence header at the top?
https://codereview.chromium.org/1859793002/diff/1/net/docs/README.txt File net/docs/README.txt (right): https://codereview.chromium.org/1859793002/diff/1/net/docs/README.txt#newcode4 net/docs/README.txt:4: PYTHONPATH=../../third_party python -m markdown <input>.md > output.html On 2016/04/04 21:26:50, Randy Smith - Not in Fridays wrote: > On 2016/04/04 21:19:26, mmenke wrote: > > This is not platform independent. > > I was going to ask you about that in the detailed review :-}. I presume it's ok > to just put a Windows version in here too? I think it would be something like: > > set PYTHONPATH=..\..\third_party > python -m markdown -f <output>.html <input>.md > > That an ok plan? (I haven't tested the above, but I expect it would work and > I'll make sure it's tested before landing.) > SGTM, and that does indeed work on Windows.
https://codereview.chromium.org/1859793002/diff/1/net/net.gypi File net/net.gypi (left): https://codereview.chromium.org/1859793002/diff/1/net/net.gypi#oldcode2012 net/net.gypi:2012: 'net_docs_script': 'tools/net_docs/net_docs.py', We should remove net_docs.py, too, right?
Thanks for the very quick feedback! Comments incorporated. But most of these are detail level comments--please don't ignore the big picture (should we do this at all/is it useful/how could it be more useful) in doing the detailed review. https://codereview.chromium.org/1859793002/diff/1/net/docs/README.txt File net/docs/README.txt (right): https://codereview.chromium.org/1859793002/diff/1/net/docs/README.txt#newcode4 net/docs/README.txt:4: PYTHONPATH=../../third_party python -m markdown <input>.md > output.html On 2016/04/04 21:31:12, mmenke wrote: > On 2016/04/04 21:26:50, Randy Smith - Not in Fridays wrote: > > On 2016/04/04 21:19:26, mmenke wrote: > > > This is not platform independent. > > > > I was going to ask you about that in the detailed review :-}. I presume it's > ok > > to just put a Windows version in here too? I think it would be something > like: > > > > set PYTHONPATH=..\..\third_party > > python -m markdown -f <output>.html <input>.md > > > > That an ok plan? (I haven't tested the above, but I expect it would work and > > I'll make sure it's tested before landing.) > > > > SGTM, and that does indeed work on Windows. Excellent. In it goes. https://codereview.chromium.org/1859793002/diff/1/net/docs/life-of-a-url-requ... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1859793002/diff/1/net/docs/life-of-a-url-requ... net/docs/life-of-a-url-request.md:442: shown diagramatically for the lowest layer socket pool On 2016/04/04 21:29:18, eroman wrote: > spelling: diagrammatically Done. > as an aside, I initially read that as "dramatically" and was getting all excited > for what followed. but then i realized my error and the magic moment was gone. Sorry for the let-down. I suspect it's a consistent feature of reviewing my code :-}. https://codereview.chromium.org/1859793002/diff/1/net/docs/life-of-a-url-requ... net/docs/life-of-a-url-request.md:453: When socket pools are initialized, they in term initialize their On 2016/04/04 21:29:18, eroman wrote: > should it be "in term" or "in turn"? > Asking because I genuinely don't know, but am more familiar with "in turn". Yup, it's "in turn". I should have put more energy into the writing; it was an afterthought once I got the diagrams in ok shape :-J. https://codereview.chromium.org/1859793002/diff/1/net/docs/life-of-a-url-requ... net/docs/life-of-a-url-request.md:455: should create connect jobs. That object must drive from On 2016/04/04 21:29:18, eroman wrote: > "must drive" ? must derive? Yep; fixed. https://codereview.chromium.org/1859793002/diff/1/net/docs/life-of-a-url-requ... net/docs/life-of-a-url-request.md:464: API to the initialization of the socket pool. On 2016/04/04 21:29:18, eroman wrote: > and for the reader's head to explode :) Ah, comprehension has been achieved! :-} Backing up a sec, this is very much one of the things I want you to review. I started out with pretty pictures I created for my own understanding of some particular problem I was chasing at the time, and am now trying to make those useful for other folks. But that doesn't mean the pictures (or the picture description) is actually useful; we might be better off just not having the socket pool stuff I've put together in the code base. I think if you need to do any work *inside* socket pools, the complicated derivation and relationship structure is important to understand, but if you just need to understand the interface, it's probably better not to know. And even in the first case, I don't know if I provide *enough* information to be useful; I haven't gone into any of the gotchas involved in our complex layering (mostly because I don't understand them, I just know they're there). So: Should the above paragraph and it's related picture be in the docs, or are we better off without? https://codereview.chromium.org/1859793002/diff/1/net/docs/pools.dot File net/docs/pools.dot (right): https://codereview.chromium.org/1859793002/diff/1/net/docs/pools.dot#newcode1 net/docs/pools.dot:1: digraph SocketPools { On 2016/04/04 21:29:18, eroman wrote: > does this need a liscence header at the top? Probably. I'll put it on my todo list, for when I clean up the files. Think we're ok with the SVGs not having a license header? https://codereview.chromium.org/1859793002/diff/1/net/net.gypi File net/net.gypi (left): https://codereview.chromium.org/1859793002/diff/1/net/net.gypi#oldcode2012 net/net.gypi:2012: 'net_docs_script': 'tools/net_docs/net_docs.py', On 2016/04/04 21:34:38, mmenke wrote: > We should remove net_docs.py, too, right? Right, thanks, missed that. Done.
Ping? (This CL should not be considered high priority, but I don't want it to fall off radar screens either, and it got such quick responses on the first round I'm worried that's what happened.)
On 2016/04/06 16:54:03, Randy Smith - Not in Fridays wrote: > Ping? > > (This CL should not be considered high priority, but I don't want it to fall off > radar screens either, and it got such quick responses on the first round I'm > worried that's what happened.) (I'll also note that I'm doing some work that is enhancing my understanding of the socket pools, so I'm open to feedback of "pull out the socket pool doc and do that in a separate CL once you know what you're talking about" if either of you feel like giving that feedback :-}.)
On 2016/04/06 16:55:02, Randy Smith - Not in Fridays wrote: > On 2016/04/06 16:54:03, Randy Smith - Not in Fridays wrote: > > Ping? > > > > (This CL should not be considered high priority, but I don't want it to fall > off > > radar screens either, and it got such quick responses on the first round I'm > > worried that's what happened.) > > (I'll also note that I'm doing some work that is enhancing my understanding of > the socket pools, so I'm open to feedback of "pull out the socket pool doc and > do that in a separate CL once you know what you're talking about" if either of > you feel like giving that feedback :-}.) Are you pinging Eric or me? I assumed it was in Eric's court, given your initial message.
On 2016/04/06 17:05:48, mmenke wrote: > On 2016/04/06 16:55:02, Randy Smith - Not in Fridays wrote: > > On 2016/04/06 16:54:03, Randy Smith - Not in Fridays wrote: > > > Ping? > > > > > > (This CL should not be considered high priority, but I don't want it to fall > > off > > > radar screens either, and it got such quick responses on the first round I'm > > > worried that's what happened.) > > > > (I'll also note that I'm doing some work that is enhancing my understanding of > > the socket pools, so I'm open to feedback of "pull out the socket pool doc and > > do that in a separate CL once you know what you're talking about" if either of > > you feel like giving that feedback :-}.) > > Are you pinging Eric or me? I assumed it was in Eric's court, given your > initial message. Quite right, sorry for the confusion. Eric: Ping? :-}
lgtm https://codereview.chromium.org/1859793002/diff/1/net/docs/life-of-a-url-requ... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1859793002/diff/1/net/docs/life-of-a-url-requ... net/docs/life-of-a-url-request.md:464: API to the initialization of the socket pool. On 2016/04/04 21:44:50, Randy Smith - Not in Fridays wrote: > On 2016/04/04 21:29:18, eroman wrote: > > and for the reader's head to explode :) > > Ah, comprehension has been achieved! :-} > > Backing up a sec, this is very much one of the things I want you to review. I > started out with pretty pictures I created for my own understanding of some > particular problem I was chasing at the time, and am now trying to make those > useful for other folks. But that doesn't mean the pictures (or the picture > description) is actually useful; we might be better off just not having the > socket pool stuff I've put together in the code base. I think if you need to do > any work *inside* socket pools, the complicated derivation and relationship > structure is important to understand, but if you just need to understand the > interface, it's probably better not to know. And even in the first case, I > don't know if I provide *enough* information to be useful; I haven't gone into > any of the gotchas involved in our complex layering (mostly because I don't > understand them, I just know they're there). > > So: Should the above paragraph and it's related picture be in the docs, or are > we better off without? We are better off with the docs IMO. Thanks! It risks becoming stale, but let's see how it works :) https://codereview.chromium.org/1859793002/diff/20001/net/docs/README.txt File net/docs/README.txt (right): https://codereview.chromium.org/1859793002/diff/20001/net/docs/README.txt#new... net/docs/README.txt:20: dot dot -Tsvg <name>.dot > <name>.svg nit: this is indented more than the other command lines. https://codereview.chromium.org/1859793002/diff/20001/net/docs/life-of-a-url-... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1859793002/diff/20001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:459: ClientSocketPoolBase::ConnectJobFactory<TransportSocketParams>.) Note that angle brackets are interpreted as HTML in markdown, so this needs to be escaped. Searching the document I see we have the problem higher up too, and confirmed that on https://chromium.googlesource.com/chromium/src/+/master/net/docs/life-of-a-ur... it is not rendering as desired. I expect the fix is < >
https://codereview.chromium.org/1859793002/diff/20001/net/docs/life-of-a-url-... File net/docs/life-of-a-url-request.md (right): https://codereview.chromium.org/1859793002/diff/20001/net/docs/life-of-a-url-... net/docs/life-of-a-url-request.md:459: ClientSocketPoolBase::ConnectJobFactory<TransportSocketParams>.) On 2016/04/06 17:41:17, eroman wrote: > Note that angle brackets are interpreted as HTML in markdown, so this needs to > be escaped. > > Searching the document I see we have the problem higher up too, and confirmed > that on > https://chromium.googlesource.com/chromium/src/+/master/net/docs/life-of-a-ur... > it is not rendering as desired. > > I expect the fix is < > (Another fix would be to make it a `codeblock`, since angle brackets inside of codeblocks is fine, but that would be inconsistent with rest of this doc)
Thanks, Eric!
Matt, I think this is ready for your detailed review. A couple of notes:
* License headers. I noticed that none of the docs in this directory have
license information in them. I opted to stay consistent with that in this
patchset, but if you'd like me to be consistent going the other way (i.e. put
license information in all the docs) just let me know. (If we go this way, let
me know whether you think we need license info in the .svg files as well--that
would be a moderate pain, mostly in that it complicates the process of updating
those files.)
* I created a mini-style guide for myself for .dot files and made the files
match it. You are absolutely welcome to not review at this level of detail, but
if you choose to, I thought it might be useful to be explicit about what that
style guide was, so you could figure out if strange things were intended or
accidental. It was:
* A file may have multiple sections picked for clarity and grouping.
* The legend should always be the first section.
* Within each section, lines should occur in the following order:
** Declaration of nodes (all nodes should be declared).
** Inheritance relationships
** Ownership relationships
** Associative relationships
** Generative relationships
* Use of a "subgraph { rank=same; ...}" wrapping for generative relationships is
optional and should be used if it enhances the readability of the resulting
diagram.
** Multiple relationships (..e {A,B,C}->D) may be declared on the same line only
for the inheritance relationship of sibling subclasses to their parent class.
** The 80 char line limit should be respected.
Let me know what you think.
https://codereview.chromium.org/1859793002/diff/20001/net/docs/README.txt
File net/docs/README.txt (right):
https://codereview.chromium.org/1859793002/diff/20001/net/docs/README.txt#new...
net/docs/README.txt:20: dot dot -Tsvg <name>.dot > <name>.svg
On 2016/04/06 17:41:17, eroman wrote:
> nit: this is indented more than the other command lines.
Done.
https://codereview.chromium.org/1859793002/diff/20001/net/docs/life-of-a-url-...
File net/docs/life-of-a-url-request.md (right):
https://codereview.chromium.org/1859793002/diff/20001/net/docs/life-of-a-url-...
net/docs/life-of-a-url-request.md:459:
ClientSocketPoolBase::ConnectJobFactory<TransportSocketParams>.)
On 2016/04/06 17:41:17, eroman wrote:
> Note that angle brackets are interpreted as HTML in markdown, so this needs to
> be escaped.
>
> Searching the document I see we have the problem higher up too, and confirmed
> that on
>
https://chromium.googlesource.com/chromium/src/+/master/net/docs/life-of-a-ur...
> it is not rendering as desired.
>
> I expect the fix is < >
Done.
https://codereview.chromium.org/1859793002/diff/20001/net/docs/life-of-a-url-...
net/docs/life-of-a-url-request.md:459:
ClientSocketPoolBase::ConnectJobFactory<TransportSocketParams>.)
On 2016/04/06 18:11:07, eroman wrote:
> On 2016/04/06 17:41:17, eroman wrote:
> > Note that angle brackets are interpreted as HTML in markdown, so this needs
to
> > be escaped.
> >
> > Searching the document I see we have the problem higher up too, and
confirmed
> > that on
> >
>
https://chromium.googlesource.com/chromium/src/+/master/net/docs/life-of-a-ur...
> > it is not rendering as desired.
> >
> > I expect the fix is < >
>
> (Another fix would be to make it a `codeblock`, since angle brackets inside of
> codeblocks is fine, but that would be inconsistent with rest of this doc)
Acknowledged.
On 2016/04/06 20:57:55, Randy Smith - Not in Fridays wrote:
> Thanks, Eric!
>
> Matt, I think this is ready for your detailed review. A couple of notes:
>
> * License headers. I noticed that none of the docs in this directory have
> license information in them. I opted to stay consistent with that in this
> patchset, but if you'd like me to be consistent going the other way (i.e. put
> license information in all the docs) just let me know. (If we go this way,
let
> me know whether you think we need license info in the .svg files as well--that
> would be a moderate pain, mostly in that it complicates the process of
updating
> those files.)
Looks like md files in src/docs don't have license headers, either, so I'd say
just follow that patter (I'm not a lawyer, nor do I know the logic behind
license headers in every file, particularly when the licenses are as permissive
as ours).
> * I created a mini-style guide for myself for .dot files and made the files
> match it. You are absolutely welcome to not review at this level of detail,
but
> if you choose to, I thought it might be useful to be explicit about what that
> style guide was, so you could figure out if strange things were intended or
> accidental. It was:
> * A file may have multiple sections picked for clarity and grouping.
> * The legend should always be the first section.
> * Within each section, lines should occur in the following order:
> ** Declaration of nodes (all nodes should be declared).
> ** Inheritance relationships
> ** Ownership relationships
> ** Associative relationships
> ** Generative relationships
> * Use of a "subgraph { rank=same; ...}" wrapping for generative relationships
is
> optional and should be used if it enhances the readability of the resulting
> diagram.
> ** Multiple relationships (..e {A,B,C}->D) may be declared on the same line
only
> for the inheritance relationship of sibling subclasses to their parent class.
> ** The 80 char line limit should be respected.
>
> Let me know what you think.
https://codereview.chromium.org/1859793002/diff/60001/net/docs/pools.dot
File net/docs/pools.dot (right):
https://codereview.chromium.org/1859793002/diff/60001/net/docs/pools.dot#newc...
net/docs/pools.dot:73: [arrowhead=diamond, color=red];
Why is this red? The cast?
https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.dot
File net/docs/url_request.dot (right):
https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do...
net/docs/url_request.dot:31: URLRequestJobManager;
Suggest removing this one - we want to get rid of it, and it's not terribly
exciting.
URLRequestJobFactory is more relevant, but it's not currently used directly by
the URLRequestJob, so probably not worth adding int, in its current state.
https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do...
net/docs/url_request.dot:34: URLRequestJob_Others [label="...others..."];
Don't suppose we could squeeze in a more complete description -- something like
other job types? We do have that subtype arrow, which does indicate that's the
case, but it involves a greater cognitive load.
https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do...
net/docs/url_request.dot:35: URLRequestHttpJob;
There's no link between the URLRequestHttpJob and the HttpTransactionFactory...I
guess there's no "Calls into factory" link type?
https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do...
net/docs/url_request.dot:66: HttpTransaction_Others [label="...others..."];
The only others here are dev tool's network simulation thing, which I think is
beyond the scope of this document, and a mock class for tests. Worth removing?
https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do...
net/docs/url_request.dot:95: HttpStreamFactoryImpl_Job
[label="HttpStreamFactoryImpl::Job"];
When we create two jobs, they talk to each other (We block the
non-alternate-protocol-job on the alternate-protocol-job). Not sure if it's
worth showing that link, or if it makes sense, given the set of link types.
https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do...
net/docs/url_request.dot:101: HttpBasicState;
Wonder if it's even worth including this one.
https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do...
net/docs/url_request.dot:144: TCPClientSocket;
Seems a little weird that you have nothing creating these - there is a layer of
indirection there (TransportConnectJobHelper calls into a socket factory or
something?), so maybe that's why.
https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do...
net/docs/url_request.dot:165: [arrowhead=diamond, label=TransportSocketParams];
SSLClientSocketPool should have this same relation, no?
https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do...
net/docs/url_request.dot:183: TransportClientSocketPool -> ClientSocketHandle
[arrowhead=veevee];
This one is incorrect - ClientSocketHandles are created by
HttpStreamFactoryImpl::Jobs. They're also created by ConnectJobs for most
(all?) socket pool types *except* the TransportClientSocketPool.
I believe I've addressed all your comments; PTAL and tell me what you think? https://codereview.chromium.org/1859793002/diff/60001/net/docs/pools.dot File net/docs/pools.dot (right): https://codereview.chromium.org/1859793002/diff/60001/net/docs/pools.dot#newc... net/docs/pools.dot:73: [arrowhead=diamond, color=red]; On 2016/04/07 16:00:11, mmenke wrote: > Why is this red? The cast? Whoops, sorry, put that back into the legend. It's a scoped_refptr<> partial ownership line. https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.dot File net/docs/url_request.dot (right): https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:31: URLRequestJobManager; On 2016/04/07 16:00:11, mmenke wrote: > Suggest removing this one - we want to get rid of it, and it's not terribly > exciting. > > URLRequestJobFactory is more relevant, but it's not currently used directly by > the URLRequestJob, so probably not worth adding int, in its current state. Done. https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:34: URLRequestJob_Others [label="...others..."]; On 2016/04/07 16:00:11, mmenke wrote: > Don't suppose we could squeeze in a more complete description -- something like > other job types? We do have that subtype arrow, which does indicate that's the > case, but it involves a greater cognitive load. Doesn't seem to hurt the diagram too much; done. Do you want me to do this for the other uses of "...others..." in the diagram, which mean pretty much the same thing (for their separate base classes)? https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:35: URLRequestHttpJob; On 2016/04/07 16:00:11, mmenke wrote: > There's no link between the URLRequestHttpJob and the HttpTransactionFactory...I > guess there's no "Calls into factory" link type? Yeah. This diagram is a (semi- :-}) static object relationship diagram rather than any kind of call graph. I think it would be too busy with that extra information in it, but I can try to add it if you want. https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:66: HttpTransaction_Others [label="...others..."]; On 2016/04/07 16:00:11, mmenke wrote: > The only others here are dev tool's network simulation thing, which I think is > beyond the scope of this document, and a mock class for tests. Worth removing? Done. https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:95: HttpStreamFactoryImpl_Job [label="HttpStreamFactoryImpl::Job"]; On 2016/04/07 16:00:11, mmenke wrote: > When we create two jobs, they talk to each other (We block the > non-alternate-protocol-job on the alternate-protocol-job). Not sure if it's > worth showing that link, or if it makes sense, given the set of link types. I've put in an associative link labeled with the data member names from HttpStreamFactoryImpl::Job to itself; let me know if you think that helps. https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:101: HttpBasicState; On 2016/04/07 16:00:11, mmenke wrote: > Wonder if it's even worth including this one. Well, on the one hand it doesn't really add much, but on the other hand leaving it out would make it harder for the reader to go from diagram to code; they'd search for ClientSocketHandle in HttpBasicStream and not find it. So I'm inclined to leave it in, but I'm happy to hear arguments against that. https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:144: TCPClientSocket; On 2016/04/07 16:00:11, mmenke wrote: > Seems a little weird that you have nothing creating these - there is a layer of > indirection there (TransportConnectJobHelper calls into a socket factory or > something?), so maybe that's why. More lack of knowledge when I originally did the diagram that I didn't re-address. (Scans for instantiations ...) Fascinating and slightly messy. I think I'll say that, at the level of the url_request.dot diagram, it's reasonable to claim that the TransportConnectJob creates the socket. In an ideal world, I'd spell that out in more detail in pools.dot, but there's a lot of detail to spell out. I guess I'd represent it as the helper has a pointer (does not own) to a ClientSocketFactory, which is what generates the TCPClientSocket, and DefaultClientSocketFactory inherits from ClientSocketFactory. I'll try to put that into pools.dot. Let me know if you'd like me to tweak that (either the high level shortcut in url_request.dot or the attempt to represent the details in pools.dot). https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:165: [arrowhead=diamond, label=TransportSocketParams]; On 2016/04/07 16:00:11, mmenke wrote: > SSLClientSocketPool should have this same relation, no? Done. (I had thought of this diagram as doing a simple spike down through the stack, and just going into detail on one path at each layer, thus the focus on the transport level for socket pools. But I agree if I'm going to include SSLClientSocketPool, it should have the same link to ClientSocketPoolBAse.) https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:183: TransportClientSocketPool -> ClientSocketHandle [arrowhead=veevee]; On 2016/04/07 16:00:11, mmenke wrote: > This one is incorrect - ClientSocketHandles are created by > HttpStreamFactoryImpl::Jobs. They're also created by ConnectJobs for most > (all?) socket pool types *except* the TransportClientSocketPool. So one think I've realized in doing these diagrams is how fuzzy the notion of "created" is. Agreed, the new is done at HttpStreamFactoryImpl::Job. But the guts are put in place by lower layers, including for TransportClientSocketPools. Having said all that, yeah, I'm focussing on the transport layer in this diagram, and I think it's truer to say that for that layer the HttpStreamFActoryImpl::Job creates the ClientSocketHandle. Done.
LGTM. Just did a high level review of the text on the diagrams, after my nitty first pass. https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.dot File net/docs/url_request.dot (right): https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:34: URLRequestJob_Others [label="...others..."]; On 2016/04/07 18:31:02, Randy Smith - Not in Fridays wrote: > On 2016/04/07 16:00:11, mmenke wrote: > > Don't suppose we could squeeze in a more complete description -- something > like > > other job types? We do have that subtype arrow, which does indicate that's > the > > case, but it involves a greater cognitive load. > > Doesn't seem to hurt the diagram too much; done. > > Do you want me to do this for the other uses of "...others..." in the diagram, > which mean pretty much the same thing (for their separate base classes)? I think it makes sense to make the other other(s) clearer as well. This one stuck out at me because there's only one sibling - with multiple siblings, it's a bit clearer from context what it actually means. https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:101: HttpBasicState; On 2016/04/07 18:31:02, Randy Smith - Not in Fridays wrote: > On 2016/04/07 16:00:11, mmenke wrote: > > Wonder if it's even worth including this one. > > Well, on the one hand it doesn't really add much, but on the other hand leaving > it out would make it harder for the reader to go from diagram to code; they'd > search for ClientSocketHandle in HttpBasicStream and not find it. So I'm > inclined to leave it in, but I'm happy to hear arguments against that. My thinking here is that we are (Are should be) focusing on a high level overview here, and including it just adds noise when you're trying to get the big picture. I'm fine with keeping it, just thought I'd explain my reasoning. https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:183: TransportClientSocketPool -> ClientSocketHandle [arrowhead=veevee]; On 2016/04/07 18:31:02, Randy Smith - Not in Fridays wrote: > On 2016/04/07 16:00:11, mmenke wrote: > > This one is incorrect - ClientSocketHandles are created by > > HttpStreamFactoryImpl::Jobs. They're also created by ConnectJobs for most > > (all?) socket pool types *except* the TransportClientSocketPool. > > So one think I've realized in doing these diagrams is how fuzzy the notion of > "created" is. Agreed, the new is done at HttpStreamFactoryImpl::Job. But the > guts are put in place by lower layers, including for TransportClientSocketPools. > Having said all that, yeah, I'm focussing on the transport layer in this > diagram, and I think it's truer to say that for that layer the > HttpStreamFActoryImpl::Job creates the ClientSocketHandle. Done. I agree the ownership is really weird here. It may make sense to change creation here so that when you call into the socket pool, it creates the handle...But doesn't seem like a huge priority cleanup.
https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.dot File net/docs/url_request.dot (right): https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:101: HttpBasicState; On 2016/04/07 20:02:25, mmenke wrote: > On 2016/04/07 18:31:02, Randy Smith - Not in Fridays wrote: > > On 2016/04/07 16:00:11, mmenke wrote: > > > Wonder if it's even worth including this one. > > > > Well, on the one hand it doesn't really add much, but on the other hand > leaving > > it out would make it harder for the reader to go from diagram to code; they'd > > search for ClientSocketHandle in HttpBasicStream and not find it. So I'm > > inclined to leave it in, but I'm happy to hear arguments against that. > > My thinking here is that we are (Are should be) focusing on a high level > overview here, and including it just adds noise when you're trying to get the > big picture. I'm fine with keeping it, just thought I'd explain my reasoning. Are should be == Or should be
https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.dot File net/docs/url_request.dot (right): https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:183: TransportClientSocketPool -> ClientSocketHandle [arrowhead=veevee]; On 2016/04/07 20:02:25, mmenke wrote: > On 2016/04/07 18:31:02, Randy Smith - Not in Fridays wrote: > > On 2016/04/07 16:00:11, mmenke wrote: > > > This one is incorrect - ClientSocketHandles are created by > > > HttpStreamFactoryImpl::Jobs. They're also created by ConnectJobs for most > > > (all?) socket pool types *except* the TransportClientSocketPool. > > > > So one think I've realized in doing these diagrams is how fuzzy the notion of > > "created" is. Agreed, the new is done at HttpStreamFactoryImpl::Job. But the > > guts are put in place by lower layers, including for > TransportClientSocketPools. > > Having said all that, yeah, I'm focussing on the transport layer in this > > diagram, and I think it's truer to say that for that layer the > > HttpStreamFActoryImpl::Job creates the ClientSocketHandle. Done. > > I agree the ownership is really weird here. It may make sense to change > creation here so that when you call into the socket pool, it creates the > handle...But doesn't seem like a huge priority cleanup. By changing creation here I mean in the code, not the diagram (Yes, I really should proofread before I send comments, not after...)
On 2016/04/07 20:04:35, mmenke wrote: > https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.dot > File net/docs/url_request.dot (right): > > https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... > net/docs/url_request.dot:183: TransportClientSocketPool -> ClientSocketHandle > [arrowhead=veevee]; > On 2016/04/07 20:02:25, mmenke wrote: > > On 2016/04/07 18:31:02, Randy Smith - Not in Fridays wrote: > > > On 2016/04/07 16:00:11, mmenke wrote: > > > > This one is incorrect - ClientSocketHandles are created by > > > > HttpStreamFactoryImpl::Jobs. They're also created by ConnectJobs for most > > > > (all?) socket pool types *except* the TransportClientSocketPool. > > > > > > So one think I've realized in doing these diagrams is how fuzzy the notion > of > > > "created" is. Agreed, the new is done at HttpStreamFactoryImpl::Job. But > the > > > guts are put in place by lower layers, including for > > TransportClientSocketPools. > > > Having said all that, yeah, I'm focussing on the transport layer in this > > > diagram, and I think it's truer to say that for that layer the > > > HttpStreamFActoryImpl::Job creates the ClientSocketHandle. Done. > > > > I agree the ownership is really weird here. It may make sense to change > > creation here so that when you call into the socket pool, it creates the > > handle...But doesn't seem like a huge priority cleanup. > > By changing creation here I mean in the code, not the diagram (Yes, I really > should proofread before I send comments, not after...) Hey, as long as the meaning makes it across :-}.
rdsmith@chromium.org changed reviewers: + scottmg@chromium.org
Thanks for the review, Matt! Scott: Would you stamp build/gn_migration.gypi? I think they removals there are just goodness (we got rid of the net_docs target). https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.dot File net/docs/url_request.dot (right): https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:34: URLRequestJob_Others [label="...others..."]; On 2016/04/07 20:02:25, mmenke wrote: > On 2016/04/07 18:31:02, Randy Smith - Not in Fridays wrote: > > On 2016/04/07 16:00:11, mmenke wrote: > > > Don't suppose we could squeeze in a more complete description -- something > > like > > > other job types? We do have that subtype arrow, which does indicate that's > > the > > > case, but it involves a greater cognitive load. > > > > Doesn't seem to hurt the diagram too much; done. > > > > Do you want me to do this for the other uses of "...others..." in the diagram, > > which mean pretty much the same thing (for their separate base classes)? > > I think it makes sense to make the other other(s) clearer as well. This one > stuck out at me because there's only one sibling - with multiple siblings, it's > a bit clearer from context what it actually means. Done. https://codereview.chromium.org/1859793002/diff/60001/net/docs/url_request.do... net/docs/url_request.dot:101: HttpBasicState; On 2016/04/07 20:02:25, mmenke wrote: > On 2016/04/07 18:31:02, Randy Smith - Not in Fridays wrote: > > On 2016/04/07 16:00:11, mmenke wrote: > > > Wonder if it's even worth including this one. > > > > Well, on the one hand it doesn't really add much, but on the other hand > leaving > > it out would make it harder for the reader to go from diagram to code; they'd > > search for ClientSocketHandle in HttpBasicStream and not find it. So I'm > > inclined to leave it in, but I'm happy to hear arguments against that. > > My thinking here is that we are (Are should be) focusing on a high level > overview here, and including it just adds noise when you're trying to get the > big picture. I'm fine with keeping it, just thought I'd explain my reasoning. Understood and agreed with the adding noise; I'm just going with keeping it because I want to keep the transition to code clean. Sometimes there isn't a great answer.
build/ lgtm
The CQ bit was checked by rdsmith@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1859793002/#ps100001 (title: "Incorporated final diagram comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1859793002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859793002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Include class relationship diagrams in network stack documentation. This CL includes class relationship diagrams for most of the classes mentioned in life_of_a_url_request.md and a sketch of the object relationships inside socket pools. It also removes the net_docs target (which wasn't really being used) and adds information for debugging markdown changes and updating SVG files from dot source. BUG=None R=eroman@chromium.org R=mmenke@chromium.org ========== to ========== Include class relationship diagrams in network stack documentation. This CL includes class relationship diagrams for most of the classes mentioned in life_of_a_url_request.md and a sketch of the object relationships inside socket pools. It also removes the net_docs target (which wasn't really being used) and adds information for debugging markdown changes and updating SVG files from dot source. BUG=None R=eroman@chromium.org R=mmenke@chromium.org Committed: https://crrev.com/fb2fd16bec430431971d14658ef3800b23f0ab3f Cr-Commit-Position: refs/heads/master@{#385934} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/fb2fd16bec430431971d14658ef3800b23f0ab3f Cr-Commit-Position: refs/heads/master@{#385934}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1875583002/ by rdsmith@chromium.org. The reason for reverting is: Something is still using the net_docs target, which was removed; builds are failing on the bots (but didn't fail on any try jobs above--see green)..
Message was sent while issue was closed.
Description was changed from ========== Include class relationship diagrams in network stack documentation. This CL includes class relationship diagrams for most of the classes mentioned in life_of_a_url_request.md and a sketch of the object relationships inside socket pools. It also removes the net_docs target (which wasn't really being used) and adds information for debugging markdown changes and updating SVG files from dot source. BUG=None R=eroman@chromium.org R=mmenke@chromium.org Committed: https://crrev.com/fb2fd16bec430431971d14658ef3800b23f0ab3f Cr-Commit-Position: refs/heads/master@{#385934} ========== to ========== Include class relationship diagrams in network stack documentation. This CL includes class relationship diagrams for most of the classes mentioned in life_of_a_url_request.md and a sketch of the object relationships inside socket pools. It also removes the net_docs target (which wasn't really being used) and adds information for debugging markdown changes and updating SVG files from dot source. BUG=None R=eroman@chromium.org R=mmenke@chromium.org Committed: https://crrev.com/fb2fd16bec430431971d14658ef3800b23f0ab3f Cr-Commit-Position: refs/heads/master@{#385934} ==========
The CQ bit was checked by rdsmith@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org, mmenke@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1859793002/#ps120001 (title: "Put net_docs.py back for Cronet.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1859793002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859793002/120001
Message was sent while issue was closed.
Description was changed from ========== Include class relationship diagrams in network stack documentation. This CL includes class relationship diagrams for most of the classes mentioned in life_of_a_url_request.md and a sketch of the object relationships inside socket pools. It also removes the net_docs target (which wasn't really being used) and adds information for debugging markdown changes and updating SVG files from dot source. BUG=None R=eroman@chromium.org R=mmenke@chromium.org Committed: https://crrev.com/fb2fd16bec430431971d14658ef3800b23f0ab3f Cr-Commit-Position: refs/heads/master@{#385934} ========== to ========== Include class relationship diagrams in network stack documentation. This CL includes class relationship diagrams for most of the classes mentioned in life_of_a_url_request.md and a sketch of the object relationships inside socket pools. It also removes the net_docs target (which wasn't really being used) and adds information for debugging markdown changes and updating SVG files from dot source. BUG=None R=eroman@chromium.org R=mmenke@chromium.org Committed: https://crrev.com/fb2fd16bec430431971d14658ef3800b23f0ab3f Cr-Commit-Position: refs/heads/master@{#385934} ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Include class relationship diagrams in network stack documentation. This CL includes class relationship diagrams for most of the classes mentioned in life_of_a_url_request.md and a sketch of the object relationships inside socket pools. It also removes the net_docs target (which wasn't really being used) and adds information for debugging markdown changes and updating SVG files from dot source. BUG=None R=eroman@chromium.org R=mmenke@chromium.org Committed: https://crrev.com/fb2fd16bec430431971d14658ef3800b23f0ab3f Cr-Commit-Position: refs/heads/master@{#385934} ========== to ========== Include class relationship diagrams in network stack documentation. This CL includes class relationship diagrams for most of the classes mentioned in life_of_a_url_request.md and a sketch of the object relationships inside socket pools. It also removes the net_docs target (which wasn't really being used) and adds information for debugging markdown changes and updating SVG files from dot source. BUG=None R=eroman@chromium.org R=mmenke@chromium.org Committed: https://crrev.com/fb2fd16bec430431971d14658ef3800b23f0ab3f Cr-Commit-Position: refs/heads/master@{#385934} Committed: https://crrev.com/6bc403b64d692b7510525254acecfd40abef1d3b Cr-Commit-Position: refs/heads/master@{#386113} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6bc403b64d692b7510525254acecfd40abef1d3b Cr-Commit-Position: refs/heads/master@{#386113} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
