content::ResourceDispatcherHostImpl changes for stale-while-revalidate

content/browser/loader/async_revalidation_manager.cc

+// Copyright 2015 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 "content/browser/loader/async_revalidation_manager.h"
+
+#include <utility>
+
+#include "base/logging.h"
+#include "content/browser/loader/async_revalidation_driver.h"
+#include "content/browser/loader/resource_message_filter.h"
+#include "content/browser/loader/resource_request_info_impl.h"
+#include "content/browser/loader/resource_scheduler.h"
+#include "content/common/resource_messages.h"
+#include "content/public/browser/resource_context.h"

[Bence, 2015/11/17 13:12:22]

You only have pointers to ResourceContext, I don't believe you need this
include.

[Adam Rice, 2015/11/17 17:45:52]

Good point. Removed.

+#include "content/public/browser/resource_throttle.h"

[Bence, 2015/11/17 13:12:22]

I believe a forward declaration would be enough for a scoped_ptr<>, but I'm not
sure.

[Adam Rice, 2015/11/17 17:45:52]

A scoped_ptr<ResourceThrottle> object is instantiated in
AsyncRevalidationManager::BeginAsyncRevalidation(). The scoped_ptr destructor
references the ResourceThrottle destructor, which requires the declaration of
the destructor to be visible.

[Bence, 2015/11/17 21:34:28]

Of course, what was I thinking?  Thanks for clarifying.

