Show service worker navigation preload requests in DevTools Network tab

content/renderer/service_worker/service_worker_context_client.cc

Index: content/renderer/service_worker/service_worker_context_client.cc
diff --git a/content/renderer/service_worker/service_worker_context_client.cc b/content/renderer/service_worker/service_worker_context_client.cc
index 4569c4b15c118023841d135470d3d5cf9a0d5b78..efd72cebc36038f0bbc5af98b6907bc7117a18fe 100644
--- a/content/renderer/service_worker/service_worker_context_client.cc
+++ b/content/renderer/service_worker/service_worker_context_client.cc
@@ -267,19 +267,13 @@ class ServiceWorkerContextClient::NavigationPreloadRequest final
         binding_(this, std::move(preload_handle->url_loader_client_request)) {}
 
   ~NavigationPreloadRequest() override {
-    if (result_reported_)
-      return;
-    ServiceWorkerContextClient* client =
-        ServiceWorkerContextClient::ThreadSpecificInstance();
-    if (!client)
-      return;
-    client->OnNavigationPreloadError(
-        fetch_event_id_,
-        base::MakeUnique<blink::WebServiceWorkerError>(
-            blink::WebServiceWorkerError::ErrorTypeAbort,
-            blink::WebString::fromASCII(
-                "Service Worker navigation preload aborted. Need to guard with "
-                "respondWith or waitUntil.")));
+    if (!response_reported_) {
+      MaybeReportErrorToClient(
+          blink::WebServiceWorkerError::ErrorTypeAbort,
+          "Service Worker navigation preload aborted. Need to guard with "
+          "respondWith or waitUntil.");

[falken, 2017/01/17 14:38:36]

FWIW this error message confused me, since I was not trying to use navigation
preload except for certain URLs. I think DevTools didn't really highlight that
the error came from the FetchEvent.preloadResponse being rejected. Maybe a
better error message is "The service worker navigation preload request was
aborted because the FetchEvent finished before FetchEvent.preloadResponse could
be resolved. If you intend to use preloadResponse, it must be guarded by
respondWith or waitUntil."

[horo, 2017/01/18 14:25:02]

Removed this error in https://codereview.chromium.org/2645493002.

+    }
+    MaybeReportCompletionToClient();
   }
 
   void OnReceiveResponse(
@@ -299,7 +293,8 @@ class ServiceWorkerContextClient::NavigationPreloadRequest final
                          const ResourceResponseHead& response_head) override {
     // Cancel the request.
     url_loader_ = nullptr;
-    ReportErrorToClient(
+    MaybeReportErrorToClient(
+        blink::WebServiceWorkerError::ErrorTypeNetwork,
         "Service Worker navigation preload doesn't suport redirect.");

[falken, 2017/01/17 14:38:36]

nit: "support redirects"

[horo, 2017/01/18 14:25:02]

Done.

   }
 
@@ -309,7 +304,7 @@ class ServiceWorkerContextClient::NavigationPreloadRequest final
   }
 
   void OnTransferSizeUpdated(int32_t transfer_size_diff) override {
-    NOTREACHED();
+    // TODO(horo): Send this transfer size update notification to DevTools.
   }
 
   void OnStartLoadingResponseBody(
@@ -320,17 +315,19 @@ class ServiceWorkerContextClient::NavigationPreloadRequest final
   }
 
   void OnComplete(const ResourceRequestCompletionStatus& status) override {
-    // We don't report to |client| if OnStartLoadingResponseBody() has already
-    // called OnNavigationPreloadResponse().
-    if (result_reported_)
-      return;
-    DCHECK_NE(0, status.error_code);
-    ReportErrorToClient("Service Worker navigation preload network error.");
+    if (status.error_code != net::OK) {
+      MaybeReportErrorToClient(
+          blink::WebServiceWorkerError::ErrorTypeNetwork,
+          "Service Worker navigation preload network error.");
+    }
+    MaybeReportCompletionToClient();
   }
 
  private:
   void MaybeReportResponseToClient() {
-    DCHECK(!result_reported_);
+    if (error_reported_)
+      return;
+    DCHECK(!response_reported_);
     if (!response_ || !body_.is_valid())
       return;
     ServiceWorkerContextClient* client =
@@ -341,19 +338,34 @@ class ServiceWorkerContextClient::NavigationPreloadRequest final
     client->OnNavigationPreloadResponse(
         fetch_event_id_, std::move(response_),
         base::MakeUnique<WebDataConsumerHandleImpl>(std::move(body_)));
-    result_reported_ = true;
+    response_reported_ = true;
   }
 
