Create a separate NetLog source type QUIC_STREAM_FACTORY_JOB

Create a separate NetLog source type QUIC_STREAM_FACTORY_JOB
for each QuicStreamFactory::Job created.

BUG=702411
Review-Url: https://codereview.chromium.org/2800023002
Cr-Commit-Position: refs/heads/master@{#463023}
Committed: https://chromium.googlesource.com/chromium/src/+/96b7a82c70628fa03e8972668fe26310fc1e52ba

[Zhongyi Shi, 2017-04-06 23:50:17]

zhongyi@chromium.org changed reviewers:
+ eroman@chromium.org, rch@chromium.org

[Zhongyi Shi, 2017-04-06 23:50:17]

A follow-up CL will add more NetLogs in QUIC_STREAM_FACTORY_JOB to log the
QuicStreamFactory::Job state machine. 

A sample net-log file is at https://goo.gl/qxbjiA. PTAL, thanks!

[Ryan Hamilton, 2017-04-06 23:55:12]

lgtm, but please wait for eroman since he commonly notices net log subtleties
that I miss.

https://codereview.chromium.org/2800023002/diff/1/net/quic/chromium/quic_stre...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2800023002/diff/1/net/quic/chromium/quic_stre...
net/quic/chromium/quic_stream_factory.cc:403:
net_log.source().ToEventParametersCallback());
Can you add comments to these. Also can you put the two "net_log_" lines next to
each other instead of wrapping the "net_log" line, just to make it easier to
parse mentally.

[Zhongyi Shi, 2017-04-07 00:17:42]

eroman@: Eric, ptal!

rch@: thanks for review :)

https://codereview.chromium.org/2800023002/diff/1/net/quic/chromium/quic_stre...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2800023002/diff/1/net/quic/chromium/quic_stre...
net/quic/chromium/quic_stream_factory.cc:403:
net_log.source().ToEventParametersCallback());
On 2017/04/06 23:55:11, Ryan Hamilton wrote:
> Can you add comments to these. Also can you put the two "net_log_" lines next
to
> each other instead of wrapping the "net_log" line, just to make it easier to
> parse mentally.

Done. That's a good idea, sorry about the inconvenience.

[Ryan Hamilton, 2017-04-07 00:20:32]

lgtm

https://codereview.chromium.org/2800023002/diff/1/net/quic/chromium/quic_stre...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2800023002/diff/1/net/quic/chromium/quic_stre...
net/quic/chromium/quic_stream_factory.cc:403:
net_log.source().ToEventParametersCallback());
On 2017/04/07 00:17:42, Zhongyi Shi wrote:
> On 2017/04/06 23:55:11, Ryan Hamilton wrote:
> > Can you add comments to these. Also can you put the two "net_log_" lines
next
> to
> > each other instead of wrapping the "net_log" line, just to make it easier to
> > parse mentally.
> 
> Done. That's a good idea, sorry about the inconvenience. 

Thanks for doing this work!

[eroman, 2017-04-07 01:55:29]

lgtm

https://codereview.chromium.org/2800023002/diff/20001/net/log/net_log_event_t...
File net/log/net_log_event_type_list.h (right):

https://codereview.chromium.org/2800023002/diff/20001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:1097: // Identifies the NetLogSource() for the
QuicStreamFactory::Job that the
side-note: I wonder why we have parenthesis after NetLogSource() on all of
these? I'll follow up on this, my guess is a mistake that got copy-pasted
everywhere :)

https://codereview.chromium.org/2800023002/diff/20001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:1654: // Identifies the NetLogSoure() for the
HttpStreamFactoryImpl::Job that the
typo: NetLogSource

https://codereview.chromium.org/2800023002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2800023002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:395: std::string server_id =
key_.server_id().ToString();
optional: If you want to avoid constructing this string in the case when logging
is not enabled (in other words the default case), consider adding a separate
callback function that internalizes this.

I'll leave it up to you if you think it is worth it (just a couple lines of code
to do so).

[Zhongyi Shi, 2017-04-07 03:08:53]

eroman@: Thanks a lot for the suggestions, change to use a separate callback.
PTAL!

