Add GetLoadTimingInfo to net::BidirectionalStream

Add GetLoadTimingInfo to net::BidirectionalStream

This CL adds GetLoadTimingInfo to net::BidirectionalStream.
This is a part of the effort to expose timing metrics to
net embedders.

This CL additionally moves |start_time| to BidirectionalStream's
constructor so we measure the true request start time (before
dns, ssl)

BUG=648346
Committed: https://crrev.com/369d092c31f1bec8a61eb2802274bae38e29261a
Cr-Commit-Position: refs/heads/master@{#420676}

[xunjieli, 2016-09-21 13:37:06]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-09-21 13:38:07]

Description was changed from

==========
Add LoadTimingInfo to BidirectionalStream

BUG=
==========

to

==========
Add GetLoadTimingInfo to net::BidirectionalStream

This CL adds GetLoadTimingInfo to net::BidirectionalStream.
This is a part of the effort to expose timing metrics to
net embedders.

BUG=648346
==========

[xunjieli, 2016-09-21 18:18:50]

Patchset #1 (id:20001) has been deleted

[xunjieli, 2016-09-21 18:18:59]

Patchset #1 (id:40001) has been deleted

[xunjieli, 2016-09-21 18:19:57]

Description was changed from

==========
Add GetLoadTimingInfo to net::BidirectionalStream

This CL adds GetLoadTimingInfo to net::BidirectionalStream.
This is a part of the effort to expose timing metrics to
net embedders.

BUG=648346
==========

to

==========
Add GetLoadTimingInfo to net::BidirectionalStream

This CL adds GetLoadTimingInfo to net::BidirectionalStream.
This is a part of the effort to expose timing metrics to
net embedders.

This CL additionally moves |start_time| to BidirectionalStream's
constructor so we measure the true request start time (before
dns, ssl)

BUG=648346
==========

[xunjieli, 2016-09-21 18:22:36]

xunjieli@chromium.org changed reviewers:
+ mgersh@chromium.org

[xunjieli, 2016-09-21 18:22:37]

Miriam, could you take a look? This is one of the plumbing CLs. It should be
pretty straightforward. Let me know if there's anything missing. Thanks!

[xunjieli, 2016-09-22 13:03:54]

xunjieli@chromium.org changed reviewers:
+ mef@chromium.org

[xunjieli, 2016-09-22 13:16:35]

+ Misha,
Hi Misha, Miriam isn't feeling well. Could you take over this review? Thanks!

[xunjieli, 2016-09-23 14:56:43]

Misha, a friendly ping.

[mef, 2016-09-23 15:50:56]

https://codereview.chromium.org/2359493003/diff/60001/net/http/bidirectional_...
File net/http/bidirectional_stream.cc (right):

https://codereview.chromium.org/2359493003/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream.cc:252: // Subclasses should only provide
|connect_timing| and |socket_reused| info,
Technically BidirectionalStreamImpl is not subclass of BidirectionalStream,
right?
Also, it is a bit weird get a copy of LoadTimingInfo and throw away everything
except for two fields.

Maybe replace BidirectionalStreamImpl::GetLoadTimingInfo with
GetConnectTimingInfo which just produces connect timing and socket reused?

[xunjieli, 2016-09-23 16:43:24]

Thanks. PTAL.

https://codereview.chromium.org/2359493003/diff/60001/net/http/bidirectional_...
File net/http/bidirectional_stream.cc (right):

https://codereview.chromium.org/2359493003/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream.cc:252: // Subclasses should only provide
|connect_timing| and |socket_reused| info,
On 2016/09/23 15:50:56, mef wrote:
> Technically BidirectionalStreamImpl is not subclass of BidirectionalStream,
> right?
> Also, it is a bit weird get a copy of LoadTimingInfo and throw away everything
> except for two fields.
> 
> Maybe replace BidirectionalStreamImpl::GetLoadTimingInfo with
> GetConnectTimingInfo which just produces connect timing and socket reused?

