Initial implementation of RequestPriority-based HTTP/2 dependencies.

Initial implementation of RequestPriority-based HTTP/2 dependencies.

BUG=500673
R=bnc@chromium.org

Committed: https://crrev.com/ebb50aa1b6d1a7ccb6eb892aca79dd6de4910d55
Cr-Commit-Position: refs/heads/master@{#359247}

[Randy Smith (Not in Mondays), 2015-10-21 20:56:16]

Bence, could you take an initial look at this?  It's got some large holes :-}
(specifically a) tests, and b) finch integration, because I'd like to roll this
out gradually to see if there are any servers out there we're going to mess up)
but I'm curious what you think of the  direction.

Specific things I'd like your opinion on:
* Whether it's ok to depend on those things I call out depending on in the
comment--my read of the code is that it is, but that doesn't mean either that I
read the code correctly or that architecturally this is a direction in which we
want to go.

* Whether you think I should get rid of active_streams_ and just use
active_streams_by_priority_.  It'd be a bit of a pain and cost a bit of
performance to cycle through all the priority levels elsewhere (though I'd
probably keep an active stream count to simplify the empty() tests), but it
would avoid completely duplicating the stream pointer storage.  

Thanks!

[Bence, 2015-10-22 19:34:16]

On 2015/10/21 20:56:16, rdsmith wrote:
> Bence, could you take an initial look at this?  It's got some large holes :-}
> (specifically a) tests, and b) finch integration, because I'd like to roll
this
> out gradually to see if there are any servers out there we're going to mess
up)
> but I'm curious what you think of the  direction.
> 
> Specific things I'd like your opinion on:
> * Whether it's ok to depend on those things I call out depending on in the
> comment--my read of the code is that it is, but that doesn't mean either that
I
> read the code correctly or that architecturally this is a direction in which
we
> want to go.
> 
> * Whether you think I should get rid of active_streams_ and just use
> active_streams_by_priority_.  It'd be a bit of a pain and cost a bit of
> performance to cycle through all the priority levels elsewhere (though I'd
> probably keep an active stream count to simplify the empty() tests), but it
> would avoid completely duplicating the stream pointer storage.  
> 
> Thanks!

Hi Randy, I think this is a valid approach, and fits architecturally.  However,
there are a couple of other options, and I would like to brainstorm a little bit
before choosing one.

1. Along the lines of this draft, you could separate out |active_streams_| and
|active_streams_by_priority_| in their own private class ActiveStreamsMap (or
ActiveStreamsByPriorityMap) that would support empty(), size(), find(),
insert(SpdyStreamId, SpdyStream*, SpdyPriority), erase(SpdyStreamId), and
last_by_priority(SpdyPriority).  This would help guarantee the invariants
between these two containers.

2. As you said, you could get rid of one or the other active_streams_* member,
and extract information in O(n) time.

3. You could add a private SpdyPriorityTree member to SpdySession, which I think
will be inevitable sooner or later anyway.  That would then be kept up to date
with what was sent out on the wire.  In this case, you only need a singe
array<SpdyStream*> to know where to insert new streams with given priority, and
you could update elements of this array using SpdyPriorityTree.GetParent()
and/or SpdyPriorityTree.GetChildren()[0] upon deletion of streams.

3b. These (priority_tree_ and active_streams_ and streams_by_priority_), in
addition, could be wrapped to a private container class.

What do you think about these options, is there yet another one we didn't think
about, what would be your preference?

[Randy Smith (Not in Mondays), 2015-10-26 17:40:25]

