service worker: Various style cleanup in ServiceWorkerControlleeRequestHandler.

Including:
- std::move() when appropriate.
- Change diagnostic CHECKs to DCHECKs.
- |self| doesn't seem like Chromium style.
- Don't repeat base class documentation in overrides.

Bug: 866353
Change-Id: I9d7d3653cd801770251936bcae330bf1610d0fdb
Reviewed-on: https://chromium-review.googlesource.com/1159933Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580099}

content/browser/service_worker/service_worker_controllee_request_handler.cc

@@ -4,9 +4,8 @@
 
 #include "content/browser/service_worker/service_worker_controllee_request_handler.h"
 
-#include <memory>
 #include <set>
-#include <string>
+#include <utility>
 
 #include "base/trace_event/trace_event.h"
 #include "components/offline_pages/buildflags/buildflags.h"
@@ -120,14 +119,13 @@ ServiceWorkerControlleeRequestHandler::ServiceWorkerControlleeRequestHandler(
     RequestContextType request_context_type,
     network::mojom::RequestContextFrameType frame_type,
     scoped_refptr<network::ResourceRequestBody> body)
-    : ServiceWorkerRequestHandler(context,
-                                  provider_host,
-                                  blob_storage_context,
+    : ServiceWorkerRequestHandler(std::move(context),
+                                  std::move(provider_host),
+                                  std::move(blob_storage_context),
                                   resource_type),
       resource_type_(resource_type),
       is_main_resource_load_(
           ServiceWorkerUtils::IsMainResourceType(resource_type)),
-      is_main_frame_load_(resource_type == RESOURCE_TYPE_MAIN_FRAME),
       request_mode_(request_mode),
       credentials_mode_(credentials_mode),
       redirect_mode_(redirect_mode),
@@ -135,7 +133,7 @@ ServiceWorkerControlleeRequestHandler::ServiceWorkerControlleeRequestHandler(
       keepalive_(keepalive),
       request_context_type_(request_context_type),
       frame_type_(frame_type),
-      body_(body),
+      body_(std::move(body)),
       force_update_started_(false),
       use_network_(false),
       weak_factory_(this) {}
@@ -212,12 +210,11 @@ net::URLRequestJob* ServiceWorkerControlleeRequestHandler::MaybeCreateJob(
 #endif  // BUILDFLAG(ENABLE_OFFLINE_PAGES)
 
   // It's for original request (A) or redirect case (B-a or B-b).
-  std::unique_ptr<ServiceWorkerURLRequestJob> job(
-      new ServiceWorkerURLRequestJob(
-          request, network_delegate, provider_host_->client_uuid(),
-          blob_storage_context_, resource_context, request_mode_,
-          credentials_mode_, redirect_mode_, integrity_, keepalive_,
-          resource_type_, request_context_type_, frame_type_, body_, this));
+  auto job = std::make_unique<ServiceWorkerURLRequestJob>(
+      request, network_delegate, provider_host_->client_uuid(),
+      blob_storage_context_, resource_context, request_mode_, credentials_mode_,
+      redirect_mode_, integrity_, keepalive_, resource_type_,
+      request_context_type_, frame_type_, body_, this);
   url_job_ = std::make_unique<ServiceWorkerURLJobWrapper>(job->GetWeakPtr());
 
   resource_context_ = resource_context;
@@ -354,7 +351,8 @@ void ServiceWorkerControlleeRequestHandler::PrepareForMainResource(
   provider_host_->SetDocumentUrl(stripped_url_);
   provider_host_->SetTopmostFrameUrl(site_for_cookies);
   context_->storage()->FindRegistrationForDocument(
-      stripped_url_, base::BindOnce(&self::DidLookupRegistrationForMainResource,
+      stripped_url_, base::BindOnce(&ServiceWorkerControlleeRequestHandler::
+                                        DidLookupRegistrationForMainResource,
                                     weak_factory_.GetWeakPtr(),
                                     std::move(disallow_controller)));
 }
@@ -377,7 +375,7 @@ void ServiceWorkerControlleeRequestHandler::
         url_job_.get(), "Status", blink::ServiceWorkerStatusToString(status));
     return;
   }
-  DCHECK(registration.get());
+  DCHECK(registration);
 
   if (!provider_host_) {
     url_job_->FallbackToNetwork();
@@ -420,15 +418,17 @@ void ServiceWorkerControlleeRequestHandler::
     return;
   }
 
-  const bool need_to_update = !force_update_started_ && registration &&
-                              context_->force_update_on_page_load();
+  const bool need_to_update =
+      !force_update_started_ && context_->force_update_on_page_load();
   if (need_to_update) {
     force_update_started_ = true;
     context_->UpdateServiceWorker(
         registration.get(), true /* force_bypass_cache */,
         true /* skip_script_comparison */,
-        base::BindOnce(&self::DidUpdateRegistration, weak_factory_.GetWeakPtr(),
-                       registration, std::move(disallow_controller)));
+        base::BindOnce(
+            &ServiceWorkerControlleeRequestHandler::DidUpdateRegistration,
+            weak_factory_.GetWeakPtr(), registration,
+            std::move(disallow_controller)));
     return;
   }
 
@@ -450,10 +450,9 @@ void ServiceWorkerControlleeRequestHandler::
     return;
   }
 
