Fix bug in net::RequestPriority -> HTTP/2 dependency conversion.

Fix bug in net::RequestPriority -> HTTP/2 dependency conversion.

This CL also defaults this behavior on (subject to finch override), refactors the finch control to be in line with network stack defaults (== based in io_thread.cc),  and removes the non-default performance bot configuration.

BUG=590225
R=bnc@chromium.org
R=isherman@chromium.org

Committed: https://crrev.com/2e54d1f83e6ae57cb404658f361e469dd9ed2a92
Cr-Commit-Position: refs/heads/master@{#382360}

[Randy Smith (Not in Mondays), 2016-03-09 17:16:53]

Bence, PTAL?  Ilya, you're included for the reversion of the priority bot
configuration now that we're making this the default behavior.

Bence: This is a less space efficient solution than the old code, for two
reasons:
* We aren't relying on monotonically increasing stream ids, so we need to store
things in a list (to preserve order and allow for quick deletion).  A vector
(slower deletion) would get us back the space but cost the time.
* Because we're no longer storing a pointer to the SpdyStream, we can't get the
priority from that structure in order to remove a destructed stream from the
priority lists, so we have to store it too.  
I'm inclined to think the extra space is worth it--when you think about all the
space a URL fetch takes up and down the stack, this is minimal additional, and
it does provide a cleaner, more abstract interface.  But I'm specifically
curious as to your opinion on that.

I'm hoping I'll be able to just re-use this class to fix the QUIC code, but QUIC
is going to be noticeably harder for various reasons, so I'm not trying to
include that in this CL.

[Ilya Sherman, 2016-03-09 20:15:41]

//testing/variations lgmt

[Ilya Sherman, 2016-03-09 20:17:02]

lgmt lmgt lgtm mglt mgmt

[Bence, 2016-03-10 20:21:14]

Randy: this refactoring is great.  I really like your Http2PriorityDependencies
class.

There's one thing I'd like you to do differently.  Please do the
FieldTrialList::FindFullName call in IOThread, where all the SPDY and QUIC field
trial code lives.  Add your Boolean flag to IOThread::Globals, and to
HttpNetworkSession::Params, and SpdySessionPool constructor and member, and
SpdySession constructor and member, and Http2PriorityDependencies constructor,
and mark Http2PriorityDependencies::enabled as const.  This is the way most net/
field trial parameters are wired up.  Oh and of course don't forget to do the
same thing in IOSChromeIOThread.  Of course I'm giving myself extra work when
I'm asking you to do this, because I have a CL in progress for factoring out
these methods, https://crrev.com/1687423002, which is blocked on
https://crrev.com/1699653002, so it will take a while, and I'll have to rebase
on your change.  Oh well.

For tests, also add the flag to SpdySessionDependencies.  Please don't forget
that SpdySessionDependencies has two constructors (it's on my list to
consolidate them using a delegate constructor), so you'll have to add the
default value for tests (presumably true) twice.  Also copy the flag in
CreateSessionParams.  Then in tests where you only want to test for flag==true,
you do not need to do anything.  And in tests where you want to test for both
values of the flag, you can do that through the SpdySessionDependencies member. 
This way there is no need to use a static in Http2PriorityDependencies, and also
there is no need to restore the value when the test classes are destroyed.  I
think this is much clearer.

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
File net/spdy/http2_priority_dependencies.cc (right):

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
net/spdy/http2_priority_dependencies.cc:46: DCHECK(entry_by_stream_id_.end() ==
entry_by_stream_id_.find(id));
I'm not sure it is customary to use Yoda condition as in DCHECK(expected ==
actual), I would write DCHECK(actual == expected) instead (though I am aware of
the DCHECK(expected, actual) order).

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
net/spdy/http2_priority_dependencies.cc:61: entry_by_stream_id_[id] = --it;
Nit: use back instead of end-1.

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
net/spdy/http2_priority_dependencies.cc:74: return;
Nit: add empty line after return.

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
net/spdy/http2_priority_dependencies.cc:75:
id_priority_lists_[emit->second->second].erase(emit->second);
Optional: |IdList::iterator it = emit->second| for better readability.

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
File net/spdy/http2_priority_dependencies.h (right):

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
net/spdy/http2_priority_dependencies.h:35: // priority anyway.
Nit: close opening parenthesis.

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
net/spdy/http2_priority_dependencies.h:56: // to erase from.
Makes sense.  Too bad you have multiple lists and thus cannot use
linked_hash_map.

