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

net/http/http_cache_transaction.cc

Index: net/http/http_cache_transaction.cc
diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index 518944650747e6a3dc7f39c58b4b4d7cb1610758..670b872f33a78c75fcf8d7e8486467e5e636ac31 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -164,7 +164,6 @@ HttpCache::Transaction::Transaction(RequestPriority priority, HttpCache* cache)
       handling_206_(false),
       cache_pending_(false),
       done_reading_(false),
-      vary_mismatch_(false),
       couldnt_conditionalize_request_(false),
       bypass_lock_for_test_(false),
       fail_conditionalization_for_test_(false),
@@ -2097,7 +2096,7 @@ int HttpCache::Transaction::BeginCacheRead() {
     return ERR_CACHE_MISS;
   }
 
-  if (RequiresValidation()) {
+  if (GetEntryValidationStatus() != USABLE) {
     TransitionToState(STATE_NONE);
     return ERR_CACHE_MISS;
   }
@@ -2116,12 +2115,13 @@ int HttpCache::Transaction::BeginCacheRead() {
 int HttpCache::Transaction::BeginCacheValidation() {
   DCHECK_EQ(mode_, READ_WRITE);
 
-  bool skip_validation = !RequiresValidation();
+  enum EntryValidationStatus entry_validation_status =
+      GetEntryValidationStatus();
 
   if (request_->method == "HEAD" &&
       (truncated_ || response_.headers->response_code() == 206)) {
     DCHECK(!partial_);
-    if (skip_validation)
+    if (entry_validation_status == USABLE)
       return SetupEntryForRead();
 
     // Bail out!
@@ -2134,37 +2134,76 @@ int HttpCache::Transaction::BeginCacheValidation() {
     // Truncated entries can cause partial gets, so we shouldn't record this
     // load in histograms.
     UpdateCacheEntryStatus(CacheEntryStatus::ENTRY_OTHER);
-    skip_validation = !partial_->initial_validation();
   }
 
-  if (partial_ && (is_sparse_ || truncated_) &&
-      (!partial_->IsCurrentRangeCached() || invalid_range_)) {
-    // Force revalidation for sparse or truncated entries. Note that we don't
-    // want to ignore the regular validation logic just because a byte range was
-    // part of the request.
-    skip_validation = false;
-  }
-
-  if (skip_validation) {
+  if (entry_validation_status == USABLE) {
     UpdateCacheEntryStatus(CacheEntryStatus::ENTRY_USED);
     return SetupEntryForRead();
-  } else {
-    // Make the network request conditional, to see if we may reuse our cached
-    // response.  If we cannot do so, then we just resort to a normal fetch.
-    // Our mode remains READ_WRITE for a conditional request.  Even if the
-    // conditionalization fails, we don't switch to WRITE mode until we
-    // know we won't be falling back to using the cache entry in the
-    // LOAD_FROM_CACHE_IF_OFFLINE case.
-    if (!ConditionalizeRequest()) {
-      couldnt_conditionalize_request_ = true;
-      UpdateCacheEntryStatus(CacheEntryStatus::ENTRY_CANT_CONDITIONALIZE);
-      if (partial_)
-        return DoRestartPartialRequest();
+  }
 
-      DCHECK_NE(206, response_.headers->response_code());
-    }
+  // If the network request cannot be conditional, setup a normal
+  // fetch.  The mode remains READ_WRITE for a conditional request.
+  // Even if the conditionalization fails, don't switch to WRITE mode
+  // until it is confirmed that the transaction won't be falling back
+  // to using the cache entry in the LOAD_FROM_CACHE_IF_OFFLINE case.
+  if (entry_validation_status == REQUIRES_REFETCH) {
+    couldnt_conditionalize_request_ = true;

[shivanisha, 2017/05/02 16:03:25]

Can we save entry_validation_status as a member variable and then we do not need
to have an additional couldnt_conditionalize_request_?

+    UpdateCacheEntryStatus(CacheEntryStatus::ENTRY_CANT_CONDITIONALIZE);
+    if (partial_)
+      return DoRestartPartialRequest();
+
+    DCHECK_NE(206, response_.headers->response_code());
     TransitionToState(STATE_SEND_REQUEST);
+    return OK;
+  }
+
+  // Sending a conditional request to see if the cache entry is still valid.
+
+  // Setup custom request if needed.
+  if (!partial_) {
+    custom_request_.reset(new HttpRequestInfo(*request_));
+    request_ = custom_request_.get();
+  }
+  DCHECK(custom_request_.get());
+  bool use_if_range =
+      partial_ && !partial_->IsCurrentRangeCached() && !invalid_range_;
+
+  std::string etag_value;
+  if (response_.headers->GetHttpVersion() >= HttpVersion(1, 1))
+    response_.headers->EnumerateHeader(NULL, "etag", &etag_value);
+
+  std::string last_modified_value;
+  if (entry_validation_status == REQUIRES_VALIDATION_VARY_MISMATCH) {
+    response_.headers->EnumerateHeader(NULL, "last-modified",
+                                       &last_modified_value);

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

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.

   }
+

[shivanisha, 2017/05/02 16:03:25]

DCHECK(!etag_value.empty() || !last_modified_value.empty());
to emphasize one of these are non empty to be in this code path.

+  if (!etag_value.empty()) {
+    if (use_if_range) {
+      // We don't want to switch to WRITE mode if we don't have this block of a
+      // byte-range request because we may have other parts cached.
+      custom_request_->extra_headers.SetHeader(HttpRequestHeaders::kIfRange,
+                                               etag_value);
+    } else {
+      custom_request_->extra_headers.SetHeader(HttpRequestHeaders::kIfNoneMatch,
+                                               etag_value);
+    }
+  }
+
+  if (!last_modified_value.empty() &&
+      // For byte-range requests, make sure that we use only one way to validate
+      // the request.
+      (!partial_ || partial_->IsCurrentRangeCached())) {
+    if (use_if_range) {
+      custom_request_->extra_headers.SetHeader(HttpRequestHeaders::kIfRange,
+                                               last_modified_value);
+    } else {
+      custom_request_->extra_headers.SetHeader(
+          HttpRequestHeaders::kIfModifiedSince, last_modified_value);
+    }
+  }
+
+  TransitionToState(STATE_SEND_REQUEST);
   return OK;
 }
 
@@ -2282,16 +2321,30 @@ int HttpCache::Transaction::RestartNetworkRequestWithAuth(
   return rv;
 }
 
-bool HttpCache::Transaction::RequiresValidation() {
+// Helper function for GetEntryValidationStatus.
+bool HttpCache::Transaction::RequiresValidation(bool* vary_mismatch) const {
+  if (partial_ && (is_sparse_ || truncated_) &&
+      (!partial_->IsCurrentRangeCached() || invalid_range_)) {
+    // Force revalidation for sparse or truncated entries. Note that
+    // we don't want to ignore the regular validation logic just
+    // because a byte range was part of the request.
+    return true;
+  }
+
+  // In the truncation case, validation status is controlled by the
+  // partial_ state; that state either determines that it's usable, or
+  // forces validation.
+  if (truncated_ && request_->method != "HEAD")
+    return partial_->initial_validation();
+
   // TODO(darin): need to do more work here:
   //  - make sure we have a matching request method
   //  - watch out for cached responses that depend on authentication
-
   if (!(effective_load_flags_ & LOAD_SKIP_VARY_CHECK) &&
       response_.vary_data.is_valid() &&
       !response_.vary_data.MatchesRequest(*request_,
                                           *response_.headers.get())) {
-    vary_mismatch_ = true;
+    *vary_mismatch = true;
     validation_cause_ = VALIDATION_CAUSE_VARY_MISMATCH;
     return true;
   }
@@ -2301,9 +2354,10 @@ bool HttpCache::Transaction::RequiresValidation() {
 
   if (response_.unused_since_prefetch &&
       !(effective_load_flags_ & LOAD_PREFETCH) &&
-      response_.headers->GetCurrentAge(
-          response_.request_time, response_.response_time,
-          cache_->clock_->Now()) < TimeDelta::FromMinutes(kPrefetchReuseMins)) {
+      (response_.headers->GetCurrentAge(response_.request_time,
+                                        response_.response_time,
+                                        cache_->clock_->Now()) <
+       TimeDelta::FromMinutes(kPrefetchReuseMins))) {
     // The first use of a resource after prefetch within a short window skips
     // validation.
     return false;
@@ -2314,9 +2368,6 @@ bool HttpCache::Transaction::RequiresValidation() {
     return true;
   }
 
-  if (request_->method == "PUT" || request_->method == "DELETE")
-    return true;
-
   bool validation_required_by_headers = response_.headers->RequiresValidation(
       response_.request_time, response_.response_time, cache_->clock_->Now());
 
@@ -2337,20 +2388,24 @@ bool HttpCache::Transaction::RequiresValidation() {
   return validation_required_by_headers;
 }
 
-bool HttpCache::Transaction::ConditionalizeRequest() {
-  DCHECK(response_.headers.get());
-
+HttpCache::Transaction::EntryValidationStatus
+HttpCache::Transaction::GetEntryValidationStatus() const {
   if (request_->method == "PUT" || request_->method == "DELETE")
-    return false;
+    return REQUIRES_REFETCH;

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

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()?

+
+  bool vary_mismatch = false;
+  if (!RequiresValidation(&vary_mismatch))
+    return USABLE;
 
-  // This only makes sense for cached 200 or 206 responses.
+  // The transaction requires revalidation with the server, and may require
+  // a re-fetch.
   if (response_.headers->response_code() != 200 &&
       response_.headers->response_code() != 206) {
-    return false;
+    return REQUIRES_REFETCH;
   }
 
   if (fail_conditionalization_for_test_)
-    return false;
+    return REQUIRES_REFETCH;
 
   DCHECK(response_.headers->response_code() != 206 ||
          response_.headers->HasStrongValidators());
@@ -2363,51 +2418,16 @@ bool HttpCache::Transaction::ConditionalizeRequest() {
     response_.headers->EnumerateHeader(NULL, "etag", &etag_value);
 
   std::string last_modified_value;
-  if (!vary_mismatch_) {
+  if (!vary_mismatch) {
     response_.headers->EnumerateHeader(NULL, "last-modified",
                                        &last_modified_value);
   }
 
   if (etag_value.empty() && last_modified_value.empty())
-    return false;
-
-  if (!partial_) {
-    // Need to customize the request, so this forces us to allocate :(
-    custom_request_.reset(new HttpRequestInfo(*request_));
-    request_ = custom_request_.get();
-  }
-  DCHECK(custom_request_.get());
+    return REQUIRES_REFETCH;
 
-  bool use_if_range =
-      partial_ && !partial_->IsCurrentRangeCached() && !invalid_range_;
-
-  if (!etag_value.empty()) {
-    if (use_if_range) {
-      // We don't want to switch to WRITE mode if we don't have this block of a
-      // byte-range request because we may have other parts cached.
-      custom_request_->extra_headers.SetHeader(
-          HttpRequestHeaders::kIfRange, etag_value);
-    } else {
-      custom_request_->extra_headers.SetHeader(
-          HttpRequestHeaders::kIfNoneMatch, etag_value);
-    }
-    // For byte-range requests, make sure that we use only one way to validate
-    // the request.
-    if (partial_ && !partial_->IsCurrentRangeCached())
-      return true;
-  }
-
-  if (!last_modified_value.empty()) {
-    if (use_if_range) {
-      custom_request_->extra_headers.SetHeader(
-          HttpRequestHeaders::kIfRange, last_modified_value);
-    } else {
-      custom_request_->extra_headers.SetHeader(
-          HttpRequestHeaders::kIfModifiedSince, last_modified_value);
-    }
-  }
-
-  return true;
+  return vary_mismatch ? REQUIRES_VALIDATION
+                       : REQUIRES_VALIDATION_VARY_MISMATCH;

[shivanisha, 2017/05/02 16:03:25]

Looks like it should be REQUIRES_VALIDATION_VARY_MISMATCH if vary_mismatch is
true?

 }
 
 // We just received some headers from the server. We may have asked for a range,