Include class relationship diagrams in network stack documentation.

net/docs/url_request.dot

Index: net/docs/url_request.dot
diff --git a/net/docs/url_request.dot b/net/docs/url_request.dot
new file mode 100644
index 0000000000000000000000000000000000000000..2b7c29f4fd8afc2a8dbe40b3076800cd56e0f195
--- /dev/null
+++ b/net/docs/url_request.dot
@@ -0,0 +1,185 @@
+digraph URLRequestRoot {
+    subgraph cluster_legend {
+	label="Legend";
+
+	## The following legend is an attempt to match UML notation,
+	## except for template_class and Factory->object, which are
+	## invented for this diagram.
+	BaseClass;
+	SubClass [label="Derived Class"];
+	Whole;
+	Part;
+	A;
+	B;
+	Interface [label="Interface / ABC", style=dashed];
+	template_class [shape=diamond]; # Link will name parameter(s)
+
+	SubClass -> BaseClass [arrowhead="empty"];
+	SubClass -> Interface [arrowhead="empty", style=dashed];
+	Part -> Whole [arrowhead="diamond", label="ownership"];
+	Part -> Whole [arrowhead="odiamond", label="pointer"];
+	A -> B [arrowhead="none", headlabel="?..?", taillabel="?..?",
+		label="association"];
+	// Often a "subgraph { rank=same; .. }" is used to wrap the
+	// below to make the generative relationship distinctive
+	// from the other class relationships.
+	Factory -> object [arrowhead=veevee];
+    };
+
+    ## URLRequest, URLRequestJob, and subclasses
+    URLRequestContext;
+    URLRequestJobManager;

[mmenke, 2016/04/07 16:00:11]

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.

[Randy Smith (Not in Mondays), 2016/04/07 18:31:02]

Done.

+    URLRequest;
+    URLRequestJob [style=dashed];
+    URLRequestJob_Others [label="...others..."];

[mmenke, 2016/04/07 16:00:11]

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.

[Randy Smith (Not in Mondays), 2016/04/07 18:31:02]

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

[mmenke, 2016/04/07 20:02:25]

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.

[Randy Smith (Not in Mondays), 2016/04/07 21:37:14]

Done.

+    URLRequestHttpJob;

[mmenke, 2016/04/07 16:00:11]

There's no link between the URLRequestHttpJob and the HttpTransactionFactory...I
guess there's no "Calls into factory" link type?

[Randy Smith (Not in Mondays), 2016/04/07 18:31:02]

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.

+    Filter;
+
+    {URLRequestHttpJob, URLRequestJob_Others} -> URLRequestJob
+	[arrowhead="empty"];
+
+    URLRequestJob -> URLRequest [arrowhead="diamond"];
+    Filter -> URLRequestJob [arrowhead="diamond"];
+    Filter -> Filter [arrowhead="diamond", taillabel="0..1"];
+
+    subgraph {
+	rank=same;
+	URLRequestContext -> URLRequest [arrowhead=veevee];
+    }
+    subgraph {
+	rank=same;
+	URLRequestJobManager -> URLRequestJob [arrowhead=veevee];
+    }
+    subgraph {
+	rank=same;
+	URLRequestHttpJob -> Filter [arrowhead=veevee];
+    }
+
+
+    ## HttpTransaction, subclasses, and generative classes.
+    HttpTransactionFactory;
+    HttpCache;
+    HttpNetworkLayer;
+    HttpTransaction [style=dashed];
+    HttpCache_Transaction [label="HttpCache::Transaction"];
+    HttpNetworkTransaction;
+    HttpTransaction_Others [label="...others..."];

[mmenke, 2016/04/07 16:00:11]

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?

[Randy Smith (Not in Mondays), 2016/04/07 18:31:02]

Done.

+
+    { HttpNetworkTransaction, HttpCache_Transaction,
+      HttpTransaction_Others } -> HttpTransaction
+	[style=dashed, arrowhead="empty"];
+    { HttpNetworkLayer, HttpCache } -> HttpTransactionFactory
+	[arrowhead=empty, style=dashed];
+
+    HttpTransaction -> HttpCache_Transaction  [arrowhead=diamond];
+    HttpTransaction -> URLRequestHttpJob [arrowhead="diamond"]
+
+    subgraph {
+	rank=same;
+	HttpCache -> HttpCache_Transaction [arrowhead=veevee];
+    }
+    subgraph {
+	rank=same;
+	HttpTransactionFactory -> HttpTransaction [arrowhead=veevee];
+    }
+    subgraph {
+	rank=same;
+	HttpNetworkLayer -> HttpNetworkTransaction [arrowhead=veevee];
+    }
+
+    ## HttpStreamFactory and related.
+    HttpStreamFactoryImpl;
+    HttpStreamRequest [style=dashed];
+    HttpStream [style=dashed];
+    HttpStreamFactoryImpl_Request [label="HttpStreamFactoryImpl::Request"];
+    HttpStreamFactoryImpl_Job [label="HttpStreamFactoryImpl::Job"];

[mmenke, 2016/04/07 16:00:11]

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.

[Randy Smith (Not in Mondays), 2016/04/07 18:31:02]

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.

+    HttpStreamRequest_Delegate
+	[label="HttpStreamRequest::Delegate",style=dashed];
+    HttpBasicStream;
+    QuicHttpStream;
+    SpdyHttpStream;
+    HttpBasicState;

[mmenke, 2016/04/07 16:00:11]

Wonder if it's even worth including this one.

[Randy Smith (Not in Mondays), 2016/04/07 18:31:02]

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.

[mmenke, 2016/04/07 20:02:25]

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.

[mmenke, 2016/04/07 20:03:17]

Are should be == Or should be

[Randy Smith (Not in Mondays), 2016/04/07 21:37:14]

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.

+
+    HttpNetworkTransaction -> HttpStreamRequest_Delegate
+	[style=dashed, arrowhead="empty"];
+    HttpStreamFactoryImpl_Request -> HttpStreamRequest
+	[style=dashed, arrowhead="empty"];
+    { HttpBasicStream, QuicHttpStream, SpdyHttpStream } -> HttpStream
+	[style=dashed, arrowhead="empty"];
+
+    HttpStreamRequest -> HttpNetworkTransaction [arrowhead="diamond"];
+    HttpStream -> HttpNetworkTransaction [arrowhead="diamond"];
+    HttpBasicState -> HttpBasicStream [arrowhead=diamond];
+    HttpStreamFactoryImpl_Job -> HttpStreamFactoryImpl_Request
+	[arrowhead="diamond",taillabel="1..*"];
+
+     HttpStreamRequest_Delegate -> HttpStreamFactoryImpl_Request
+	 [arrowhead=odiamond];
+
+    subgraph {
+	rank=same;
+	HttpStreamFactoryImpl -> HttpStreamRequest [arrowhead=veevee];
+    }
+    subgraph {
+	rank=same;
+	HttpStreamRequest -> HttpStream [arrowhead=veevee];
+    }
+
+    ## ClientSocketHandle and socket pools.
+    ClientSocketPool [style=dashed];
+    TransportClientSocketPool;
+    SSLClientSocketPool;
+    ClientSocketPool_Others [label="...others..."];
+    ClientSocketPoolBase [label="ClientSocketPoolBase", shape=diamond];
+    ClientSocketPoolBaseHelper;
+
+    ConnectJobFactory;
+    ConnectJob [style=dashed];
+    TransportConnectJob;
+    SSLConnectJob;
+    ConnectJob_Others [label="...others..."];
+    ConnectJob_Delegate [label="ConnectJob::Delegate",style=dashed];
+
+    StreamSocket [style=dashed];
+    TCPClientSocket;

[mmenke, 2016/04/07 16:00:11]

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.

[Randy Smith (Not in Mondays), 2016/04/07 18:31:02]

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

+    StreamSocket_Others [label="...others..."];
+
+    TransportConnectJobHelper;
+    SingleRequestHostResolver;
+
+    { SSLClientSocketPool, TransportClientSocketPool,
+      ClientSocketPool_Others} -> ClientSocketPool
+	[style=dashed, arrowhead=empty];
+    ClientSocketPoolBaseHelper -> ConnectJob_Delegate
+	[arrowhead=empty, style=dashed];
+    StreamSocket -> Socket [arrowhead=empty, style=dashed];
+    { TCPClientSocket, StreamSocket_Others } -> StreamSocket
+	[arrowhead=empty, style=dashed];
+    {SSLConnectJob, TransportConnectJob, ConnectJob_Others} -> ConnectJob
+	[style=dashed, arrowhead=empty];
+
+    ClientSocketHandle -> HttpStreamFactoryImpl_Job [arrowhead="diamond"];
+    ClientSocketHandle -> HttpBasicState [arrowhead="diamond"];
+    ClientSocketPoolBaseHelper -> ClientSocketPoolBase [arrowhead=diamond];
+    ClientSocketPoolBase -> TransportClientSocketPool
+	[arrowhead=diamond, label=TransportSocketParams];

[mmenke, 2016/04/07 16:00:11]

SSLClientSocketPool should have this same relation, no?

[Randy Smith (Not in Mondays), 2016/04/07 18:31:02]

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

+
+    StreamSocket -> ClientSocketHandle [arrowhead=diamond];
+    ConnectJobFactory -> ClientSocketPoolBase [arrowhead=diamond];
+    StreamSocket -> ConnectJob [arrowhead=diamond];
+    SingleRequestHostResolver -> TransportConnectJobHelper
+	[arrowhead=diamond];
+    TransportConnectJobHelper -> TransportConnectJob [arrowhead=diamond];
+
+    ClientSocketPool -> ClientSocketHandle  [arrowhead=odiamond];
+    ConnectJob_Delegate -> ConnectJob  [arrowhead=odiamond];
+
+    subgraph {
+	rank=same;
+	ConnectJobFactory -> ConnectJob [arrowhead=veevee];
+    }
+    subgraph {
+	rank=same;
+	TransportClientSocketPool -> ClientSocketHandle [arrowhead=veevee];

[mmenke, 2016/04/07 16:00:11]

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.

[Randy Smith (Not in Mondays), 2016/04/07 18:31:02]

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.

[mmenke, 2016/04/07 20:02:25]

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.

[mmenke, 2016/04/07 20:04:35]

By changing creation here I mean in the code, not the diagram (Yes, I really
should proofread before I send comments, not after...)

+    }
+}