Revert "chromeos: removes time_to_first_present.mojom"

This reverts commit f9d8f7f6.

Reason for revert: Apologies, this test seems to be timing out:
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8913896950441933072/+/steps/browser_tests/0/logs/Deterministic_failure:_TimeToFirstPresentRecorderTest.VerifyTimeCalculated__status_TIMEOUT_/0

Original change's description:
> 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/+/1602854
> Reviewed-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}

TBR=jamescook@chromium.org,sky@chromium.org,tsepez@chromium.org

Change-Id: I272066f1adc2866821a5195dca1907a6f077069a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 958213
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1605255Reviewed-by: Alice Boxhall <aboxhall@chromium.org>
Commit-Queue: Alice Boxhall <aboxhall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658455}

ash/BUILD.gn

@@ -1361,7 +1361,6 @@ 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,7 +13,6 @@ 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,7 +8,6 @@
 #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"
@@ -16,10 +15,6 @@
 
 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);
@@ -30,13 +25,42 @@ TimeToFirstPresentRecorder::TimeToFirstPresentRecorder(aura::Window* window) {
 
 TimeToFirstPresentRecorder::~TimeToFirstPresentRecorder() = default;
 
-void TimeToFirstPresentRecorder::DidPresentCompositorFrame(
-    const gfx::PresentationFeedback& feedback) {
-  const base::TimeDelta time_to_first_present =
-      feedback.timestamp - startup_metric_utils::MainEntryPointTicks();
-  UMA_HISTOGRAM_TIMES(kMetricName, time_to_first_present);
+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();
+}
+
 }  // namespace ash

ash/metrics/time_to_first_present_recorder.h

@@ -7,9 +7,11 @@
 
 #include <stdint.h>
 
-#include "ash/ash_export.h"
+#include "ash/public/interfaces/process_creation_time_recorder.mojom.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;
@@ -24,26 +26,41 @@ 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.
+// the screen and logging a histogram of the time. Chrome is responsible for
+// providing the start time by way of ProcessCreationTimeRecorder.
 //
 // This only logs the time to present the primary root window.
-class ASH_EXPORT TimeToFirstPresentRecorder {
+class TimeToFirstPresentRecorder : public mojom::ProcessCreationTimeRecorder {
  public:
-  // The name of the histogram the time is logged against.
-  static const char kMetricName[];
-
   explicit TimeToFirstPresentRecorder(aura::Window* window);
-  ~TimeToFirstPresentRecorder();
+  ~TimeToFirstPresentRecorder() override;
+
+  void Bind(mojom::ProcessCreationTimeRecorderRequest request);
 
  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);
 
-  // Only used by tests. If valid it's Run() when the time is recorded.
+  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.
   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,14 +7,52 @@
 #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::SetTimeToFirstPresentCallback(
-    base::OnceClosure callback) {
-  Shell::Get()->time_to_first_present_recorder()->log_callback_ =
-      std::move(callback);
+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());
 }
 
 }  // namespace ash

ash/metrics/time_to_first_present_recorder_test_api.h

@@ -5,17 +5,33 @@
 #ifndef ASH_METRICS_TIME_TO_FIRST_PRESENT_RECORDER_TEST_API_H_
 #define ASH_METRICS_TIME_TO_FIRST_PRESENT_RECORDER_TEST_API_H_
 
-#include "base/callback_forward.h"
+#include "ash/public/interfaces/time_to_first_present_recorder_test_api.test-mojom.h"
 #include "base/macros.h"
 
 namespace ash {
 
-class TimeToFirstPresentRecorderTestApi {
+class TimeToFirstPresentRecorderTestApi
+    : public mojom::TimeToFirstPresentRecorderTestApi {
  public:
-  static void SetTimeToFirstPresentCallback(base::OnceClosure callback);
+  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;
 
  private:
-  DISALLOW_IMPLICIT_CONSTRUCTORS(TimeToFirstPresentRecorderTestApi);
+  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);
 };
 
 }  // namespace ash

ash/mojo_interface_factory.cc

@@ -210,6 +210,11 @@ 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));
 }
@@ -347,6 +352,9 @@ 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,6 +13,7 @@
 #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"
@@ -50,6 +51,11 @@ void BindSystemTrayTestApiOnMainThread(
   UnifiedSystemTrayTestApi::BindRequest(std::move(request));
 }
 
+void BindTimeToFirstPresentRecorderTestApiOnMainThread(
+    mojom::TimeToFirstPresentRecorderTestApiRequest request) {
+  TimeToFirstPresentRecorderTestApi::BindRequest(std::move(request));
+}
+
 }  // namespace
 
 void RegisterInterfaces(
@@ -65,6 +71,9 @@ 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,6 +29,7 @@
 #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"
@@ -90,7 +91,8 @@ const service_manager::Manifest& GetManifest() {
                   mojom::KeyboardController, mojom::LocaleUpdateController,
                   mojom::LoginScreen, mojom::MediaController,
                   mojom::NewWindowController, mojom::NightLightController,
-                  mojom::NoteTakingController, mojom::ShelfController,
+                  mojom::NoteTakingController,
+                  mojom::ProcessCreationTimeRecorder, mojom::ShelfController,
                   mojom::ShutdownController, mojom::SystemTray,
                   mojom::TabletModeController, mojom::TrayAction,
                   mojom::VoiceInteractionController, mojom::VpnList,

ash/public/cpp/test_manifest.cc

@@ -9,6 +9,7 @@
 #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"
 
@@ -21,7 +22,8 @@ const service_manager::Manifest& GetManifestOverlayForTesting() {
               "test", service_manager::Manifest::InterfaceList<
                           mojom::LoginScreenTestApi, mojom::ShelfTestApi,
                           mojom::ShellTestApi, mojom::StatusAreaWidgetTestApi,
-                          mojom::SystemTrayTestApi>())
+                          mojom::SystemTrayTestApi,
+                          mojom::TimeToFirstPresentRecorderTestApi>())
           .Build()};
   return *manifest;
 }

ash/public/interfaces/BUILD.gn

@@ -50,6 +50,7 @@ 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",
@@ -103,6 +104,7 @@ 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,6 +11,7 @@
 #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"
@@ -73,6 +74,20 @@
 #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.
@@ -189,6 +204,8 @@ 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,30 +2,24 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "ash/metrics/time_to_first_present_recorder.h"
-
-#include "ash/metrics/time_to_first_present_recorder_test_api.h"
+#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 "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) {
-  // 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]);
+  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());
 }