ServiceWorker: Store the existence of fetch event handler

content/browser/service_worker/service_worker_version.h

Index: content/browser/service_worker/service_worker_version.h
diff --git a/content/browser/service_worker/service_worker_version.h b/content/browser/service_worker/service_worker_version.h
index d2ed73600c9dc5dd282d303508ebefe442193d27..7b4d2d7927db9117c1e3e6618528e6d77d682805 100644
--- a/content/browser/service_worker/service_worker_version.h
+++ b/content/browser/service_worker/service_worker_version.h
@@ -137,6 +137,10 @@ class CONTENT_EXPORT ServiceWorkerVersion
   }
   ServiceWorkerVersionInfo GetInfo();
   Status status() const { return status_; }
+  bool has_fetch_handler() const { return has_fetch_handler_; }
+  void set_has_fetch_handler(bool has_fetch_handler) {
+    has_fetch_handler_ = has_fetch_handler;
+  }
 
   const std::vector<GURL>& foreign_fetch_scopes() const {
     return foreign_fetch_scopes_;
@@ -248,6 +252,11 @@ class CONTENT_EXPORT ServiceWorkerVersion
   template <typename ResponseMessage>
   void DispatchSimpleEvent(int request_id, const IPC::Message& message);
 
+  // For install event, the callback should be specialized to update the stat
+  // of version object because |has_fetch_handler| should be set corresponding
+  // to the result of script evaluation.

[falken, 2016/05/23 08:41:13]

I think DispatchEvent() (not DispatchSimpleEvent) was designed to handle
specialized callbacks. Can you use that instead of special casing Install? (It
looks like currently only "fetch" uses it.)

That would move handling the has_fetch_handler flag to ServiceWorkerRegisterJob,
which feels a bit cleaner, too.

[shimazu, 2016/05/23 10:44:24]

I considered has_fetch_handler is a property which should be bound to
ServiceWorkerVersion, so that is updated only by ServiceWorkerVersion itself or
ServiceWorkerStorage in this patch.

What do you think?

[falken, 2016/05/24 01:15:34]

But SWVersion is kind of transient: it can be destroyed and recreated for the
same worker. This patch is depending on the version getting persisted
immediately after setting the bit. We know that is what happens, but I'd rather
make that dependency explicit. The current patch does:
1. SWRegisterJob dispatches install event.
2. SWVersion sets the bit on itself.
3. SWRegisterJob gets the bit and persists the worker.

Alternatively, this seems clearer:
1. SWRegisterJob dispatches the install event.
2. SWRegisterJob install event callback is passed the bit and persists the
worker.

But more than that, the special casing of DispatchInstallEvent seems unnecessary
since we have DispatchEvent.

[shimazu, 2016/05/24 04:17:46]

Thanks. Done.

+  void DispatchInstallEvent(int request_id);
+
   // Adds and removes |provider_host| as a controllee of this ServiceWorker.
   // A potential controllee is a host having the version as its .installing
   // or .waiting version.
@@ -500,6 +509,9 @@ class CONTENT_EXPORT ServiceWorkerVersion
 
   void OnSimpleEventResponse(int request_id,
                              blink::WebServiceWorkerEventResult result);
+  void OnInstallEventResponse(int request_id,
+                              blink::WebServiceWorkerEventResult result,
+                              bool has_fetch_handler);
   void OnOpenWindow(int request_id, GURL url);
   void OnOpenWindowFinished(int request_id,
                             ServiceWorkerStatusCode status,
@@ -603,6 +615,7 @@ class CONTENT_EXPORT ServiceWorkerVersion
   const GURL scope_;
   std::vector<GURL> foreign_fetch_scopes_;
   std::vector<url::Origin> foreign_fetch_origins_;
+  bool has_fetch_handler_ = true;
 
   Status status_ = NEW;
   std::unique_ptr<EmbeddedWorkerInstance> embedded_worker_;