chromeos: removes time_to_first_present.mojom

And instead has ash directly access the startup time.

BUG=958213
TEST=covered by tests
TBR=tsepez@chromium.org

Change-Id: I146c27edef79ff8fb69ca7cdd7f230ad77f040a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1602854Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658315}

ash/BUILD.gn

@@ -1369,6 +1369,7 @@ component("ash") {
     "//components/prefs",
     "//components/quirks",
     "//components/session_manager:base",
+    "//components/startup_metric_utils/browser:lib",
     "//components/strings",
     "//components/sync",
     "//components/user_manager",

ash/DEPS

@@ -13,6 +13,7 @@ include_rules = [
   "+components/prefs",
   "+components/quirks",
   "+components/session_manager",
+  "+components/startup_metric_utils",
   "+components/strings",
   "+components/sync",
   "+components/ui_devtools",

ash/metrics/time_to_first_present_recorder.cc

@@ -8,6 +8,7 @@
 #include "base/bind_helpers.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/time/time.h"
+#include "components/startup_metric_utils/browser/startup_metric_utils.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_tree_host.h"
 #include "ui/compositor/compositor.h"
@@ -15,6 +16,10 @@
 
 namespace ash {
 
+// static
+const char TimeToFirstPresentRecorder::kMetricName[] =
+    "Ash.ProcessCreationToFirstPresent";
+
 TimeToFirstPresentRecorder::TimeToFirstPresentRecorder(aura::Window* window) {
   aura::WindowTreeHost* window_tree_host = window->GetHost();
   DCHECK(window_tree_host);
@@ -25,42 +30,13 @@ TimeToFirstPresentRecorder::TimeToFirstPresentRecorder(aura::Window* window) {
 
 TimeToFirstPresentRecorder::~TimeToFirstPresentRecorder() = default;
 
-void TimeToFirstPresentRecorder::Bind(
-    mojom::ProcessCreationTimeRecorderRequest request) {
-  // Process createion time should only be set once.
-  if (binding_.is_bound() || !process_creation_time_.is_null())
-    return;
-
-  binding_.Bind(std::move(request));
-}
-
-void TimeToFirstPresentRecorder::SetMainProcessCreationTime(
-    base::TimeTicks start_time) {
-  if (!process_creation_time_.is_null())
-    return;
-
-  process_creation_time_ = start_time;
-  LogTime();
-
-  // Process creation time should be set only once.
-  binding_.Close();
-}
-
-void TimeToFirstPresentRecorder::LogTime() {
-  if (present_time_.is_null() || process_creation_time_.is_null())
-    return;
-
-  UMA_HISTOGRAM_TIMES("Ash.ProcessCreationToFirstPresent",
-                      time_to_first_present());
-  if (log_callback_)
-    std::move(log_callback_).Run();
-}
-
 void TimeToFirstPresentRecorder::DidPresentCompositorFrame(
     const gfx::PresentationFeedback& feedback) {
-  DCHECK(present_time_.is_null());  // This should only be called once.
-  present_time_ = feedback.timestamp;
-  LogTime();
+  const base::TimeDelta time_to_first_present =
+      feedback.timestamp - startup_metric_utils::MainEntryPointTicks();
+  UMA_HISTOGRAM_TIMES(kMetricName, time_to_first_present);
+  if (log_callback_)
+    std::move(log_callback_).Run();
 }
 
 }  // namespace ash

ash/metrics/time_to_first_present_recorder.h

@@ -7,11 +7,9 @@
 
 #include <stdint.h>
 
-#include "ash/public/interfaces/process_creation_time_recorder.mojom.h"
+#include "ash/ash_export.h"
 #include "base/callback.h"
 #include "base/macros.h"
-#include "base/time/time.h"
-#include "mojo/public/cpp/bindings/binding.h"
 
 namespace aura {
 class Window;
@@ -26,41 +24,26 @@ namespace ash {
 class TimeToFirstPresentRecorderTestApi;
 
 // Used for tracking the time main started to the time the first bits make it
-// the screen and logging a histogram of the time. Chrome is responsible for
-// providing the start time by way of ProcessCreationTimeRecorder.
+// the screen and logging a histogram of the time.
 //
 // This only logs the time to present the primary root window.
-class TimeToFirstPresentRecorder : public mojom::ProcessCreationTimeRecorder {
+class ASH_EXPORT TimeToFirstPresentRecorder {
  public:
-  explicit TimeToFirstPresentRecorder(aura::Window* window);
-  ~TimeToFirstPresentRecorder() override;
+  // The name of the histogram the time is logged against.
+  static const char kMetricName[];
 
-  void Bind(mojom::ProcessCreationTimeRecorderRequest request);
+  explicit TimeToFirstPresentRecorder(aura::Window* window);
+  ~TimeToFirstPresentRecorder();
 
  private:
   friend class TimeToFirstPresentRecorderTestApi;
 
-  // If both times are available the time to present is logged.
-  void LogTime();
-
   // Callback from the compositor when it presented a valid frame.
   void DidPresentCompositorFrame(const gfx::PresentationFeedback& feedback);
 
-  base::TimeDelta time_to_first_present() const {
-    return present_time_ - process_creation_time_;
-  }
-
-  // mojom::ProcessCreationTimeRecorder:
-  void SetMainProcessCreationTime(base::TimeTicks start_time) override;
-
-  base::TimeTicks process_creation_time_;
-  base::TimeTicks present_time_;
-
-  // Only used by tests. If valid it's Run() when both times are determined.
+  // Only used by tests. If valid it's Run() when the time is recorded.
   base::OnceClosure log_callback_;
 
-  mojo::Binding<mojom::ProcessCreationTimeRecorder> binding_{this};
-
   DISALLOW_COPY_AND_ASSIGN(TimeToFirstPresentRecorder);
 };
 

ash/metrics/time_to_first_present_recorder_test_api.cc

@@ -7,52 +7,14 @@
 #include "ash/metrics/time_to_first_present_recorder.h"
 #include "ash/shell.h"
 #include "base/bind.h"
-#include "base/bind_helpers.h"
-#include "mojo/public/cpp/bindings/strong_binding.h"
 
 namespace ash {
 
-TimeToFirstPresentRecorderTestApi::TimeToFirstPresentRecorderTestApi() =
-    default;
-
-TimeToFirstPresentRecorderTestApi::~TimeToFirstPresentRecorderTestApi() =
-    default;
-
 // static
-void TimeToFirstPresentRecorderTestApi::BindRequest(
-    mojom::TimeToFirstPresentRecorderTestApiRequest request) {
-  mojo::MakeStrongBinding(std::make_unique<TimeToFirstPresentRecorderTestApi>(),
-                          std::move(request));
-}
-
-void TimeToFirstPresentRecorderTestApi::GetProcessCreationToFirstPresentTime(
-    GetProcessCreationToFirstPresentTimeCallback callback) {
-  TimeToFirstPresentRecorder* recorder =
-      Shell::Get()->time_to_first_present_recorder();
-  if (recorder->process_creation_time_.is_null() ||
-      recorder->present_time_.is_null()) {
-    // Still waiting for time. Schedule a callback with
-    // TimeToFirstPresentRecorder. This only supports one callback at a time,
-    // which should be fine for tests.
-    DCHECK(recorder->log_callback_.is_null());
-    recorder->log_callback_ = base::BindOnce(
-        &TimeToFirstPresentRecorderTestApi::OnLog, base::Unretained(this));
-    DCHECK(!get_creation_time_callback_);
-    get_creation_time_callback_ = std::move(callback);
-    return;
-  }
-  std::move(callback).Run(recorder->time_to_first_present());
-}
-
-void TimeToFirstPresentRecorderTestApi::OnLog() {
-  TimeToFirstPresentRecorder* recorder =
-      Shell::Get()->time_to_first_present_recorder();
-  DCHECK(!recorder->process_creation_time_.is_null() &&
-         !recorder->present_time_.is_null());
-  std::move(get_creation_time_callback_)
-      .Run(Shell::Get()
-               ->time_to_first_present_recorder()
-               ->time_to_first_present());
+void TimeToFirstPresentRecorderTestApi::SetTimeToFirstPresentCallback(
+    base::OnceClosure callback) {
+  Shell::Get()->time_to_first_present_recorder()->log_callback_ =
+      std::move(callback);
 }
 
 }  // namespace ash

ash/metrics/time_to_first_present_recorder_test_api.h

@@ -5,33 +5,17 @@
 #ifndef ASH_METRICS_TIME_TO_FIRST_PRESENT_RECORDER_TEST_API_H_
 #define ASH_METRICS_TIME_TO_FIRST_PRESENT_RECORDER_TEST_API_H_
 
-#include "ash/public/interfaces/time_to_first_present_recorder_test_api.test-mojom.h"
+#include "base/callback_forward.h"
 #include "base/macros.h"
 
 namespace ash {
 
-class TimeToFirstPresentRecorderTestApi
-    : public mojom::TimeToFirstPresentRecorderTestApi {
+class TimeToFirstPresentRecorderTestApi {
  public:
-  TimeToFirstPresentRecorderTestApi();
-  ~TimeToFirstPresentRecorderTestApi() override;
-
-  // Creates and binds an instance from a remote request (e.g. from chrome).
-  static void BindRequest(
-      mojom::TimeToFirstPresentRecorderTestApiRequest request);
-
-  // mojom::TimeToFirstPresentRecorderTestApi:
-  void GetProcessCreationToFirstPresentTime(
-      GetProcessCreationToFirstPresentTimeCallback callback) override;
+  static void SetTimeToFirstPresentCallback(base::OnceClosure callback);
 
  private:
-  void OnLog();
-
-  // If valid GetProcessCreationToFirstPresentTimeCallback() was called and
-  // we're waiting for TimeToFirstPresentRecorder to see the first log.
-  GetProcessCreationToFirstPresentTimeCallback get_creation_time_callback_;
-
-  DISALLOW_COPY_AND_ASSIGN(TimeToFirstPresentRecorderTestApi);
+  DISALLOW_IMPLICIT_CONSTRUCTORS(TimeToFirstPresentRecorderTestApi);
 };
 
 }  // namespace ash

ash/mojo_interface_factory.cc

@@ -210,11 +210,6 @@ void BindNoteTakingControllerRequestOnMainThread(
   Shell::Get()->note_taking_controller()->BindRequest(std::move(request));
 }
 
-void BindProcessCreationTimeRecorderOnMainThread(
-    mojom::ProcessCreationTimeRecorderRequest request) {
-  Shell::Get()->time_to_first_present_recorder()->Bind(std::move(request));
-}
-
 void BindShelfRequestOnMainThread(mojom::ShelfControllerRequest request) {
   Shell::Get()->shelf_controller()->BindRequest(std::move(request));
 }
@@ -352,9 +347,6 @@ void RegisterInterfaces(
   registry->AddInterface(
       base::BindRepeating(&BindNoteTakingControllerRequestOnMainThread),
       main_thread_task_runner);
-  registry->AddInterface(
-      base::BindRepeating(&BindProcessCreationTimeRecorderOnMainThread),
-      main_thread_task_runner);
   registry->AddInterface(base::BindRepeating(&BindShelfRequestOnMainThread),
                          main_thread_task_runner);
   registry->AddInterface(

ash/mojo_test_interface_factory.cc

@@ -13,7 +13,6 @@
 #include "ash/public/interfaces/shell_test_api.test-mojom.h"
 #include "ash/public/interfaces/status_area_widget_test_api.test-mojom.h"
 #include "ash/public/interfaces/system_tray_test_api.test-mojom.h"
-#include "ash/public/interfaces/time_to_first_present_recorder_test_api.test-mojom.h"
 #include "ash/shelf/shelf_test_api.h"
 #include "ash/shell_test_api.h"
 #include "ash/system/status_area_widget_test_api.h"
@@ -51,11 +50,6 @@ void BindSystemTrayTestApiOnMainThread(
   UnifiedSystemTrayTestApi::BindRequest(std::move(request));
 }
 
-void BindTimeToFirstPresentRecorderTestApiOnMainThread(
-    mojom::TimeToFirstPresentRecorderTestApiRequest request) {
-  TimeToFirstPresentRecorderTestApi::BindRequest(std::move(request));
-}
-
 }  // namespace
 
 void RegisterInterfaces(
@@ -71,9 +65,6 @@ void RegisterInterfaces(
                          main_thread_task_runner);
   registry->AddInterface(base::Bind(&BindSystemTrayTestApiOnMainThread),
                          main_thread_task_runner);
-  registry->AddInterface(
-      base::Bind(&BindTimeToFirstPresentRecorderTestApiOnMainThread),
-      main_thread_task_runner);
 }
 
 }  // namespace mojo_test_interface_factory

ash/public/cpp/manifest.cc

@@ -29,7 +29,6 @@
 #include "ash/public/interfaces/night_light_controller.mojom.h"
 #include "ash/public/interfaces/note_taking_controller.mojom.h"
 #include "ash/public/interfaces/pref_connector.mojom.h"
-#include "ash/public/interfaces/process_creation_time_recorder.mojom.h"
 #include "ash/public/interfaces/shelf.mojom.h"
 #include "ash/public/interfaces/shelf_integration_test_api.mojom.h"
 #include "ash/public/interfaces/shutdown.mojom.h"
@@ -91,8 +90,7 @@ const service_manager::Manifest& GetManifest() {
                   mojom::KeyboardController, mojom::LocaleUpdateController,
                   mojom::LoginScreen, mojom::MediaController,
                   mojom::NewWindowController, mojom::NightLightController,
-                  mojom::NoteTakingController,
-                  mojom::ProcessCreationTimeRecorder, mojom::ShelfController,
+                  mojom::NoteTakingController, mojom::ShelfController,
                   mojom::ShutdownController, mojom::SystemTray,
                   mojom::TabletModeController, mojom::TrayAction,
                   mojom::VoiceInteractionController, mojom::VpnList,

ash/public/cpp/test_manifest.cc

@@ -9,7 +9,6 @@
 #include "ash/public/interfaces/shell_test_api.test-mojom.h"
 #include "ash/public/interfaces/status_area_widget_test_api.test-mojom.h"
 #include "ash/public/interfaces/system_tray_test_api.test-mojom.h"
-#include "ash/public/interfaces/time_to_first_present_recorder_test_api.test-mojom.h"
 #include "base/no_destructor.h"
 #include "services/service_manager/public/cpp/manifest_builder.h"
 
@@ -22,8 +21,7 @@ const service_manager::Manifest& GetManifestOverlayForTesting() {
               "test", service_manager::Manifest::InterfaceList<
                           mojom::LoginScreenTestApi, mojom::ShelfTestApi,
                           mojom::ShellTestApi, mojom::StatusAreaWidgetTestApi,
-                          mojom::SystemTrayTestApi,
-                          mojom::TimeToFirstPresentRecorderTestApi>())
+                          mojom::SystemTrayTestApi>())
           .Build()};
   return *manifest;
 }

ash/public/interfaces/BUILD.gn

@@ -50,7 +50,6 @@ mojom("interfaces_internal") {
     "night_light_controller.mojom",
     "note_taking_controller.mojom",
     "pref_connector.mojom",
-    "process_creation_time_recorder.mojom",
     "shelf.mojom",
     "shelf_integration_test_api.mojom",
     "shutdown.mojom",
@@ -105,7 +104,6 @@ mojom("test_interfaces") {
     "shell_test_api.test-mojom",
     "status_area_widget_test_api.test-mojom",
     "system_tray_test_api.test-mojom",
-    "time_to_first_present_recorder_test_api.test-mojom",
   ]
   deps = [
     "//ash/public/interfaces:interfaces_internal",

ash/public/interfaces/process_creation_time_recorder.mojom

-// 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.
-
-module ash.mojom;
-
-import "mojo/public/mojom/base/time.mojom";
-
-interface ProcessCreationTimeRecorder {
-  // Sets the time the main process was created at, used for logging metrics.
-  SetMainProcessCreationTime(mojo_base.mojom.TimeTicks start_time);
-};

ash/public/interfaces/time_to_first_present_recorder_test_api.test-mojom

-// 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.
-
-module ash.mojom;
-
-import "mojo/public/mojom/base/time.mojom";
-
-// Used to test internal state of TimeToFirstPresentRecorder.
-interface TimeToFirstPresentRecorderTestApi {
-  // Returns the time between process creation and the first pixels shown on
-  // screen.
-  GetProcessCreationToFirstPresentTime() => (mojo_base.mojom.TimeDelta delta);
-};

chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc

@@ -11,7 +11,6 @@
 #include "ash/public/cpp/shelf_model.h"
 #include "ash/public/cpp/window_properties.h"
 #include "ash/public/interfaces/constants.mojom.h"
-#include "ash/public/interfaces/process_creation_time_recorder.mojom.h"
 #include "ash/shell.h"
 #include "base/command_line.h"
 #include "base/task/post_task.h"
@@ -74,20 +73,6 @@
 #include "chrome/browser/exo_parts.h"
 #endif
 
-namespace {
-
-void PushProcessCreationTimeToAsh() {
-  ash::mojom::ProcessCreationTimeRecorderPtr recorder;
-  content::ServiceManagerConnection::GetForProcess()
-      ->GetConnector()
-      ->BindInterface(ash::mojom::kServiceName, &recorder);
-  DCHECK(!startup_metric_utils::MainEntryPointTicks().is_null());
-  recorder->SetMainProcessCreationTime(
-      startup_metric_utils::MainEntryPointTicks());
-}
-
-}  // namespace
-
 namespace internal {
 
 // Creates a ChromeLauncherController on the first active session notification.
@@ -204,8 +189,6 @@ void ChromeBrowserMainExtraPartsAsh::PreProfileInit() {
 #if BUILDFLAG(ENABLE_WAYLAND_SERVER)
   exo_parts_ = ExoParts::CreateIfNecessary();
 #endif
-
-  PushProcessCreationTimeToAsh();
 }
 
 void ChromeBrowserMainExtraPartsAsh::PostProfileInit() {

chrome/browser/ui/ash/time_to_first_present_recorder_browsertest.cc

@@ -2,24 +2,30 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "ash/public/interfaces/constants.mojom.h"
-#include "ash/public/interfaces/time_to_first_present_recorder_test_api.test-mojom-test-utils.h"
-#include "ash/public/interfaces/time_to_first_present_recorder_test_api.test-mojom.h"
+#include "ash/metrics/time_to_first_present_recorder.h"
+
+#include "ash/metrics/time_to_first_present_recorder_test_api.h"
 #include "base/command_line.h"
+#include "base/run_loop.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "chrome/test/base/in_process_browser_test.h"
-#include "content/public/common/service_manager_connection.h"
-#include "services/service_manager/public/cpp/connector.h"
 
 using TimeToFirstPresentRecorderTest = InProcessBrowserTest;
 
 IN_PROC_BROWSER_TEST_F(TimeToFirstPresentRecorderTest, VerifyTimeCalculated) {
-  ash::mojom::TimeToFirstPresentRecorderTestApiPtr recorder_test_api;
-  content::ServiceManagerConnection::GetForProcess()
-      ->GetConnector()
-      ->BindInterface(ash::mojom::kServiceName, &recorder_test_api);
-  ash::mojom::TimeToFirstPresentRecorderTestApiAsyncWaiter recorder(
-      recorder_test_api.get());
-  base::TimeDelta time_delta;
-  recorder.GetProcessCreationToFirstPresentTime(&time_delta);
-  EXPECT_FALSE(time_delta.is_zero());
+  // It's possible that the metric was already recorded.
+  base::HistogramTester tester;
+  auto counts = tester.GetTotalCountsForPrefix(
+      ash::TimeToFirstPresentRecorder::kMetricName);
+  if (counts[ash::TimeToFirstPresentRecorder::kMetricName] == 1)
+    return;
+
+  // The metric wasn't recorded. Wait for it to be recorded.
+  base::RunLoop run_loop;
+  ash::TimeToFirstPresentRecorderTestApi::SetTimeToFirstPresentCallback(
+      run_loop.QuitClosure());
+  run_loop.Run();
+  counts = tester.GetTotalCountsForPrefix(
+      ash::TimeToFirstPresentRecorder::kMetricName);
+  EXPECT_EQ(1, counts[ash::TimeToFirstPresentRecorder::kMetricName]);
 }