[Randy Smith (Not in Mondays), 2016-03-13 17:22:17]

On 2016/03/10 20:21:14, Bence wrote:
> Randy: this refactoring is great.  I really like your
Http2PriorityDependencies
> class.
> 
> There's one thing I'd like you to do differently.  Please do the
> FieldTrialList::FindFullName call in IOThread, where all the SPDY and QUIC
field
> trial code lives.  Add your Boolean flag to IOThread::Globals, and to
> HttpNetworkSession::Params, and SpdySessionPool constructor and member, and
> SpdySession constructor and member, and Http2PriorityDependencies constructor,
> and mark Http2PriorityDependencies::enabled as const.  This is the way most
net/
> field trial parameters are wired up.  Oh and of course don't forget to do the
> same thing in IOSChromeIOThread.  Of course I'm giving myself extra work when
> I'm asking you to do this, because I have a CL in progress for factoring out
> these methods, https://crrev.com/1687423002, which is blocked on
> https://crrev.com/1699653002, so it will take a while, and I'll have to rebase
> on your change.  Oh well.
> 
> For tests, also add the flag to SpdySessionDependencies.  Please don't forget
> that SpdySessionDependencies has two constructors (it's on my list to
> consolidate them using a delegate constructor), so you'll have to add the
> default value for tests (presumably true) twice.  Also copy the flag in
> CreateSessionParams.  Then in tests where you only want to test for
flag==true,
> you do not need to do anything.  And in tests where you want to test for both
> values of the flag, you can do that through the SpdySessionDependencies
member. 
> This way there is no need to use a static in Http2PriorityDependencies, and
also
> there is no need to restore the value when the test classes are destroyed.  I
> think this is much clearer.
> 
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> File net/spdy/http2_priority_dependencies.cc (right):
> 
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> net/spdy/http2_priority_dependencies.cc:46: DCHECK(entry_by_stream_id_.end()
==
> entry_by_stream_id_.find(id));
> I'm not sure it is customary to use Yoda condition as in DCHECK(expected ==
> actual), I would write DCHECK(actual == expected) instead (though I am aware
of
> the DCHECK(expected, actual) order).
> 
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> net/spdy/http2_priority_dependencies.cc:61: entry_by_stream_id_[id] = --it;
> Nit: use back instead of end-1.
> 
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> net/spdy/http2_priority_dependencies.cc:74: return;
> Nit: add empty line after return.
> 
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> net/spdy/http2_priority_dependencies.cc:75:
> id_priority_lists_[emit->second->second].erase(emit->second);
> Optional: |IdList::iterator it = emit->second| for better readability.
> 
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> File net/spdy/http2_priority_dependencies.h (right):
> 
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> net/spdy/http2_priority_dependencies.h:35: // priority anyway.
> Nit: close opening parenthesis.
> 
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> net/spdy/http2_priority_dependencies.h:56: // to erase from.
> Makes sense.  Too bad you have multiple lists and thus cannot use
> linked_hash_map.

Hmmm.  I agree that that's the better, cleaner architecture.  But it's also a
whole lot of plumbing for something we're going to presumably rip out in a
release or two after we've confirmed that nothing's gone horribly wrong, which
seems close to busywork.  So my personal judgement would be that it's not worth
it; I'd instead suggest moving the finch control over to io_thread.cc as you
say, but leaving that control exercised through the static on
Http2PriorityDependencies.  

If you tell me again you'd really like the extended plumbing, I'll do it, but I
wanted to at least take one try at convincing you that the work isn't worth it
:-}.

[Bence, 2016-03-14 01:26:53]