+#include "net/base/load_flags.h"
+#include "net/http/http_transaction_factory.h"
+#include "net/http/http_util.h"
+#include "net/url_request/url_request.h"
+#include "net/url_request/url_request_context.h"
+#include "url/gurl.h"
+
+namespace content {
+
+AsyncRevalidationManager::AsyncRevalidationKey::AsyncRevalidationKey(
+    const ResourceContext* resource_context,
+    const net::HttpCache* http_cache,
+    const GURL& url)
+    : resource_context(resource_context),
+      http_cache(http_cache),
+      url_key(net::HttpUtil::SpecForRequest(url)) {}
+
+AsyncRevalidationManager::AsyncRevalidationKey::AsyncRevalidationKey(
+    const ResourceContext* resource_context)
+    : resource_context(resource_context), http_cache(nullptr), url_key() {}
+
+AsyncRevalidationManager::AsyncRevalidationKey::~AsyncRevalidationKey() {}
+
+bool AsyncRevalidationManager::AsyncRevalidationKey::LessThan::operator()(
+    const AsyncRevalidationKey& lhs,
+    const AsyncRevalidationKey& rhs) const {
+  if (lhs.resource_context != rhs.resource_context)
+    return lhs.resource_context < rhs.resource_context;
+
+  if (lhs.http_cache != rhs.http_cache)
+    return lhs.http_cache < rhs.http_cache;
+
+  return lhs.url_key < rhs.url_key;
+}
+
+AsyncRevalidationManager::AsyncRevalidationManager() {}
+
+AsyncRevalidationManager::~AsyncRevalidationManager() {
+  DCHECK(in_progress_.empty());
+}
+
+void AsyncRevalidationManager::BeginAsyncRevalidation(
+    net::URLRequest* for_request,
+    ResourceScheduler* scheduler) {
+  DCHECK_EQ(for_request->url_chain().size(), 1u);
+  ResourceRequestInfoImpl* info =
+      ResourceRequestInfoImpl::ForRequest(for_request);
+  DCHECK(info);
+  if (!info->filter()) {
+    // The child has gone away. Since it never received the body of the stale
+    // resource, it is okay not to perform an async revalidation.
+    return;
+  }
+
+  int child_id = info->GetChildID();
+  int route_id = info->GetRouteID();
+
+  // ResourceMsgFilter::GetContexts requires a ResourceHostMsg_Request object be
+  // passed but only cares about the resource_type field.
+  ResourceHostMsg_Request dummy_resource_host_msg_request;
+  dummy_resource_host_msg_request.resource_type = info->GetResourceType();
+  ResourceContext* resource_context = NULL;

[Bence, 2015/11/17 13:12:23]

Please use nullptr in this line and the one below.

[Adam Rice, 2015/11/17 17:45:52]

Done.

+  net::URLRequestContext* request_context = NULL;
+  // This |request_context| needs to be valid until

[Bence, 2015/11/17 13:12:23]

Optional: add newline above this comment for better structure.

[Adam Rice, 2015/11/17 17:45:52]

Done.

+  // RemoveResourceContext(resource_context) is called. This is currently
+  // correct, see
+  // https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/profiles/profile_io_data.h&sq=package:chromium&l=481

[Bence, 2015/11/17 13:12:22]

Please don't use such URL, because (1) the awkwardness of the unusually long
line outweights the convenience of having a clickable URL in your editor (and
some environments cannot use URLs correctly if it is broken into multiple lines,
like vim in tmux), (2) since there is no tagged revision, the line number will
go stale very quickly, and (3) there is no guarantee on how long this hostname
will work.

[Adam Rice, 2015/11/17 17:45:52]

Okay, replaced with a simple reference to the file.

[Bence, 2015/11/17 21:34:28]

Neat.

+  info->filter()->GetContexts(dummy_resource_host_msg_request,
+                              &resource_context, &request_context);
+
+  AsyncRevalidationKey async_revalidation_key(
+      resource_context, request_context->http_transaction_factory()->GetCache(),
+      for_request->url());
+  std::pair<AsyncRevalidationMap::iterator, bool> insert_result =
+      in_progress_.insert(
+          AsyncRevalidationMap::value_type(async_revalidation_key, nullptr));
+  if (!insert_result.second) {
+    // A matching async revalidation is already in progress for this cache; we
+    // don't need another one.
+    return;
+  }
+  auto& async_revalidation = insert_result.first->second;
+  DCHECK(async_revalidation == nullptr);
+
+  net::HttpRequestHeaders headers;
+  headers.AddHeadersFromString(info->original_headers());
+
+  // Construct the request.
+  scoped_ptr<net::URLRequest> new_request = request_context->CreateRequest(
+      for_request->url(), net::MINIMUM_PRIORITY, NULL);

[Bence, 2015/11/17 13:12:22]

Please use nullptr instead of NULL.

[Adam Rice, 2015/11/17 17:45:52]

Done.

+
+  new_request->set_method(for_request->method());
+  new_request->set_first_party_for_cookies(
+      for_request->first_party_for_cookies());
+  new_request->set_first_party_url_policy(
+      for_request->first_party_url_policy());
+
+  new_request->SetReferrer(for_request->referrer());
+  new_request->set_referrer_policy(for_request->referrer_policy());
+
+  new_request->SetExtraRequestHeaders(headers);
+
+  int load_flags = for_request->load_flags() &
+                   ~net::LOAD_SUPPORT_ASYNC_REVALIDATION &
+                   ~net::LOAD_MAIN_FRAME;
+  DCHECK_EQ(load_flags & net::LOAD_IGNORE_LIMITS, 0);
+  new_request->SetLoadFlags(load_flags);
+
+  scoped_ptr<ResourceThrottle> throttle =
+      scheduler->ScheduleRequest(child_id, route_id, false, new_request.get());
+  // This use of base::Unretained() is safe because the AsyncRevalidatonDriver
+  // object will be destroyed before this object is. The use of the raw iterator
+  // is safe because objects are only erased from the map after being deleted,
+  // and a deleted AsyncRevalidationDriver will not call the completion
+  // callback.
+  async_revalidation = new AsyncRevalidationDriver(
+      new_request.Pass(), throttle.Pass(),
+      base::Bind(&AsyncRevalidationManager::OnAsyncRevalidationComplete,
+                 base::Unretained(this), insert_result.first));
+  async_revalidation->StartRequest();
+}
+
+void AsyncRevalidationManager::CancelAsyncRevalidationsForResourceContext(
+    ResourceContext* resource_context) {
+  // For this algorithm to work, elements using |resource_context| must be
+  // contiguous in the map (ie. it must form the first part of the key).
+  AsyncRevalidationKey partial_key(resource_context);
+  auto next_it = in_progress_.lower_bound(partial_key);

[Bence, 2015/11/17 13:12:22]

Optional: use |it| instead of |next_it|, |prev_it| instead of |current_it|.

[Adam Rice, 2015/11/17 17:45:52]

Done.

+  while (next_it != in_progress_.end() &&

[Bence, 2015/11/17 13:12:22]

Optional: use for loop instead, make |next_it| local to loop by defining it in
the init-statement.

[Adam Rice, 2015/11/17 17:45:52]

Done.

+         next_it->first.resource_context == resource_context) {
+    // Erasing the element invalidates the iterator. Increment it first.
+    auto current_it = next_it++;

[Bence, 2015/11/17 13:12:22]

Please don't do assignment and increment in the same expression, the code reads
easier if the readers are not expected to remember whether the postfix syntax
evaluates to the value before or after incrementing.  (I certainly didn't
remember.)

[Adam Rice, 2015/11/17 17:45:52]

Done.

+    current_it->second->CancelRequest();
+    delete current_it->second;
+    in_progress_.erase(current_it);
+  }
+}
+
+bool AsyncRevalidationManager::QualifiesForAsyncRevalidation(
+    const ResourceHostMsg_Request& request) {
+  if (request.load_flags &
+      (net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE |
+       net::LOAD_VALIDATE_CACHE | net::LOAD_PREFERRING_CACHE |
+       net::LOAD_ONLY_FROM_CACHE | net::LOAD_IGNORE_LIMITS)) {
+    return false;
+  }
+  if (request.method != "GET")
+    return false;
+  // A GET request should not have a body, but don't leave it to chance.
+  if (request.request_body.get())
+    return false;
+  if (!request.url.SchemeIsHTTPOrHTTPS())
+    return false;
+  return true;

[Bence, 2015/11/17 13:12:23]

Please add an empty line above this final return.

[Adam Rice, 2015/11/17 17:45:52]

Done.

+}
+
+void AsyncRevalidationManager::OnAsyncRevalidationComplete(
+    AsyncRevalidationMap::iterator it) {
+  delete it->second;
+  in_progress_.erase(it);
+}
+
+}  // namespace content