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

net/http/http_cache_transaction.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_cache_transaction.h"
 
 #include "build/build_config.h"  // For OS_POSIX
 
 #if defined(OS_POSIX)
 #include <unistd.h>
@@ skipping 146 matching lines @@
       new_response_(NULL),
       mode_(NONE),
       reading_(false),
       invalid_range_(false),
       truncated_(false),
       is_sparse_(false),
       range_requested_(false),
       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),
       io_buf_len_(0),
       read_offset_(0),
       effective_load_flags_(0),
       write_len_(0),
       cache_entry_status_(CacheEntryStatus::ENTRY_UNDEFINED),
       validation_cause_(VALIDATION_CAUSE_UNDEFINED),
       total_received_bytes_(0),
@@ skipping 1912 matching lines @@
     TransitionToState(STATE_NONE);
     return ERR_CACHE_MISS;
   }
 
   // We don't have the whole resource.
   if (truncated_) {
     TransitionToState(STATE_NONE);
     return ERR_CACHE_MISS;
   }
 
-  if (RequiresValidation()) {
+  if (GetEntryValidationStatus() != USABLE) {
     TransitionToState(STATE_NONE);
     return ERR_CACHE_MISS;
   }
 
   if (request_->method == "HEAD")
     FixHeadersForHead();
 
   if (entry_->disk_entry->GetDataSize(kMetadataIndex))
     TransitionToState(STATE_CACHE_READ_METADATA);
   else
     TransitionToState(STATE_NONE);
 
   return OK;
 }
 
 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!
     TransitionToState(STATE_SEND_REQUEST);
     mode_ = NONE;
     return OK;
   }
 
   if (truncated_) {
     // 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 (entry_validation_status == USABLE) {
+    UpdateCacheEntryStatus(CacheEntryStatus::ENTRY_USED);
+    return SetupEntryForRead();
   }
 
-  if (skip_validation) {
-    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();
+  // 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());
+    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);
     }
-    TransitionToState(STATE_SEND_REQUEST);
   }
+
+  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;
 }
 
 int HttpCache::Transaction::BeginPartialCacheValidation() {
   DCHECK_EQ(mode_, READ_WRITE);
 
   if (response_.headers->response_code() != 206 && !partial_ && !truncated_)
     return BeginCacheValidation();
 
   // Partial requests should not be recorded in histograms.
@@ skipping 97 matching lines @@
   DCHECK(network_trans_.get());
   DCHECK_EQ(STATE_NONE, next_state_);
 
   next_state_ = STATE_SEND_REQUEST_COMPLETE;
   int rv = network_trans_->RestartWithAuth(credentials, io_callback_);
   if (rv != ERR_IO_PENDING)
     return DoLoop(rv);
   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;
   }
 
   if (effective_load_flags_ & LOAD_SKIP_CACHE_VALIDATION)
     return false;
 
   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;
   }
 
   if (effective_load_flags_ & LOAD_VALIDATE_CACHE) {
     validation_cause_ = VALIDATION_CAUSE_VALIDATE_FLAG;
     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());
 
   if (validation_required_by_headers) {
     HttpResponseHeaders::FreshnessLifetimes lifetimes =
         response_.headers->GetFreshnessLifetimes(response_.response_time);
     if (lifetimes.freshness == base::TimeDelta()) {
       validation_cause_ = VALIDATION_CAUSE_ZERO_FRESHNESS;
     } else {
       validation_cause_ = VALIDATION_CAUSE_STALE;
       stale_entry_freshness_ = lifetimes.freshness;
       stale_entry_age_ = response_.headers->GetCurrentAge(
           response_.request_time, response_.response_time,
           cache_->clock_->Now());
     }
   }
 
   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 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()?

 
-  if (request_->method == "PUT" || request_->method == "DELETE")
-    return false;
+  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());
 
   // Just use the first available ETag and/or Last-Modified header value.
   // TODO(darin): Or should we use the last?
 
   std::string etag_value;
   if (response_.headers->GetHttpVersion() >= HttpVersion(1, 1))
     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;
+    return REQUIRES_REFETCH;
 
-  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());
-
-  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,
 // in which case partial_ has an object. This could be the first network request
 // we make to fulfill the original request, or we may be already reading (from
 // the net and / or the cache). If we are not expecting a certain response, we
 // just bypass the cache for this request (but again, maybe we are reading), and
 // delete partial_ (so we are not able to "fix" the headers that we return to
 // the user). This results in either a weird response for the caller (we don't
 // expect it after all), or maybe a range that was not exactly what it was asked
@@ skipping 596 matching lines @@
 }
 
 void HttpCache::Transaction::TransitionToState(State state) {
   // Ensure that the state is only set once per Do* state.
   DCHECK(in_do_loop_);
   DCHECK_EQ(STATE_UNSET, next_state_) << "Next state is " << state;
   next_state_ = state;
 }
 
 }  // namespace net