Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2501113002:
Improve net-internals crash course. (Closed)
DescriptionImprove net-internals crash course.
Committed: https://crrev.com/16cf6923451b77fd75f669dcf5632d143e5d98b1
Cr-Commit-Position: refs/heads/master@{#437884}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Re: #3. #Patch Set 3 : Accord indefinite article with pronunciation not spelling of HTTP. #Patch Set 4 : Expand on dt. #
Total comments: 28
Patch Set 5 : Re: #10. #Patch Set 6 : Nit. #Messages
Total messages: 24 (8 generated)
bnc@chromium.org changed reviewers: + rdsmith@chromium.org
Randy: would you please have free cycles to take a look at this? Thank you.
I'd recommend two things: a) make sure you actually look at the final .md after md->HTML translation; sometimes the formatting looks naively good but produces bad HTML. See README.txt for details on how to do that. b) Ask someone without much experience on the network stack to review this as well to see what I've missed because I (basically :-}) know net-internals already. Beyond that, LGTM; you can treat the below comments as suggestions (because they are :-}). https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... File net/docs/crash-course-in-net-internals.md (left): https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:107: about:net-internals. FWIW, I find this a more natural place to mention the "?" hover functionality than in the event/source discussion. https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... File net/docs/crash-course-in-net-internals.md (right): https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:57: source. Suggestion (if you agree): I might make a stronger statement, that large time gaps usually means that the time is being spent in the context of another source. https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:63: event has no parameters, then they are collapsed in a single line. Is the dt not present if the end event hasn't yet occurred? https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:65: Some other events only occur at a single point in time. Suggestion: "... and will not have a sign prefix." https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:69: sources to represent the parallelism. FYI (not actionable): I've repeatedly wished for (and once tried to do as a 20% project) a visualization where next to the source list would be effectively a gaant chart showing how long each source was active in relationship to other sources. You could imagine extending that to include some graphical indication of begin/end events within a source. https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:94: for one for QUIC, and one for HTTP. One of the final events of this source, duplicate "one for" https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:107: reusing existing ones. I'd suggest reformatting this paragraph as a bulleted list.
Randy: PTAL. (a) I used the Markdown Reader Chrome extension, which might not give the exact same result as md_browser, so thank you for pointing me to the right direction. (Of course md_reader does not conform exactly to https://gerrit.googlesource.com/gitiles/+/master/Documentation/markdown.md either.) (b) Yes I will. Thank you. On 2016/11/15 23:18:42, Randy Smith - Not in Mondays wrote: > I'd recommend two things: a) make sure you actually look at the final .md after > md->HTML translation; sometimes the formatting looks naively good but produces > bad HTML. See README.txt for details on how to do that. b) Ask someone without > much experience on the network stack to review this as well to see what I've > missed because I (basically :-}) know net-internals already. > > Beyond that, LGTM; you can treat the below comments as suggestions (because they > are :-}). > > https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... > File net/docs/crash-course-in-net-internals.md (left): > > https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... > net/docs/crash-course-in-net-internals.md:107: about:net-internals. > FWIW, I find this a more natural place to mention the "?" hover functionality > than in the event/source discussion. > > https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... > File net/docs/crash-course-in-net-internals.md (right): > > https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... > net/docs/crash-course-in-net-internals.md:57: source. > Suggestion (if you agree): I might make a stronger statement, that large time > gaps usually means that the time is being spent in the context of another > source. > > https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... > net/docs/crash-course-in-net-internals.md:63: event has no parameters, then they > are collapsed in a single line. > Is the dt not present if the end event hasn't yet occurred? > > https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... > net/docs/crash-course-in-net-internals.md:65: Some other events only occur at a > single point in time. > Suggestion: "... and will not have a sign prefix." > > https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... > net/docs/crash-course-in-net-internals.md:69: sources to represent the > parallelism. > FYI (not actionable): I've repeatedly wished for (and once tried to do as a 20% > project) a visualization where next to the source list would be effectively a > gaant chart showing how long each source was active in relationship to other > sources. You could imagine extending that to include some graphical indication > of begin/end events within a source. > > https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... > net/docs/crash-course-in-net-internals.md:94: for one for QUIC, and one for > HTTP. One of the final events of this source, > duplicate "one for" > > https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... > net/docs/crash-course-in-net-internals.md:107: reusing existing ones. > I'd suggest reformatting this paragraph as a bulleted list.
Publishing comments: https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... File net/docs/crash-course-in-net-internals.md (left): https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:107: about:net-internals. On 2016/11/15 23:18:41, Randy Smith - Not in Mondays wrote: > FWIW, I find this a more natural place to mention the "?" hover functionality > than in the event/source discussion. Done. https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... File net/docs/crash-course-in-net-internals.md (right): https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:57: source. On 2016/11/15 23:18:42, Randy Smith - Not in Mondays wrote: > Suggestion (if you agree): I might make a stronger statement, that large time > gaps usually means that the time is being spent in the context of another > source. Done. https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:63: event has no parameters, then they are collapsed in a single line. On 2016/11/15 23:18:42, Randy Smith - Not in Mondays wrote: > Is the dt not present if the end event hasn't yet occurred? I am not sure there is any added value in spelling it out here, but I can add the following two sentences if you think it is worthwhile: If the end event is not present and net-log is still capturing, "dt=?" is displayed. If capturing was stopped before an end event was logged, a value followed by a + sign is displayed, for example, "dt=120+", which means that capturing was stopped 120 ms after the begin event. Note that strictly speaking the statement "if the end event is present, then the begin event has a dt value" is correct nonetheless. https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:65: Some other events only occur at a single point in time. On 2016/11/15 23:18:41, Randy Smith - Not in Mondays wrote: > Suggestion: "... and will not have a sign prefix." Done. https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:69: sources to represent the parallelism. On 2016/11/15 23:18:41, Randy Smith - Not in Mondays wrote: > FYI (not actionable): I've repeatedly wished for (and once tried to do as a 20% > project) a visualization where next to the source list would be effectively a > gaant chart showing how long each source was active in relationship to other > sources. You could imagine extending that to include some graphical indication > of begin/end events within a source. Acknowledged. https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:94: for one for QUIC, and one for HTTP. One of the final events of this source, On 2016/11/15 23:18:41, Randy Smith - Not in Mondays wrote: > duplicate "one for" Done. https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:107: reusing existing ones. On 2016/11/15 23:18:41, Randy Smith - Not in Mondays wrote: > I'd suggest reformatting this paragraph as a bulleted list. Done.
Still LGTM; thanks! https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... File net/docs/crash-course-in-net-internals.md (right): https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:63: event has no parameters, then they are collapsed in a single line. On 2016/11/16 14:12:51, Bence wrote: > On 2016/11/15 23:18:42, Randy Smith - Not in Mondays wrote: > > Is the dt not present if the end event hasn't yet occurred? > > I am not sure there is any added value in spelling it out here, but I can add > the following two sentences if you think it is worthwhile: > > If the end event is not present and net-log is still capturing, "dt=?" is > displayed. If capturing was stopped before an end event was logged, a value > followed by a + sign is displayed, for example, "dt=120+", which means that > capturing was stopped 120 ms after the begin event. > > Note that strictly speaking the statement "if the end event is present, then the > begin event has a dt value" is correct nonetheless. Certainly a fair point (whether or not the extra detail is worth including). My inclination is to say that explaining the "dt=n+" syntax is worthwhile but the dt=? syntax is pretty obvious, but up to you.
Thank you. https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... File net/docs/crash-course-in-net-internals.md (right): https://codereview.chromium.org/2501113002/diff/1/net/docs/crash-course-in-ne... net/docs/crash-course-in-net-internals.md:63: event has no parameters, then they are collapsed in a single line. On 2016/11/16 14:17:25, Randy Smith - Not in Mondays wrote: > On 2016/11/16 14:12:51, Bence wrote: > > On 2016/11/15 23:18:42, Randy Smith - Not in Mondays wrote: > > > Is the dt not present if the end event hasn't yet occurred? > > > > I am not sure there is any added value in spelling it out here, but I can add > > the following two sentences if you think it is worthwhile: > > > > If the end event is not present and net-log is still capturing, "dt=?" is > > displayed. If capturing was stopped before an end event was logged, a value > > followed by a + sign is displayed, for example, "dt=120+", which means that > > capturing was stopped 120 ms after the begin event. > > > > Note that strictly speaking the statement "if the end event is present, then > the > > begin event has a dt value" is correct nonetheless. > > Certainly a fair point (whether or not the extra detail is worth including). My > inclination is to say that explaining the "dt=n+" syntax is worthwhile but the > dt=? syntax is pretty obvious, but up to you. I agree. Not mentioning what happens in live view is also more robust against a potential removal of the live view feature. :)
bnc@chromium.org changed reviewers: + lilyhoughton@chromium.org
Lily: PTAL. Thank you.
https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... File net/docs/crash-course-in-net-internals.md (right): https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:17: The top level network stack object is the URLRequestContext. The Events View >The Events View Mention what this is (i.e., the views are listed in the leftmost column)? https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:37: lifetime. When looking at the code, a NetLogWithSource object contains a source >When looking at the code, What is the point of this clause? https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:40: Sources show up in the left column of the Event View. Sources that include an >Sources show up in the left column of the Event View. Afaict, Before an event is actually selected, this column appears to be the right column. (The actual right column being completely empty, and this being oriented left of the list of Views). Perhaps clarify this? https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:47: Events corresponding to selected sources show up in the right column, organized >selected sources Maybe emphasize/clarify the need to and process of selecting [=clicking on] sources? It was very easy to skip over this on my first read-through. https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:54: different sources ordered by time. Large time gaps in the event list of a >Event View does not feature showing events from different sources ordered by time. It does appear to feature showing events from different sources simultaneously, sorted by source, though. https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:69: prefix. >Some other events only occur at a single point in time, and will not have a sign prefix. Do these events also have a dt, or does that apply only to collapsed beginning+end events? https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:77: Examples include NETWORK_CHANGED, DNS_CONFIG_CHANGED, and PROXY_CONFIG_CHANGED. >Examples Of global events, right? https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:123: pool uses. A successful CONNECT_JOB returns a SOCKET. The events here vary a What does it mean for a source to be successful? https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:137: * HOST_RESOLVER_IMPL_JOB: These correspond to HostResolverImpl::Job. The The -> They? https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:144: related events. >which lets you jump between the two related events. Elaborate on this? https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:169: down into other related sources, or up from layers below URL_REQUEST. >up from layers below URL_REQUEST. This phrasing is a little confusing - it's saying that you can drill up layers, but no higher than URL_REQUEST, right? https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:173: that the event names used in net-internals are not the entire string names, so >event names used in net-internals are not the entire string names Not sure if it's necessary, but it might be worth elaborating on what this is/why this is the case? And the string name is the name of the event as used in the codebase, correct? https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:194: (reused) socket, what was the previous request that used the socket, how long >what was the previous request that used the socket Is it possible to get this information from net_internals? https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:205: a CONNECT_JOB actually connects. This is so the highest priority pending job >highest priority pending job Referring to the HTTP_STREAM_JOB, not the CONNECT_JOB?
lilyhoughton@: Thank you for the very thorough review. PTAL. https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... File net/docs/crash-course-in-net-internals.md (right): https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:17: The top level network stack object is the URLRequestContext. The Events View On 2016/12/01 18:26:32, lilyhoughton wrote: > >The Events View > > Mention what this is (i.e., the views are listed in the leftmost column)? Done. https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:37: lifetime. When looking at the code, a NetLogWithSource object contains a source On 2016/12/01 18:26:32, lilyhoughton wrote: > >When looking at the code, > What is the point of this clause? Nothing. I guess I'm better off removing it. https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:40: Sources show up in the left column of the Event View. Sources that include an On 2016/12/01 18:26:32, lilyhoughton wrote: > >Sources show up in the left column of the Event View. > Afaict, Before an event is actually selected, this column appears to be the > right column. (The actual right column being completely empty, and this being > oriented left of the list of Views). Perhaps clarify this? Done. https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:54: different sources ordered by time. Large time gaps in the event list of a On 2016/12/01 18:26:32, lilyhoughton wrote: > >Event View does not feature showing events from different sources ordered by > time. > It does appear to feature showing events from different sources simultaneously, > sorted by source, though. Correct. I replaced the word "organized" with "sorted" in the previous paragraph to clarify this. I am not sure it is worth repeating here though. https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:69: prefix. On 2016/12/01 18:26:32, lilyhoughton wrote: > >Some other events only occur at a single point in time, and will not have a > sign prefix. > Do these events also have a dt, or does that apply only to collapsed > beginning+end events? No duration. Added clarification to the text. https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:77: Examples include NETWORK_CHANGED, DNS_CONFIG_CHANGED, and PROXY_CONFIG_CHANGED. On 2016/12/01 18:26:32, lilyhoughton wrote: > >Examples > Of global events, right? Correct. Added clarification to text. https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:123: pool uses. A successful CONNECT_JOB returns a SOCKET. The events here vary a On 2016/12/01 18:26:32, lilyhoughton wrote: > What does it mean for a source to be successful? I guess that it signals OK (and not ERR_*) (via ConnectJob::Delegate::OnConnectJobComplete()). Or one can just take this sentence as a definition of "successful", that is, a ConnectJob is successful if it returns a socket (via ConnectJob::PassSocket()). https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:137: * HOST_RESOLVER_IMPL_JOB: These correspond to HostResolverImpl::Job. The On 2016/12/01 18:26:32, lilyhoughton wrote: > The -> They? Done. https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:144: related events. On 2016/12/01 18:26:32, lilyhoughton wrote: > >which lets you jump between the two related events. > Elaborate on this? Done. https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:169: down into other related sources, or up from layers below URL_REQUEST. On 2016/12/01 18:26:32, lilyhoughton wrote: > >up from layers below URL_REQUEST. > This phrasing is a little confusing - it's saying that you can drill up layers, > but no higher than URL_REQUEST, right? Indeed this is confusing. I agree with your interpretation. I guess it was phrased this way because one cannot go further up from URL_REQUEST which is the topmost layer, only from layers that are below it. I do not think this adds any value, so I'm chopping off half of this sentence. https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:173: that the event names used in net-internals are not the entire string names, so On 2016/12/01 18:26:32, lilyhoughton wrote: > >event names used in net-internals are not the entire string names > Not sure if it's necessary, but it might be worth elaborating on what this > is/why this is the case? > > And the string name is the name of the event as used in the codebase, correct? This is outdated, I am removing the last sentence and merging the previous one into the previous paragraph. It used to be that enum values had SOURCE_ and EVENT_ prefix in the codebase, but not since NetLogSourceType and NetLogEventType have been converted to enum classes at https://crrev.com/2315613002. https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:194: (reused) socket, what was the previous request that used the socket, how long On 2016/12/01 18:26:32, lilyhoughton wrote: > >what was the previous request that used the socket > Is it possible to get this information from net_internals? I believe so, I added a sentence here. https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:205: a CONNECT_JOB actually connects. This is so the highest priority pending job On 2016/12/01 18:26:32, lilyhoughton wrote: > >highest priority pending job > Referring to the HTTP_STREAM_JOB, not the CONNECT_JOB? I think this sentence is correct. When ConnectJob connects, it calls HandOutSocket, which hands out the shiny new socket to ClientSocketHandle that HttpStreamFactoryImpl::Job owns. This is how HttpStreamFactoryImpl::Job gets associated with the socket.
https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... File net/docs/crash-course-in-net-internals.md (right): https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... net/docs/crash-course-in-net-internals.md:205: a CONNECT_JOB actually connects. This is so the highest priority pending job On 2016/12/05 17:58:01, Bence wrote: > On 2016/12/01 18:26:32, lilyhoughton wrote: > > >highest priority pending job > > Referring to the HTTP_STREAM_JOB, not the CONNECT_JOB? > > I think this sentence is correct. When ConnectJob connects, it calls > HandOutSocket, which hands out the shiny new socket to ClientSocketHandle that > HttpStreamFactoryImpl::Job owns. This is how HttpStreamFactoryImpl::Job gets > associated with the socket. Specifically, it isn't super clear to me what "the highest priority pending job" is referring to.
On 2016/12/09 19:56:09, lilyhoughton wrote: > https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... > File net/docs/crash-course-in-net-internals.md (right): > > https://codereview.chromium.org/2501113002/diff/60001/net/docs/crash-course-i... > net/docs/crash-course-in-net-internals.md:205: a CONNECT_JOB actually connects. > This is so the highest priority pending job > On 2016/12/05 17:58:01, Bence wrote: > > On 2016/12/01 18:26:32, lilyhoughton wrote: > > > >highest priority pending job > > > Referring to the HTTP_STREAM_JOB, not the CONNECT_JOB? > > > > I think this sentence is correct. When ConnectJob connects, it calls > > HandOutSocket, which hands out the shiny new socket to ClientSocketHandle that > > HttpStreamFactoryImpl::Job owns. This is how HttpStreamFactoryImpl::Job gets > > associated with the socket. > > Specifically, it isn't super clear to me what "the highest priority pending job" > is referring to. Oh I understand your question now. Sorry. When HttpStreamFactoryImpl::Job tries to connect, it calls InitSocketHandleForHttpRequest(), which calls ClientSocketHandle::Init(). That calls ClientSocketPool::RequestSocket(). That creates a ClientSocketPoolBase::Request, and calls ClientSocketPoolBaseHelper::RequestSocket(). (There's a few more steps in between.) If there is no warm socket available, then ClientSocketPoolBaseHelper::Group::InsertPendingRequest() is called. This could happen for multiple requests within the same group ("group" in the socket limit sense), with different priorities. Priorities are plumbed through all of these function calls. Once a socket is available, ClientSocketPoolBaseHelper::Group::PopNextPendingRequest() finds the highest priority ClientSocketPoolBase::Request, and the callbacks are called, which then eventually notify HttpStreamFactoryImpl::Job. So I believe that you are right, "highest priority pending job" refers to HTTP_STREAM_JOB.
That being fixed, LGTM
lilyhoughton@google.com changed reviewers: + lilyhoughton@google.com
The CQ bit was checked by bnc@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/2501113002/#ps100001 (title: "Nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481557459915740,
"parent_rev": "eb6c88c0deff43369721791ef1e088595401182c", "commit_rev":
"4e887c9dc1192de2673da07d66247c11f4846f26"}
Message was sent while issue was closed.
Description was changed from ========== Improve net-internals crash course. ========== to ========== Improve net-internals crash course. Review-Url: https://codereview.chromium.org/2501113002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Improve net-internals crash course. Review-Url: https://codereview.chromium.org/2501113002 ========== to ========== Improve net-internals crash course. Committed: https://crrev.com/16cf6923451b77fd75f669dcf5632d143e5d98b1 Cr-Commit-Position: refs/heads/master@{#437884} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/16cf6923451b77fd75f669dcf5632d143e5d98b1 Cr-Commit-Position: refs/heads/master@{#437884} |
