Add headers argument to BufferedSpdyFramerVisitorInterface::OnPushPromise

Add headers argument to
BufferedSpdyFramerVisitorInterface::OnPushPromise

BUG=377538
R=jgraettinger@chromium.org

TEST=BufferedSpdyFramerTest.ReadPushPromiseHeaderBlock

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273722

[baranovich, 2014-05-28 20:27:00]

https://codereview.chromium.org/300553013/diff/1/net/spdy/buffered_spdy_frame...
File net/spdy/buffered_spdy_framer.cc (right):

https://codereview.chromium.org/300553013/diff/1/net/spdy/buffered_spdy_frame...
net/spdy/buffered_spdy_framer.cc:370: SpdyFrame*
BufferedSpdyFramer::CreatePushPromise(
I was not sure whether should I add this method, since all of them are marked
for removal, but while they are still here, for consistency, I added he same
about PUSH_PROMISE

[Johnny, 2014-05-28 22:33:51]

Couple of nits. Otherwise looks good!

https://codereview.chromium.org/300553013/diff/1/net/spdy/buffered_spdy_frame...
File net/spdy/buffered_spdy_framer.cc (right):

https://codereview.chromium.org/300553013/diff/1/net/spdy/buffered_spdy_frame...
net/spdy/buffered_spdy_framer.cc:370: SpdyFrame*
BufferedSpdyFramer::CreatePushPromise(
On 2014/05/28 20:27:00, baranovich wrote:
> I was not sure whether should I add this method, since all of them are marked
> for removal, but while they are still here, for consistency, I added he same
> about PUSH_PROMISE

I think it's not needed, but I've no objection to it.

https://codereview.chromium.org/300553013/diff/1/net/spdy/buffered_spdy_frame...
File net/spdy/buffered_spdy_framer_unittest.cc (right):

https://codereview.chromium.org/300553013/diff/1/net/spdy/buffered_spdy_frame...
net/spdy/buffered_spdy_framer_unittest.cc:119: header_stream_id_ = stream_id;
Add and track |promised_stream_id|?

https://codereview.chromium.org/300553013/diff/1/net/spdy/buffered_spdy_frame...
net/spdy/buffered_spdy_framer_unittest.cc:319:
EXPECT_TRUE(CompareHeaderBlocks(&headers, &visitor.headers_));
Also check |header_stream_id_| & |promised_stream_id_|?

[baranovich, 2014-05-29 12:50:45]

https://codereview.chromium.org/300553013/diff/1/net/spdy/buffered_spdy_frame...
File net/spdy/buffered_spdy_framer_unittest.cc (right):

https://codereview.chromium.org/300553013/diff/1/net/spdy/buffered_spdy_frame...
net/spdy/buffered_spdy_framer_unittest.cc:319:
EXPECT_TRUE(CompareHeaderBlocks(&headers, &visitor.headers_));
On 2014/05/28 22:33:51, Johnny wrote:
> Also check |header_stream_id_| & |promised_stream_id_|?

Done.

I did't to it from the start, since it was not checked in other tests. Actually,
|header_stream_id_| is checked in BufferedSpdyFramer::OnControlFrameHeaderData
(CHECK_EQ), this check in tests seems excessive. And in ReadSynStreamHeaderBlock
|associated_stream_id| is not checked as well, but without any good reason:)
Should we check it then too?

[Johnny, 2014-05-29 18:19:30]

Looks good. There are a couple of compile errors according to the trybots, which
you'll see if you build the target flip_in_mem_edsm_server. Fix those, and I
think this is good to go.

https://codereview.chromium.org/300553013/diff/1/net/spdy/buffered_spdy_frame...
File net/spdy/buffered_spdy_framer_unittest.cc (right):

https://codereview.chromium.org/300553013/diff/1/net/spdy/buffered_spdy_frame...
net/spdy/buffered_spdy_framer_unittest.cc:319:
EXPECT_TRUE(CompareHeaderBlocks(&headers, &visitor.headers_));
On 2014/05/29 12:50:46, baranovich wrote:
> On 2014/05/28 22:33:51, Johnny wrote:
> > Also check |header_stream_id_| & |promised_stream_id_|?
> 
> Done.
> 
> I did't to it from the start, since it was not checked in other tests.
Actually,
> |header_stream_id_| is checked in BufferedSpdyFramer::OnControlFrameHeaderData
> (CHECK_EQ), this check in tests seems excessive. And in
ReadSynStreamHeaderBlock
> |associated_stream_id| is not checked as well, but without any good reason:)
> Should we check it then too?

Up to you. It's not tightly related to your CL, so IMO it's also fine to leave.

[baranovich, 2014-05-29 18:28:17]

On 2014/05/29 18:19:30, Johnny wrote:
> Looks good. There are a couple of compile errors according to the trybots,
which
> you'll see if you build the target flip_in_mem_edsm_server. Fix those, and I
> think this is good to go.
> 
>
https://codereview.chromium.org/300553013/diff/1/net/spdy/buffered_spdy_frame...
> File net/spdy/buffered_spdy_framer_unittest.cc (right):
> 
>
https://codereview.chromium.org/300553013/diff/1/net/spdy/buffered_spdy_frame...
> net/spdy/buffered_spdy_framer_unittest.cc:319:
> EXPECT_TRUE(CompareHeaderBlocks(&headers, &visitor.headers_));
> On 2014/05/29 12:50:46, baranovich wrote:
> > On 2014/05/28 22:33:51, Johnny wrote:
> > > Also check |header_stream_id_| & |promised_stream_id_|?
> > 
> > Done.
> > 
> > I did't to it from the start, since it was not checked in other tests.
> Actually,
> > |header_stream_id_| is checked in
BufferedSpdyFramer::OnControlFrameHeaderData
> > (CHECK_EQ), this check in tests seems excessive. And in
> ReadSynStreamHeaderBlock
> > |associated_stream_id| is not checked as well, but without any good reason:)
> > Should we check it then too?
> 
> Up to you. It's not tightly related to your CL, so IMO it's also fine to
leave.

Fixed flip_server. BTW is it still used?

[Johnny, 2014-05-29 18:31:43]

lgtm.

You know, that's a good question for net-dev. I've not used it.

[Johnny, 2014-05-29 18:31:52]

The CQ bit was checked by jgraettinger@chromium.org

[commit-bot: I haz the power, 2014-05-29 18:33:45]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/300553013/30001

[baranovich, 2014-05-29 19:53:19]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2014-05-29 19:54:48]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/300553013/50001

[baranovich, 2014-05-29 20:37:23]

The CQ bit was unchecked by baranovich@yandex-team.ru

[baranovich, 2014-05-29 20:37:26]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2014-05-29 20:38:29]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/300553013/50001

[baranovich, 2014-05-29 22:32:14]

The CQ bit was unchecked by baranovich@yandex-team.ru

[baranovich, 2014-05-29 22:32:18]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2014-05-29 22:35:35]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/300553013/50001

[commit-bot: I haz the power, 2014-05-30 05:24:23]

Change committed as 273722