On 2016/03/13 17:22:17, Randy Smith - Not in Fridays wrote:
> On 2016/03/10 20:21:14, Bence wrote:
> > Randy: this refactoring is great.  I really like your
> Http2PriorityDependencies
> > class.
> > 
> > There's one thing I'd like you to do differently.  Please do the
> > FieldTrialList::FindFullName call in IOThread, where all the SPDY and QUIC
> field
> > trial code lives.  Add your Boolean flag to IOThread::Globals, and to
> > HttpNetworkSession::Params, and SpdySessionPool constructor and member, and
> > SpdySession constructor and member, and Http2PriorityDependencies
constructor,
> > and mark Http2PriorityDependencies::enabled as const.  This is the way most
> net/
> > field trial parameters are wired up.  Oh and of course don't forget to do
the
> > same thing in IOSChromeIOThread.  Of course I'm giving myself extra work
when
> > I'm asking you to do this, because I have a CL in progress for factoring out
> > these methods, https://crrev.com/1687423002, which is blocked on
> > https://crrev.com/1699653002, so it will take a while, and I'll have to
rebase
> > on your change.  Oh well.
> > 
> > For tests, also add the flag to SpdySessionDependencies.  Please don't
forget
> > that SpdySessionDependencies has two constructors (it's on my list to
> > consolidate them using a delegate constructor), so you'll have to add the
> > default value for tests (presumably true) twice.  Also copy the flag in
> > CreateSessionParams.  Then in tests where you only want to test for
> flag==true,
> > you do not need to do anything.  And in tests where you want to test for
both
> > values of the flag, you can do that through the SpdySessionDependencies
> member. 
> > This way there is no need to use a static in Http2PriorityDependencies, and
> also
> > there is no need to restore the value when the test classes are destroyed. 
I
> > think this is much clearer.
> > 
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > File net/spdy/http2_priority_dependencies.cc (right):
> > 
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > net/spdy/http2_priority_dependencies.cc:46: DCHECK(entry_by_stream_id_.end()
> ==
> > entry_by_stream_id_.find(id));
> > I'm not sure it is customary to use Yoda condition as in DCHECK(expected ==
> > actual), I would write DCHECK(actual == expected) instead (though I am aware
> of
> > the DCHECK(expected, actual) order).
> > 
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > net/spdy/http2_priority_dependencies.cc:61: entry_by_stream_id_[id] = --it;
> > Nit: use back instead of end-1.
> > 
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > net/spdy/http2_priority_dependencies.cc:74: return;
> > Nit: add empty line after return.
> > 
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > net/spdy/http2_priority_dependencies.cc:75:
> > id_priority_lists_[emit->second->second].erase(emit->second);
> > Optional: |IdList::iterator it = emit->second| for better readability.
> > 
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > File net/spdy/http2_priority_dependencies.h (right):
> > 
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > net/spdy/http2_priority_dependencies.h:35: // priority anyway.
> > Nit: close opening parenthesis.
> > 
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > net/spdy/http2_priority_dependencies.h:56: // to erase from.
> > Makes sense.  Too bad you have multiple lists and thus cannot use
> > linked_hash_map.
> 
> Hmmm.  I agree that that's the better, cleaner architecture.  But it's also a
> whole lot of plumbing for something we're going to presumably rip out in a
> release or two after we've confirmed that nothing's gone horribly wrong, which
> seems close to busywork.  So my personal judgement would be that it's not
worth
> it; I'd instead suggest moving the finch control over to io_thread.cc as you
> say, but leaving that control exercised through the static on
> Http2PriorityDependencies.  
> 
> If you tell me again you'd really like the extended plumbing, I'll do it, but
I
> wanted to at least take one try at convincing you that the work isn't worth it
> :-}.

I'm very sorry to make you do extra work, but I would indeed really like the
extra plumbing.  I also went through this plumbing recently with a flag that is
only going to be in the codebase for one release or two, and I think this is the
right way to do it.  I believe it is easier to maintain the code if all SPDY
(and QUIC) related flags are wired through the same way.  Besides, your tests
will look so much nicer with the SpdySessionDependencies flag instead of
changing and reverting the value of a static.

[Randy Smith (Not in Mondays), 2016-03-14 02:05:36]