On 2015/10/22 19:34:16, Bence wrote:
> On 2015/10/21 20:56:16, rdsmith wrote:
> > Bence, could you take an initial look at this?  It's got some large holes
:-}
> > (specifically a) tests, and b) finch integration, because I'd like to roll
> this
> > out gradually to see if there are any servers out there we're going to mess
> up)
> > but I'm curious what you think of the  direction.
> > 
> > Specific things I'd like your opinion on:
> > * Whether it's ok to depend on those things I call out depending on in the
> > comment--my read of the code is that it is, but that doesn't mean either
that
> I
> > read the code correctly or that architecturally this is a direction in which
> we
> > want to go.
> > 
> > * Whether you think I should get rid of active_streams_ and just use
> > active_streams_by_priority_.  It'd be a bit of a pain and cost a bit of
> > performance to cycle through all the priority levels elsewhere (though I'd
> > probably keep an active stream count to simplify the empty() tests), but it
> > would avoid completely duplicating the stream pointer storage.  
> > 
> > Thanks!
> 
> Hi Randy, I think this is a valid approach, and fits architecturally. 
However,
> there are a couple of other options, and I would like to brainstorm a little
bit
> before choosing one.
> 
> 1. Along the lines of this draft, you could separate out |active_streams_| and
> |active_streams_by_priority_| in their own private class ActiveStreamsMap (or
> ActiveStreamsByPriorityMap) that would support empty(), size(), find(),
> insert(SpdyStreamId, SpdyStream*, SpdyPriority), erase(SpdyStreamId), and
> last_by_priority(SpdyPriority).  This would help guarantee the invariants
> between these two containers.
> 
> 2. As you said, you could get rid of one or the other active_streams_* member,
> and extract information in O(n) time.
> 
> 3. You could add a private SpdyPriorityTree member to SpdySession, which I
think
> will be inevitable sooner or later anyway.  That would then be kept up to date
> with what was sent out on the wire.  In this case, you only need a singe
> array<SpdyStream*> to know where to insert new streams with given priority,
and
> you could update elements of this array using SpdyPriorityTree.GetParent()
> and/or SpdyPriorityTree.GetChildren()[0] upon deletion of streams.
> 
> 3b. These (priority_tree_ and active_streams_ and streams_by_priority_), in
> addition, could be wrapped to a private container class.
> 
> What do you think about these options, is there yet another one we didn't
think
> about, what would be your preference?

Hmmm.  I hadn't thought about the new class creation option.  At a DS level, all
that's really required is adding a doubly linked list to the existing structure
that holds the order by priority, and that's not that hard (or ugly, given that
it's bounded within a separate class).  I'm inclined against the O(n) lookup;
it's not that big a deal performance wise, but it feels wrong, and it doesn't
scale.  The "right answer" is probably the SpdyPriorityTree option; the only
reason I'm reluctant is that it would be very useful to get this into the next
MS, and I'm a bit reluctant to take on more learning curve climbing.  Having
said that, getting to the current point wasn't very hard, so maybe I should just
do the next leg of the climb :-}.  

How about this as a proposal: Accept SpdyPriorityTree as the right long term
thing, first make sure the current code works and is ready to drop in if we need
it, then work on the SpdyPriorityTree version and if that's ready in time for
the milestone, use it, otherwise retrofit it after the MS?

[Bence, 2015-10-26 18:12:21]

