[Net] Propagate priority changes from URLRequest to HttpTransaction

[Net] Propagate priority changes from URLRequest to HttpTransaction

This is in preparation for propagating priority changes from
ResourceScheduler all the way to HostResolver and ClientSocketPool.

Add some NetLog events and parameters for priority.

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

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

[akalin, 2013-03-11 20:10:52]

+mmenke for net/
+simonjam for content/browser/loader
+sky for chrome/

[mmenke, 2013-03-12 18:40:22]

https://codereview.chromium.org/12701011/diff/1/net/http/http_cache_transacti...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/12701011/diff/1/net/http/http_cache_transacti...
net/http/http_cache_transaction.cc:470: NetLog::IntegerCallback("priority",
priority));
Same comment goes here as for the HttpNetworkTransaction logging.

https://codereview.chromium.org/12701011/diff/1/net/http/http_cache_transacti...
net/http/http_cache_transaction.cc:471: // Do nothing for now.
The cache transaction wraps the network transaction, so this should be:

if (network_trans_)
  network_trans_->SetPriority(priority);

https://codereview.chromium.org/12701011/diff/1/net/http/http_network_transac...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/12701011/diff/1/net/http/http_network_transac...
net/http/http_network_transaction.cc:388: bool
HttpNetworkTransaction::GetLoadTimingInfo(
Thanks for fixing my misplacement of this function!

https://codereview.chromium.org/12701011/diff/1/net/http/http_network_transac...
net/http/http_network_transaction.cc:404: NetLog::IntegerCallback("priority",
priority));
I don't think this is an event worth logging.

HttpNetworkTransactions, HttpCache::Transactions, and URLRequests all share the
same BoundNetLog object.  The structure of the log itself makes it clear when
there's a transaction going on and when there isn't.

So we'd basically always see 3 of these events in a row, except when the
priority is set very early on in the process.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request.c...
net/url_request/url_request.cc:833: DCHECK_LT(priority, NUM_PRIORITIES);
if priority matches priority_, should we abort early (Either with or without
logging)?

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request.h
File net/url_request/url_request.h (right):

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request.h...
net/url_request/url_request.h:664: FRIEND_TEST_ALL_PREFIXES(URLRequestTest,
Priority);
I don't believe this is currently needed.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job.cc:1581: void
URLRequestHttpJob::SetPriority(RequestPriority priority) {
request_info_.priority = priority;?

We may want to separate priority out from request_info_ in a future CL.  I
believe request_info_ is currently constant for the duration of a transaction,
and seems a little unexpected to be changing something that's constant out from
under lower level objects that have access to it, even if we notify them of the
change.

Downside of that is that we'd need to track the priority directly at a couple
levels that we currently just rely on a pointer to the HttpRequestInfo for.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job.cc:1582: if (transaction_.get())
nit:  "transaction_" is now preferred.  This file uses .get() a lot
unnecessarily, for legacy reasons, but figure we should stick with the preferred
style for new code.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
File net/url_request/url_request_http_job.h (right):

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job.h:147:
FRIEND_TEST_ALL_PREFIXES(URLRequestHttpJob, Priority);
Rather than maintain a list of every URLRequestHttpJob here (Which, admittedly,
is currently rather short), why don't we just make a child class of
URLRequestHttpJob in the test file.  It can have an accessor for transaction_,
which is already protected.

Alternatively, you could add a text fixture and friend that.  I tend to prefer
the inheritance route, since friending functions exposes more, and adds stuff to
the header, but I'm fine with either approach.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit:  2013

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job_unittest.cc:26:
EXPECT_TRUE(!job->transaction_);
EXPECT_FALSE(job->transaction_);?

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job_unittest.cc:34:
EXPECT_TRUE(job->transaction_ != NULL);
EXPECT_TRUE(job->transaction_);?

Think it's a little weird to use != NULL here, and "!job->transaction_" above,
instead of == NULL.

EXPECTing on the raw pointer values should also provide a little more
information in debug output.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_u...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_u...
net/url_request/url_request_unittest.cc:1412:
interceptor.intercept_main_request_ = true;
Rather than having URLRequest friend the individual test, how about using
URLRequestTest::AddTestInterceptor().

Then you can set the TestJobInterceptor return return a URLRequestTestJob
created by the test.  This also avoids the cast, as well as using the deprecated
Interceptor class.

[cbentzel, 2013-03-12 18:58:28]

General question - at one point the priorities were going to be based
on an explicit dependency graph between resources, rather than
priorities (at least at the SPDY layer, and we would do that
propagation into the net stack as well).

I guess that is no longer the case, and that we are now targeting
dynamic numerical priorities?

On Mon, Mar 11, 2013 at 4:10 PM,  <akalin@chromium.org> wrote:
> Reviewers: mmenke,
>
> Message:
> +mmenke for net/
> +simonjam for content/browser/loader
> +sky for chrome/
>
> Description:
> [Net] Propagate priority changes from URLRequest to HttpTransaction
>
> This is in preparation for propagating priority changes from
> ResourceScheduler all the way to HostResolver and ClientSocketPool.
>
> Add some NetLog events and parameters for priority.
>
> BUG=166689
>
>
> Please review this at https://codereview.chromium.org/12701011/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files:
>   M chrome/browser/predictors/resource_prefetcher.cc
>   M chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
>   M content/browser/loader/resource_dispatcher_host_impl.cc
>   M content/browser/loader/resource_scheduler.cc
>   M content/browser/loader/resource_scheduler_unittest.cc
>   M net/base/net_log_event_type_list.h
>   M net/http/http_cache_transaction.h
>   M net/http/http_cache_transaction.cc
>   M net/http/http_network_transaction.h
>   M net/http/http_network_transaction.cc
>   M net/http/http_stream_factory_impl_job.cc
>   M net/http/http_transaction.h
>   M net/http/http_transaction_unittest.h
>   M net/http/http_transaction_unittest.cc
>   M net/net.gyp
>   M net/url_request/url_request.h
>   M net/url_request/url_request.cc
>   M net/url_request/url_request_http_job.h
>   M net/url_request/url_request_http_job.cc
>   A net/url_request/url_request_http_job_unittest.cc
>   M net/url_request/url_request_job.h
>   M net/url_request/url_request_job.cc
>   M net/url_request/url_request_test_job.h
>   M net/url_request/url_request_test_job.cc
>   M net/url_request/url_request_unittest.cc
>
>

[mmenke, 2013-03-12 18:59:39]

Also, we aren't currently testing that a real URLRequestJob will respect its new
priority.  Doing this is tricky, since we have to change the priority when
there's a URLRequestJob, but either before that job starts an HttpTransaction,
or when it's creating a new HttpTransaction (Like for an auth request). 
Redirects use new Jobs, so they won't work.

I think the easiest way to do this would be to add priority to NetLog
HTTP_STREAM_JOBs on creation.  And to use a CapturingNetLog for the test, then
just dig through the objects looking for the higher priority job.

This is complicated, but it's a case we should be handling correctly anyways,
and don't see us specifically testing it later, when there will be more
interesting cases to test for.

I'm open to other ideas.

[akalin, 2013-03-12 22:08:27]

PTAL

https://codereview.chromium.org/12701011/diff/1/net/http/http_cache_transacti...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/12701011/diff/1/net/http/http_cache_transacti...
net/http/http_cache_transaction.cc:470: NetLog::IntegerCallback("priority",
priority));
On 2013/03/12 18:40:22, mmenke wrote:
> Same comment goes here as for the HttpNetworkTransaction logging.