On 2016/03/14 01:26:53, Bence wrote:
> On 2016/03/13 17:22:17, Randy Smith - Not in Fridays wrote:
> > On 2016/03/10 20:21:14, Bence wrote:
> > > Randy: this refactoring is great.  I really like your
> > Http2PriorityDependencies
> > > class.
> > > 
> > > There's one thing I'd like you to do differently.  Please do the
> > > FieldTrialList::FindFullName call in IOThread, where all the SPDY and QUIC
> > field
> > > trial code lives.  Add your Boolean flag to IOThread::Globals, and to
> > > HttpNetworkSession::Params, and SpdySessionPool constructor and member,
and
> > > SpdySession constructor and member, and Http2PriorityDependencies
> constructor,
> > > and mark Http2PriorityDependencies::enabled as const.  This is the way
most
> > net/
> > > field trial parameters are wired up.  Oh and of course don't forget to do
> the
> > > same thing in IOSChromeIOThread.  Of course I'm giving myself extra work
> when
> > > I'm asking you to do this, because I have a CL in progress for factoring
out
> > > these methods, https://crrev.com/1687423002, which is blocked on
> > > https://crrev.com/1699653002, so it will take a while, and I'll have to
> rebase
> > > on your change.  Oh well.
> > > 
> > > For tests, also add the flag to SpdySessionDependencies.  Please don't
> forget
> > > that SpdySessionDependencies has two constructors (it's on my list to
> > > consolidate them using a delegate constructor), so you'll have to add the
> > > default value for tests (presumably true) twice.  Also copy the flag in
> > > CreateSessionParams.  Then in tests where you only want to test for
> > flag==true,
> > > you do not need to do anything.  And in tests where you want to test for
> both
> > > values of the flag, you can do that through the SpdySessionDependencies
> > member. 
> > > This way there is no need to use a static in Http2PriorityDependencies,
and
> > also
> > > there is no need to restore the value when the test classes are destroyed.

> I
> > > think this is much clearer.
> > > 
> > >
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > > File net/spdy/http2_priority_dependencies.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > > net/spdy/http2_priority_dependencies.cc:46:
DCHECK(entry_by_stream_id_.end()
> > ==
> > > entry_by_stream_id_.find(id));
> > > I'm not sure it is customary to use Yoda condition as in DCHECK(expected
==
> > > actual), I would write DCHECK(actual == expected) instead (though I am
aware
> > of
> > > the DCHECK(expected, actual) order).
> > > 
> > >
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > > net/spdy/http2_priority_dependencies.cc:61: entry_by_stream_id_[id] =
--it;
> > > Nit: use back instead of end-1.
> > > 
> > >
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > > net/spdy/http2_priority_dependencies.cc:74: return;
> > > Nit: add empty line after return.
> > > 
> > >
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > > net/spdy/http2_priority_dependencies.cc:75:
> > > id_priority_lists_[emit->second->second].erase(emit->second);
> > > Optional: |IdList::iterator it = emit->second| for better readability.
> > > 
> > >
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > > File net/spdy/http2_priority_dependencies.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > > net/spdy/http2_priority_dependencies.h:35: // priority anyway.
> > > Nit: close opening parenthesis.
> > > 
> > >
> >
>
https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
> > > net/spdy/http2_priority_dependencies.h:56: // to erase from.
> > > Makes sense.  Too bad you have multiple lists and thus cannot use
> > > linked_hash_map.
> > 
> > Hmmm.  I agree that that's the better, cleaner architecture.  But it's also
a
> > whole lot of plumbing for something we're going to presumably rip out in a
> > release or two after we've confirmed that nothing's gone horribly wrong,
which
> > seems close to busywork.  So my personal judgement would be that it's not
> worth
> > it; I'd instead suggest moving the finch control over to io_thread.cc as you
> > say, but leaving that control exercised through the static on
> > Http2PriorityDependencies.  
> > 
> > If you tell me again you'd really like the extended plumbing, I'll do it,
but
> I
> > wanted to at least take one try at convincing you that the work isn't worth
it
> > :-}.
> 
> I'm very sorry to make you do extra work, but I would indeed really like the
> extra plumbing.  I also went through this plumbing recently with a flag that
is
> only going to be in the codebase for one release or two, and I think this is
the
> right way to do it.  I believe it is easier to maintain the code if all SPDY
> (and QUIC) related flags are wired through the same way.  Besides, your tests
> will look so much nicer with the SpdySessionDependencies flag instead of
> changing and reverting the value of a static.