On 2015/10/26 17:40:25, rdsmith wrote:
> On 2015/10/22 19:34:16, Bence wrote:
> > On 2015/10/21 20:56:16, rdsmith wrote:
> > > Bence, could you take an initial look at this?  It's got some large holes
> :-}
> > > (specifically a) tests, and b) finch integration, because I'd like to roll
> > this
> > > out gradually to see if there are any servers out there we're going to
mess
> > up)
> > > but I'm curious what you think of the  direction.
> > > 
> > > Specific things I'd like your opinion on:
> > > * Whether it's ok to depend on those things I call out depending on in the
> > > comment--my read of the code is that it is, but that doesn't mean either
> that
> > I
> > > read the code correctly or that architecturally this is a direction in
which
> > we
> > > want to go.
> > > 
> > > * Whether you think I should get rid of active_streams_ and just use
> > > active_streams_by_priority_.  It'd be a bit of a pain and cost a bit of
> > > performance to cycle through all the priority levels elsewhere (though I'd
> > > probably keep an active stream count to simplify the empty() tests), but
it
> > > would avoid completely duplicating the stream pointer storage.  
> > > 
> > > Thanks!
> > 
> > Hi Randy, I think this is a valid approach, and fits architecturally. 
> However,
> > there are a couple of other options, and I would like to brainstorm a little
> bit
> > before choosing one.
> > 
> > 1. Along the lines of this draft, you could separate out |active_streams_|
and
> > |active_streams_by_priority_| in their own private class ActiveStreamsMap
(or
> > ActiveStreamsByPriorityMap) that would support empty(), size(), find(),
> > insert(SpdyStreamId, SpdyStream*, SpdyPriority), erase(SpdyStreamId), and
> > last_by_priority(SpdyPriority).  This would help guarantee the invariants
> > between these two containers.
> > 
> > 2. As you said, you could get rid of one or the other active_streams_*
member,
> > and extract information in O(n) time.
> > 
> > 3. You could add a private SpdyPriorityTree member to SpdySession, which I
> think
> > will be inevitable sooner or later anyway.  That would then be kept up to
date
> > with what was sent out on the wire.  In this case, you only need a singe
> > array<SpdyStream*> to know where to insert new streams with given priority,
> and
> > you could update elements of this array using SpdyPriorityTree.GetParent()
> > and/or SpdyPriorityTree.GetChildren()[0] upon deletion of streams.
> > 
> > 3b. These (priority_tree_ and active_streams_ and streams_by_priority_), in
> > addition, could be wrapped to a private container class.
> > 
> > What do you think about these options, is there yet another one we didn't
> think
> > about, what would be your preference?
> 
> Hmmm.  I hadn't thought about the new class creation option.  At a DS level,
all
> that's really required is adding a doubly linked list to the existing
structure
> that holds the order by priority, and that's not that hard (or ugly, given
that
> it's bounded within a separate class).  I'm inclined against the O(n) lookup;
> it's not that big a deal performance wise, but it feels wrong, and it doesn't
> scale.  The "right answer" is probably the SpdyPriorityTree option; the only
> reason I'm reluctant is that it would be very useful to get this into the next
> MS, and I'm a bit reluctant to take on more learning curve climbing.  Having
> said that, getting to the current point wasn't very hard, so maybe I should
just
> do the next leg of the climb :-}.  
> 
> How about this as a proposal: Accept SpdyPriorityTree as the right long term
> thing, first make sure the current code works and is ready to drop in if we
need
> it, then work on the SpdyPriorityTree version and if that's ready in time for
> the milestone, use it, otherwise retrofit it after the MS?

SGTM.  Do you plan to fill in the aforementioned holes (tests and field trial)
in this very CL?

[Randy Smith (Not in Mondays), 2015-10-26 18:14:10]