https://codereview.chromium.org/2800023002/diff/20001/net/log/net_log_event_t...
File net/log/net_log_event_type_list.h (right):

https://codereview.chromium.org/2800023002/diff/20001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:1097: // Identifies the NetLogSource() for the
QuicStreamFactory::Job that the
On 2017/04/07 01:55:29, eroman wrote:
> side-note: I wonder why we have parenthesis after NetLogSource() on all of
> these? I'll follow up on this, my guess is a mistake that got copy-pasted
> everywhere :)

Oh, yeah, at least I copied that from HTTP_STREAM_JOB_BOUND_TO_REQUEST.
*facepalm*

https://codereview.chromium.org/2800023002/diff/20001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:1654: // Identifies the NetLogSoure() for the
HttpStreamFactoryImpl::Job that the
On 2017/04/07 01:55:29, eroman wrote:
> typo: NetLogSource

Done.

https://codereview.chromium.org/2800023002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2800023002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:395: std::string server_id =
key_.server_id().ToString();
On 2017/04/07 01:55:29, eroman wrote:
> optional: If you want to avoid constructing this string in the case when
logging
> is not enabled (in other words the default case), consider adding a separate
> callback function that internalizes this.
> 
> I'll leave it up to you if you think it is worth it (just a couple lines of
code
> to do so).

Thanks for the suggestion, change to use a separate callback.

[eroman, 2017-04-07 19:16:40]

https://codereview.chromium.org/2800023002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2800023002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:403: std::string server_id =
key_.server_id().ToString();
Delete this.

https://codereview.chromium.org/2800023002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:406:
base::Bind(&NetLogQuicStreamFactoryJobCallback, key_.server_id()));
This is still making a copy of the server ID (just now it is part of the bound
function).

Either pass a pointer, or base::ConstRef(key_.server_id()), or pass a pointer.

I prefer making NetLogQuicStreamFactoryJobCallback() take a const pointer, that
way it is harder for callers to inadvertently make copies when binding the
function.

[Zhongyi Shi, 2017-04-07 20:33:56]

eroman@: ptal, thanks!

https://codereview.chromium.org/2800023002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2800023002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:403: std::string server_id =
key_.server_id().ToString();
On 2017/04/07 19:16:40, eroman wrote:
> Delete this.

Whoops, forgot to delete this.

https://codereview.chromium.org/2800023002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:406:
base::Bind(&NetLogQuicStreamFactoryJobCallback, key_.server_id()));
On 2017/04/07 19:16:40, eroman wrote:
> This is still making a copy of the server ID (just now it is part of the bound
> function).
> 
> Either pass a pointer, or base::ConstRef(key_.server_id()), or pass a pointer.
> 
> I prefer making NetLogQuicStreamFactoryJobCallback() take a const pointer,
that
> way it is harder for callers to inadvertently make copies when binding the
> function.

Done.

[eroman, 2017-04-07 20:43:29]

lgtm

[Zhongyi Shi, 2017-04-07 20:44:55]

The CQ bit was checked by zhongyi@chromium.org

[Zhongyi Shi, 2017-04-07 20:44:55]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/2800023002/#ps60001
(title: "address #8")

[commit-bot: I haz the power, 2017-04-07 20:45:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 22:37:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491597895656450,
"parent_rev": "45739334a54b563922c9af81bfa2519787d453fa", "commit_rev":
"96b7a82c70628fa03e8972668fe26310fc1e52ba"}

[commit-bot: I haz the power, 2017-04-07 22:38:31]

Description was changed from

==========
Create a separate NetLog source type QUIC_STREAM_FACTORY_JOB
for each QuicStreamFactory::Job created.

BUG=702411
==========

to

==========
Create a separate NetLog source type QUIC_STREAM_FACTORY_JOB
for each QuicStreamFactory::Job created.

BUG=702411

Review-Url: https://codereview.chromium.org/2800023002
Cr-Commit-Position: refs/heads/master@{#463023}
Committed:
https://chromium.googlesource.com/chromium/src/+/96b7a82c70628fa03e8972668fe2...
==========

[commit-bot: I haz the power, 2017-04-07 22:38:39]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/96b7a82c70628fa03e8972668fe2...