Expose SPDY pushes in DevTools

Expose SPDY pushes in DevTools

This adds push start/end times to LoadTimingInfo and plumbs it all
the way to DevTools Network panel.

BUG=464501
Committed: https://crrev.com/e8340bc90789f2084eb1cb128b87679170a5d59d
Cr-Commit-Position: refs/heads/master@{#388367}

[caseq, 2016-03-24 22:59:16]

caseq@chromium.org changed reviewers:
+ dgozman@chromium.org, mmenke@chromium.org, pfeldman@chromium.org

[caseq, 2016-03-24 22:59:17]

Matt, please take a look at net/ part.
Dmitri, please review the DevTools part.

[mmenke, 2016-03-24 23:06:33]

mmenke@chromium.org changed reviewers:
+ bnc@chromium.org

[mmenke, 2016-03-24 23:06:33]

[+bnc]:  Bence, this more in your neck of the woods than mine.  Mind taking lead
on this one?  I still want to do a pass.

[dgozman, 2016-03-24 23:56:00]

Could you please add a screenshot of UI changes?

[ajayapra, 2016-03-25 00:00:22]

On 2016/03/24 23:56:00, dgozman wrote:
> Could you please add a screenshot of UI changes?

a UI screenshot of how this would look like will be good ...if not I can provide
one as feedback.

[caseq, 2016-03-25 00:09:44]

On 2016/03/25 00:00:22, ajayapra wrote:
> On 2016/03/24 23:56:00, dgozman wrote:
> > Could you please add a screenshot of UI changes?
> 
> a UI screenshot of how this would look like will be good ...if not I can
provide
> one as feedback.

Screenshot:
https://bugs.chromium.org/p/chromium/issues/attachment?aid=228735&inline=1

[caseq, 2016-03-25 01:47:50]

Offline comments from Dmitry and Paul addressed:
- extract Push into a category of its own in waterfall timing breakdown popover
and put it first;
- Rename "Content Download" to "Reading Push" when push is used;

Also, fix inter-process time correction.

[Bence, 2016-03-25 14:42:54]

https://codereview.chromium.org/1828203005/diff/40001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/1828203005/diff/40001/net/spdy/spdy_stream.cc...
net/spdy/spdy_stream.cc:782: load_timing_info->push_end =
How about this instead:
if (done_receiving) push_end = recv_last_byte_time_;

https://codereview.chromium.org/1828203005/diff/40001/net/spdy/spdy_stream_un...
File net/spdy/spdy_stream_unittest.cc (right):

https://codereview.chromium.org/1828203005/diff/40001/net/spdy/spdy_stream_un...
net/spdy/spdy_stream_unittest.cc:343:
EXPECT_TRUE(load_timing_info.push_end.is_null());
Can you also test for push_end (at least that it's not null) after the push is
completed?

[mmenke, 2016-03-25 14:50:28]

https://codereview.chromium.org/1828203005/diff/40001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/1828203005/diff/40001/content/child/resource_...
content/child/resource_dispatcher.cc:636: RemoteToLocalTimeTicks(converter,
&load_timing->push_end);
This is somewhat weird...Since renderer process times != browser process times,
we need some conversion.  What this code does is take two known times from the
renderer, two known times from the browser, and a bunch of times between them,
and linearly interpolate those intermediary times from browser to renderer
times.  This mostly works... at least we don't do anything crazy...  *However* 
push_start and push_end can be outside that range, and if a request is fast, we
could have, say, a renderer delta of 100 milliseconds, and a browser delta of 10
milliseconds.  Then interpolation becomes a real source of error here.

Have you thought about this?

https://codereview.chromium.org/1828203005/diff/40001/net/base/load_timing_in...
File net/base/load_timing_info.h (right):

https://codereview.chromium.org/1828203005/diff/40001/net/base/load_timing_in...
net/base/load_timing_info.h:57: // blocked on each state.
This is kinda weird...We're introducing new behavior here.  In
URLRequest::OnHeadersComplete, we make sure none of these times are before
RequestStart, because we don't trust anything to handle such times correctly. 
However, the times you're adding don't have this behavior, and this comment does
not apply to them.

[dgozman, 2016-03-25 16:25:42]

https://codereview.chromium.org/1828203005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js
(right):

https://codereview.chromium.org/1828203005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:205:
addOffsetRange(WebInspector.RequestTimeRangeNames.Blocking, 0, blocking);
Are you sure there could not be blocking or waiting segments for pushed
resources?

[caseq, 2016-03-25 19:30:32]

ptal -- I changed wording in comments and added another expectation to
spdy_stream_unittest

> https://codereview.chromium.org/1828203005/diff/40001/net/spdy/spdy_stream.cc
> File net/spdy/spdy_stream.cc (right):
> 
>
https://codereview.chromium.org/1828203005/diff/40001/net/spdy/spdy_stream.cc...
> net/spdy/spdy_stream.cc:782: load_timing_info->push_end =
> How about this instead:
> if (done_receiving) push_end = recv_last_byte_time_;

I'm kinda paranoid to return predictable value rather than just leave the field
in indeterminate state in case whatever caller passes as output is reused, so if
you don't mind I'd like to leave this as is.
 
> 
>
https://codereview.chromium.org/1828203005/diff/40001/net/spdy/spdy_stream_un...
> File net/spdy/spdy_stream_unittest.cc (right):
> 
>
https://codereview.chromium.org/1828203005/diff/40001/net/spdy/spdy_stream_un...
> net/spdy/spdy_stream_unittest.cc:343:
> EXPECT_TRUE(load_timing_info.push_end.is_null());
> Can you also test for push_end (at least that it's not null) after the push is
> completed?

Done, though note we already have somewhat better coverage for this in
SpdyNetworkTransactionTest.ServerPush*.

>
https://codereview.chromium.org/1828203005/diff/40001/content/child/resource_...
> content/child/resource_dispatcher.cc:636: RemoteToLocalTimeTicks(converter,
> &load_timing->push_end);
> This is somewhat weird...Since renderer process times != browser process
times,
> we need some conversion.  What this code does is take two known times from the
> renderer, two known times from the browser, and a bunch of times between them,
> and linearly interpolate those intermediary times from browser to renderer
> times.  This mostly works... at least we don't do anything crazy...  *However*

> push_start and push_end can be outside that range, and if a request is fast,
we
> could have, say, a renderer delta of 100 milliseconds, and a browser delta of
10
> milliseconds.  Then interpolation becomes a real source of error here.
>
> Have you thought about this?

So we basically start doing extrapolation instead of interpolation -- I think
this is ok here, cause we basically have two cases, when the push overlaps
actual request, in which case it's important to get push times on the same scale
as other request times to visualize them correctly, and the case when push
happened ahead of the request, when the most important thing about the timing is
that, duh, that the push happened before the request, and getting the actual
times precisely is perhaps less important.

Also, do we have actual range for potential clock skew between processes? I
think this originally comes from h/w per-core time counters being not in sync,
so we're likely have some offset between the two and somewhat less likely to
have a major difference in scale, aren't we?

>
https://codereview.chromium.org/1828203005/diff/40001/net/base/load_timing_in...
> File net/base/load_timing_info.h (right):
>
https://codereview.chromium.org/1828203005/diff/40001/net/base/load_timing_in...
> net/base/load_timing_info.h:57: // blocked on each state.
> This is kinda weird...We're introducing new behavior here.  In
> URLRequest::OnHeadersComplete, we make sure none of these times are before
> RequestStart, because we don't trust anything to handle such times correctly. 
> However, the times you're adding don't have this behavior, and this comment
does
> not apply to them.

Ah, thanks for spotting! I think there's also another problem in that comment
with the section starting at line 55 essentially changing the meaning of what's
said in line 41, while there's a totally unrelated section in between :-) I've
re-worded this to make it a bit more clear.

>
https://codereview.chromium.org/1828203005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js
> (right):
>
https://codereview.chromium.org/1828203005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:205:
> addOffsetRange(WebInspector.RequestTimeRangeNames.Blocking, 0, blocking);
> Are you sure there could not be blocking or waiting segments for pushed
> resources?

Well, if we're getting a push it means the connection was already setup and the
main resource was already started, doesn't it?

[mmenke, 2016-03-25 20:34:08]

On 2016/03/25 19:30:32, caseq wrote:
> ptal -- I changed wording in comments and added another expectation to
> spdy_stream_unittest
> 
> >
https://codereview.chromium.org/1828203005/diff/40001/net/spdy/spdy_stream.cc
> > File net/spdy/spdy_stream.cc (right):
> > 
> >
>
https://codereview.chromium.org/1828203005/diff/40001/net/spdy/spdy_stream.cc...
> > net/spdy/spdy_stream.cc:782: load_timing_info->push_end =
> > How about this instead:
> > if (done_receiving) push_end = recv_last_byte_time_;
> 
> I'm kinda paranoid to return predictable value rather than just leave the
field
> in indeterminate state in case whatever caller passes as output is reused, so
if
> you don't mind I'd like to leave this as is.
>  
> > 
> >
>
https://codereview.chromium.org/1828203005/diff/40001/net/spdy/spdy_stream_un...
> > File net/spdy/spdy_stream_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/1828203005/diff/40001/net/spdy/spdy_stream_un...
> > net/spdy/spdy_stream_unittest.cc:343:
> > EXPECT_TRUE(load_timing_info.push_end.is_null());
> > Can you also test for push_end (at least that it's not null) after the push
is
> > completed?
> 
> Done, though note we already have somewhat better coverage for this in
> SpdyNetworkTransactionTest.ServerPush*.
> 
> >
>
https://codereview.chromium.org/1828203005/diff/40001/content/child/resource_...
> > content/child/resource_dispatcher.cc:636: RemoteToLocalTimeTicks(converter,
> > &load_timing->push_end);
> > This is somewhat weird...Since renderer process times != browser process
> times,
> > we need some conversion.  What this code does is take two known times from
the
> > renderer, two known times from the browser, and a bunch of times between
them,
> > and linearly interpolate those intermediary times from browser to renderer
> > times.  This mostly works... at least we don't do anything crazy... 
*However*
> 
> > push_start and push_end can be outside that range, and if a request is fast,
> we
> > could have, say, a renderer delta of 100 milliseconds, and a browser delta
of
> 10
> > milliseconds.  Then interpolation becomes a real source of error here.
> >
> > Have you thought about this?
> 
> So we basically start doing extrapolation instead of interpolation -- I think
> this is ok here, cause we basically have two cases, when the push overlaps
> actual request, in which case it's important to get push times on the same
scale
> as other request times to visualize them correctly, and the case when push
> happened ahead of the request, when the most important thing about the timing
is
> that, duh, that the push happened before the request, and getting the actual
> times precisely is perhaps less important.
> 
> Also, do we have actual range for potential clock skew between processes? I
> think this originally comes from h/w per-core time counters being not in sync,
> so we're likely have some offset between the two and somewhat less likely to
> have a major difference in scale, aren't we?

So there's clock skew...And since we try to account for clock skew, we suddenly
have latency as an issue as well, since the instructions to get clock time run
at different times (The browser code to get the time could be blocked behind a
bunch of other operations on the IOThread, for instance).  Suppose we have
something in incognito's in-memory cache, so browser time is fast, but the
system is currently hosed due to a lot of file I/O, for instance.  The problem
isn't the skew between the times, so much as the skew between the time ranges.

Here's my suggestion:  If the time is between the browser start/end times, use
linear interpolate.  If it's before the browser start time, use |time -
browser_start + renderer_start|.  That should at least prevent any crazy
scaling, and suffers much less in the worst case, I believe.


> >
>
https://codereview.chromium.org/1828203005/diff/40001/net/base/load_timing_in...
> > File net/base/load_timing_info.h (right):
> >
>
https://codereview.chromium.org/1828203005/diff/40001/net/base/load_timing_in...
> > net/base/load_timing_info.h:57: // blocked on each state.
> > This is kinda weird...We're introducing new behavior here.  In
> > URLRequest::OnHeadersComplete, we make sure none of these times are before
> > RequestStart, because we don't trust anything to handle such times
correctly. 
> > However, the times you're adding don't have this behavior, and this comment
> does
> > not apply to them.
> 
> Ah, thanks for spotting! I think there's also another problem in that comment
> with the section starting at line 55 essentially changing the meaning of
what's
> said in line 41, while there's a totally unrelated section in between :-) I've
> re-worded this to make it a bit more clear.
> 
> >
>
https://codereview.chromium.org/1828203005/diff/40001/third_party/WebKit/Sour...
> > File
third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js
> > (right):
> >
>
https://codereview.chromium.org/1828203005/diff/40001/third_party/WebKit/Sour...
> >
third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:205:
> > addOffsetRange(WebInspector.RequestTimeRangeNames.Blocking, 0, blocking);
> > Are you sure there could not be blocking or waiting segments for pushed
> > resources?
> 
> Well, if we're getting a push it means the connection was already setup and
the
> main resource was already started, doesn't it?

[dgozman, 2016-03-25 21:46:29]

devtools lgtm

[caseq, 2016-03-25 21:52:59]

> Here's my suggestion:  If the time is between the browser
> start/end times, use linear interpolate.  If it's before
> the browser start time, use 
>|time - browser_start + renderer_start|.
> That should at least prevent any crazy scaling, and 
> suffers much less in the worst case, I believe.

Done, ptal -- turns out we even had an asssert in InterProcessTimeTicksConverter
to assure the source value fits the remote time range.

[mmenke, 2016-03-25 21:56:42]

On 2016/03/25 21:52:59, caseq wrote:
> > Here's my suggestion:  If the time is between the browser
> > start/end times, use linear interpolate.  If it's before
> > the browser start time, use 
> >|time - browser_start + renderer_start|.
> > That should at least prevent any crazy scaling, and 
> > suffers much less in the worst case, I believe.
> 
> Done, ptal -- turns out we even had an asssert in
InterProcessTimeTicksConverter
> to assure the source value fits the remote time range.

Guess that means we have no browser tests for SPDY push?  :(

I'll take another look Monday, expect that will be it for me.

[caseq, 2016-03-25 22:03:20]

On 2016/03/25 21:56:42, mmenke wrote:

> 
> Guess that means we have no browser tests for SPDY push?  :(
> 

Yup :( I'll be happy to have some sort of integration test for that, but do we
have an infrastructure to mock the SPDY server in the browser tests?

[mmenke, 2016-03-28 17:57:57]

On 2016/03/25 22:03:20, caseq wrote:
> On 2016/03/25 21:56:42, mmenke wrote:
> 
> > 
> > Guess that means we have no browser tests for SPDY push?  :(
> > 
> 
> Yup :( I'll be happy to have some sort of integration test for that, but do we
> have an infrastructure to mock the SPDY server in the browser tests?

chrome/browser/net/load_timing_browsertest.cc mocks out requests and returns
bogus times, and makes sure they make it to the navigation timing API.

[mmenke, 2016-03-28 18:04:54]

https://codereview.chromium.org/1828203005/diff/80001/content/common/inter_pr...
File content/common/inter_process_time_ticks_converter.cc (right):

https://codereview.chromium.org/1828203005/diff/80001/content/common/inter_pr...
content/common/inter_process_time_ticks_converter.cc:63: return
LocalTimeTicks(local_base_time_ + remote_delta.value_);
Shouldn't this be:

if (remote_ms.value_ < remote_lower_bound_)
  return LocalTimeTicks(local_base_time_ + remote_delta.value_);
if (remote_upper_bound_ < remote_ms.value_)
  return LocalTimeTicks(remote_ms - remove_upper_bound_ + local_upper_bound_);

Or you can still keep the second DCHECK, instead of the second condition -
remove upper bound is calculated immediately before sending the IPC, so no times
should be in front of it.

[mmenke, 2016-03-28 18:06:44]

On 2016/03/28 17:57:57, mmenke wrote:
> On 2016/03/25 22:03:20, caseq wrote:
> > On 2016/03/25 21:56:42, mmenke wrote:
> > 
> > > 
> > > Guess that means we have no browser tests for SPDY push?  :(
> > > 
> > 
> > Yup :( I'll be happy to have some sort of integration test for that, but do
we
> > have an infrastructure to mock the SPDY server in the browser tests?
> 
> chrome/browser/net/load_timing_browsertest.cc mocks out requests and returns
> bogus times, and makes sure they make it to the navigation timing API.

We also have a SPDY test server, I believe, but I'm unfamiliar with it, and not
sure if it supports server push.

[caseq, 2016-04-07 23:49:31]

On 2016/03/28 17:57:57, mmenke wrote:
> On 2016/03/25 22:03:20, caseq wrote:
> > On 2016/03/25 21:56:42, mmenke wrote:
> > 
> > > 
> > > Guess that means we have no browser tests for SPDY push?  :(
> > > 
> > 
> > Yup :( I'll be happy to have some sort of integration test for that, but do
we
> > have an infrastructure to mock the SPDY server in the browser tests?
> 
> chrome/browser/net/load_timing_browsertest.cc mocks out requests and returns
> bogus times, and makes sure they make it to the navigation timing API.

Thanks, I've added the test along the lines of what load_timing_browsertest does
(i.e. mocked on the URLRequestJob level) to devtools_sanity_browsertests, as we
already have similar timing tests there.

[caseq, 2016-04-07 23:50:33]

On 2016/03/28 18:04:54, mmenke wrote:
>
https://codereview.chromium.org/1828203005/diff/80001/content/common/inter_pr...
> File content/common/inter_process_time_ticks_converter.cc (right):
> 
>
https://codereview.chromium.org/1828203005/diff/80001/content/common/inter_pr...
> content/common/inter_process_time_ticks_converter.cc:63: return
> LocalTimeTicks(local_base_time_ + remote_delta.value_);
> Shouldn't this be:
> 
> if (remote_ms.value_ < remote_lower_bound_)
>   return LocalTimeTicks(local_base_time_ + remote_delta.value_);
> if (remote_upper_bound_ < remote_ms.value_)
>   return LocalTimeTicks(remote_ms - remove_upper_bound_ + local_upper_bound_);
> 
> Or you can still keep the second DCHECK, instead of the second condition -
> remove upper bound is calculated immediately before sending the IPC, so no
times
> should be in front of it.

Thanks for spotting, done!

[mmenke, 2016-04-08 16:21:36]

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:174: class
PushTimesMockURLRequestJob : public net::URLRequestHttpJob {
I'd rather this not inherit from URLRequestHttpJob, which it should be noted is
not intended for public consumption (See the "NET_EXPORT_PRIVATE" as a hint). 
We should just inherit from URLRequestMockHTTPJob (Which does require we give
the file path to the mock job).

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:202: void Register() {
Suggest making this static, and having it create the TestInterceptor, and give
the filter ownership of it.  scoped_ptr(this) is just weird.

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:205: url_,
scoped_ptr<net::URLRequestInterceptor>(this));
scoped_ptr<net::URLRequestInterceptor> -> make_scoped_ptr?

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:210: void Unregister() {
Again, suggest making this static.  Does mean you'll need to pass in URL again,
or just clear all handlers.

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:1015:
DispatchOnTestSuite(window_, "testPushTimes", push_url.spec().c_str());
Can we check that the push time is less that send_start...Or better, get a time
before the navigation starts, subtract 10 seconds from it, and check that the
push time is before the earliest normally reported time (navigation start?  Some
other start time?  Not too familiar with the timing APIs)

https://codereview.chromium.org/1828203005/diff/100001/content/common/inter_p...
File content/common/inter_process_time_ticks_converter.cc (right):

https://codereview.chromium.org/1828203005/diff/100001/content/common/inter_p...
content/common/inter_process_time_ticks_converter.cc:59: // For remote times
that come beforeremote time range, apply just time
beforeremote -> before remote

[caseq, 2016-04-08 22:03:06]

Patchset #7 (id:120001) has been deleted

[caseq, 2016-04-08 22:39:28]

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:174: class
PushTimesMockURLRequestJob : public net::URLRequestHttpJob {
On 2016/04/08 16:21:36, mmenke wrote:
> I'd rather this not inherit from URLRequestHttpJob, which it should be noted
is
> not intended for public consumption (See the "NET_EXPORT_PRIVATE" as a hint). 
> We should just inherit from URLRequestMockHTTPJob (Which does require we give
> the file path to the mock job).

Done. Note I also took a liberty to nuke another subclass of it,
URLRequestNonsecureConnection as a drive by.

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:202: void Register() {
On 2016/04/08 16:21:36, mmenke wrote:
> Suggest making this static, and having it create the TestInterceptor, and give
> the filter ownership of it.  scoped_ptr(this) is just weird.

Done.

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:205: url_,
scoped_ptr<net::URLRequestInterceptor>(this));
On 2016/04/08 16:21:36, mmenke wrote:
> scoped_ptr<net::URLRequestInterceptor> -> make_scoped_ptr?

Done.

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:210: void Unregister() {
On 2016/04/08 16:21:36, mmenke wrote:
> Again, suggest making this static.  Does mean you'll need to pass in URL
again,
> or just clear all handlers.

Done.

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:1015:
DispatchOnTestSuite(window_, "testPushTimes", push_url.spec().c_str());
On 2016/04/08 16:21:36, mmenke wrote:
> Can we check that the push time is less that send_start...

That's exactly what happens in testPushTime() -- have you had a chance to look
at Tests.js?

> Or better, get a time
> before the navigation starts, subtract 10 seconds from it, and check that the
> push time is before the earliest normally reported time (navigation start? 
Some
> other start time?  Not too familiar with the timing APIs)

Well, as for navigation start -- I would expect it to always come before the
push time unless the page is being reloaded, since we would normally push only
after the main resource was requested and the connection was opened.

https://codereview.chromium.org/1828203005/diff/100001/content/common/inter_p...
File content/common/inter_process_time_ticks_converter.cc (right):

https://codereview.chromium.org/1828203005/diff/100001/content/common/inter_p...
content/common/inter_process_time_ticks_converter.cc:59: // For remote times
that come beforeremote time range, apply just time
On 2016/04/08 16:21:36, mmenke wrote:
> beforeremote -> before remote

Done.

[mmenke, 2016-04-11 17:45:52]

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:1015:
DispatchOnTestSuite(window_, "testPushTimes", push_url.spec().c_str());
On 2016/04/08 22:39:28, caseq wrote:
> On 2016/04/08 16:21:36, mmenke wrote:
> > Can we check that the push time is less that send_start...
> 
> That's exactly what happens in testPushTime() -- have you had a chance to look
> at Tests.js?
> 
> > Or better, get a time
> > before the navigation starts, subtract 10 seconds from it, and check that
the
> > push time is before the earliest normally reported time (navigation start? 
> Some
> > other start time?  Not too familiar with the timing APIs)
> 
> Well, as for navigation start -- I would expect it to always come before the
> push time unless the page is being reloaded, since we would normally push only
> after the main resource was requested and the connection was opened.

Wait....So a chrome/browser/devtools/ test is using test code in
third_party/WebKit/Source/devtools/front_end/Tests.js?  That seems very unusual.
 Chrome browsertest-specific Javascript usually lives in chrome/test/data.

I think someone else should review the wiring to the Javascript test stuff here.

https://codereview.chromium.org/1828203005/diff/140001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/1828203005/diff/140001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:201:
load_timing_info_.push_end = base::TimeTicks::Now();
If we're going to keep the behavior, we should also have a test where push_end
is null.

https://codereview.chromium.org/1828203005/diff/140001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:240: base::FilePath
file_path_;
nit:  Both these can be const

https://codereview.chromium.org/1828203005/diff/140001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:240: base::FilePath
file_path_;
nit:  include url/gurl.h, and file_path.h

https://codereview.chromium.org/1828203005/diff/140001/net/base/load_timing_i...
File net/base/load_timing_info.h (right):

https://codereview.chromium.org/1828203005/diff/140001/net/base/load_timing_i...
net/base/load_timing_info.h:147: // if the request is still being pushed.
If the request is still being pushed?  Once we create a URLRequest around the
request, is it still being pushed, or is it now being pulled?

[caseq, 2016-04-13 21:48:46]

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:1015:
DispatchOnTestSuite(window_, "testPushTimes", push_url.spec().c_str());
On 2016/04/11 17:45:52, mmenke wrote:
> On 2016/04/08 22:39:28, caseq wrote:
> > On 2016/04/08 16:21:36, mmenke wrote:
> > > Can we check that the push time is less that send_start...
> > 
> > That's exactly what happens in testPushTime() -- have you had a chance to
look
> > at Tests.js?
> > 
> > > Or better, get a time
> > > before the navigation starts, subtract 10 seconds from it, and check that
> the
> > > push time is before the earliest normally reported time (navigation start?

> > Some
> > > other start time?  Not too familiar with the timing APIs)
> > 
> > Well, as for navigation start -- I would expect it to always come before the
> > push time unless the page is being reloaded, since we would normally push
only
> > after the main resource was requested and the connection was opened.
> 
> Wait....So a chrome/browser/devtools/ test is using test code in
> third_party/WebKit/Source/devtools/front_end/Tests.js?  That seems very
unusual.
>  Chrome browsertest-specific Javascript usually lives in chrome/test/data.

It's been this way since about the time we introduced browser tests for
devtools. The original motivation is that the JS code there is tightly coupled
to the devtools front-end and less coupled to the test invocations on the
browser side. It's less of a matter since blink repo merge, but I wouldn't want
to change this in this CL.

> I think someone else should review the wiring to the Javascript test stuff
here.

Understood -- I think dgozman@ has good expertise there and he's already on the
list of reviewers.

https://codereview.chromium.org/1828203005/diff/140001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/1828203005/diff/140001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:201:
load_timing_info_.push_end = base::TimeTicks::Now();
On 2016/04/11 17:45:52, mmenke wrote:
> If we're going to keep the behavior, we should also have a test where push_end
> is null.

Done.

https://codereview.chromium.org/1828203005/diff/140001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:240: base::FilePath
file_path_;
On 2016/04/11 17:45:52, mmenke wrote:
> nit:  Both these can be const

Done.

https://codereview.chromium.org/1828203005/diff/140001/net/base/load_timing_i...
File net/base/load_timing_info.h (right):

https://codereview.chromium.org/1828203005/diff/140001/net/base/load_timing_i...
net/base/load_timing_info.h:147: // if the request is still being pushed.
On 2016/04/11 17:45:52, mmenke wrote:
> If the request is still being pushed?  Once we create a URLRequest around the
> request, is it still being pushed, or is it now being pulled?

Well, that probably depends on how you look on it, but if you look at the wire,
nothing changes when a push stream gets claimed -- i.e. server-initiated push
stream remains as such. So I'd  say it's still being pushed.

[mmenke, 2016-04-14 15:50:33]

Just 3 followup responses

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:1015:
DispatchOnTestSuite(window_, "testPushTimes", push_url.spec().c_str());
On 2016/04/13 21:48:45, caseq wrote:
> On 2016/04/11 17:45:52, mmenke wrote:
> > On 2016/04/08 22:39:28, caseq wrote:
> > > On 2016/04/08 16:21:36, mmenke wrote:
> > > > Can we check that the push time is less that send_start...
> > > 
> > > That's exactly what happens in testPushTime() -- have you had a chance to
> look
> > > at Tests.js?
> > > 
> > > > Or better, get a time
> > > > before the navigation starts, subtract 10 seconds from it, and check
that
> > the
> > > > push time is before the earliest normally reported time (navigation
start?
> 
> > > Some
> > > > other start time?  Not too familiar with the timing APIs)
> > > 
> > > Well, as for navigation start -- I would expect it to always come before
the
> > > push time unless the page is being reloaded, since we would normally push
> only
> > > after the main resource was requested and the connection was opened.
> > 
> > Wait....So a chrome/browser/devtools/ test is using test code in
> > third_party/WebKit/Source/devtools/front_end/Tests.js?  That seems very
> unusual.
> >  Chrome browsertest-specific Javascript usually lives in chrome/test/data.
> 
> It's been this way since about the time we introduced browser tests for
> devtools. The original motivation is that the JS code there is tightly coupled
> to the devtools front-end and less coupled to the test invocations on the
> browser side. It's less of a matter since blink repo merge, but I wouldn't
want
> to change this in this CL.
> 
> > I think someone else should review the wiring to the Javascript test stuff
> here.
> 
> Understood -- I think dgozman@ has good expertise there and he's already on
the
> list of reviewers.
> 

Happy to defer to him on that, but he should take another look, now that you've
added tests.

https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:1015:
DispatchOnTestSuite(window_, "testPushTimes", push_url.spec().c_str());
On 2016/04/08 22:39:28, caseq wrote:
> On 2016/04/08 16:21:36, mmenke wrote:
> > Can we check that the push time is less that send_start...
> 
> That's exactly what happens in testPushTime() -- have you had a chance to look
> at Tests.js?
> 
> > Or better, get a time
> > before the navigation starts, subtract 10 seconds from it, and check that
the
> > push time is before the earliest normally reported time (navigation start? 
> Some
> > other start time?  Not too familiar with the timing APIs)
> 
> Well, as for navigation start -- I would expect it to always come before the
> push time unless the page is being reloaded, since we would normally push only
> after the main resource was requested and the connection was opened.

The question isn't what's common, or what we expect, but what's possible.  Bugs
often crop up in obscure cases that are unexpected, but still possible.

https://codereview.chromium.org/1828203005/diff/140001/net/base/load_timing_i...
File net/base/load_timing_info.h (right):

https://codereview.chromium.org/1828203005/diff/140001/net/base/load_timing_i...
net/base/load_timing_info.h:147: // if the request is still being pushed.
On 2016/04/13 21:48:45, caseq wrote:
> On 2016/04/11 17:45:52, mmenke wrote:
> > If the request is still being pushed?  Once we create a URLRequest around
the
> > request, is it still being pushed, or is it now being pulled?
> 
> Well, that probably depends on how you look on it, but if you look at the
wire,
> nothing changes when a push stream gets claimed -- i.e. server-initiated push
> stream remains as such. So I'd  say it's still being pushed.

Think the comment needs to be clearer.  Just reading it, I still have no idea
when it's no longer null.

[caseq, 2016-04-14 19:07:05]

On 2016/04/14 15:50:33, mmenke wrote:

Bence, are you fine with the spdy part of the change?

> Just 3 followup responses
> 
>
https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
> File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
> chrome/browser/devtools/devtools_sanity_browsertest.cc:1015:
> DispatchOnTestSuite(window_, "testPushTimes", push_url.spec().c_str());
> On 2016/04/13 21:48:45, caseq wrote:
> > On 2016/04/11 17:45:52, mmenke wrote:
> > > On 2016/04/08 22:39:28, caseq wrote:
> > > > On 2016/04/08 16:21:36, mmenke wrote:
> > > > > Can we check that the push time is less that send_start...
> > > > 
> > > > That's exactly what happens in testPushTime() -- have you had a chance
to
> > look
> > > > at Tests.js?
> > > > 
> > > > > Or better, get a time
> > > > > before the navigation starts, subtract 10 seconds from it, and check
> that
> > > the
> > > > > push time is before the earliest normally reported time (navigation
> start?
> > 
> > > > Some
> > > > > other start time?  Not too familiar with the timing APIs)
> > > > 
> > > > Well, as for navigation start -- I would expect it to always come before
> the
> > > > push time unless the page is being reloaded, since we would normally
push
> > only
> > > > after the main resource was requested and the connection was opened.
> > > 
> > > Wait....So a chrome/browser/devtools/ test is using test code in
> > > third_party/WebKit/Source/devtools/front_end/Tests.js?  That seems very
> > unusual.
> > >  Chrome browsertest-specific Javascript usually lives in chrome/test/data.
> > 
> > It's been this way since about the time we introduced browser tests for
> > devtools. The original motivation is that the JS code there is tightly
coupled
> > to the devtools front-end and less coupled to the test invocations on the
> > browser side. It's less of a matter since blink repo merge, but I wouldn't
> want
> > to change this in this CL.
> > 
> > > I think someone else should review the wiring to the Javascript test stuff
> > here.
> > 
> > Understood -- I think dgozman@ has good expertise there and he's already on
> the
> > list of reviewers.
> > 
> 
> Happy to defer to him on that, but he should take another look, now that
you've
> added tests.

Dmitry, ptal!

> 
>
https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
> chrome/browser/devtools/devtools_sanity_browsertest.cc:1015:
> DispatchOnTestSuite(window_, "testPushTimes", push_url.spec().c_str());
> On 2016/04/08 22:39:28, caseq wrote:
> > On 2016/04/08 16:21:36, mmenke wrote:
> > > Can we check that the push time is less that send_start...
> > 
> > That's exactly what happens in testPushTime() -- have you had a chance to
look
> > at Tests.js?
> > 
> > > Or better, get a time
> > > before the navigation starts, subtract 10 seconds from it, and check that
> the
> > > push time is before the earliest normally reported time (navigation start?

> > Some
> > > other start time?  Not too familiar with the timing APIs)
> > 
> > Well, as for navigation start -- I would expect it to always come before the
> > push time unless the page is being reloaded, since we would normally push
only
> > after the main resource was requested and the connection was opened.
> 
> The question isn't what's common, or what we expect, but what's possible. 
Bugs
> often crop up in obscure cases that are unexpected, but still possible.

I'm not sure I understand what your point is here. Mine is that we can't have an
expectation in test that push happens before navigation, because it may happen
either
way -- so we only test for invariants here, and I think that it gives us
reasonable
coverage given that we have both times within and outside of time adjustment
window.


>
https://codereview.chromium.org/1828203005/diff/140001/net/base/load_timing_i...
> File net/base/load_timing_info.h (right):
> 
>
https://codereview.chromium.org/1828203005/diff/140001/net/base/load_timing_i...
> net/base/load_timing_info.h:147: // if the request is still being pushed.
> On 2016/04/13 21:48:45, caseq wrote:
> > On 2016/04/11 17:45:52, mmenke wrote:
> > > If the request is still being pushed?  Once we create a URLRequest around
> the
> > > request, is it still being pushed, or is it now being pulled?
> > 
> > Well, that probably depends on how you look on it, but if you look at the
> wire,
> > nothing changes when a push stream gets claimed -- i.e. server-initiated
push
> > stream remains as such. So I'd  say it's still being pushed.
> 
> Think the comment needs to be clearer.  Just reading it, I still have no idea
> when it's no longer null.

I've updated wording there to be more specific, does it look better now?

[mmenke, 2016-04-14 19:15:40]

On 2016/04/14 19:07:05, caseq wrote:
> On 2016/04/14 15:50:33, mmenke wrote:
> 
> Bence, are you fine with the spdy part of the change?
> 
> > Just 3 followup responses
> > 
> >
>
https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
> > File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
> > chrome/browser/devtools/devtools_sanity_browsertest.cc:1015:
> > DispatchOnTestSuite(window_, "testPushTimes", push_url.spec().c_str());
> > On 2016/04/13 21:48:45, caseq wrote:
> > > On 2016/04/11 17:45:52, mmenke wrote:
> > > > On 2016/04/08 22:39:28, caseq wrote:
> > > > > On 2016/04/08 16:21:36, mmenke wrote:
> > > > > > Can we check that the push time is less that send_start...
> > > > > 
> > > > > That's exactly what happens in testPushTime() -- have you had a chance
> to
> > > look
> > > > > at Tests.js?
> > > > > 
> > > > > > Or better, get a time
> > > > > > before the navigation starts, subtract 10 seconds from it, and check
> > that
> > > > the
> > > > > > push time is before the earliest normally reported time (navigation
> > start?
> > > 
> > > > > Some
> > > > > > other start time?  Not too familiar with the timing APIs)
> > > > > 
> > > > > Well, as for navigation start -- I would expect it to always come
before
> > the
> > > > > push time unless the page is being reloaded, since we would normally
> push
> > > only
> > > > > after the main resource was requested and the connection was opened.
> > > > 
> > > > Wait....So a chrome/browser/devtools/ test is using test code in
> > > > third_party/WebKit/Source/devtools/front_end/Tests.js?  That seems very
> > > unusual.
> > > >  Chrome browsertest-specific Javascript usually lives in
chrome/test/data.
> > > 
> > > It's been this way since about the time we introduced browser tests for
> > > devtools. The original motivation is that the JS code there is tightly
> coupled
> > > to the devtools front-end and less coupled to the test invocations on the
> > > browser side. It's less of a matter since blink repo merge, but I wouldn't
> > want
> > > to change this in this CL.
> > > 
> > > > I think someone else should review the wiring to the Javascript test
stuff
> > > here.
> > > 
> > > Understood -- I think dgozman@ has good expertise there and he's already
on
> > the
> > > list of reviewers.
> > > 
> > 
> > Happy to defer to him on that, but he should take another look, now that
> you've
> > added tests.
> 
> Dmitry, ptal!
> 
> > 
> >
>
https://codereview.chromium.org/1828203005/diff/100001/chrome/browser/devtool...
> > chrome/browser/devtools/devtools_sanity_browsertest.cc:1015:
> > DispatchOnTestSuite(window_, "testPushTimes", push_url.spec().c_str());
> > On 2016/04/08 22:39:28, caseq wrote:
> > > On 2016/04/08 16:21:36, mmenke wrote:
> > > > Can we check that the push time is less that send_start...
> > > 
> > > That's exactly what happens in testPushTime() -- have you had a chance to
> look
> > > at Tests.js?
> > > 
> > > > Or better, get a time
> > > > before the navigation starts, subtract 10 seconds from it, and check
that
> > the
> > > > push time is before the earliest normally reported time (navigation
start?
> 
> > > Some
> > > > other start time?  Not too familiar with the timing APIs)
> > > 
> > > Well, as for navigation start -- I would expect it to always come before
the
> > > push time unless the page is being reloaded, since we would normally push
> only
> > > after the main resource was requested and the connection was opened.
> > 
> > The question isn't what's common, or what we expect, but what's possible. 
> Bugs
> > often crop up in obscure cases that are unexpected, but still possible.
> 
> I'm not sure I understand what your point is here. Mine is that we can't have
an
> expectation in test that push happens before navigation, because it may happen
> either
> way -- so we only test for invariants here, and I think that it gives us
> reasonable
> coverage given that we have both times within and outside of time adjustment
> window.

I'm paranoid about hard dependencies elsewhere in the timing logic - I think
this is fine for checking inter_process_time_ticks_converter.  I'm not sure what
other assumptions there may be elsewhere in the consumers of this data, though. 
I'm not going to block on this, so LGTM.

>
https://codereview.chromium.org/1828203005/diff/140001/net/base/load_timing_i...
> > File net/base/load_timing_info.h (right):
> > 
> >
>
https://codereview.chromium.org/1828203005/diff/140001/net/base/load_timing_i...
> > net/base/load_timing_info.h:147: // if the request is still being pushed.
> > On 2016/04/13 21:48:45, caseq wrote:
> > > On 2016/04/11 17:45:52, mmenke wrote:
> > > > If the request is still being pushed?  Once we create a URLRequest
around
> > the
> > > > request, is it still being pushed, or is it now being pulled?
> > > 
> > > Well, that probably depends on how you look on it, but if you look at the
> > wire,
> > > nothing changes when a push stream gets claimed -- i.e. server-initiated
> push
> > > stream remains as such. So I'd  say it's still being pushed.
> > 
> > Think the comment needs to be clearer.  Just reading it, I still have no
idea
> > when it's no longer null.
> 
> I've updated wording there to be more specific, does it look better now?

Looks good.

[dgozman, 2016-04-15 21:13:17]

devtools with test lgtm

https://codereview.chromium.org/1828203005/diff/180001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/1828203005/diff/180001/chrome/browser/devtool...
chrome/browser/devtools/devtools_sanity_browsertest.cc:1032: // Tests raw
headers text.
Wrong comment. Just remove it.

https://codereview.chromium.org/1828203005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/Tests.js (right):

https://codereview.chromium.org/1828203005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/Tests.js:544:
test.assertTrue(resource.startTime < resource.timing.pushEnd, "pushEnd should be
after startTime");
Let's comment this is not true generally, but we make it so specifically in the
test.

[caseq, 2016-04-15 21:55:04]

ping bnc, do you want to take a look?

On 2016/04/15 21:13:17, dgozman wrote:
> devtools with test lgtm
> 
>
https://codereview.chromium.org/1828203005/diff/180001/chrome/browser/devtool...
> File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1828203005/diff/180001/chrome/browser/devtool...
> chrome/browser/devtools/devtools_sanity_browsertest.cc:1032: // Tests raw
> headers text.
> Wrong comment. Just remove it.

Done.

>
https://codereview.chromium.org/1828203005/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/devtools/front_end/Tests.js (right):
> 
>
https://codereview.chromium.org/1828203005/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/devtools/front_end/Tests.js:544:
> test.assertTrue(resource.startTime < resource.timing.pushEnd, "pushEnd should
be
> after startTime");
> Let's comment this is not true generally, but we make it so specifically in
the
> test.

Done.

[Bence, 2016-04-18 16:45:47]

LGTM with nit.  Thanks for the ping, sorry for the delay.

https://codereview.chromium.org/1828203005/diff/200001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/1828203005/diff/200001/net/spdy/spdy_stream.c...
net/spdy/spdy_stream.cc:779: done_receiving ? recv_last_byte_time_ :
base::TimeTicks();
I still don't think you need to set this in case done_receiving == False,
because LoadTimingInfo::push_end is default constructed already.  See also
https://crrev.com/1828203005/#msg9.

[pfeldman, 2016-04-18 19:31:39]

lgtm

[caseq, 2016-04-18 20:45:52]

The CQ bit was checked by caseq@chromium.org

[caseq, 2016-04-18 20:45:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, mmenke@chromium.org, bnc@chromium.org
Link to the patchset: https://codereview.chromium.org/1828203005/#ps220001
(title: "Bence's comment addressed")

[commit-bot: I haz the power, 2016-04-18 20:46:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828203005/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828203005/220001

[commit-bot: I haz the power, 2016-04-18 20:59:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-18 20:59:01]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[caseq, 2016-04-18 21:37:57]

caseq@chromium.org changed reviewers:
+ palmer@chromium.org

[caseq, 2016-04-18 21:37:58]

+palmer for content/common/*_message* OWNERs -- could you please take a look?

[caseq, 2016-04-18 21:43:48]

Patchset #12 (id:240001) has been deleted

[palmer, 2016-04-18 21:56:28]

lgtm

[caseq, 2016-04-18 21:56:54]

The CQ bit was checked by caseq@chromium.org

[caseq, 2016-04-18 21:56:55]

The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org,
dgozman@chromium.org, mmenke@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1828203005/#ps260001
(title: "extraced URLRequestNonsecureConnection cleanup into a separate CL")

[commit-bot: I haz the power, 2016-04-18 21:57:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828203005/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828203005/260001

[commit-bot: I haz the power, 2016-04-18 22:26:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-18 22:26:56]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[caseq, 2016-04-18 22:54:29]

The CQ bit was checked by caseq@chromium.org

[commit-bot: I haz the power, 2016-04-18 22:55:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828203005/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828203005/260001

[caseq, 2016-04-18 22:55:08]

The CQ bit was unchecked by caseq@chromium.org

[caseq, 2016-04-19 03:09:42]

The CQ bit was checked by caseq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-19 03:09:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828203005/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828203005/280001

[commit-bot: I haz the power, 2016-04-19 07:08:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-19 07:08:09]

Dry run: This issue passed the CQ dry run.

[caseq, 2016-04-19 17:08:53]

The CQ bit was checked by caseq@chromium.org

[caseq, 2016-04-19 17:08:54]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
bnc@chromium.org, dgozman@chromium.org, mmenke@chromium.org,
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1828203005/#ps300001
(title: "fixed tests")

[commit-bot: I haz the power, 2016-04-19 17:09:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828203005/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828203005/300001

[commit-bot: I haz the power, 2016-04-19 19:34:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-19 19:34:37]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[caseq, 2016-04-19 21:02:48]

The CQ bit was checked by caseq@chromium.org

[commit-bot: I haz the power, 2016-04-19 21:03:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828203005/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828203005/300001

[commit-bot: I haz the power, 2016-04-19 22:48:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-19 22:48:13]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[caseq, 2016-04-19 22:50:38]

The CQ bit was checked by caseq@chromium.org

[commit-bot: I haz the power, 2016-04-19 22:51:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828203005/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828203005/300001

[commit-bot: I haz the power, 2016-04-20 00:03:08]

Committed patchset #14 (id:300001)

[commit-bot: I haz the power, 2016-04-22 19:18:30]

Description was changed from

==========
Expose SPDY pushes in DevTools

This adds push start/end times to LoadTimingInfo and plumbs it all
the way to DevTools Network panel.

BUG=464501
==========

to

==========
Expose SPDY pushes in DevTools

This adds push start/end times to LoadTimingInfo and plumbs it all
the way to DevTools Network panel.

BUG=464501

Committed: https://crrev.com/e8340bc90789f2084eb1cb128b87679170a5d59d
Cr-Commit-Position: refs/heads/master@{#388367}
==========

[commit-bot: I haz the power, 2016-04-22 19:18:32]

Patchset 14 (id:??) landed as
https://crrev.com/e8340bc90789f2084eb1cb128b87679170a5d59d
Cr-Commit-Position: refs/heads/master@{#388367}