Move url_keyed_metrics capability from browser to an embedded service.

This removes the need for resource_coordinator service to have SetUkmRecorder interface, and just has it bind directly. Also introduces MojoUkmRecorder::Create to access this service, and uses this from Document::UkmRecorder(). Bug: 733995 Change-Id: Ie8320cbd12c54af90e35ba21b8f30eb4e3ef4a0b Reviewed-on: https://chromium-review.googlesource.com/611560 Commit-Queue: Steven Holte <[email protected]> Reviewed-by: Steven Holte <[email protected]> Reviewed-by: Daniel Cheng <[email protected]> Reviewed-by: Alexei Svitkine <[email protected]> Reviewed-by: Scott Violet <[email protected]> Reviewed-by: oysteine <[email protected]> Cr-Commit-Position: refs/heads/master@{#514656}
feiyue1206 · Nov 7, 2017 · e830e7e · e830e7e
1 parent d5cf29a
commit e830e7e
Show file tree

Hide file tree

Showing 33 changed files with 238 additions and 304 deletions.
diff --git a/chrome/app/BUILD.gn b/chrome/app/BUILD.gn
@@ -387,6 +387,7 @@ static_library("test_support") {
 chrome_packaged_services = [
   ":chrome_manifest",
   "//chrome/profiling:manifest",
+  "//services/metrics:manifest",
   "//services/proxy_resolver:proxy_resolver_manifest",
   "//services/preferences:local_state_manifest",
 ]

diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn
@@ -1673,7 +1673,6 @@ split_static_library("browser") {
     "//components/translate/core/browser",
     "//components/translate/core/common",
     "//components/ukm:observers",
-    "//components/ukm:ukm_interface",
     "//components/ukm/debug_page",
     "//components/undo",
     "//components/update_client",
@@ -1728,6 +1727,7 @@ split_static_library("browser") {
     "//services/data_decoder/public/cpp",
     "//services/device/public/cpp:device_features",
     "//services/identity:lib",
+    "//services/metrics",
     "//services/metrics/public/cpp:ukm_builders",
     "//services/preferences/public/cpp",
     "//services/preferences/public/cpp:service_main",

diff --git a/chrome/browser/DEPS b/chrome/browser/DEPS
@@ -48,6 +48,7 @@ include_rules = [
   "+services/data_decoder/public",
   "+services/device/public/cpp/device_features.h",
   "+services/identity/public",
+  "+services/metrics/metrics_mojo_service.h",
   "+services/metrics/public",
   "+services/preferences/public/cpp",
   "+services/preferences/public/interfaces",

diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
@@ -174,7 +174,6 @@
 #include "components/task_scheduler_util/browser/initialization.h"
 #include "components/task_scheduler_util/common/variations_util.h"
 #include "components/translate/core/common/translate_switches.h"
-#include "components/ukm/ukm_interface.h"
 #include "components/url_formatter/url_fixer.h"
 #include "components/variations/variations_associated_data.h"
 #include "components/variations/variations_switches.h"
@@ -227,6 +226,8 @@
 #include "ppapi/features/features.h"
 #include "ppapi/host/ppapi_host.h"
 #include "printing/features/features.h"
+#include "services/metrics/metrics_mojo_service.h"
+#include "services/metrics/public/interfaces/constants.mojom.h"
 #include "services/preferences/public/cpp/in_process_service_factory.h"
 #include "services/preferences/public/interfaces/preferences.mojom.h"
 #include "services/proxy_resolver/public/interfaces/proxy_resolver.mojom.h"
@@ -2947,9 +2948,6 @@ void ChromeContentBrowserClient::ExposeInterfacesToRenderer(
       base::Bind(&rappor::RapporRecorderImpl::Create,
                  g_browser_process->rappor_service()),
       ui_task_runner);
-  registry->AddInterface(
-      base::Bind(&ukm::UkmInterface::Create, ukm::UkmRecorder::Get()),
-      ui_task_runner);
   if (NetBenchmarking::CheckBenchmarkingEnabled()) {
     Profile* profile =
         Profile::FromBrowserContext(render_process_host->GetBrowserContext());
@@ -3082,6 +3080,9 @@ void ChromeContentBrowserClient::RegisterInProcessServices(
     services->insert(
         std::make_pair(prefs::mojom::kLocalStateServiceName, info));
   }
+  service_manager::EmbeddedServiceInfo info;
+  info.factory = base::Bind(&metrics::CreateMetricsService);
+  services->emplace(metrics::mojom::kMetricsServiceName, info);
 #if BUILDFLAG(ENABLE_MOJO_MEDIA_IN_BROWSER_PROCESS)
   {
     service_manager::EmbeddedServiceInfo info;

diff --git a/chrome/browser/chrome_content_browser_manifest_overlay.json b/chrome/browser/chrome_content_browser_manifest_overlay.json
@@ -16,9 +16,6 @@
           "safe_browsing::mojom::SafeBrowsing",
           "translate::mojom::ContentTranslateDriver"
         ],
-        "url_keyed_metrics": [
-          "ukm::mojom::UkmRecorderInterface"
-        ],
         "gpu": [
           "metrics::mojom::CallStackProfileCollector"
         ],

diff --git a/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc b/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
@@ -10,28 +10,22 @@
 #include "base/feature_list.h"
 #include "base/strings/string_number_conversions.h"
 #include "build/build_config.h"
-#include "chrome/browser/browser_process.h"
 #include "chrome/browser/resource_coordinator/page_signal_receiver.h"
-#include "components/ukm/ukm_interface.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/common/service_manager_connection.h"
 #include "content/public/common/service_names.mojom.h"
-#include "services/metrics/public/cpp/ukm_recorder.h"
-#include "services/metrics/public/interfaces/ukm_interface.mojom.h"
+#include "services/metrics/public/cpp/ukm_source_id.h"
 #include "services/resource_coordinator/public/cpp/page_resource_coordinator.h"
 #include "services/resource_coordinator/public/cpp/process_resource_coordinator.h"
 #include "services/resource_coordinator/public/cpp/resource_coordinator_features.h"
 #include "services/resource_coordinator/public/interfaces/coordination_unit.mojom.h"
-#include "services/resource_coordinator/public/interfaces/service_callbacks.mojom.h"
 #include "services/resource_coordinator/public/interfaces/service_constants.mojom.h"
 #include "services/service_manager/public/cpp/connector.h"
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(ResourceCoordinatorWebContentsObserver);
 
-bool ResourceCoordinatorWebContentsObserver::ukm_recorder_initialized = false;
-
 ResourceCoordinatorWebContentsObserver::ResourceCoordinatorWebContentsObserver(
     content::WebContents* web_contents)
     : WebContentsObserver(web_contents) {
@@ -46,13 +40,6 @@ ResourceCoordinatorWebContentsObserver::ResourceCoordinatorWebContentsObserver(
   // |page_resource_coordinator_|.
   page_resource_coordinator_->SetVisibility(web_contents->IsVisible());
 
-  connector->BindInterface(resource_coordinator::mojom::kServiceName,
-                           mojo::MakeRequest(&service_callbacks_));
-
-  if (base::FeatureList::IsEnabled(ukm::kUkmFeature)) {
-    EnsureUkmRecorderInterface();
-  }
-
   if (auto* page_signal_receiver =
           resource_coordinator::PageSignalReceiver::GetInstance()) {
     // Gets CoordinationUnitID for this WebContents and adds it to
@@ -62,45 +49,6 @@ ResourceCoordinatorWebContentsObserver::ResourceCoordinatorWebContentsObserver(
   }
 }
 
-// TODO(matthalp) integrate into ResourceCoordinatorService once the UKM mojo
-// interface can be accessed directly within GRC. GRC cannot currently use
-// the UKM mojo inteface directly because of software layering issues
-// (i.e. the UKM mojo interface is only reachable from the content_browser
-// service which cannot be accesses by services in the services/ directory).
-// Instead the ResourceCoordinatorWebContentsObserver is used as it lives
-// within the content_browser service and can initialize the
-// UkmRecorderInterface and pass that interface into GRC through
-// resource_coordinator::mojom::ServiceCallbacks.
-void ResourceCoordinatorWebContentsObserver::EnsureUkmRecorderInterface() {
-  if (ukm_recorder_initialized) {
-    return;
-  }
-
-  service_manager::Connector* connector =
-      content::ServiceManagerConnection::GetForProcess()->GetConnector();
-  connector->BindInterface(resource_coordinator::mojom::kServiceName,
-                           mojo::MakeRequest(&service_callbacks_));
-
-  service_callbacks_->IsUkmRecorderInterfaceInitialized(base::Bind(
-      &ResourceCoordinatorWebContentsObserver::MaybeSetUkmRecorderInterface,
-      base::Unretained(this)));
-}
-
-void ResourceCoordinatorWebContentsObserver::MaybeSetUkmRecorderInterface(
-    bool ukm_recorder_already_initialized) {
-  if (ukm_recorder_already_initialized) {
-    return;
-  }
-
-  ukm::mojom::UkmRecorderInterfacePtr ukm_recorder_interface;
-  ukm::UkmInterface::Create(ukm::UkmRecorder::Get(),
-                            mojo::MakeRequest(&ukm_recorder_interface));
-
-  service_callbacks_->SetUkmRecorderInterface(
-      std::move(ukm_recorder_interface));
-  ukm_recorder_initialized = true;
-}
-
 ResourceCoordinatorWebContentsObserver::
     ~ResourceCoordinatorWebContentsObserver() = default;
 
@@ -174,10 +122,6 @@ void ResourceCoordinatorWebContentsObserver::DidUpdateFaviconURL(
 
 void ResourceCoordinatorWebContentsObserver::UpdateUkmRecorder(
     int64_t navigation_id) {
-  if (!base::FeatureList::IsEnabled(ukm::kUkmFeature)) {
-    return;
-  }
-
   ukm_source_id_ =
       ukm::ConvertToSourceId(navigation_id, ukm::SourceIdType::NAVIGATION_ID);
   page_resource_coordinator_->SetUKMSourceId(ukm_source_id_);

diff --git a/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.h b/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.h
@@ -12,7 +12,6 @@
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
-#include "services/resource_coordinator/public/interfaces/service_callbacks.mojom.h"
 #include "url/gurl.h"
 
 namespace resource_coordinator {
@@ -44,8 +43,6 @@ class ResourceCoordinatorWebContentsObserver
   void DidUpdateFaviconURL(
       const std::vector<content::FaviconURL>& candidates) override;
 
-  void EnsureUkmRecorderInterface();
-  void MaybeSetUkmRecorderInterface(bool ukm_recorder_already_initialized);
   void UpdateUkmRecorder(int64_t navigation_id);
   ukm::SourceId ukm_source_id() const { return ukm_source_id_; }
 
@@ -71,8 +68,6 @@ class ResourceCoordinatorWebContentsObserver
   bool first_time_favicon_set_ = false;
   bool first_time_title_set_ = false;
 
-  resource_coordinator::mojom::ServiceCallbacksPtr service_callbacks_;
-
   DISALLOW_COPY_AND_ASSIGN(ResourceCoordinatorWebContentsObserver);
 };
 

diff --git a/components/ukm/BUILD.gn b/components/ukm/BUILD.gn
@@ -41,25 +41,6 @@ static_library("ukm") {
   ]
 }
 
-static_library("ukm_interface") {
-  sources = [
-    "ukm_interface.cc",
-    "ukm_interface.h",
-  ]
-
-  public_deps = [
-    "//base",
-    "//services/metrics/public/cpp:metrics_cpp",
-    "//services/metrics/public/interfaces",
-  ]
-
-  deps = [
-    ":ukm",
-    "//mojo/public/cpp/bindings",
-    "//services/metrics/public/cpp:metrics_cpp",
-  ]
-}
-
 # Helper library for observing signals that we need to clear any local data.
 static_library("observers") {
   sources = [

diff --git a/components/ukm/ukm_interface.cc b/components/ukm/ukm_interface.cc
diff --git a/content/public/app/mojo/content_browser_manifest.json b/content/public/app/mojo/content_browser_manifest.json
@@ -63,7 +63,6 @@
         "*": [ "app" ],
         "cdm": [ "media:cdm" ],
         "content_browser": [
-          "url_keyed_metrics",
           "geolocation_config"
         ],
         "content_gpu": [ "browser" ],
@@ -83,6 +82,7 @@
         ],
         "file": [ "file:filesystem", "file:leveldb" ],
         "media": [ "media:media" ],
+        "metrics": [ "url_keyed_metrics" ],
         "network": [
           "network_service",
           "test",

diff --git a/content/public/app/mojo/content_renderer_manifest.json b/content/public/app/mojo/content_renderer_manifest.json
@@ -26,7 +26,8 @@
       },
       "requires": {
         "*": [ "app" ],
-        "content_browser": [ "renderer", "url_keyed_metrics" ],
+        "content_browser": [ "renderer" ],
+        "metrics": [ "url_keyed_metrics" ],
         "device": [
           "device:power_monitor",
           "device:screen_orientation",

diff --git a/services/metrics/BUILD.gn b/services/metrics/BUILD.gn
@@ -0,0 +1,27 @@
+# Copyright 2017 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.
+
+import("//services/service_manager/public/service_manifest.gni")
+
+service_manifest("manifest") {
+  name = "metrics"
+  source = "manifest.json"
+}
+
+source_set("metrics") {
+  sources = [
+    "metrics_mojo_service.cc",
+    "metrics_mojo_service.h",
+    "ukm_recorder_interface.cc",
+    "ukm_recorder_interface.h",
+  ]
+
+  deps = [
+    "//mojo/public/cpp/bindings",
+    "//services/metrics/public/cpp:metrics_cpp",
+    "//services/metrics/public/interfaces",
+    "//services/service_manager/public/cpp",
+    "//url",
+  ]
+}
diff --git a/services/metrics/OWNERS b/services/metrics/OWNERS
@@ -1,3 +1,6 @@
 file://base/metrics/OWNERS
 
+per-file manifest.json=set noparent
+per-file manifest.json=file://ipc/SECURITY_OWNERS
+
 # COMPONENT: Internals>Metrics
diff --git a/services/metrics/manifest.json b/services/metrics/manifest.json
@@ -0,0 +1,13 @@
+{
+  "name": "metrics",
+  "display_name": "Metrics Service",
+  "interface_provider_specs": {
+    "service_manager:connector": {
+      "provides": {
+        "url_keyed_metrics": [
+          "ukm::mojom::UkmRecorderInterface"
+        ]
+      }
+    }
+  }
+}