Done.

https://codereview.chromium.org/12701011/diff/1/net/http/http_cache_transacti...
net/http/http_cache_transaction.cc:471: // Do nothing for now.
On 2013/03/12 18:40:22, mmenke wrote:
> The cache transaction wraps the network transaction, so this should be:
> 
> if (network_trans_)
>   network_trans_->SetPriority(priority);

Oh!  Done.

https://codereview.chromium.org/12701011/diff/1/net/http/http_network_transac...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/12701011/diff/1/net/http/http_network_transac...
net/http/http_network_transaction.cc:404: NetLog::IntegerCallback("priority",
priority));
On 2013/03/12 18:40:22, mmenke wrote:
> I don't think this is an event worth logging.
> 
> HttpNetworkTransactions, HttpCache::Transactions, and URLRequests all share
the
> same BoundNetLog object.  The structure of the log itself makes it clear when
> there's a transaction going on and when there isn't.
> 
> So we'd basically always see 3 of these events in a row, except when the
> priority is set very early on in the process.

Okay. I think we should still keep the new one URLRequest, since having the
priority updated is a distinct event overall. Ideally, the only other things
that would log the new priority would be the actual priority queues themselves
(or the owner of the queues), but I haven't plumbed it through yet.  How about I
keep the one in HttpNetworkTransaction (the lowest-layer one) and change the
TODO to remove the log once it's plumbed through?

Also, I think having the priority set early on is the common case.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request.c...
net/url_request/url_request.cc:833: DCHECK_LT(priority, NUM_PRIORITIES);
On 2013/03/12 18:40:22, mmenke wrote:
> if priority matches priority_, should we abort early (Either with or without
> logging)?

Done.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request.h
File net/url_request/url_request.h (right):

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request.h...
net/url_request/url_request.h:664: FRIEND_TEST_ALL_PREFIXES(URLRequestTest,
Priority);
On 2013/03/12 18:40:22, mmenke wrote:
> I don't believe this is currently needed.

