Let prefetched resources skip cache revalidation once for a short duration.

Let prefetched resources skip cache revalidation once for a short duration by adding an "unused_since_prefetch" boolean to HttpResponseInfo and setting it when LOAD_PREFETCH is used on a new or existing entry.

The first read of a prefetched resource will skip validation if it is within 5 minutes and unset the "unused_since_prefetch" bit. Unsetting the bit requires writing metadata on a cache hit, which is a slight change to the state machine.

I also added a test clock to http_cache_unittests which makes testing easier.

BUG=383394
Committed: https://crrev.com/fb1d517f7bf920bf130abba2c6434381478790ff
Cr-Commit-Position: refs/heads/master@{#311046}

[jkarlin, 2014-12-11 18:04:17]

jkarlin@chromium.org changed reviewers:
+ rvargas@chromium.org

[jkarlin, 2014-12-11 18:04:17]

Hi Ricardo. PTAL at everything, with special attention to
http_cache_transaction.cc. Thanks!

[rvargas (doing something else), 2014-12-12 02:20:50]

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache.cc#n...
net/http/http_cache.cc:365: start_time_ = cache_->clock_->Now();
Please declare an accessor instead of grabbing the variable directly.

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_tran...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2187: response_.unused_since_prefetch =
!response_.unused_since_prefetch;
READ and UPDATE requests don't go through this code so this should not be done
here. The decision has to be made from DoCacheReadResponseComplete.

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2188: target_state_ =
STATE_UPDATE_CACHED_RESPONSE_COMPLETE;
Joining back in DoUpdateCachedResponseComplete looks weird as that state
implicitly assumes that we are handling 304 or 206 from the server... (and
jumping to FooComplete from anywhere else but Foo is unexpected)

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6786: : transaction_(new
ScopedMockTransaction(kSimpleGET_Transaction)) {
This class is borderline to require separation between declaration and
definition of methods... I have to scroll to see the member variables :(

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6787: DCHECK(kMaxAge < prefetch_reuse_mins() *
kNumSecondsPerMinute)
_GT

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6788: << "Test requirement";
Does this string really provide any info?

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6790: cache_.http_cache()->clock_.reset(clock_);
If the accessor is public (for tests), this class doesn't have to be declared as
a friend of the class under test and tests can remain outside the net namespace.

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6793: response_headers_ = base::StringPrintf(
it is slightly weird to use StringPrintf with all constants... I guess it is a
test and we don't care? You could declare the whole string a constant and
compile assert at line 6787

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6809: RunTransactionAndGetReceivedBytes(cache_,
*transaction_);
you mean some form of get() ?

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6827: virtual bool ShouldUseRangeTransaction()
const { return false; }
wait until this is actually used?

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6846: scoped_ptr<ScopedMockTransaction>
transaction_;
why a pointer?

[jkarlin, 2014-12-15 17:19:25]

Thanks Ricardo!

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache.cc#n...
net/http/http_cache.cc:365: start_time_ = cache_->clock_->Now();
On 2014/12/12 02:20:49, rvargas Out until dec 22 wrote:
> Please declare an accessor instead of grabbing the variable directly.

Done.

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_tran...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2187: response_.unused_since_prefetch =
!response_.unused_since_prefetch;
On 2014/12/12 02:20:49, rvargas Out until dec 22 wrote:
> READ and UPDATE requests don't go through this code so this should not be done
> here. The decision has to be made from DoCacheReadResponseComplete.

Done.

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2188: target_state_ =
STATE_UPDATE_CACHED_RESPONSE_COMPLETE;
On 2014/12/12 02:20:49, rvargas Out until dec 22 wrote:
> Joining back in DoUpdateCachedResponseComplete looks weird as that state
> implicitly assumes that we are handling 304 or 206 from the server... (and
> jumping to FooComplete from anywhere else but Foo is unexpected)

Done.

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6786: : transaction_(new
ScopedMockTransaction(kSimpleGET_Transaction)) {
On 2014/12/12 02:20:50, rvargas Out until dec 22 wrote:
> This class is borderline to require separation between declaration and
> definition of methods... I have to scroll to see the member variables :(

I've removed the 206 tests as they're unnecessary, it's now much smaller.

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6787: DCHECK(kMaxAge < prefetch_reuse_mins() *
kNumSecondsPerMinute)
On 2014/12/12 02:20:50, rvargas Out until dec 22 wrote:
> _GT

Done, but _LT.

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6788: << "Test requirement";
On 2014/12/12 02:20:50, rvargas Out until dec 22 wrote:
> Does this string really provide any info?

Done.

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6790: cache_.http_cache()->clock_.reset(clock_);
On 2014/12/12 02:20:50, rvargas Out until dec 22 wrote:
> If the accessor is public (for tests), this class doesn't have to be declared
as
> a friend of the class under test and tests can remain outside the net
namespace.

I somewhat agree on making the accessors public (as I expect future tests will
want to use the clock) but it seems like it should be private for now, no?
Anyway, I've made the change.

Why keep the tests outside of net? Isn't it pretty standard to put tests inside
their respective namespace?

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6793: response_headers_ = base::StringPrintf(
On 2014/12/12 02:20:50, rvargas Out until dec 22 wrote:
> it is slightly weird to use StringPrintf with all constants... I guess it is a
> test and we don't care? You could declare the whole string a constant and
> compile assert at line 6787

Done. I only need this string once now so it's just a string literal in the
constructor.

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6809: RunTransactionAndGetReceivedBytes(cache_,
*transaction_);
On 2014/12/12 02:20:50, rvargas Out until dec 22 wrote:
> you mean some form of get() ?

No longer pertinent as transaction_ is no longer on the stack.

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6827: virtual bool ShouldUseRangeTransaction()
const { return false; }
On 2014/12/12 02:20:50, rvargas Out until dec 22 wrote:
> wait until this is actually used?

I've deleted the range request parameterized test so this is now deleted.

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6846: scoped_ptr<ScopedMockTransaction>
transaction_;
On 2014/12/12 02:20:50, rvargas Out until dec 22 wrote:
> why a pointer?

So that the original transaction can be swapped at a later point (first line of
SetUpRangeTransaction). No longer needed since removing the range request tests.

[jkarlin, 2014-12-16 18:48:48]

Patchset #4 (id:60001) has been deleted

[rvargas (doing something else), 2014-12-24 02:15:25]

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/793823002/diff/20001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6790: cache_.http_cache()->clock_.reset(clock_);
On 2014/12/15 17:19:25, jkarlin wrote:
> On 2014/12/12 02:20:50, rvargas Out until dec 22 wrote:
> > If the accessor is public (for tests), this class doesn't have to be
declared
> as
> > a friend of the class under test and tests can remain outside the net
> namespace.
> 
> I somewhat agree on making the accessors public (as I expect future tests will
> want to use the clock) but it seems like it should be private for now, no?
> Anyway, I've made the change.
> 
> Why keep the tests outside of net? Isn't it pretty standard to put tests
inside
> their respective namespace?

If the name of the method clearly specify that it should be used only for tests,
I don't see much value in having to list the names of the tests that can access
that method. We have scripts that warn if the method is used by non-test code.

On the other hand, keeping the method private means that now the tests cannot
only access methods that are intended for tests, but have complete access to any
internal information about the object, which sounds worse to me.

Regarding the namespace, I can see both opinions. First, it is required when
tests access private members of the class under test... other than that, it just
"saves" some typing of the namespace under test (net).

I actually prefer tests to do things like external users are supposed to do
things, to make the tests as realistic as possible. It also has the advantage of
making the code easier to understand when I'm reading code that I'm completely
unfamiliar with (which should be the priority): it tells me right away where
that code lives, versus having to figure out if it is functionality exposed by
net or by a test helper somewhere or maybe even by something related to the OS.
The last thing I want to do when trying to land something is to have to
understand the whole test infrastructure of a random component to figure out why
a bot is not happy.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache.h#ne...
net/http/http_cache.h:190: void set_clock_for_test(scoped_ptr<base::Clock>
clock) {
I doubt the script recognizes hacker_names... besides, to do this the variable
would have to be named clock_for_test. Use CamelCase.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:956: rv = DoCacheToggleUnusedSincePrefetch();
DCHECK rv == ok?

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:958: case
STATE_TOGGLE_UNUSED_SINCE_PREFETCH_COMPLETE:
These states have to be incorporated on the comment preceding this method.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:1720: (!response_.unused_since_prefetch &&
!response_.unused_since_prefetch seems unnecessary.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:1725: next_state_ =
STATE_TOGGLE_UNUSED_SINCE_PREFETCH;
looks like this is not really toggle, as two consecutive prefetches do not reset
the flag. Actually, you reset the bit, which seems wrong.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:1729: return
DoCacheReadResponseCompleteContinuation();
This begs to be a dedicated state on the machine.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:1781: result = ERR_FAILED;
nit: this line is now redundant.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2219: 
?

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_unit...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6793: bool TransactionRequiresNetwork(int
load_flags) {
Given that this method runs the transaction, maybe it should be called
TransactionRequiredNetwork

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6798: int64 received_bytes =
I'm wondering if this is not a distraction for this test (I mean the
GetReceivedBytes part). As in tests should be as specific as possible, and that
looks like a completely unrelated feature (and tests).

https://codereview.chromium.org/793823002/diff/80001/net/http/http_response_i...
File net/http/http_response_info.cc (right):

https://codereview.chromium.org/793823002/diff/80001/net/http/http_response_i...
net/http/http_response_info.cc:285: unused_since_prefetch = (flags &
RESPONSE_INFO_UNUSED_SINCE_PREFETCH) != 0;
This file doesn't have any tests :(. We should at least add a test for this
flag.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_transactio...
File net/http/http_transaction_test_util.cc (right):

https://codereview.chromium.org/793823002/diff/80001/net/http/http_transactio...
net/http/http_transaction_test_util.cc:500: default_clock_.reset();
sounds like we need only one variable to store the clock.

[jkarlin, 2015-01-08 16:12:29]

Patchset #6 (id:120001) has been deleted

[jkarlin, 2015-01-08 16:13:04]

Thanks! PTAL.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache.h#ne...
net/http/http_cache.h:190: void set_clock_for_test(scoped_ptr<base::Clock>
clock) {
On 2014/12/24 02:15:24, rvargas wrote:
> I doubt the script recognizes hacker_names... besides, to do this the variable
> would have to be named clock_for_test. Use CamelCase.

Done.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:956: rv = DoCacheToggleUnusedSincePrefetch();
On 2014/12/24 02:15:24, rvargas wrote:
> DCHECK rv == ok?

Done.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:958: case
STATE_TOGGLE_UNUSED_SINCE_PREFETCH_COMPLETE:
On 2014/12/24 02:15:24, rvargas wrote:
> These states have to be incorporated on the comment preceding this method. 

Done.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:1720: (!response_.unused_since_prefetch &&
On 2014/12/24 02:15:24, rvargas wrote:
> !response_.unused_since_prefetch seems unnecessary.

Thanks!

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:1725: next_state_ =
STATE_TOGGLE_UNUSED_SINCE_PREFETCH;
On 2014/12/24 02:15:24, rvargas wrote:
> looks like this is not really toggle, as two consecutive prefetches do not
reset
> the flag. Actually, you reset the bit, which seems wrong.

Nice catch on two consecutive prefetches. Fixed and added tests.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:1729: return
DoCacheReadResponseCompleteContinuation();
On 2014/12/24 02:15:24, rvargas wrote:
> This begs to be a dedicated state on the machine.

Done.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:1781: result = ERR_FAILED;
On 2014/12/24 02:15:24, rvargas wrote:
> nit: this line is now redundant.

Done.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2219: 
On 2014/12/24 02:15:24, rvargas wrote:
> ?

Done.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_unit...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6793: bool TransactionRequiresNetwork(int
load_flags) {
On 2014/12/24 02:15:25, rvargas wrote:
> Given that this method runs the transaction, maybe it should be called
> TransactionRequiredNetwork

Done.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_unit...
net/http/http_cache_unittest.cc:6798: int64 received_bytes =
On 2014/12/24 02:15:25, rvargas wrote:
> I'm wondering if this is not a distraction for this test (I mean the
> GetReceivedBytes part). As in tests should be as specific as possible, and
that
> looks like a completely unrelated feature (and tests).

Done.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_response_i...
File net/http/http_response_info.cc (right):

https://codereview.chromium.org/793823002/diff/80001/net/http/http_response_i...
net/http/http_response_info.cc:285: unused_since_prefetch = (flags &
RESPONSE_INFO_UNUSED_SINCE_PREFETCH) != 0;
On 2014/12/24 02:15:25, rvargas wrote:
> This file doesn't have any tests :(. We should at least add a test for this
> flag.

Done.

https://codereview.chromium.org/793823002/diff/80001/net/http/http_transactio...
File net/http/http_transaction_test_util.cc (right):

https://codereview.chromium.org/793823002/diff/80001/net/http/http_transactio...
net/http/http_transaction_test_util.cc:500: default_clock_.reset();
On 2014/12/24 02:15:25, rvargas wrote:
> sounds like we need only one variable to store the clock.

Done.

[rvargas (doing something else), 2015-01-09 02:25:34]

Just nits

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/793823002/diff/80001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:846: //
nit: keep this //

https://codereview.chromium.org/793823002/diff/140001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/793823002/diff/140001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:746: //                        [Foo] means
state Foo may be skipped
Instead of adding an optional state pairs to known patterns I think it is better
to leave the optional states out (the common case is no prefetch) and add
explicit cases for a prefetch load and, say, an expired load that is used
because it was prefetched.

https://codereview.chromium.org/793823002/diff/140001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:962: rv = DoCacheReadResponseContinue();
dcheck rv == ok

https://codereview.chromium.org/793823002/diff/140001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1904: int
HttpCache::Transaction::DoCacheReadResponseContinue() {
Ideally states should be named by what is done in that state not how we got
there... and we are not concerned about reading the response anymore.

How about STATE_CACHE_DISPATCH_READ? STATE_CACHE_USE_RESPONSE? I'm not really
happy with either one but they stack on top of READ_RESPONSE_CONTINUE to me.

(-this is a dispatch but not of a read)
(-the response may not be really used, and it was used before this state)

STATE_CACHE_DISPATCH_VALIDATION ?

https://codereview.chromium.org/793823002/diff/140001/net/http/http_cache_uni...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/793823002/diff/140001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7048:
RunTransactionTestBase(cache_.http_cache(), transaction_,
use RunTransacionTest

https://codereview.chromium.org/793823002/diff/140001/net/http/http_transacti...
File net/http/http_transaction_test_util.cc (right):

https://codereview.chromium.org/793823002/diff/140001/net/http/http_transacti...
net/http/http_transaction_test_util.cc:13: #include "base/time/default_clock.h"
This is not needed anymore, right?

https://codereview.chromium.org/793823002/diff/140001/net/http/http_transacti...
File net/http/http_transaction_test_util.h (right):

https://codereview.chromium.org/793823002/diff/140001/net/http/http_transacti...
net/http/http_transaction_test_util.h:296: DCHECK(!clock_);
Logging doesn't qualify anymore as trivial accessor. (and it's a good practice
to avoid logging from header files)

[jkarlin, 2015-01-09 14:03:51]

Thanks! PTAL.

https://codereview.chromium.org/793823002/diff/140001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/793823002/diff/140001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:746: //                        [Foo] means
state Foo may be skipped
On 2015/01/09 02:25:34, rvargas wrote:
> Instead of adding an optional state pairs to known patterns I think it is
better
> to leave the optional states out (the common case is no prefetch) and add
> explicit cases for a prefetch load and, say, an expired load that is used
> because it was prefetched.

Done.

https://codereview.chromium.org/793823002/diff/140001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:962: rv = DoCacheReadResponseContinue();
On 2015/01/09 02:25:33, rvargas wrote:
> dcheck rv == ok

Done.

https://codereview.chromium.org/793823002/diff/140001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1904: int
HttpCache::Transaction::DoCacheReadResponseContinue() {
On 2015/01/09 02:25:34, rvargas wrote:
> Ideally states should be named by what is done in that state not how we got
> there... and we are not concerned about reading the response anymore.
> 
> How about STATE_CACHE_DISPATCH_READ? STATE_CACHE_USE_RESPONSE? I'm not really
> happy with either one but they stack on top of READ_RESPONSE_CONTINUE to me.
> 
> (-this is a dispatch but not of a read)
> (-the response may not be really used, and it was used before this state)
> 
> STATE_CACHE_DISPATCH_VALIDATION ?

Yeah, I had a hell of a time coming up with an accurate name and just went with
Continue. At one point I had "CacheReadOrValidate" but that left out the UPDATE
case. READ_WRITE is the common case so I could see naming it for that.
STATE_CACHE_DISPATCH_VALIDATION it is.

https://codereview.chromium.org/793823002/diff/140001/net/http/http_cache_uni...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/793823002/diff/140001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7048:
RunTransactionTestBase(cache_.http_cache(), transaction_,
On 2015/01/09 02:25:34, rvargas wrote:
> use RunTransacionTest

Done.

https://codereview.chromium.org/793823002/diff/140001/net/http/http_transacti...
File net/http/http_transaction_test_util.cc (right):

https://codereview.chromium.org/793823002/diff/140001/net/http/http_transacti...
net/http/http_transaction_test_util.cc:13: #include "base/time/default_clock.h"
On 2015/01/09 02:25:34, rvargas wrote:
> This is not needed anymore, right?

Done.

https://codereview.chromium.org/793823002/diff/140001/net/http/http_transacti...
File net/http/http_transaction_test_util.h (right):

https://codereview.chromium.org/793823002/diff/140001/net/http/http_transacti...
net/http/http_transaction_test_util.h:296: DCHECK(!clock_);
On 2015/01/09 02:25:34, rvargas wrote:
> Logging doesn't qualify anymore as trivial accessor. (and it's a good practice
> to avoid logging from header files)

Done.

[rvargas (doing something else), 2015-01-09 20:16:13]

LGTM after final nits.

https://codereview.chromium.org/793823002/diff/180001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/793823002/diff/180001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:858: //   Like examples 2-4, only
CacheToggleUnusedSincePrefetch* is inserted between
mind making this explicit? Assuming the entry is used soon enough (hopefully the
common case), this morphs #s 3-4 to follow #2, so it's not really like them.

https://codereview.chromium.org/793823002/diff/180001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1913: int
HttpCache::Transaction::DoCacheDispatchValidation() {
Time to make sure that method definition follows the declaration order.

[jkarlin, 2015-01-12 13:01:14]

https://codereview.chromium.org/793823002/diff/180001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/793823002/diff/180001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:858: //   Like examples 2-4, only
CacheToggleUnusedSincePrefetch* is inserted between
On 2015/01/09 20:16:13, rvargas wrote:
> mind making this explicit? Assuming the entry is used soon enough (hopefully
the
> common case), this morphs #s 3-4 to follow #2, so it's not really like them.

Done.

https://codereview.chromium.org/793823002/diff/180001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1913: int
HttpCache::Transaction::DoCacheDispatchValidation() {
On 2015/01/09 20:16:13, rvargas wrote:
> Time to make sure that method definition follows the declaration order.

Done. Thanks!

[jkarlin, 2015-01-12 13:01:20]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2015-01-12 13:01:26]

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

[commit-bot: I haz the power, 2015-01-12 14:10:59]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2015-01-12 14:12:26]

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

[hayato, 2015-01-13 05:44:37]

FYI. This might be a cause of
https://code.google.com/p/chromium/issues/detail?id=448274.