-  void ReportErrorToClient(const char* error_message) {
+  void MaybeReportErrorToClient(
+      blink::WebServiceWorkerError::ErrorType error_type,
+      const char* error_message) {
+    if (response_reported_ || error_reported_)
+      return;
     ServiceWorkerContextClient* client =
         ServiceWorkerContextClient::ThreadSpecificInstance();
     if (!client)
       return;
     client->OnNavigationPreloadError(
-        fetch_event_id_, base::MakeUnique<blink::WebServiceWorkerError>(
-                             blink::WebServiceWorkerError::ErrorTypeNetwork,
-                             blink::WebString::fromUTF8(error_message)));
-    result_reported_ = true;
+        fetch_event_id_,
+        base::MakeUnique<blink::WebServiceWorkerError>(
+            error_type, blink::WebString::fromUTF8(error_message)));
+    error_reported_ = true;
+  }
+
+  void MaybeReportCompletionToClient() {
+    if (error_reported_ || completion_reported_)
+      return;
+    ServiceWorkerContextClient* client =
+        ServiceWorkerContextClient::ThreadSpecificInstance();
+    if (!client)
+      return;
+    client->OnNavigationPreloadCompleted(fetch_event_id_);
+    completion_reported_ = true;
   }
 
   const int fetch_event_id_;
@@ -363,7 +375,9 @@ class ServiceWorkerContextClient::NavigationPreloadRequest final
 
   std::unique_ptr<blink::WebURLResponse> response_;
   mojo::ScopedDataPipeConsumerHandle body_;
-  bool result_reported_ = false;
+  bool response_reported_ = false;
+  bool error_reported_ = false;
+  bool completion_reported_ = false;

[falken, 2017/01/17 14:38:36]

I didn't look too carefully but can we turn this into an enum instead of three
bools? So we know what's really possible of the 8 possible states.

[horo, 2017/01/18 14:25:02]

Removed this in https://codereview.chromium.org/2645493002.

 };
 
 ServiceWorkerContextClient*
@@ -980,11 +994,14 @@ void ServiceWorkerContextClient::DispatchFetchEvent(
     const ServiceWorkerFetchRequest& request,
     mojom::FetchEventPreloadHandlePtr preload_handle,
     const DispatchFetchEventCallback& callback) {
-  std::unique_ptr<NavigationPreloadRequest> preload_request =
-      preload_handle
-          ? base::MakeUnique<NavigationPreloadRequest>(
-                fetch_event_id, request.url, std::move(preload_handle))
-          : nullptr;
+  std::unique_ptr<NavigationPreloadRequest> preload_request;
+  if (preload_handle) {
+    proxy_->onNavigationPreloadSent(fetch_event_id, request.url,
+                                    preload_handle->sent_timestamp,
+                                    preload_handle->sent_wall_time);
+    preload_request = base::MakeUnique<NavigationPreloadRequest>(
+        fetch_event_id, request.url, std::move(preload_handle));
+  }
   const bool navigation_preload_sent = !!preload_request;
   blink::WebServiceWorkerRequest webRequest;
   TRACE_EVENT0("ServiceWorker",
@@ -1271,6 +1288,11 @@ void ServiceWorkerContextClient::OnNavigationPreloadError(
   proxy_->onNavigationPreloadError(fetch_event_id, std::move(error));
 }
 
+void ServiceWorkerContextClient::OnNavigationPreloadCompleted(
+    int fetch_event_id) {
+  proxy_->onNavigationPreloadCompleted(fetch_event_id);
+}
+
 void ServiceWorkerContextClient::BindEventDispatcher(
     mojom::ServiceWorkerEventDispatcherRequest request) {
   DCHECK(context_);