Keep ServiceWorkerMetrics::Site in ServiceWorkerVersion

content/browser/service_worker/service_worker_version.h

 // Copyright 2013 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.
 
 #ifndef CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_VERSION_H_
 #define CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_VERSION_H_
 
 #include <stdint.h>
 
 #include <functional>
@@ skipping 121 matching lines @@
   EmbeddedWorkerStatus running_status() const {
     return embedded_worker_->status();
   }
   ServiceWorkerVersionInfo GetInfo();
   Status status() const { return status_; }
 
   // This flag is set when the install event has been executed in a new version
   // or when an installed version is loaded from the storage. When a new version
   // is not installed yet, returns base::nullopt.
   base::Optional<bool> has_fetch_handler() const { return has_fetch_handler_; }
-  void set_has_fetch_handler(bool has_fetch_handler) {
-    DCHECK(!has_fetch_handler_);
-    has_fetch_handler_ = has_fetch_handler;
-  }
+  // This also updates |site_for_uma_| when it was Site::OTHER.
+  void set_has_fetch_handler(bool has_fetch_handler);
 
   const std::vector<GURL>& foreign_fetch_scopes() const {
     return foreign_fetch_scopes_;
   }
   void set_foreign_fetch_scopes(const std::vector<GURL>& scopes) {
     foreign_fetch_scopes_ = scopes;
   }
 
   const std::vector<url::Origin>& foreign_fetch_origins() const {
     return foreign_fetch_origins_;
   }
   void set_foreign_fetch_origins(const std::vector<url::Origin>& origins) {
     foreign_fetch_origins_ = origins;
   }
 
+  ServiceWorkerMetrics::Site site_for_uma() const { return site_for_uma_; }

[shimazu, 2016/08/18 04:47:36]

I think storing Site as |site_for_uma_| is adding a bit complexity of keeping
the state up to date. If there is no other concern (about performance?), I
prefer calling SiteFromURL(script_url_, has_fetch_handler_).
WDYT?

[horo, 2016/08/18 05:44:54]

We will use this Site for UMA of all fetch events.
https://codereview.chromium.org/2218943002/
I don't think calling SiteFromURL() every time is a good idea.

For example, HistogramCustomizer in the renderer process keep
common_host_histogram_suffix_ which is set in the page navigation
and will be used when recording UMAs.

[shimazu, 2016/08/18 06:53:24]

I see. Let's follow HistogramCustomizer:)

# The reason of the comment was that I considered SiteFromURL wouldn't take much
time (I guess it might be at most ~1us or so?).

+
   // This sets the new status and also run status change callbacks
   // if there're any (see RegisterStatusChangeCallback).
   void SetStatus(Status status);
 
   // Registers status change callback. (This is for one-off observation,
   // the consumer needs to re-register if it wants to continue observing
   // status changes)
   void RegisterStatusChangeCallback(const base::Closure& callback);
 
   // Starts an embedded worker for this version.
@@ skipping 492 matching lines @@
   void FinishStartWorker(ServiceWorkerStatusCode status);
 
   const int64_t version_id_;
   const int64_t registration_id_;
   const GURL script_url_;
   const GURL scope_;
   std::vector<GURL> foreign_fetch_scopes_;
   std::vector<url::Origin> foreign_fetch_origins_;
   base::Optional<bool> has_fetch_handler_;
 
+  ServiceWorkerMetrics::Site site_for_uma_;
+
   Status status_ = NEW;
   std::unique_ptr<EmbeddedWorkerInstance> embedded_worker_;
   std::vector<StatusCallback> start_callbacks_;
   std::vector<StatusCallback> stop_callbacks_;
   std::vector<base::Closure> status_change_callbacks_;
 
   // Holds in-flight requests, including requests due to outstanding push,
   // fetch, sync, etc. events.
   IDMap<PendingRequest, IDMapOwnPointer> pending_requests_;
 
@@ skipping 135 matching lines @@
 
   // At this point |this| can have been deleted, so don't do anything other
   // than returning.
 
   return true;
 }
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_VERSION_H_