On 2015/10/26 18:12:21, Bence wrote:
> On 2015/10/26 17:40:25, rdsmith wrote:
> > On 2015/10/22 19:34:16, Bence wrote:
> > > On 2015/10/21 20:56:16, rdsmith wrote:
> > > > Bence, could you take an initial look at this?  It's got some large
holes
> > :-}
> > > > (specifically a) tests, and b) finch integration, because I'd like to
roll
> > > this
> > > > out gradually to see if there are any servers out there we're going to
> mess
> > > up)
> > > > but I'm curious what you think of the  direction.
> > > > 
> > > > Specific things I'd like your opinion on:
> > > > * Whether it's ok to depend on those things I call out depending on in
the
> > > > comment--my read of the code is that it is, but that doesn't mean either
> > that
> > > I
> > > > read the code correctly or that architecturally this is a direction in
> which
> > > we
> > > > want to go.
> > > > 
> > > > * Whether you think I should get rid of active_streams_ and just use
> > > > active_streams_by_priority_.  It'd be a bit of a pain and cost a bit of
> > > > performance to cycle through all the priority levels elsewhere (though
I'd
> > > > probably keep an active stream count to simplify the empty() tests), but
> it
> > > > would avoid completely duplicating the stream pointer storage.  
> > > > 
> > > > Thanks!
> > > 
> > > Hi Randy, I think this is a valid approach, and fits architecturally. 
> > However,
> > > there are a couple of other options, and I would like to brainstorm a
little
> > bit
> > > before choosing one.
> > > 
> > > 1. Along the lines of this draft, you could separate out |active_streams_|
> and
> > > |active_streams_by_priority_| in their own private class ActiveStreamsMap
> (or
> > > ActiveStreamsByPriorityMap) that would support empty(), size(), find(),
> > > insert(SpdyStreamId, SpdyStream*, SpdyPriority), erase(SpdyStreamId), and
> > > last_by_priority(SpdyPriority).  This would help guarantee the invariants
> > > between these two containers.
> > > 
> > > 2. As you said, you could get rid of one or the other active_streams_*
> member,
> > > and extract information in O(n) time.
> > > 
> > > 3. You could add a private SpdyPriorityTree member to SpdySession, which I
> > think
> > > will be inevitable sooner or later anyway.  That would then be kept up to
> date
> > > with what was sent out on the wire.  In this case, you only need a singe
> > > array<SpdyStream*> to know where to insert new streams with given
priority,
> > and
> > > you could update elements of this array using SpdyPriorityTree.GetParent()
> > > and/or SpdyPriorityTree.GetChildren()[0] upon deletion of streams.
> > > 
> > > 3b. These (priority_tree_ and active_streams_ and streams_by_priority_),
in
> > > addition, could be wrapped to a private container class.
> > > 
> > > What do you think about these options, is there yet another one we didn't
> > think
> > > about, what would be your preference?
> > 
> > Hmmm.  I hadn't thought about the new class creation option.  At a DS level,
> all
> > that's really required is adding a doubly linked list to the existing
> structure
> > that holds the order by priority, and that's not that hard (or ugly, given
> that
> > it's bounded within a separate class).  I'm inclined against the O(n)
lookup;
> > it's not that big a deal performance wise, but it feels wrong, and it
doesn't
> > scale.  The "right answer" is probably the SpdyPriorityTree option; the only
> > reason I'm reluctant is that it would be very useful to get this into the
next
> > MS, and I'm a bit reluctant to take on more learning curve climbing.  Having
> > said that, getting to the current point wasn't very hard, so maybe I should
> just
> > do the next leg of the climb :-}.  
> > 
> > How about this as a proposal: Accept SpdyPriorityTree as the right long term
> > thing, first make sure the current code works and is ready to drop in if we
> need
> > it, then work on the SpdyPriorityTree version and if that's ready in time
for
> > the milestone, use it, otherwise retrofit it after the MS?
> 
> SGTM.  Do you plan to fill in the aforementioned holes (tests and field trial)
> in this very CL?

I do.  I'm going to attack that next, along with the parallel modifications to
QUIC.  But I wanted to get initial feedback on approach first.

[Randy Smith (Not in Mondays), 2015-11-11 06:48:30]

Bence: Want to take another look?  The CL isn't done, but it passes all
net_unittests (in my Mac checkout; try jobs running).   I'd value your high
level eval; I still have some cleanup to do on the details.  In addition, a
couple of specific questions:

* SpdyNetworkTransactionTest.OutOfOrderSynStream: This looks like there's a
mismatch between comments and reality (comments say the third request is highest
priority; in actually the third request is MEDIUM priority)--is there something
I'm missing?

* There are a couple of tests which do tunneling or wrapping of requests, and I
don't completely understand the nesting.  I believe that a) that ends up using
two different SpdySessions (one for the tunnel itself and one for the requests
tunnelled) and duplicated SpdyTestUtil similarly, but I'm uncertain as to
whether I really understand what's going on--could you take a look at those and
see if what I've done makes sense to you?

* I'm a bit uncertain as to how to use
SpdyTestUtil::OnStreamDestructed(stream_id) (my new method).  I could imagine an
argument that it should be used in all tests everywhere when a stream is
destroyed, but that's an amazing amount of code to change and it makes me tired
thinking of it.  I'm instead inclined to believe that it should only be used in
tests that require it (i.e. in which stream destruction changes the dependencies
if we're setting dependencies based on priority); is that ok with you?  (What's
actually in this CL is just using it for only those streams in those tests which
need to have their destruction signaled, but I'm planning to go through and
clean those up.)