-  // TODO(falken): Change these to DCHECK if it holds.
-  CHECK(active_version->status() == ServiceWorkerVersion::ACTIVATING ||
-        active_version->status() == ServiceWorkerVersion::ACTIVATED);
-
+  DCHECK(active_version->status() == ServiceWorkerVersion::ACTIVATING ||
+         active_version->status() == ServiceWorkerVersion::ACTIVATED)
+      << ServiceWorkerVersion::VersionStatusToString(active_version->status());
   // Wait until it's activated before firing fetch events.
   if (active_version->status() == ServiceWorkerVersion::ACTIVATING) {
     registration->active_version()->RegisterStatusChangeCallback(
@@ -534,15 +533,13 @@ void ServiceWorkerControlleeRequestHandler::
   provider_host_->SetControllerRegistration(
       registration, false /* notify_controllerchange */);
 
-  // TODO(falken): Change these to DCHECK if it holds, or else figure out
-  // how this happens.
-  CHECK_EQ(active_version, registration->active_version());
-  CHECK_EQ(active_version, provider_host_->controller());
-
+  DCHECK_EQ(active_version, registration->active_version());
+  DCHECK_EQ(active_version, provider_host_->controller());
   DCHECK_NE(active_version->fetch_handler_existence(),
             ServiceWorkerVersion::FetchHandlerExistence::UNKNOWN);
   ServiceWorkerMetrics::CountControlledPageLoad(
-      active_version->site_for_uma(), stripped_url_, is_main_frame_load_);
+      active_version->site_for_uma(), stripped_url_,
+      resource_type_ == RESOURCE_TYPE_MAIN_FRAME);
 
   if (blink::ServiceWorkerUtils::IsServicificationEnabled() &&
       IsResourceTypeFrame(resource_type_)) {
@@ -559,7 +556,7 @@ void ServiceWorkerControlleeRequestHandler::
 }
 
 void ServiceWorkerControlleeRequestHandler::DidUpdateRegistration(
-    const scoped_refptr<ServiceWorkerRegistration>& original_registration,
+    scoped_refptr<ServiceWorkerRegistration> original_registration,
     std::unique_ptr<ScopedDisallowSetControllerRegistration>
         disallow_controller,
     blink::ServiceWorkerStatusCode status,
@@ -580,25 +577,26 @@ void ServiceWorkerControlleeRequestHandler::DidUpdateRegistration(
     // Update failed. Look up the registration again since the original
     // registration was possibly unregistered in the meantime.
     context_->storage()->FindRegistrationForDocument(
-        stripped_url_,
-        base::BindOnce(&self::DidLookupRegistrationForMainResource,
-                       weak_factory_.GetWeakPtr(),
-                       std::move(disallow_controller)));
+        stripped_url_, base::BindOnce(&ServiceWorkerControlleeRequestHandler::
+                                          DidLookupRegistrationForMainResource,
+                                      weak_factory_.GetWeakPtr(),
+                                      std::move(disallow_controller)));
     return;
   }
   DCHECK_EQ(original_registration->id(), registration_id);
-  scoped_refptr<ServiceWorkerVersion> new_version =
+  ServiceWorkerVersion* new_version =
       original_registration->installing_version();
   new_version->ReportForceUpdateToDevTools();
   new_version->set_skip_waiting(true);
   new_version->RegisterStatusChangeCallback(base::BindOnce(
-      &self::OnUpdatedVersionStatusChanged, weak_factory_.GetWeakPtr(),
-      original_registration, new_version, std::move(disallow_controller)));
+      &ServiceWorkerControlleeRequestHandler::OnUpdatedVersionStatusChanged,
+      weak_factory_.GetWeakPtr(), std::move(original_registration),
+      base::WrapRefCounted(new_version), std::move(disallow_controller)));
 }
 
 void ServiceWorkerControlleeRequestHandler::OnUpdatedVersionStatusChanged(
-    const scoped_refptr<ServiceWorkerRegistration>& registration,
-    const scoped_refptr<ServiceWorkerVersion>& version,
+    scoped_refptr<ServiceWorkerRegistration> registration,
+    scoped_refptr<ServiceWorkerVersion> version,
     std::unique_ptr<ScopedDisallowSetControllerRegistration>
         disallow_controller) {
   // The job may have been canceled before this was invoked.
@@ -615,15 +613,16 @@ void ServiceWorkerControlleeRequestHandler::OnUpdatedVersionStatusChanged(
     // continue with the incumbent version.
     // In case unregister job may have run, look up the registration again.
     context_->storage()->FindRegistrationForDocument(
-        stripped_url_,
-        base::BindOnce(&self::DidLookupRegistrationForMainResource,
-                       weak_factory_.GetWeakPtr(),
-                       std::move(disallow_controller)));
+        stripped_url_, base::BindOnce(&ServiceWorkerControlleeRequestHandler::
+                                          DidLookupRegistrationForMainResource,
+                                      weak_factory_.GetWeakPtr(),
+                                      std::move(disallow_controller)));
     return;
   }
   version->RegisterStatusChangeCallback(base::BindOnce(
-      &self::OnUpdatedVersionStatusChanged, weak_factory_.GetWeakPtr(),
-      registration, version, std::move(disallow_controller)));
+      &ServiceWorkerControlleeRequestHandler::OnUpdatedVersionStatusChanged,
+      weak_factory_.GetWeakPtr(), std::move(registration), version,
+      std::move(disallow_controller)));
 }
 
 void ServiceWorkerControlleeRequestHandler::PrepareForSubResource() {

content/browser/service_worker/service_worker_controllee_request_handler.h

@@ -6,6 +6,8 @@
 #define CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_CONTROLLEE_REQUEST_HANDLER_H_
 
 #include <stdint.h>
+#include <memory>
+#include <string>
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
@@ -37,8 +39,9 @@ namespace content {
 class ServiceWorkerRegistration;
 class ServiceWorkerVersion;
 
-// A request handler derivative used to handle requests from
-// controlled documents.
+// A request handler derivative used to handle requests for,
+// and requests from, controlled documents and shared workers.
+//
 // Note that in IsServicificationEnabled cases this is used only for
 // main resource fetch during navigation or shared worker creation.
 class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler
@@ -92,8 +95,6 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler
  private:
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerControlleeRequestHandlerTest,
                            ActivateWaitingVersion);
-  typedef ServiceWorkerControlleeRequestHandler self;
-
   class ScopedDisallowSetControllerRegistration;
 
   // For main resource case.
@@ -109,16 +110,17 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler
       std::unique_ptr<ScopedDisallowSetControllerRegistration>
           disallow_controller);
 
