Combine and make const RequiresValidation() and ConditionalizeRequest().

Combine and make const RequiresValidation() and ConditionalizeRequest().

Also fix implementation of MockNetworkTransaction::GetConnectionAttempts()
that was causing a lot of spam in the cache unit tests.

BUG=None
R=shivanisha@chromium.org

[Randy Smith (Not in Mondays), 2017-04-28 19:32:26]

Description was changed from

==========
Combine and make const RequiresValidation() and ConditionalizeRequest().

BUG=None
==========

to

==========
Combine and make const RequiresValidation() and ConditionalizeRequest().

Also fix implementation of MockNetworkTransaction::GetConnectionAttempts()
that was causing a lot of spam in the cache unit tests.

BUG=None
==========

[Randy Smith (Not in Mondays), 2017-04-28 19:33:43]

Description was changed from

==========
Combine and make const RequiresValidation() and ConditionalizeRequest().

Also fix implementation of MockNetworkTransaction::GetConnectionAttempts()
that was causing a lot of spam in the cache unit tests.

BUG=None
==========

to

==========
Combine and make const RequiresValidation() and ConditionalizeRequest().

Also fix implementation of MockNetworkTransaction::GetConnectionAttempts()
that was causing a lot of spam in the cache unit tests.

BUG=None
R=shivanisha@chromium.org
==========

[Randy Smith (Not in Mondays), 2017-04-28 19:42:30]

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

[Randy Smith (Not in Mondays), 2017-04-28 19:42:30]

Shivani: PTAL?  

This is a pure refactor, so in theory shouldn't require any test changes :-}.  I
think it'll make the cache validation based on encoding type CL somewhat easier,
as this'll allow me to just check for that case in one place, as opposed to have
to do it in both RequiresValidation() and ConditionalizeRequest().  I also like
this refactor for its own sake (mostly because I can declare the relevant
routine "const", thus asserting no side effects, which I think makes it easier
to reason about at calling sites.  If the refactor isn't good for its own sake,
then I'll find some other way to solve that problem (probably with the addition
of another bool to the class, which option I hate :-{).  

Let me know what you think.

[Randy Smith (Not in Mondays), 2017-04-28 19:43:04]

The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 19:43:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 21:30:37]

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

[commit-bot: I haz the power, 2017-04-28 21:30:38]

Dry run: This issue passed the CQ dry run.

[shivanisha, 2017-05-02 16:03:26]

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

https://codereview.chromium.org/2854433002/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2150: couldnt_conditionalize_request_ = true;
Can we save entry_validation_status as a member variable and then we do not need
to have an additional couldnt_conditionalize_request_?

https://codereview.chromium.org/2854433002/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2178: &last_modified_value);
The above two conditions seem to be getting executed twice, once in
GetEntryValidationStatus and then here? It should be only here, so that all
settings in response_ and custom_request_ are done in the same place for
REQUIRES_VALIDATION* cases.

https://codereview.chromium.org/2854433002/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2180: 
DCHECK(!etag_value.empty() || !last_modified_value.empty());
to emphasize one of these are non empty to be in this code path.

https://codereview.chromium.org/2854433002/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2394: return REQUIRES_REFETCH;
In the original code, this check comes after the original RequiresValidation().
Is it possible that the other REQUIRES_REFETCH conditions, specifically those
checking response_code and fail_conditionalization_for_test_ are also checked
before we invoke RequiresValidation()?

https://codereview.chromium.org/2854433002/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2430: : REQUIRES_VALIDATION_VARY_MISMATCH;
Looks like it should be REQUIRES_VALIDATION_VARY_MISMATCH if vary_mismatch is
true?

https://codereview.chromium.org/2854433002/diff/40001/net/http/http_cache_tra...
File net/http/http_cache_transaction.h (right):

https://codereview.chromium.org/2854433002/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.h:252: REQUIRES_REFETCH                    //
Entry needs to be fetched.
nit: prefer enum class for type safety.

https://codereview.chromium.org/2854433002/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.h:439: void RecordHistograms();
Request: Can RecordHistograms be made const as well? (Not related to your
change, but since this CL is making change to histogram related fields)