I have some other minor cleanup to go through and do, and some bits of code to
grok more thoroughly, so there may be more questions, but that'll do for now
:-}.

Thanks!

[Bence, 2015-11-11 18:47:15]

So far, so good.  I really like your approach to tests, I'm glad you convinced
me that this is the right path to follow.

https://codereview.chromium.org/1411383005/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1411383005/diff/20001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:473:
INSTANTIATE_TEST_CASE_P(TestCase,
I am not familiar with the test name prefix convention, but this does not look
optimal to me, as TestCase is in anonymous namespace (also not a very meaningful
name as a test prefix), but test name is visible to everybody running the test.

https://codereview.chromium.org/1411383005/diff/20001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/1411383005/diff/20001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool_unittest.cc:341:
HttpProxyClientSocketPoolTestParams(SPDY, kProtoHTTP2, false)));
Why is (SPDY, kProtoHTTP2, true) not included here?  Maybe consider adding a
comment to explain.

https://codereview.chromium.org/1411383005/diff/20001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/1411383005/diff/20001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:521: static void
SetPriorityDependencyDefaultForTesting(bool enable);
I don't like the idea of having a global variable for this, but I see that there
is no easy way around it.

I don't like the idea of making this method public, and if every test file
already had a peer class, I would propose to friend those peer classes here and
make this method private, but they don't, and it would be unreasonable to create
them just for this.

Maybe "ForTesting" in the method name is enough to deter developers from calling
it in non-test code.

https://codereview.chromium.org/1411383005/diff/20001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:1197: // Should priority-based depencency information be
sent in stream header
s/depencency/dependency/

https://codereview.chromium.org/1411383005/diff/20001/net/spdy/spdy_test_util...
File net/spdy/spdy_test_util_common.cc (right):

https://codereview.chromium.org/1411383005/diff/20001/net/spdy/spdy_test_util...
net/spdy/spdy_test_util_common.cc:1296: LOG(ERROR) << __FUNCTION__ << "
stream_id " << stream_id;
I assume you do not intend to keep these LOG statements, right?

https://codereview.chromium.org/1411383005/diff/20001/net/spdy/spdy_test_util...
File net/spdy/spdy_test_util_common.h (right):

https://codereview.chromium.org/1411383005/diff/20001/net/spdy/spdy_test_util...
net/spdy/spdy_test_util_common.h:599: mutable std::map<int, std::vector<int>>
priority_to_stream_id_list_;
I don't understand why relevant SpdyTestUtil methods are const.  (I actually
don't see a const SpdyTestUtil anywhere.)  Maybe methods changing this member
should not be const in the first place, instead of making this member mutable.

[Bence, 2015-11-11 19:25:57]

Sorry for missing these earlier.

On 2015/11/11 06:48:30, rdsmith wrote:
> Bence: Want to take another look?  The CL isn't done, but it passes all
> net_unittests (in my Mac checkout; try jobs running).   I'd value your high
> level eval; I still have some cleanup to do on the details.  In addition, a
> couple of specific questions:
> 
> * SpdyNetworkTransactionTest.OutOfOrderSynStream: This looks like there's a
> mismatch between comments and reality (comments say the third request is
highest
> priority; in actually the third request is MEDIUM priority)--is there
something
> I'm missing?
> 