+  // For forced update.
   void DidUpdateRegistration(
-      const scoped_refptr<ServiceWorkerRegistration>& original_registration,
+      scoped_refptr<ServiceWorkerRegistration> original_registration,
       std::unique_ptr<ScopedDisallowSetControllerRegistration>
           disallow_controller,
       blink::ServiceWorkerStatusCode status,
       const std::string& status_message,
       int64_t registration_id);
   void OnUpdatedVersionStatusChanged(
-      const scoped_refptr<ServiceWorkerRegistration>& registration,
-      const scoped_refptr<ServiceWorkerVersion>& version,
+      scoped_refptr<ServiceWorkerRegistration> registration,
+      scoped_refptr<ServiceWorkerVersion> version,
       std::unique_ptr<ScopedDisallowSetControllerRegistration>
           disallow_controller);
 
@@ -126,11 +128,7 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler
   void PrepareForSubResource();
 
   // ServiceWorkerURLJobWrapper::Delegate implementation:
-
-  // Called just before the request is restarted. Makes sure the next request
-  // goes over the network.
   void OnPrepareToRestart() override;
-
   ServiceWorkerVersion* GetServiceWorkerVersion(
       ServiceWorkerMetrics::URLRequestJobResult* result) override;
   bool RequestStillValid(
@@ -149,7 +147,6 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler
 
   const ResourceType resource_type_;
   const bool is_main_resource_load_;
-  const bool is_main_frame_load_;
   std::unique_ptr<ServiceWorkerURLJobWrapper> url_job_;
   network::mojom::FetchRequestMode request_mode_;
   network::mojom::FetchCredentialsMode credentials_mode_;