*chuckle*  Ok, will do.  And it has the extra value that it'll be a bit longer
until you get the code review sent back at you :-}.

[Randy Smith (Not in Mondays), 2016-03-16 19:38:10]

Bence: PTAL?

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
File net/spdy/http2_priority_dependencies.cc (right):

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
net/spdy/http2_priority_dependencies.cc:46: DCHECK(entry_by_stream_id_.end() ==
entry_by_stream_id_.find(id));
On 2016/03/10 20:21:13, Bence OOO until March 23 wrote:
> I'm not sure it is customary to use Yoda condition as in DCHECK(expected ==
> actual), I would write DCHECK(actual == expected) instead (though I am aware
of
> the DCHECK(expected, actual) order).

I've seen at least one tear on chromium-dev supporting the expected, actual
order, but I don't think it's policy and I agree with you about readability. 
Done.

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
net/spdy/http2_priority_dependencies.cc:61: entry_by_stream_id_[id] = --it;
On 2016/03/10 20:21:13, Bence OOO until March 23 wrote:
> Nit: use back instead of end-1.

The problem with back() is that it returns the entry, and not an iterator.  We
need an iterator because that's what you use to erase entries in constant time.

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
net/spdy/http2_priority_dependencies.cc:74: return;
On 2016/03/10 20:21:13, Bence OOO until March 23 wrote:
> Nit: add empty line after return.

Done.

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
net/spdy/http2_priority_dependencies.cc:75:
id_priority_lists_[emit->second->second].erase(emit->second);
On 2016/03/10 20:21:14, Bence OOO until March 23 wrote:
> Optional: |IdList::iterator it = emit->second| for better readability.

Done.

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
File net/spdy/http2_priority_dependencies.h (right):

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
net/spdy/http2_priority_dependencies.h:35: // priority anyway.
On 2016/03/10 20:21:14, Bence OOO until March 23 wrote:
> Nit: close opening parenthesis.

Done.

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
net/spdy/http2_priority_dependencies.h:56: // to erase from.
On 2016/03/10 20:21:14, Bence OOO until March 23 wrote:
> Makes sense.  Too bad you have multiple lists and thus cannot use
> linked_hash_map.

Acknowledged.

[Bence, 2016-03-17 01:48:59]