Comment seems to be correct to me.  |trans2| is MEDIUM, |trans3| is HIGHEST. 
Therefore |trans3| gets on the wire first, and thus |req2| corresponds to
|trans3|, and |req3| corresponds to |trans2|.  Feel free to augment comment
and/or rename req* (maybe req_medium and req_highest if you'd like) accordingly.

> * There are a couple of tests which do tunneling or wrapping of requests, and
I
> don't completely understand the nesting.  I believe that a) that ends up using
> two different SpdySessions (one for the tunnel itself and one for the requests
> tunnelled) and duplicated SpdyTestUtil similarly, but I'm uncertain as to
> whether I really understand what's going on--could you take a look at those
and
> see if what I've done makes sense to you?
> 

I see that you created multiple SpdyTestUtil instances for some tests that were
previously using the same one for multiple different connections, and this makes
sense because SpdyTestUtil now has more state.  For example,
HttpNetworkTransactionTest, HttpsProxySpdyConnectSpdy.  The only issue I can
think of is that the two instances have the same prioritydependency value.  Do
you think that testing all four possibilities ((false, false), (false, true),
(true, false), (true, true) for the tunnel and tunnelled SpdyTestUtil,
respectively) would bring value to the tests?  I do not think it's worth the
effort.

> * I'm a bit uncertain as to how to use
> SpdyTestUtil::OnStreamDestructed(stream_id) (my new method).  I could imagine
an
> argument that it should be used in all tests everywhere when a stream is
> destroyed, but that's an amazing amount of code to change and it makes me
tired
> thinking of it.  I'm instead inclined to believe that it should only be used
in
> tests that require it (i.e. in which stream destruction changes the
dependencies
> if we're setting dependencies based on priority); is that ok with you? 
(What's
> actually in this CL is just using it for only those streams in those tests
which
> need to have their destruction signaled, but I'm planning to go through and
> clean those up.)
> 

It's perfectly okay with me.  I wish I could come up with a better name that
better reflects that it's optional to call this method.

> I have some other minor cleanup to go through and do, and some bits of code to
> grok more thoroughly, so there may be more questions, but that'll do for now
> :-}.
> 
> Thanks!

[Randy Smith (Not in Mondays), 2015-11-11 20:49:59]

On 2015/11/11 19:25:57, Bence wrote:
> Sorry for missing these earlier.
> 
> On 2015/11/11 06:48:30, rdsmith wrote:
> > Bence: Want to take another look?  The CL isn't done, but it passes all
> > net_unittests (in my Mac checkout; try jobs running).   I'd value your high
> > level eval; I still have some cleanup to do on the details.  In addition, a
> > couple of specific questions:
> > 
> > * SpdyNetworkTransactionTest.OutOfOrderSynStream: This looks like there's a
> > mismatch between comments and reality (comments say the third request is
> highest
> > priority; in actually the third request is MEDIUM priority)--is there
> something
> > I'm missing?
> > 
> 
> Comment seems to be correct to me.  |trans2| is MEDIUM, |trans3| is HIGHEST. 
> Therefore |trans3| gets on the wire first, and thus |req2| corresponds to
> |trans3|, and |req3| corresponds to |trans2|.  Feel free to augment comment
> and/or rename req* (maybe req_medium and req_highest if you'd like)
accordingly.

Gotcha (from our offline conversation).  I've attempted to rewrite the comment
to make this all clear(er) to future readers.

> 
> > * There are a couple of tests which do tunneling or wrapping of requests,
and
> I
> > don't completely understand the nesting.  I believe that a) that ends up
using
> > two different SpdySessions (one for the tunnel itself and one for the
requests
> > tunnelled) and duplicated SpdyTestUtil similarly, but I'm uncertain as to
> > whether I really understand what's going on--could you take a look at those
> and
> > see if what I've done makes sense to you?
> > 
> 
> I see that you created multiple SpdyTestUtil instances for some tests that
were
> previously using the same one for multiple different connections, and this
makes
> sense because SpdyTestUtil now has more state.  For example,
> HttpNetworkTransactionTest, HttpsProxySpdyConnectSpdy.  The only issue I can
> think of is that the two instances have the same prioritydependency value.  Do
> you think that testing all four possibilities ((false, false), (false, true),
> (true, false), (true, true) for the tunnel and tunnelled SpdyTestUtil,
> respectively) would bring value to the tests?  I do not think it's worth the
> effort.

SG.

> 
> > * I'm a bit uncertain as to how to use
> > SpdyTestUtil::OnStreamDestructed(stream_id) (my new method).  I could
imagine
> an
> > argument that it should be used in all tests everywhere when a stream is
> > destroyed, but that's an amazing amount of code to change and it makes me
> tired
> > thinking of it.  I'm instead inclined to believe that it should only be used
> in
> > tests that require it (i.e. in which stream destruction changes the
> dependencies
> > if we're setting dependencies based on priority); is that ok with you? 
> (What's
> > actually in this CL is just using it for only those streams in those tests
> which
> > need to have their destruction signaled, but I'm planning to go through and
> > clean those up.)
> > 
> 
> It's perfectly okay with me.  I wish I could come up with a better name that
> better reflects that it's optional to call this method.

I'll think about the naming issue and see if I come up with something better.

> 
> > I have some other minor cleanup to go through and do, and some bits of code
to
> > grok more thoroughly, so there may be more questions, but that'll do for now
> > :-}.
> > 
> > Thanks!

[Randy Smith (Not in Mondays), 2015-11-11 23:25:59]

Bence, PTAL?  This is my first serious candidate for landing.  I'll try to look
into the alternatives we talked about for the base functionality (e.g.
SpdyPriorityTree, encapsulation) but I don't think I'm going to make that by
branch point.

I did try modifying the destruction notification to a hopefully more optionality
implied name; let me know what you think.

https://codereview.chromium.org/1411383005/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1411383005/diff/20001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:473:
INSTANTIATE_TEST_CASE_P(TestCase,
On 2015/11/11 18:47:14, Bence wrote:
> I am not familiar with the test name prefix convention, but this does not look
> optimal to me, as TestCase is in anonymous namespace (also not a very
meaningful
> name as a test prefix), but test name is visible to everybody running the
test.

Right, I had erroneously concluded that this was a typename.  I've changed it
over to something more descriptive; let me know what you think.

https://codereview.chromium.org/1411383005/diff/20001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/1411383005/diff/20001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool_unittest.cc:341:
HttpProxyClientSocketPoolTestParams(SPDY, kProtoHTTP2, false)));
On 2015/11/11 18:47:14, Bence wrote:
> Why is (SPDY, kProtoHTTP2, true) not included here?  Maybe consider adding a
> comment to explain.

Actually, it not being there was me just spacing.  I've added it; let me know if
you think that's the wrong thing.

https://codereview.chromium.org/1411383005/diff/20001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/1411383005/diff/20001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:521: static void
SetPriorityDependencyDefaultForTesting(bool enable);
On 2015/11/11 18:47:15, Bence wrote:
> I don't like the idea of having a global variable for this, but I see that
there
> is no easy way around it.

Agreed.

> I don't like the idea of making this method public, and if every test file
> already had a peer class, I would propose to friend those peer classes here
and
> make this method private, but they don't, and it would be unreasonable to
create
> them just for this.

I'm not sure what you mean by peer class?  I added a mess of "friends" and
shifted around some anonymous namespaces, and got this method private; does that
do what you wanted?

> 
> Maybe "ForTesting" in the method name is enough to deter developers from
calling
> it in non-test code.

https://codereview.chromium.org/1411383005/diff/20001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:1197: // Should priority-based depencency information be
sent in stream header
On 2015/11/11 18:47:15, Bence wrote:
> s/depencency/dependency/

Done.

https://codereview.chromium.org/1411383005/diff/20001/net/spdy/spdy_test_util...
File net/spdy/spdy_test_util_common.cc (right):

https://codereview.chromium.org/1411383005/diff/20001/net/spdy/spdy_test_util...
net/spdy/spdy_test_util_common.cc:1296: LOG(ERROR) << __FUNCTION__ << "
stream_id " << stream_id;
On 2015/11/11 18:47:15, Bence wrote:
> I assume you do not intend to keep these LOG statements, right?

Whoops, I was trying to keep these in the git stash.  Done.

https://codereview.chromium.org/1411383005/diff/20001/net/spdy/spdy_test_util...
File net/spdy/spdy_test_util_common.h (right):

https://codereview.chromium.org/1411383005/diff/20001/net/spdy/spdy_test_util...
net/spdy/spdy_test_util_common.h:599: mutable std::map<int, std::vector<int>>
priority_to_stream_id_list_;
On 2015/11/11 18:47:15, Bence wrote:
> I don't understand why relevant SpdyTestUtil methods are const.  (I actually
> don't see a const SpdyTestUtil anywhere.)  Maybe methods changing this member
> should not be const in the first place, instead of making this member mutable.

Done.

[Bence, 2015-11-12 01:33:29]

LGTM % nits.  Thanks!

https://codereview.chromium.org/1411383005/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1411383005/diff/20001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:473:
INSTANTIATE_TEST_CASE_P(TestCase,
I like it, thanks.

https://codereview.chromium.org/1411383005/diff/60001/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1411383005/diff/60001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:13607: // Use a separate test
class for the separate SpdySession that will be
optional: s/class/instance/

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_network_t...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:4706: // New SpdyTestUtil class
for the session that will be used for the
optional: s/class/instance/

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_session_p...
File net/spdy/spdy_session_pool_unittest.cc (right):

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_session_p...
net/spdy/spdy_session_pool_unittest.cc:512: // This isn't testing anything
having to do with Spdy frames; we
s/Spdy/SPDY/

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_session_u...
File net/spdy/spdy_session_unittest.cc (right):

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:39: // Test using the SPDY 3.1 protocol.
s/"SPDY 3.1"/"SPYD/3.1"/ here and in other files with this enum.

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:42: // Test using the HTTP2 protocol, without
specifying a stream
s/"HTTP2"/"HTTP/2"/ here and four lines below and in other files with this enum.

[Randy Smith (Not in Mondays), 2015-11-12 01:59:32]

Thanks!!

https://codereview.chromium.org/1411383005/diff/60001/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1411383005/diff/60001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:13607: // Use a separate test
class for the separate SpdySession that will be
On 2015/11/12 01:33:29, Bence wrote:
> optional: s/class/instance/

Done (both locations).

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_network_t...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:4706: // New SpdyTestUtil class
for the session that will be used for the
On 2015/11/12 01:33:29, Bence wrote:
> optional: s/class/instance/

Done.

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_session_p...
File net/spdy/spdy_session_pool_unittest.cc (right):

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_session_p...
net/spdy/spdy_session_pool_unittest.cc:512: // This isn't testing anything
having to do with Spdy frames; we
On 2015/11/12 01:33:29, Bence wrote:
> s/Spdy/SPDY/

Done.

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_session_u...
File net/spdy/spdy_session_unittest.cc (right):

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:39: // Test using the SPDY 3.1 protocol.
On 2015/11/12 01:33:29, Bence wrote:
> s/"SPDY 3.1"/"SPYD/3.1"/ here and in other files with this enum.

I'm assuming you're asking me to add the /, not to reverse the D & the Y.  Done.

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:42: // Test using the HTTP2 protocol, without
specifying a stream
On 2015/11/12 01:33:29, Bence wrote:
> s/"HTTP2"/"HTTP/2"/ here and four lines below and in other files with this
enum.

Done.

[Randy Smith (Not in Mondays), 2015-11-12 01:59:36]

The CQ bit was checked by rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2015-11-12 01:59:37]

The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org
Link to the patchset: https://codereview.chromium.org/1411383005/#ps80001
(title: "Final round of comments.")

[commit-bot: I haz the power, 2015-11-12 02:01:11]

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

[commit-bot: I haz the power, 2015-11-12 03:44:45]

Committed patchset #5 (id:80001)

[Bence, 2015-11-12 14:44:05]

Thanks.

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_session_u...
File net/spdy/spdy_session_unittest.cc (right):

https://codereview.chromium.org/1411383005/diff/60001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:39: // Test using the SPDY 3.1 protocol.
On 2015/11/12 01:59:32, rdsmith wrote:
> On 2015/11/12 01:33:29, Bence wrote:
> > s/"SPDY 3.1"/"SPYD/3.1"/ here and in other files with this enum.
> 
> I'm assuming you're asking me to add the /, not to reverse the D & the Y. 
Done.

Oops.  You are, of course, correct.  Thanks.

[commit-bot: I haz the power, 2015-11-12 20:04:28]

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