I've adjusted the comment to remove the subclass reference.

SpdyStream exposes GetLoadTimingInfo and that's what's BidirectionalStreamImpl
uses. There are some other information (just H2 push), but we are not currently
using it since QUIC doesn't have the counterpart.

I think we should still leave GetLoadTimingInfo. This is a defensive measure so
we don't get any unexpected fields or override the existing
|load_timing_info_|'s fields.

[xunjieli, 2016-09-23 16:53:20]

xunjieli@chromium.org changed reviewers:
- mgersh@chromium.org

[mef, 2016-09-23 17:40:06]

lgtm

https://codereview.chromium.org/2359493003/diff/60001/net/http/bidirectional_...
File net/http/bidirectional_stream.cc (right):

https://codereview.chromium.org/2359493003/diff/60001/net/http/bidirectional_...
net/http/bidirectional_stream.cc:252: // Subclasses should only provide
|connect_timing| and |socket_reused| info,
On 2016/09/23 16:43:24, xunjieli wrote:
> On 2016/09/23 15:50:56, mef wrote:
> > Technically BidirectionalStreamImpl is not subclass of BidirectionalStream,
> > right?
> > Also, it is a bit weird get a copy of LoadTimingInfo and throw away
everything
> > except for two fields.
> > 
> > Maybe replace BidirectionalStreamImpl::GetLoadTimingInfo with
> > GetConnectTimingInfo which just produces connect timing and socket reused?
> 
> I've adjusted the comment to remove the subclass reference.
> 
> SpdyStream exposes GetLoadTimingInfo and that's what's BidirectionalStreamImpl
> uses. There are some other information (just H2 push), but we are not
currently
> using it since QUIC doesn't have the counterpart.
> 
> I think we should still leave GetLoadTimingInfo. This is a defensive measure
so
> we don't get any unexpected fields or override the existing
> |load_timing_info_|'s fields.

Thanks for explanation!

[xunjieli, 2016-09-23 17:45:58]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2016-09-23 17:46:32]

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, 2016-09-23 18:45:34]

Description was changed from

==========
Add GetLoadTimingInfo to net::BidirectionalStream

This CL adds GetLoadTimingInfo to net::BidirectionalStream.
This is a part of the effort to expose timing metrics to
net embedders.

This CL additionally moves |start_time| to BidirectionalStream's
constructor so we measure the true request start time (before
dns, ssl)

BUG=648346
==========

to

==========
Add GetLoadTimingInfo to net::BidirectionalStream

This CL adds GetLoadTimingInfo to net::BidirectionalStream.
This is a part of the effort to expose timing metrics to
net embedders.

This CL additionally moves |start_time| to BidirectionalStream's
constructor so we measure the true request start time (before
dns, ssl)

BUG=648346
==========

[commit-bot: I haz the power, 2016-09-23 18:45:35]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2016-09-23 18:47:24]

Description was changed from

==========
Add GetLoadTimingInfo to net::BidirectionalStream

This CL adds GetLoadTimingInfo to net::BidirectionalStream.
This is a part of the effort to expose timing metrics to
net embedders.

This CL additionally moves |start_time| to BidirectionalStream's
constructor so we measure the true request start time (before
dns, ssl)

BUG=648346
==========

to

==========
Add GetLoadTimingInfo to net::BidirectionalStream

This CL adds GetLoadTimingInfo to net::BidirectionalStream.
This is a part of the effort to expose timing metrics to
net embedders.

This CL additionally moves |start_time| to BidirectionalStream's
constructor so we measure the true request start time (before
dns, ssl)

BUG=648346

Committed: https://crrev.com/369d092c31f1bec8a61eb2802274bae38e29261a
Cr-Commit-Position: refs/heads/master@{#420676}
==========

[commit-bot: I haz the power, 2016-09-23 18:47:26]

Patchset 2 (id:??) landed as
https://crrev.com/369d092c31f1bec8a61eb2802274bae38e29261a
Cr-Commit-Position: refs/heads/master@{#420676}