LGTM modulo nits (mostly optional), and please do the same thing to
IOSChromeIOThread (//ios/chrome/browser/ios_chrome_io_thread.mm) that you did to
IOThread.  Thank you.

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
File net/spdy/http2_priority_dependencies.cc (right):

https://codereview.chromium.org/1779733003/diff/1/net/spdy/http2_priority_dep...
net/spdy/http2_priority_dependencies.cc:61: entry_by_stream_id_[id] = --it;
On 2016/03/16 19:38:10, Randy Smith - Not in Fridays wrote:
> On 2016/03/10 20:21:13, Bence OOO until March 23 wrote:
> > Nit: use back instead of end-1.
> 
> The problem with back() is that it returns the entry, and not an iterator.  We
> need an iterator because that's what you use to erase entries in constant
time.

Sorry, I missed that.  Okay.

https://codereview.chromium.org/1779733003/diff/100001/net/spdy/http2_priorit...
File net/spdy/http2_priority_dependencies.cc (right):

https://codereview.chromium.org/1779733003/diff/100001/net/spdy/http2_priorit...
net/spdy/http2_priority_dependencies.cc:33: entry_by_stream_id_[id] = --it;
Optional: would you consider
    --it;
    entry_by_stream_id_[id] = it;
so that the reader does not have to scratch their head about whether the prefix
form is pre-decrement or post-decrement?  (I have to confess it takes me some
time to recall this.)  Alas, I understand that the style guide does not require
this.

https://codereview.chromium.org/1779733003/diff/100001/net/spdy/spdy_session_...
File net/spdy/spdy_session_pool_unittest.cc (right):

https://codereview.chromium.org/1779733003/diff/100001/net/spdy/spdy_session_...
net/spdy/spdy_session_pool_unittest.cc:522:
session_deps_.host_resolver->set_synchronous_mode(true);
Why did you move the synchronous line below the comment about dependencies?

https://codereview.chromium.org/1779733003/diff/100001/net/spdy/spdy_session_...
net/spdy/spdy_session_pool_unittest.cc:523:
session_deps_.enable_priority_dependencies = true;
Optional: const bool enable_priority_dependencies = true; and use that local
instead of true in these two lines, or add /*enable_priority_dependencies=*/ in
spdy_util arguments, so that the reader of this code does not have to look up
what the second constructor argument is.

[Bence, 2016-03-17 12:26:00]

You might also want to change CL description to include reference to refactoring
in addition to bugfix.

https://codereview.chromium.org/1779733003/diff/100001/net/spdy/spdy_session_...
File net/spdy/spdy_session_pool_unittest.cc (right):

https://codereview.chromium.org/1779733003/diff/100001/net/spdy/spdy_session_...
net/spdy/spdy_session_pool_unittest.cc:523:
session_deps_.enable_priority_dependencies = true;
I guess I mean

const bool enable_priority_dependencies = true;
session_deps_.enable_priority_dependencies = enable_priority_dependencies;
SpdyTestUtil spdy_util(GetParam(), enable_priority_dependencies);

or

session_deps_.enable_priority_dependencies = true;
SpdyTestUtil spdy_util(GetParam(), /*enable_priority_dependencies=*/ true);

or 

session_deps_.enable_priority_dependencies = true;
SpdyTestUtil spdy_util(GetParam(), session_deps_.enable_priority_dependencies);

or you can even omit setting |session_deps_.enable_priority_dependencies| and
modify the comment to say that you set up the test class to match default
behavior.

This is all still optional.

[Randy Smith (Not in Mondays), 2016-03-17 20:22:02]

Description was changed from

==========
Fix bug in net::RequestPriority -> HTTP/2 dependency conversion.

This CL also defaults this behavior on (subject to finch override) and
removes the non-default performance bot configuration.

BUG=590225
R=bnc@chromium.org
R=isherman@chromium.org
==========

to

==========
Fix bug in net::RequestPriority -> HTTP/2 dependency conversion.

This CL also defaults this behavior on (subject to finch override), refactors
the finch control to be in line with network stack defaults (== based in
io_thread.cc),  and removes the non-default performance bot configuration.

BUG=590225
R=bnc@chromium.org
R=isherman@chromium.org
==========

[Randy Smith (Not in Mondays), 2016-03-17 20:27:43]

Thanks!  I've incorporated all your comments (sorry for forgetting about
IOSChromeThread).  I'm going to hold off on pulling more reviewers in until I
have an at least moderately clean try bot run.

https://codereview.chromium.org/1779733003/diff/100001/net/spdy/http2_priorit...
File net/spdy/http2_priority_dependencies.cc (right):

https://codereview.chromium.org/1779733003/diff/100001/net/spdy/http2_priorit...
net/spdy/http2_priority_dependencies.cc:33: entry_by_stream_id_[id] = --it;
On 2016/03/17 01:48:59, Bence OOO until March 23 wrote:
> Optional: would you consider
>     --it;
>     entry_by_stream_id_[id] = it;
> so that the reader does not have to scratch their head about whether the
prefix
> form is pre-decrement or post-decrement?  (I have to confess it takes me some
> time to recall this.)  Alas, I understand that the style guide does not
require
> this.

I think of this as your code, so ok, but I tend to think of "--it" ==
pre-decrement and "it--" == post-decrement as solidly part of the language.