Not after addressing your other comments.  Done.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job.cc:1581: void
URLRequestHttpJob::SetPriority(RequestPriority priority) {
On 2013/03/12 18:40:22, mmenke wrote:
> request_info_.priority = priority;?
> 
> We may want to separate priority out from request_info_ in a future CL.  I
> believe request_info_ is currently constant for the duration of a transaction,
> and seems a little unexpected to be changing something that's constant out
from
> under lower level objects that have access to it, even if we notify them of
the
> change.
> 
> Downside of that is that we'd need to track the priority directly at a couple
> levels that we currently just rely on a pointer to the HttpRequestInfo for.

Done, added TODO to separate out priority.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job.cc:1582: if (transaction_.get())
On 2013/03/12 18:40:22, mmenke wrote:
> nit:  "transaction_" is now preferred.  This file uses .get() a lot
> unnecessarily, for legacy reasons, but figure we should stick with the
preferred
> style for new code.

Done.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
File net/url_request/url_request_http_job.h (right):

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job.h:147:
FRIEND_TEST_ALL_PREFIXES(URLRequestHttpJob, Priority);
On 2013/03/12 18:40:22, mmenke wrote:
> Rather than maintain a list of every URLRequestHttpJob here (Which,
admittedly,
> is currently rather short), why don't we just make a child class of
> URLRequestHttpJob in the test file.  It can have an accessor for transaction_,
> which is already protected.
> 
> Alternatively, you could add a text fixture and friend that.  I tend to prefer
> the inheritance route, since friending functions exposes more, and adds stuff
to
> the header, but I'm fine with either approach.

I prefer the latter, since inheriting from the class you're testing can have
inadvertent effects. Done.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/03/12 18:40:22, mmenke wrote:
> nit:  2013

Done.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job_unittest.cc:26:
EXPECT_TRUE(!job->transaction_);
On 2013/03/12 18:40:22, mmenke wrote:
> EXPECT_FALSE(job->transaction_);?

Done.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job_unittest.cc:34:
EXPECT_TRUE(job->transaction_ != NULL);
On 2013/03/12 18:40:22, mmenke wrote:
> EXPECT_TRUE(job->transaction_);?
> 
> Think it's a little weird to use != NULL here, and "!job->transaction_" above,
> instead of == NULL.
> 
> EXPECTing on the raw pointer values should also provide a little more
> information in debug output.

I think I'm just paranoid of how various compilers handle implicit conversion to
bool. We'll see if this blows up on Windows or Android.  Done.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_u...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request_u...
net/url_request/url_request_unittest.cc:1412:
interceptor.intercept_main_request_ = true;
On 2013/03/12 18:40:22, mmenke wrote:
> Rather than having URLRequest friend the individual test, how about using
> URLRequestTest::AddTestInterceptor().
> 
> Then you can set the TestJobInterceptor return return a URLRequestTestJob
> created by the test.  This also avoids the cast, as well as using the
deprecated
> Interceptor class.

Done.

[akalin, 2013-03-12 22:09:14]

On 2013/03/12 18:58:28, cbentzel wrote:
> General question - at one point the priorities were going to be based
> on an explicit dependency graph between resources, rather than
> priorities (at least at the SPDY layer, and we would do that
> propagation into the net stack as well).
> 
> I guess that is no longer the case, and that we are now targeting
> dynamic numerical priorities?

No, I think even with the dep-graph/dep-list approach we still need to support
numerical priorities. Don't the roots of the graphs/lists have only numerical
priorities, and don't we need them for e.g. foreground tab vs. background tab?

[akalin, 2013-03-12 22:09:34]

On 2013/03/12 18:59:39, mmenke wrote:
> Also, we aren't currently testing that a real URLRequestJob will respect its
new
> priority.  Doing this is tricky, since we have to change the priority when
> there's a URLRequestJob, but either before that job starts an HttpTransaction,
> or when it's creating a new HttpTransaction (Like for an auth request). 
> Redirects use new Jobs, so they won't work.
> 
> I think the easiest way to do this would be to add priority to NetLog
> HTTP_STREAM_JOBs on creation.  And to use a CapturingNetLog for the test, then
> just dig through the objects looking for the higher priority job.
> 
> This is complicated, but it's a case we should be handling correctly anyways,
> and don't see us specifically testing it later, when there will be more
> interesting cases to test for.
> 
> I'm open to other ideas.

Hmm, what exactly is URLRequestJob supposed to do on SetPriority?

[mmenke, 2013-03-12 22:21:21]

On 2013/03/12 22:09:34, akalin wrote:
> On 2013/03/12 18:59:39, mmenke wrote:
> > Also, we aren't currently testing that a real URLRequestJob will respect its
> new
> > priority.  Doing this is tricky, since we have to change the priority when
> > there's a URLRequestJob, but either before that job starts an
HttpTransaction,
> > or when it's creating a new HttpTransaction (Like for an auth request). 
> > Redirects use new Jobs, so they won't work.
> > 
> > I think the easiest way to do this would be to add priority to NetLog
> > HTTP_STREAM_JOBs on creation.  And to use a CapturingNetLog for the test,
then
> > just dig through the objects looking for the higher priority job.
> > 
> > This is complicated, but it's a case we should be handling correctly
anyways,
> > and don't see us specifically testing it later, when there will be more
> > interesting cases to test for.
> > 
> > I'm open to other ideas.
> 
> Hmm, what exactly is URLRequestJob supposed to do on SetPriority?

Nothing, but if it makes an HttpTransaction afterwards, the transaction should
have the new priority, rather than the old one.

[mmenke, 2013-03-12 22:58:26]

I'll take a closer look at your changes tomorrow morning.  My main concern is
the aforementioned unit test that makes sure the URLRequestJob's new priority
affects new HttpTransactions.

https://codereview.chromium.org/12701011/diff/1/net/http/http_network_transac...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/12701011/diff/1/net/http/http_network_transac...
net/http/http_network_transaction.cc:404: NetLog::IntegerCallback("priority",
priority));
On 2013/03/12 22:08:28, akalin wrote:
> On 2013/03/12 18:40:22, mmenke wrote:
> > I don't think this is an event worth logging.
> > 
> > HttpNetworkTransactions, HttpCache::Transactions, and URLRequests all share
> the
> > same BoundNetLog object.  The structure of the log itself makes it clear
when
> > there's a transaction going on and when there isn't.
> > 
> > So we'd basically always see 3 of these events in a row, except when the
> > priority is set very early on in the process.
> 
> Okay. I think we should still keep the new one URLRequest, since having the
> priority updated is a distinct event overall. Ideally, the only other things
> that would log the new priority would be the actual priority queues themselves
> (or the owner of the queues), but I haven't plumbed it through yet.  How about
I
> keep the one in HttpNetworkTransaction (the lowest-layer one) and change the
> TODO to remove the log once it's plumbed through?
> 
> Also, I think having the priority set early on is the common case.

I agree that we should keep the one in URLRequest.  I don't see the point of
having a temporary one here, unless we have a temporary unit test that checks it
or something.

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request.h
File net/url_request/url_request.h (right):

https://codereview.chromium.org/12701011/diff/1/net/url_request/url_request.h...
net/url_request/url_request.h:664: FRIEND_TEST_ALL_PREFIXES(URLRequestTest,
Priority);
On 2013/03/12 22:08:28, akalin wrote:
> On 2013/03/12 18:40:22, mmenke wrote:
> > I don't believe this is currently needed.
> 
> Not after addressing your other comments.  Done.

Oops - yea, comment was wrong.  I missed the last file in my first pass.

[mmenke, 2013-03-13 15:24:37]

