Reland "Rationalize call stacks targets in //components/metrics/BUILD.gn"

Relanding with fix for ChromeContentRendererClientMetricsTest.

Renames targets to better reflect what they provide. Removes the
:metrics dep from the target for child processes to avoid including
unnecessary browser-process-only code in those processes. Breaks the
direct dependency of CallStackProfileBuilder on :metrics by introducing
a receiver callback abstraction.

TBR=sky

Bug: 878509, 879513, 804942
Change-Id: Ib83ad55a31b98022531a8dad740c7d5516db8caf
Reviewed-on: https://chromium-review.googlesource.com/1199764Reviewed-by: Mike Wittman <wittman@chromium.org>
Commit-Queue: Mike Wittman <wittman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588114}

chrome/browser/BUILD.gn

@@ -1763,7 +1763,7 @@ jumbo_split_static_library("browser") {
     "//components/language/content/browser",
     "//components/language/core/browser",
     "//components/live_tab_count_metrics",
-    "//components/metrics:call_stacks",
+    "//components/metrics:call_stack_profile_collector",
     "//components/metrics:component_metrics",
     "//components/metrics:gpu",
     "//components/metrics:net",

chrome/browser/chrome_browser_main.cc

@@ -134,6 +134,7 @@
 #include "components/language/core/common/language_experiments.h"
 #include "components/language_usage_metrics/language_usage_metrics.h"
 #include "components/metrics/call_stack_profile_builder.h"
+#include "components/metrics/call_stack_profile_metrics_provider.h"
 #include "components/metrics/call_stack_profile_params.h"
 #include "components/metrics/expired_histogram_util.h"
 #include "components/metrics/metrics_reporting_default_state.h"
@@ -758,6 +759,14 @@ bool WaitUntilMachineLevelUserCloudPolicyEnrollmentFinished(
 #endif
 }
 
+// Sets up the ThreadProfiler for the browser process, runs it, and returns the
+// profiler.
+std::unique_ptr<ThreadProfiler> CreateAndStartBrowserMainThreadProfiler() {
+  ThreadProfiler::SetBrowserProcessReceiverCallback(base::BindRepeating(
+      &metrics::CallStackProfileMetricsProvider::ReceiveCompletedProfile));
+  return ThreadProfiler::CreateAndStartOnMainThread();
+}
+
 }  // namespace
 
 namespace chrome_browser {
@@ -785,7 +794,7 @@ ChromeBrowserMainParts::ChromeBrowserMainParts(
     : parameters_(parameters),
       parsed_command_line_(parameters.command_line),
       result_code_(service_manager::RESULT_CODE_NORMAL_EXIT),
-      ui_thread_profiler_(ThreadProfiler::CreateAndStartOnMainThread()),
+      ui_thread_profiler_(CreateAndStartBrowserMainThreadProfiler()),
       should_call_pre_main_loop_start_startup_on_variations_service_(
           !parameters.ui_task),
       profile_(NULL),

chrome/common/BUILD.gn

@@ -290,7 +290,7 @@ static_library("common") {
   ]
 
   deps = [
-    "//components/metrics:call_stack_profile",
+    "//components/metrics:call_stack_profile_builder",
   ]
 
   if (enable_plugins) {

chrome/common/thread_profiler.cc

@@ -146,6 +146,13 @@ void ThreadProfiler::StartOnChildThread(CallStackProfileParams::Thread thread) {
   g_child_thread_profiler_sequence_local_storage.Get().Set(std::move(profiler));
 }
 
+// static
+void ThreadProfiler::SetBrowserProcessReceiverCallback(
+    const base::RepeatingCallback<void(base::TimeTicks,
+                                       metrics::SampledProfile)>& callback) {
+  metrics::CallStackProfileBuilder::SetBrowserProcessReceiverCallback(callback);
+}
+
 // static
 void ThreadProfiler::SetServiceManagerConnectorForChildProcess(
     service_manager::Connector* connector) {

chrome/common/thread_profiler.h

@@ -72,6 +72,13 @@ class ThreadProfiler {
   static void StartOnChildThread(
       metrics::CallStackProfileParams::Thread thread);
 
+  // Sets the callback to use for reporting browser process profiles. This
+  // indirection is required to avoid a dependency on unnecessary metrics code
+  // in child processes.
+  static void SetBrowserProcessReceiverCallback(
+      const base::RepeatingCallback<void(base::TimeTicks,
+                                         metrics::SampledProfile)>& callback);
+
   // This function must be called within child processes to supply the Service
   // Manager's connector, to bind the interface through which a profile is sent
   // back to the browser process.

chrome/gpu/BUILD.gn

@@ -6,7 +6,7 @@ import("//media/media_options.gni")
 
 static_library("gpu") {
   deps = [
-    "//components/metrics:child_call_stacks",
+    "//components/metrics:child_call_stack_profile_builder",
     "//content/public/common",
     "//content/public/gpu",
   ]

chrome/renderer/chrome_content_renderer_client_unittest.cc

@@ -7,10 +7,12 @@
 #include <stddef.h>
 
 #include <map>
+#include <memory>
 #include <string>
 #include <utility>
 #include <vector>
 
+#include "base/command_line.h"
 #include "base/metrics/histogram_samples.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
@@ -18,6 +20,7 @@
 #include "build/build_config.h"
 #include "chrome/renderer/searchbox/search_bouncer.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h"
+#include "content/public/common/content_switches.h"
 #include "content/public/common/webplugininfo.h"
 #include "extensions/buildflags/buildflags.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -80,8 +83,15 @@ void AddContentTypeHandler(content::WebPluginInfo* info,
 
 }  // namespace
 
-typedef testing::Test ChromeContentRendererClientTest;
-
+class ChromeContentRendererClientTest : public testing::Test {
+ public:
+  void SetUp() override {
+    // Ensure that this looks like the renderer process based on the command
+    // line.
+    base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+        switches::kProcessType, switches::kRendererProcess);
+  }
+};
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
 scoped_refptr<const extensions::Extension> CreateTestExtension(
@@ -462,17 +472,25 @@ class ChromeContentRendererClientMetricsTest : public testing::Test {
  public:
   ChromeContentRendererClientMetricsTest() = default;
 
+  void SetUp() override {
+    // Ensure that this looks like the renderer process based on the command
+    // line.
+    base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+        switches::kProcessType, switches::kRendererProcess);
+    client_ = std::make_unique<ChromeContentRendererClient>();
+  }
+
   std::unique_ptr<base::HistogramSamples> GetHistogramSamples() {
     return histogram_tester_.GetHistogramSamplesSinceCreation(
         internal::kFlashYouTubeRewriteUMA);
   }
 
   void OverrideFlashEmbed(const GURL& gurl) {
-    client_.OverrideFlashEmbedWithHTML(gurl);
+    client_->OverrideFlashEmbedWithHTML(gurl);
   }
 
  private:
-  ChromeContentRendererClient client_;
+  std::unique_ptr<ChromeContentRendererClient> client_;
   base::HistogramTester histogram_tester_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeContentRendererClientMetricsTest);

components/metrics/BUILD.gn

@@ -105,7 +105,6 @@ static_library("metrics") {
   ]
 
   public_deps = [
-    ":child_call_stacks",
     "//third_party/metrics_proto",
   ]
 
@@ -257,43 +256,66 @@ static_library("single_sample_metrics") {
   ]
 }
 
-source_set("call_stack_profile") {
-  sources = [
-    "call_stack_profile_builder.cc",
-    "call_stack_profile_builder.h",
-    "call_stack_profile_encoding.cc",
+source_set("call_stack_profile_params") {
+  public = [
     "call_stack_profile_encoding.h",
     "call_stack_profile_params.h",
   ]
+  sources = [
+    "call_stack_profile_encoding.cc",
+  ]
 
   deps = [
-    ":metrics",
     "//base:base",
     "//third_party/metrics_proto",
   ]
 }
 
-source_set("call_stacks") {
+# Dependency for child processes that use the CallStackProfileBuilder.
+source_set("child_call_stack_profile_builder") {
+  public = [
+    "call_stack_profile_builder.h",
+    "child_call_stack_profile_collector.h",
+  ]
   sources = [
-    "call_stack_profile_collector.cc",
-    "call_stack_profile_collector.h",
+    "call_stack_profile_builder.cc",
+    "child_call_stack_profile_collector.cc",
+  ]
+  public_deps = [
+    ":call_stack_profile_params",
   ]
   deps = [
-    ":call_stack_profile",
-    ":metrics",
+    "//base",
     "//components/metrics/public/interfaces:call_stack_mojo_bindings",
+    "//third_party/metrics_proto",
   ]
+
+  # This target must not depend on :metrics because that code is intended solely
+  # for use in the browser process.
+  assert_no_deps = [ ":metrics" ]
 }
 
-source_set("child_call_stacks") {
+# Dependency for browser process use of the CallStackProfileBuilder.
+source_set("call_stack_profile_builder") {
+  deps = [
+    ":metrics",
+  ]
+  public_deps = [
+    ":child_call_stack_profile_builder",
+  ]
+}
+
+# The browser process mojo service for collecting profiles from child
+# processes.
+source_set("call_stack_profile_collector") {
   sources = [
-    "child_call_stack_profile_collector.cc",
-    "child_call_stack_profile_collector.h",
+    "call_stack_profile_collector.cc",
+    "call_stack_profile_collector.h",
   ]
   deps = [
+    ":call_stack_profile_params",
+    ":metrics",
     "//components/metrics/public/interfaces:call_stack_mojo_bindings",
-    "//services/service_manager/public/cpp",
-    "//third_party/metrics_proto",
   ]
 }
 
@@ -367,8 +389,7 @@ source_set("unit_tests") {
   ]
 
   deps = [
-    ":call_stack_profile",
-    ":child_call_stacks",
+    ":call_stack_profile_builder",
     ":component_metrics",
     ":metrics",
     ":net",

components/metrics/call_stack_profile_builder.cc

@@ -12,9 +12,9 @@
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/metrics/metrics_hashes.h"
+#include "base/no_destructor.h"
 #include "base/stl_util.h"
 #include "components/metrics/call_stack_profile_encoding.h"
-#include "components/metrics/call_stack_profile_metrics_provider.h"
 
 namespace metrics {
 
@@ -24,6 +24,14 @@ namespace {
 base::LazyInstance<ChildCallStackProfileCollector>::Leaky
     g_child_call_stack_profile_collector = LAZY_INSTANCE_INITIALIZER;
 
+base::RepeatingCallback<void(base::TimeTicks, SampledProfile)>&
+GetBrowserProcessReceiverCallbackInstance() {
+  static base::NoDestructor<
+      base::RepeatingCallback<void(base::TimeTicks, SampledProfile)>>
+      instance;
+  return *instance;
+}
+
 // Identifies an unknown module.
 const size_t kUnknownModuleIndex = static_cast<size_t>(-1);
 
@@ -266,11 +274,18 @@ void CallStackProfileBuilder::OnProfileCompleted(
     std::move(completed_callback_).Run();
 }
 
+// static
+void CallStackProfileBuilder::SetBrowserProcessReceiverCallback(
+    const base::RepeatingCallback<void(base::TimeTicks, SampledProfile)>&
+        callback) {
+  GetBrowserProcessReceiverCallbackInstance() = callback;
+}
+
 void CallStackProfileBuilder::PassProfilesToMetricsProvider(
     SampledProfile sampled_profile) {
   if (profile_params_.process == CallStackProfileParams::BROWSER_PROCESS) {
-    CallStackProfileMetricsProvider::ReceiveCompletedProfile(
-        profile_start_time_, std::move(sampled_profile));
+    GetBrowserProcessReceiverCallbackInstance().Run(profile_start_time_,
+                                                    std::move(sampled_profile));
   } else {
     g_child_call_stack_profile_collector.Get()
         .ChildCallStackProfileCollector::Collect(profile_start_time_,

components/metrics/call_stack_profile_builder.h

@@ -18,6 +18,8 @@
 
 namespace metrics {
 
+class SampledProfile;
+
 // An instance of the class is meant to be passed to base::StackSamplingProfiler
 // to collect profiles. The profiles collected are uploaded via the metrics log.
 class CallStackProfileBuilder
@@ -96,6 +98,13 @@ class CallStackProfileBuilder
   void OnProfileCompleted(base::TimeDelta profile_duration,
                           base::TimeDelta sampling_period) override;
 
+  // Sets the callback to use for reporting browser process profiles. This
+  // indirection is required to avoid a dependency on unnecessary metrics code
+  // in child processes.
+  static void SetBrowserProcessReceiverCallback(
+      const base::RepeatingCallback<void(base::TimeTicks, SampledProfile)>&
+          callback);
+
   // Sets the current system state that is recorded with each captured stack
   // frame. This is thread-safe so can be called from anywhere. The parameter
   // value should be from an enumeration of the appropriate type with values

components/startup_metric_utils/browser/BUILD.gn

@@ -25,7 +25,7 @@ static_library("lib") {
 
   deps = [
     "//base",
-    "//components/metrics:call_stack_profile",
+    "//components/metrics:call_stack_profile_builder",
     "//components/metrics:metrics",
     "//components/prefs",
     "//components/version_info",