I will include a mini-rant here that I consider it crazy-cookoo that the
iterator implementation makes you do --it instead of "it - 1" :-{.

https://codereview.chromium.org/1779733003/diff/100001/net/spdy/spdy_session_...
File net/spdy/spdy_session_pool_unittest.cc (right):

https://codereview.chromium.org/1779733003/diff/100001/net/spdy/spdy_session_...
net/spdy/spdy_session_pool_unittest.cc:522:
session_deps_.host_resolver->set_synchronous_mode(true);
On 2016/03/17 01:48:59, Bence OOO until March 23 wrote:
> Why did you move the synchronous line below the comment about dependencies?

Whoops, sorry, I think I misread it and thought it had to do with the
dependencies.  Fixed.

https://codereview.chromium.org/1779733003/diff/100001/net/spdy/spdy_session_...
net/spdy/spdy_session_pool_unittest.cc:523:
session_deps_.enable_priority_dependencies = true;
On 2016/03/17 12:25:59, Bence OOO until March 23 wrote:
> I guess I mean
> 
> const bool enable_priority_dependencies = true;
> session_deps_.enable_priority_dependencies = enable_priority_dependencies;
> SpdyTestUtil spdy_util(GetParam(), enable_priority_dependencies);
> 
> or
> 
> session_deps_.enable_priority_dependencies = true;
> SpdyTestUtil spdy_util(GetParam(), /*enable_priority_dependencies=*/ true);
> 
> or 
> 
> session_deps_.enable_priority_dependencies = true;
> SpdyTestUtil spdy_util(GetParam(),
session_deps_.enable_priority_dependencies);
> 
> or you can even omit setting |session_deps_.enable_priority_dependencies| and
> modify the comment to say that you set up the test class to match default
> behavior.
> 
> This is all still optional.

I took the comment option.

[Randy Smith (Not in Mondays), 2016-03-17 22:01:23]

rdsmith@chromium.org changed reviewers:
+ rch@chromium.org

[Randy Smith (Not in Mondays), 2016-03-17 22:02:35]

Ryan, could you take a look at chrome/browser/* and ios/chrome/browser/*?  You
have some of the context for the entire CL from our conversations, and you're
the person in the OWNERS file for those files that I'm not pretty sure is
overloaded at the moment (you might be too, I just don't know :-}).

[Ryan Hamilton, 2016-03-18 20:41:59]

chrome/browser/* and ios/chrome/browser/*: LGTM. Would it be easy to add an
io_thread_unittest?

[Bence, 2016-03-19 01:02:39]

On 2016/03/18 20:41:59, Ryan Hamilton wrote:
> chrome/browser/* and ios/chrome/browser/*: LGTM. Would it be easy to add an
> io_thread_unittest?

Ryan:  As soon as this CL lands, I'll rebase https://crrev.com/1687423002 on it,
in which I will add unittests for this trial in a lot cleaner way anyway.  I
normally strongly support adding unittests together with the change, but in this
case, my change would rewrite unittests entirely, making it kinda wasted effort,
so I think it's okay not to add them with this CL.  WDYT?

[Ryan Hamilton, 2016-03-21 14:40:49]

On 2016/03/19 01:02:39, Bence OOO until March 23 wrote:
> On 2016/03/18 20:41:59, Ryan Hamilton wrote:
> > chrome/browser/* and ios/chrome/browser/*: LGTM. Would it be easy to add an
> > io_thread_unittest?
> 
> Ryan:  As soon as this CL lands, I'll rebase https://crrev.com/1687423002 on
it,
> in which I will add unittests for this trial in a lot cleaner way anyway.  I
> normally strongly support adding unittests together with the change, but in
this
> case, my change would rewrite unittests entirely, making it kinda wasted
effort,
> so I think it's okay not to add them with this CL.  WDYT?

SGTM!

[Randy Smith (Not in Mondays), 2016-03-21 14:44:42]

On 2016/03/21 14:40:49, Ryan Hamilton wrote:
> On 2016/03/19 01:02:39, Bence OOO until March 23 wrote:
> > On 2016/03/18 20:41:59, Ryan Hamilton wrote:
> > > chrome/browser/* and ios/chrome/browser/*: LGTM. Would it be easy to add
an
> > > io_thread_unittest?
> > 
> > Ryan:  As soon as this CL lands, I'll rebase https://crrev.com/1687423002 on
> it,
> > in which I will add unittests for this trial in a lot cleaner way anyway.  I
> > normally strongly support adding unittests together with the change, but in
> this
> > case, my change would rewrite unittests entirely, making it kinda wasted
> effort,
> > so I think it's okay not to add them with this CL.  WDYT?
> 
> SGTM!

And done!!  :-}

Thank you both.

[Randy Smith (Not in Mondays), 2016-03-21 14:44:51]

The CQ bit was checked by rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2016-03-21 14:44:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, bnc@chromium.org
Link to the patchset: https://codereview.chromium.org/1779733003/#ps160001
(title: "Added field trial string constants to IOSChromeThread.")

[commit-bot: I haz the power, 2016-03-21 14:45:01]

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

[commit-bot: I haz the power, 2016-03-21 15:28:08]

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

[commit-bot: I haz the power, 2016-03-21 15:28:10]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Randy Smith (Not in Mondays), 2016-03-21 17:04:47]

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

[Randy Smith (Not in Mondays), 2016-03-21 17:04:48]

Helen, could you take a look at the PS10->PS11 change?  It's a five character
change, so what you probably most want to review is this summary :-}.

Some time ago I landed a change to set HTTP/2 dependencies based on
net::RequestPriority.  That turned out to have a large bug in it (see
http://crbug.com/590225) and this CL has the fix.  As a result of the
post-mortem, I decided that it wasn't necessary to roll the fix out using finch,
so this CL also defaults the setting of dependencies on, and modifies all the
tests to handle that.

Most of the SpdyStream tests tests with both that flag on and off (feature
enabled and disabled).  I decided that for the bidirectional stream tests, that
wasn't necessary; since I was enabling it by default in the base code, and
testing it elsewhere, I could just shift the bidi unit tests over to enabling it
by default.  That decision is what I'm asking you to review.

Thanks!

[xunjieli, 2016-03-21 17:11:56]

On 2016/03/21 17:04:48, Randy Smith - Not in Fridays wrote:
> Helen, could you take a look at the PS10->PS11 change?  It's a five character
> change, so what you probably most want to review is this summary :-}.
> 
> Some time ago I landed a change to set HTTP/2 dependencies based on
> net::RequestPriority.  That turned out to have a large bug in it (see
> http://crbug.com/590225) and this CL has the fix.  As a result of the
> post-mortem, I decided that it wasn't necessary to roll the fix out using
finch,
> so this CL also defaults the setting of dependencies on, and modifies all the
> tests to handle that.
> 
> Most of the SpdyStream tests tests with both that flag on and off (feature
> enabled and disabled).  I decided that for the bidirectional stream tests,
that
> wasn't necessary; since I was enabling it by default in the base code, and
> testing it elsewhere, I could just shift the bidi unit tests over to enabling
it
> by default.  That decision is what I'm asking you to review.

lgtm. Thanks for the context!

> Thanks!

[Randy Smith (Not in Mondays), 2016-03-21 17:14:53]

The CQ bit was checked by rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2016-03-21 17:14:54]

The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org,
isherman@chromium.org, rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1779733003/#ps200001
(title: "Make bi-directional stream unittests enable priority->dependency
setting.")

[commit-bot: I haz the power, 2016-03-21 17:15:05]

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

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

Description was changed from

==========
Fix bug in net::RequestPriority -> HTTP/2 dependency conversion.

This CL also defaults this behavior on (subject to finch override), refactors
the finch control to be in line with network stack defaults (== based in
io_thread.cc),  and removes the non-default performance bot configuration.

BUG=590225
R=bnc@chromium.org
R=isherman@chromium.org
==========

to

==========
Fix bug in net::RequestPriority -> HTTP/2 dependency conversion.

This CL also defaults this behavior on (subject to finch override), refactors
the finch control to be in line with network stack defaults (== based in
io_thread.cc),  and removes the non-default performance bot configuration.

BUG=590225
R=bnc@chromium.org
R=isherman@chromium.org
==========

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

Committed patchset #11 (id:200001)

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

Description was changed from

==========
Fix bug in net::RequestPriority -> HTTP/2 dependency conversion.

This CL also defaults this behavior on (subject to finch override), refactors
the finch control to be in line with network stack defaults (== based in
io_thread.cc),  and removes the non-default performance bot configuration.

BUG=590225
R=bnc@chromium.org
R=isherman@chromium.org
==========

to

==========
Fix bug in net::RequestPriority -> HTTP/2 dependency conversion.

This CL also defaults this behavior on (subject to finch override), refactors
the finch control to be in line with network stack defaults (== based in
io_thread.cc),  and removes the non-default performance bot configuration.

BUG=590225
R=bnc@chromium.org
R=isherman@chromium.org

Committed: https://crrev.com/2e54d1f83e6ae57cb404658f361e469dd9ed2a92
Cr-Commit-Position: refs/heads/master@{#382360}
==========

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

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