https://codereview.chromium.org/12701011/diff/13001/net/http/http_cache_trans...
File net/http/http_cache_transaction_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/13001/net/http/http_cache_trans...
net/http/http_cache_transaction_unittest.cc:39: TEST_F(HttpCacheTransactionTest,
SetPriority) {
I believe the HttpCache unittests have all the necessary infrastructure to do
this without adding any new friends.

https://codereview.chromium.org/12701011/diff/13001/net/url_request/url_reque...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/13001/net/url_request/url_reque...
net/url_request/url_request_http_job_unittest.cc:35:
static_cast<MockNetworkTransaction*>(job.transaction_.get());
I'd still really like to get rid of this friend requirement, net tests generally
avoid doing this.

Could create our own version MockNetworkLayer, and just have it make its single
network transaction accessible.  Doesn't use a whole lot of code.

https://codereview.chromium.org/12701011/diff/13001/net/url_request/url_reque...
net/url_request/url_request_http_job_unittest.cc:65: // called.
nit:  Think this fits on one line.

[mmenke, 2013-03-13 15:32:45]

Oh, and another unit test suggestion - for a request that is redirected, make
sure the second request gets a modified priority.

Shouldn't be much different from URLRequestTest.Priority.  Will probably have to
have the intercepted set a URLRequest's priority on interception.

[akalin, 2013-03-13 16:06:45]

https://codereview.chromium.org/12701011/diff/13001/net/http/http_cache_trans...
File net/http/http_cache_transaction_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/13001/net/http/http_cache_trans...
net/http/http_cache_transaction_unittest.cc:39: TEST_F(HttpCacheTransactionTest,
SetPriority) {
On 2013/03/13 15:24:38, mmenke wrote:
> I believe the HttpCache unittests have all the necessary infrastructure to do
> this without adding any new friends.

I can't see how; the name HttpCache::Transaction isn't even public. Other than
that, how would I check that the priority of the underlying mock transaction is
set properly?

[mmenke, 2013-03-13 16:16:11]

https://codereview.chromium.org/12701011/diff/13001/net/http/http_cache_trans...
File net/http/http_cache_transaction_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/13001/net/http/http_cache_trans...
net/http/http_cache_transaction_unittest.cc:39: TEST_F(HttpCacheTransactionTest,
SetPriority) {
On 2013/03/13 16:06:45, akalin wrote:
> On 2013/03/13 15:24:38, mmenke wrote:
> > I believe the HttpCache unittests have all the necessary infrastructure to
do
> > this without adding any new friends.
> 
> I can't see how; the name HttpCache::Transaction isn't even public. Other than
> that, how would I check that the priority of the underlying mock transaction
is
> set properly?

They create the MockTransaction before the HttpCacheTransaction, and then they
use cache->CreateTransaction, passing in the mock transaction as an argument.

[mmenke, 2013-03-13 16:22:23]

On 2013/03/13 16:16:11, mmenke wrote:
>
https://codereview.chromium.org/12701011/diff/13001/net/http/http_cache_trans...
> File net/http/http_cache_transaction_unittest.cc (right):
> 
>
https://codereview.chromium.org/12701011/diff/13001/net/http/http_cache_trans...
> net/http/http_cache_transaction_unittest.cc:39:
TEST_F(HttpCacheTransactionTest,
> SetPriority) {
> On 2013/03/13 16:06:45, akalin wrote:
> > On 2013/03/13 15:24:38, mmenke wrote:
> > > I believe the HttpCache unittests have all the necessary infrastructure to
> do
> > > this without adding any new friends.
> > 
> > I can't see how; the name HttpCache::Transaction isn't even public. Other
than
> > that, how would I check that the priority of the underlying mock transaction
> is
> > set properly?
> 
> They create the MockTransaction before the HttpCacheTransaction, and then they
> use cache->CreateTransaction, passing in the mock transaction as an argument.

Sorry, was wrong here...  I had thought "MockTransaction" was...well...a
MockTransaction.

[mmenke, 2013-03-13 16:27:27]

https://codereview.chromium.org/12701011/diff/13001/net/http/http_cache_trans...
File net/http/http_cache_transaction_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/13001/net/http/http_cache_trans...
net/http/http_cache_transaction_unittest.cc:39: TEST_F(HttpCacheTransactionTest,
SetPriority) {
On 2013/03/13 16:16:11, mmenke wrote:
> On 2013/03/13 16:06:45, akalin wrote:
> > On 2013/03/13 15:24:38, mmenke wrote:
> > > I believe the HttpCache unittests have all the necessary infrastructure to
> do
> > > this without adding any new friends.
> > 
> > I can't see how; the name HttpCache::Transaction isn't even public. Other
than
> > that, how would I check that the priority of the underlying mock transaction
> is
> > set properly?
> 
> They create the MockTransaction before the HttpCacheTransaction, and then they
> use cache->CreateTransaction, passing in the mock transaction as an argument.

The same thing I suggested in the other case will work here, though - a class
like MockNetworkLayer that keeps a pointer to a mock network transaction.

[akalin, 2013-03-13 16:57:14]

Still working on adding the couple of unit tests you suggested, but PTAL.

https://codereview.chromium.org/12701011/diff/13001/net/http/http_cache_trans...
File net/http/http_cache_transaction_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/13001/net/http/http_cache_trans...
net/http/http_cache_transaction_unittest.cc:39: TEST_F(HttpCacheTransactionTest,
SetPriority) {
On 2013/03/13 16:27:28, mmenke wrote:
> The same thing I suggested in the other case will work here, though - a class
> like MockNetworkLayer that keeps a pointer to a mock network transaction.

Okay that works. Since more than one thing now wants the pointer to the last
mock network transaction, just added it to MockNetworkLayer.

https://codereview.chromium.org/12701011/diff/13001/net/url_request/url_reque...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/13001/net/url_request/url_reque...
net/url_request/url_request_http_job_unittest.cc:35:
static_cast<MockNetworkTransaction*>(job.transaction_.get());
On 2013/03/13 15:24:38, mmenke wrote:
> I'd still really like to get rid of this friend requirement, net tests
generally
> avoid doing this.
> 
> Could create our own version MockNetworkLayer, and just have it make its
single
> network transaction accessible.  Doesn't use a whole lot of code.

Done. Ended up inheriting from URLRequestHttpJob and adding
last_network_transaction() to MockNetworkLayer.

https://codereview.chromium.org/12701011/diff/13001/net/url_request/url_reque...
net/url_request/url_request_http_job_unittest.cc:65: // called.
On 2013/03/13 15:24:38, mmenke wrote:
> nit:  Think this fits on one line.

Done.

[akalin, 2013-03-13 18:53:04]

I think I added the tests that you wanted. I have one question, though:

https://codereview.chromium.org/12701011/diff/51001/net/http/http_cache_trans...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/12701011/diff/51001/net/http/http_cache_trans...
net/http/http_cache_transaction.cc:470: network_trans_->SetPriority(priority);
One question: can HttpCache::Transaction have multiple network_trans_ instances
in its lifetime? i.e., do I have to stash the priority somewhere and make sure
newly-created network_trans_ instances pick it up?

[mmenke, 2013-03-13 19:01:23]

https://codereview.chromium.org/12701011/diff/51001/net/http/http_cache_trans...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/12701011/diff/51001/net/http/http_cache_trans...
net/http/http_cache_transaction.cc:470: network_trans_->SetPriority(priority);
On 2013/03/13 18:53:04, akalin wrote:
> One question: can HttpCache::Transaction have multiple network_trans_
instances
> in its lifetime? i.e., do I have to stash the priority somewhere and make sure
> newly-created network_trans_ instances pick it up?

RestartNetworkRequest() does this.  There may be other times when this is done
as well.

Since the priority is currently part of HttpRequestInfo, which we have a pointer
to, we don't need to stash it currently.

However, if we remove it from that data structure (Per the TODO), we will need
to stash it both here and in HttpNetworkTransaction.

[akalin, 2013-03-13 19:08:56]

On 2013/03/13 19:01:23, mmenke wrote:
>
https://codereview.chromium.org/12701011/diff/51001/net/http/http_cache_trans...
> File net/http/http_cache_transaction.cc (right):
> 
>
https://codereview.chromium.org/12701011/diff/51001/net/http/http_cache_trans...
> net/http/http_cache_transaction.cc:470: network_trans_->SetPriority(priority);
> On 2013/03/13 18:53:04, akalin wrote:
> > One question: can HttpCache::Transaction have multiple network_trans_
> instances
> > in its lifetime? i.e., do I have to stash the priority somewhere and make
sure
> > newly-created network_trans_ instances pick it up?
> 
> RestartNetworkRequest() does this.  There may be other times when this is done
> as well.

Ah, so I do need to handle that case and write a test for it. This is turning
surprisingly complex!

> 
> Since the priority is currently part of HttpRequestInfo, which we have a
pointer
> to, we don't need to stash it currently.
> 
> However, if we remove it from that data structure (Per the TODO), we will need
> to stash it both here and in HttpNetworkTransaction.

Right.

[mmenke, 2013-03-13 19:27:58]

On 2013/03/13 19:08:56, akalin wrote:
> On 2013/03/13 19:01:23, mmenke wrote:
> >
>
https://codereview.chromium.org/12701011/diff/51001/net/http/http_cache_trans...
> > File net/http/http_cache_transaction.cc (right):
> > 
> >
>
https://codereview.chromium.org/12701011/diff/51001/net/http/http_cache_trans...
> > net/http/http_cache_transaction.cc:470:
network_trans_->SetPriority(priority);
> > On 2013/03/13 18:53:04, akalin wrote:
> > > One question: can HttpCache::Transaction have multiple network_trans_
> > instances
> > > in its lifetime? i.e., do I have to stash the priority somewhere and make
> sure
> > > newly-created network_trans_ instances pick it up?
> > 
> > RestartNetworkRequest() does this.  There may be other times when this is
done
> > as well.
> 
> Ah, so I do need to handle that case and write a test for it. This is turning
> surprisingly complex!

Yea, changes that go through the entire network stack tend to be more complex
than they first seem.  HttpNetworkTransaction also creates new HttpStreams in a
number of cases - does it on certain network errors, and believe there are other
cases as well (proxy auth?)

[akalin, 2013-03-14 06:12:53]

On 2013/03/13 19:08:56, akalin wrote:
> > RestartNetworkRequest() does this.  There may be other times when this is
done
> > as well.
> 
> Ah, so I do need to handle that case and write a test for it. This is turning
> surprisingly complex!

While writing a test for this, I caught another bug involving
HttpCache::Transaction copying the HttpRequestInfo into its own field and using
that (making any priority changes invisible to it).

I'll probably spin up another CL to split off the priority field and rebase this
CL on that one.

[mmenke, 2013-03-14 14:31:36]

On 2013/03/14 06:12:53, akalin wrote:
> On 2013/03/13 19:08:56, akalin wrote:
> > > RestartNetworkRequest() does this.  There may be other times when this is
> done
> > > as well.
> > 
> > Ah, so I do need to handle that case and write a test for it. This is
turning
> > surprisingly complex!
> 
> While writing a test for this, I caught another bug involving
> HttpCache::Transaction copying the HttpRequestInfo into its own field and
using
> that (making any priority changes invisible to it).
> 
> I'll probably spin up another CL to split off the priority field and rebase
this
> CL on that one.

SGTM

[akalin, 2013-03-15 14:23:45]

Fix URLRequestFtpJob, added tests, rebased on my other CL (splitting off the
priority field). PTAL!

[akalin, 2013-03-19 22:31:44]

Ping! Now that r189099 is checked in, this should be ready to review.

[mmenke, 2013-03-20 17:12:59]

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
File net/url_request/url_request_ftp_job.h (right):

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
net/url_request/url_request_ftp_job.h:45: virtual HostPortPair
GetSocketAddress() const OVERRIDE;
While you're here, seems kinda weird that there are public, and the other
URLRequestJob functions are not.  Looks to me that these can be protected,
too...

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
net/url_request/url_request_ftp_job.h:55: RequestPriority priority_;
optional:  Think it's a little better to give this a const accessor, and make
the variable private, to make it clear subclasses don't set it.

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
File net/url_request/url_request_ftp_job_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
net/url_request/url_request_ftp_job_unittest.cc:76: class
URLRequestFtpJobBasicTest : public testing::Test {
I think this test fixture name is confusing (As is the other one, actually). 
This only tests the case where there's an HTTP proxy, which I wouldn't consider
a "basic" FTP test.  Maybe URLRequestFtpJobPriorityTest, and have a comment
before the class that it only tests the Http proxy case?

I don't believe ftp requests are even normally covered by connection limits,
come to think of it, though priority presumably does matter for DNS resolution.

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:429: priority_, &transaction_,
http_transaction_delegate_.get());
Hmm...We could probably get away with not caching the priority at all in the
URLRequestJob level, and just always stealing it from the request.

Would make testing a bit more tricky, though.

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
File net/url_request/url_request_http_job.h (right):

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
net/url_request/url_request_http_job.h:106: RequestPriority priority_;
Same comment as in the URLRequestFtpJob case.  Ship has kinda sailed here
already on all those other fields being protected, though it actually looks to
me like they can all be private, since nothing seems to inherit from
URLRequestHttpJob.

[akalin, 2013-03-21 01:30:32]

PTAL

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
File net/url_request/url_request_ftp_job.h (right):

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
net/url_request/url_request_ftp_job.h:45: virtual HostPortPair
GetSocketAddress() const OVERRIDE;
On 2013/03/20 17:12:59, mmenke wrote:
> While you're here, seems kinda weird that there are public, and the other
> URLRequestJob functions are not.  Looks to me that these can be protected,
> too...

Done.

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
net/url_request/url_request_ftp_job.h:55: RequestPriority priority_;
On 2013/03/20 17:12:59, mmenke wrote:
> optional:  Think it's a little better to give this a const accessor, and make
> the variable private, to make it clear subclasses don't set it.

Well, subclasses can just call SetPriority(), but sure. Done.

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
File net/url_request/url_request_ftp_job_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
net/url_request/url_request_ftp_job_unittest.cc:76: class
URLRequestFtpJobBasicTest : public testing::Test {
On 2013/03/20 17:12:59, mmenke wrote:
> I think this test fixture name is confusing (As is the other one, actually). 
> This only tests the case where there's an HTTP proxy, which I wouldn't
consider
> a "basic" FTP test.  Maybe URLRequestFtpJobPriorityTest, and have a comment
> before the class that it only tests the Http proxy case?
> 
> I don't believe ftp requests are even normally covered by connection limits,
> come to think of it, though priority presumably does matter for DNS
resolution.

Done.

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:429: priority_, &transaction_,
http_transaction_delegate_.get());
On 2013/03/20 17:12:59, mmenke wrote:
> Hmm...We could probably get away with not caching the priority at all in the
> URLRequestJob level, and just always stealing it from the request.
> 
> Would make testing a bit more tricky, though.

I actually think it's clearer this way, because:

1) It follows the pattern set in other classes, where you have a priority_
variable set at construction/Start time, and then changed in SetPriority,
possibly propagating it to children.

2) It enables one to reason about priority just by looking at this class.

And yeah, easier to test. I actually tried to remove the priority() accessor in
URLRequestJob, but it's used in other places.

So I'd like to keep this as-is, unless you feel strongly about it.

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
File net/url_request/url_request_http_job.h (right):

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
net/url_request/url_request_http_job.h:106: RequestPriority priority_;
On 2013/03/20 17:12:59, mmenke wrote:
> Same comment as in the URLRequestFtpJob case.  Ship has kinda sailed here
> already on all those other fields being protected, though it actually looks to
> me like they can all be private, since nothing seems to inherit from
> URLRequestHttpJob.

Done, privatized everything I could.

[mmenke, 2013-03-21 04:13:00]

Not sure why all of your trybot runs are red despite mostly passing, but one of
the reds looks like it's related to this CL -
HttpCache.SetPriorityNewTransaction on linux_asan.

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/12701011/diff/43003/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:429: priority_, &transaction_,
http_transaction_delegate_.get());
On 2013/03/21 01:30:33, akalin wrote:
> On 2013/03/20 17:12:59, mmenke wrote:
> > Hmm...We could probably get away with not caching the priority at all in the
> > URLRequestJob level, and just always stealing it from the request.
> > 
> > Would make testing a bit more tricky, though.
> 
> I actually think it's clearer this way, because:
> 
> 1) It follows the pattern set in other classes, where you have a priority_
> variable set at construction/Start time, and then changed in SetPriority,
> possibly propagating it to children.
> 
> 2) It enables one to reason about priority just by looking at this class.
> 
> And yeah, easier to test. I actually tried to remove the priority() accessor
in
> URLRequestJob, but it's used in other places.
> 
> So I'd like to keep this as-is, unless you feel strongly about it.

My general feeling is the more places we have values that must be the same for
the code to be correct, the more likely it is one of them is incorrect, and
keeping tack of priority in URLRequests, FtpJobs, HttpJobs, CacheTransactions,
and NetworkTransactions seems a bit excessive to me, particularly since FtpJobs
and HttpJobs already have a pointer to the parent URLRequest object.

Not a great idea to add dependencies on their parents to Cache/Network
transactions, so I think having those levels track the priority independently is
definitely the way to go there, but since we already have the dependency here...

Anyhow, I'll take another look at the tests tomorrow, and decide how much I'm
going to push back then.

[akalin, 2013-03-21 05:33:06]

On 2013/03/21 04:13:00, mmenke wrote:
> Not sure why all of your trybot runs are red despite mostly passing, but one
of
> the reds looks like it's related to this CL -
> HttpCache.SetPriorityNewTransaction on linux_asan.

Looks like a use-after-free.  Investigating...

[akalin, 2013-03-21 07:14:20]

On 2013/03/21 04:13:00, mmenke wrote:
> Not sure why all of your trybot runs are red despite mostly passing, but one
of
> the reds looks like it's related to this CL -
> HttpCache.SetPriorityNewTransaction on linux_asan.

Okay, fixed this use-after-free bug by introducing a delegating HttpTransaction
implementation.

> My general feeling is the more places we have values that must be the same for
> the code to be correct, the more likely it is one of them is incorrect, and
> keeping tack of priority in URLRequests, FtpJobs, HttpJobs, CacheTransactions,
> and NetworkTransactions seems a bit excessive to me, particularly since
FtpJobs
> and HttpJobs already have a pointer to the parent URLRequest object.
> 
> Not a great idea to add dependencies on their parents to Cache/Network
> transactions, so I think having those levels track the priority independently
is
> definitely the way to go there, but since we already have the dependency
here...
> 
> Anyhow, I'll take another look at the tests tomorrow, and decide how much I'm
> going to push back then.

Okay. If you do decide you want to use request->priority(), then I'd then like
to change SetPriority(RequestPriority priority) to something more like
OnRequestPriorityChanged().

[mmenke, 2013-03-21 15:39:23]

LGTM....

But what do you think of making more integrationy tests?  Rather than just test
that we set a value that we can later retrieve, make sure we actually respect it
instead (Or as well)?

This would probably be too complicated in the cache test, but for the HTTP and
FTP requests, we can just allow one connection per group and issue two requests.
 We can use deterministic socket data to delay the connection until both
requests have been issued, and have the same socket handle both requests.  Then
after we've received all the data, we can make sure the right request go the
right response.

https://codereview.chromium.org/12701011/diff/101001/net/http/http_transactio...
File net/http/http_transaction_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/101001/net/http/http_transactio...
net/http/http_transaction_unittest.cc:26: // pass to classes that take ownership
of the HttpTransaction.
I think this comment should be much more specific.

Maybe replace the second sentence with "Allows the state of a
MockNetworkTransaction to be queried after their owner would normally destroy
it."

https://codereview.chromium.org/12701011/diff/101001/net/http/http_transactio...
net/http/http_transaction_unittest.cc:30: explicit
DelegatingTransaction(net::HttpTransaction* delegate)
Think this should be a MockHttpTransaction.

The usual transaction types do stuff on destruction, so seems like a good idea
to make it explicit that this doesn't take one of them as an argument.  Maybe
also a comment to that effect.

https://codereview.chromium.org/12701011/diff/101001/net/http/http_transactio...
File net/http/http_transaction_unittest.h (right):

https://codereview.chromium.org/12701011/diff/101001/net/http/http_transactio...
net/http/http_transaction_unittest.h:224: // NULL if one has not been created
yet.
Think it's worth noting that this owns a reference, so the mock transaction is
safe to call after it would normally be destroyed.  Also think it may be a good
idea to throw a const or two in the declaration, to make it clear that it's not
intended to be used as an opportunity to muck with the transaction.

https://codereview.chromium.org/12701011/diff/101001/net/url_request/url_requ...
File net/url_request/url_request_ftp_job_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/101001/net/url_request/url_requ...
net/url_request/url_request_ftp_job_unittest.cc:14: #include
"net/url_request/url_request_ftp_job.h"
nit:  This should go first, up top.

https://codereview.chromium.org/12701011/diff/101001/net/url_request/url_requ...
net/url_request/url_request_ftp_job_unittest.cc:95: EXPECT_EQ(DEFAULT_PRIORITY,
job->priority());
Since this should be different than the LOW below, think we should explicitly
set a priority here, rather than using default.

[akalin, 2013-03-21 23:43:48]

Committing after tests pass

https://codereview.chromium.org/12701011/diff/101001/net/http/http_transactio...
File net/http/http_transaction_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/101001/net/http/http_transactio...
net/http/http_transaction_unittest.cc:26: // pass to classes that take ownership
of the HttpTransaction.
On 2013/03/21 15:39:23, mmenke wrote:
> I think this comment should be much more specific.
> 
> Maybe replace the second sentence with "Allows the state of a
> MockNetworkTransaction to be queried after their owner would normally destroy
> it."

moot.

https://codereview.chromium.org/12701011/diff/101001/net/http/http_transactio...
net/http/http_transaction_unittest.cc:30: explicit
DelegatingTransaction(net::HttpTransaction* delegate)
On 2013/03/21 15:39:23, mmenke wrote:
> Think this should be a MockHttpTransaction.
> 
> The usual transaction types do stuff on destruction, so seems like a good idea
> to make it explicit that this doesn't take one of them as an argument.  Maybe
> also a comment to that effect.

moot.

https://codereview.chromium.org/12701011/diff/101001/net/http/http_transactio...
File net/http/http_transaction_unittest.h (right):

https://codereview.chromium.org/12701011/diff/101001/net/http/http_transactio...
net/http/http_transaction_unittest.h:224: // NULL if one has not been created
yet.
On 2013/03/21 15:39:23, mmenke wrote:
> Think it's worth noting that this owns a reference, so the mock transaction is
> safe to call after it would normally be destroyed.  Also think it may be a
good
> idea to throw a const or two in the declaration, to make it clear that it's
not
> intended to be used as an opportunity to muck with the transaction.

Turns out the DelegatingTransaction approach doesn't work either. Changed it so
that last_transaction() returns a WeakPtr<MockNetworkTransaction>.

https://codereview.chromium.org/12701011/diff/101001/net/url_request/url_requ...
File net/url_request/url_request_ftp_job_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/101001/net/url_request/url_requ...
net/url_request/url_request_ftp_job_unittest.cc:14: #include
"net/url_request/url_request_ftp_job.h"
On 2013/03/21 15:39:23, mmenke wrote:
> nit:  This should go first, up top.

Done.

https://codereview.chromium.org/12701011/diff/101001/net/url_request/url_requ...
net/url_request/url_request_ftp_job_unittest.cc:95: EXPECT_EQ(DEFAULT_PRIORITY,
job->priority());
On 2013/03/21 15:39:23, mmenke wrote:
> Since this should be different than the LOW below, think we should explicitly
> set a priority here, rather than using default.

Hmm, I don't understand this comment. This is just verifying that a new FtpJob
starts off with DEFAULT_PRIORITY. Note that DEFAULT_PRIORITY == LOWEST, so the
SetPriority() call below does change the priority.

[akalin, 2013-03-21 23:44:38]

On 2013/03/21 15:39:23, mmenke wrote:
> LGTM....
> 
> But what do you think of making more integrationy tests?  Rather than just
test
> that we set a value that we can later retrieve, make sure we actually respect
it
> instead (Or as well)?
> 
> This would probably be too complicated in the cache test, but for the HTTP and
> FTP requests, we can just allow one connection per group and issue two
requests.
>  We can use deterministic socket data to delay the connection until both
> requests have been issued, and have the same socket handle both requests. 
Then
> after we've received all the data, we can make sure the right request go the
> right response.

Sure, yeah, I can do that. But don't I have to do that after I plumb everything
all the way through? (Probably the next CL.)

[mattmenke_gmail.com, 2013-03-21 23:46:15]

On 2013/03/21 23:44:38, akalin wrote:
> On 2013/03/21 15:39:23, mmenke wrote:
> > LGTM....
> > 
> > But what do you think of making more integrationy tests?  Rather than just
> test
> > that we set a value that we can later retrieve, make sure we actually
respect
> it
> > instead (Or as well)?
> > 
> > This would probably be too complicated in the cache test, but for the HTTP
and
> > FTP requests, we can just allow one connection per group and issue two
> requests.
> >  We can use deterministic socket data to delay the connection until both
> > requests have been issued, and have the same socket handle both requests. 
> Then
> > after we've received all the data, we can make sure the right request go the
> > right response.
> 
> Sure, yeah, I can do that. But don't I have to do that after I plumb
everything
> all the way through? (Probably the next CL.)

Those which issue requests after setting the priority would work, but the others
would not.  Fine to not worry about it in this CL.

[mattmenke_gmail.com, 2013-03-21 23:48:36]

https://codereview.chromium.org/12701011/diff/101001/net/url_request/url_requ...
File net/url_request/url_request_ftp_job_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/101001/net/url_request/url_requ...
net/url_request/url_request_ftp_job_unittest.cc:95: EXPECT_EQ(DEFAULT_PRIORITY,
job->priority());
On 2013/03/21 23:43:48, akalin wrote:
> On 2013/03/21 15:39:23, mmenke wrote:
> > Since this should be different than the LOW below, think we should
explicitly
> > set a priority here, rather than using default.
> 
> Hmm, I don't understand this comment. This is just verifying that a new FtpJob
> starts off with DEFAULT_PRIORITY. Note that DEFAULT_PRIORITY == LOWEST, so the
> SetPriority() call below does change the priority.

This test relies on DEFAULT_PRIORITY not being LOW.  If we wanted to have all
code that uses DEFAULT_PRIORITY to know that it is LOWEST, then I'd argue that
using a separate name for DEFAULT_PRIORITY serves no purpose.

As-is, it's not obvious, looking at this test, that SetPriority(LOW) is not the
same as SetPriority(DEFAULT_PRIORITY).

Please do change this.

[commit-bot: I haz the power, 2013-03-21 23:50:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/12701011/111001

[commit-bot: I haz the power, 2013-03-21 23:50:52]

Presubmit check for 12701011-111001 failed and returned exit status 1.

INFO:root:Found 29 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/net/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/predictors/resource_prefetcher.cc
    chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
    content/browser/loader/resource_scheduler.cc
    content/browser/loader/resource_dispatcher_host_impl.cc
    content/browser/loader/resource_scheduler_unittest.cc

Presubmit checks took 3.4s to calculate.

[akalin, 2013-03-22 00:14:48]

Forgot to actually add the OWNERS reviewers below:

+simonjam for content/browser/loader
+sky for chrome/

[akalin, 2013-03-22 00:16:36]

https://codereview.chromium.org/12701011/diff/101001/net/url_request/url_requ...
File net/url_request/url_request_ftp_job_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/101001/net/url_request/url_requ...
net/url_request/url_request_ftp_job_unittest.cc:95: EXPECT_EQ(DEFAULT_PRIORITY,
job->priority());
On 2013/03/21 23:48:36, mattmenke wrote:
> On 2013/03/21 23:43:48, akalin wrote:
> > On 2013/03/21 15:39:23, mmenke wrote:
> > > Since this should be different than the LOW below, think we should
> explicitly
> > > set a priority here, rather than using default.
> > 
> > Hmm, I don't understand this comment. This is just verifying that a new
FtpJob
> > starts off with DEFAULT_PRIORITY. Note that DEFAULT_PRIORITY == LOWEST, so
the
> > SetPriority() call below does change the priority.
> 
> This test relies on DEFAULT_PRIORITY not being LOW.  If we wanted to have all
> code that uses DEFAULT_PRIORITY to know that it is LOWEST, then I'd argue that
> using a separate name for DEFAULT_PRIORITY serves no purpose.
> 
> As-is, it's not obvious, looking at this test, that SetPriority(LOW) is not
the
> same as SetPriority(DEFAULT_PRIORITY).
> 
> Please do change this.

I'm not sure what you want me to do. Should I add an EXPECT_NE(DEFAULT_PRIORITY,
LOW)? Add another call to SetPriority with HIGH?

[akalin, 2013-03-22 00:22:02]

TBRing OWNERS since it's just a rename

https://codereview.chromium.org/12701011/diff/121003/net/url_request/url_requ...
File net/url_request/url_request_ftp_job_unittest.cc (right):

https://codereview.chromium.org/12701011/diff/121003/net/url_request/url_requ...
net/url_request/url_request_ftp_job_unittest.cc:98: job->SetPriority(LOWEST);
As discussed, adding an explicit test for LOWEST

[commit-bot: I haz the power, 2013-03-22 00:26:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/12701011/121003

[James Simonsen, 2013-03-22 00:36:26]

lgtm

[akalin, 2013-03-22 16:17:56]

On 2013/03/21 23:46:15, mattmenke wrote:
> Those which issue requests after setting the priority would work, but the
others
> would not.  Fine to not worry about it in this CL.

Ah, right. OK.

[akalin, 2013-03-22 16:24:16]

Committed patchset #12 manually as r189829 (presubmit successful).

[sky, 2013-03-22 19:07:07]

LGTM

[akalin, 2013-03-22 20:17:13]

On 2013/03/22 19:07:07, sky wrote:
> LGTM

Relanding after fixing memory leaks

[akalin, 2013-03-22 20:17:59]

Committed patchset #13 manually as r189894 (presubmit successful).