Record time to click metrics in UnifiedSystemTray.

ChromeOS.SystemTray.TimeToClick metrics for UnifiedSystemTray was
recorded in old SystemTray class. Reference from UnifiedSystemTray
to SystemTray will be removed in the future, so this is not preferable.

This CL adds Delegate to TimeToClickRecorder to fix this.

TEST=manual(chrome://histograms)
BUG=834462

Change-Id: I249ae40bb471d833bcc24242ade9efc778e295df
Reviewed-on: https://chromium-review.googlesource.com/1029565
Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556371}

ash/BUILD.gn

@@ -794,6 +794,8 @@ component("ash") {
     "system/tray/system_tray_notifier.h",
     "system/tray/system_tray_view.cc",
     "system/tray/system_tray_view.h",
+    "system/tray/time_to_click_recorder.cc",
+    "system/tray/time_to_click_recorder.h",
     "system/tray/tray_background_view.cc",
     "system/tray/tray_background_view.h",
     "system/tray/tray_bubble_wrapper.cc",

ash/system/tray/system_tray.cc

@@ -620,13 +620,13 @@ bool SystemTray::PerformAction(const ui::Event& event) {
   UserMetricsRecorder::RecordUserClickOnTray(
       LoginMetricsRecorder::TrayClickTarget::kSystemTray);
 
-  last_button_clicked_ = base::TimeTicks::Now();
-
   if (features::IsSystemTrayUnifiedEnabled()) {
     return shelf()->GetStatusAreaWidget()->unified_system_tray()->PerformAction(
         event);
   }
 
+  last_button_clicked_ = base::TimeTicks::Now();
+
   // If we're already showing a full system tray menu, either default or
   // detailed menu, hide it; otherwise, show it (and hide any popup that's
   // currently shown).
@@ -747,16 +747,4 @@ void SystemTray::RecordSystemMenuMetrics() {
   }
 }
 
-TimeToClickRecorder::TimeToClickRecorder(SystemTray* tray) : tray_(tray) {}
-
-void TimeToClickRecorder::OnEvent(ui::Event* event) {
-  // Ignore if the event is neither click nor tap.
-  if (event->type() != ui::ET_MOUSE_PRESSED &&
-      event->type() != ui::ET_GESTURE_TAP) {
-    return;
-  }
-
-  tray_->RecordTimeToClick();
-}
-
 }  // namespace ash

ash/system/tray/system_tray.h

@@ -12,6 +12,7 @@
 #include "ash/ash_export.h"
 #include "ash/system/tray/system_tray_bubble.h"
 #include "ash/system/tray/system_tray_view.h"
+#include "ash/system/tray/time_to_click_recorder.h"
 #include "ash/system/tray/tray_background_view.h"
 #include "base/callback.h"
 #include "base/macros.h"
@@ -53,7 +54,8 @@ enum BubbleCreationType {
 // manages all the SystemTrayItem controllers, creates icon views that appear in
 // the tray, creates the bubble menu and fills the menu with items. It is also
 // the view that contains the icons in the tray.
-class ASH_EXPORT SystemTray : public TrayBackgroundView {
+class ASH_EXPORT SystemTray : public TrayBackgroundView,
+                              public TimeToClickRecorder::Delegate {
  public:
   explicit SystemTray(Shelf* shelf);
   ~SystemTray() override;
@@ -133,10 +135,6 @@ class ASH_EXPORT SystemTray : public TrayBackgroundView {
   // Returns TrayIME object if present or null otherwise.
   TrayIME* GetTrayIME() const;
 
-  // Record TimeToClick metrics and reset |last_button_clicked_|. Called by
-  // TimeToClickRecorder which is system tray view's PreTargetHandler.
-  void RecordTimeToClick();
-
   // Determines if it's ok to switch away from the currently active user. Screen
   // casting may block this (or at least throw up a confirmation dialog). Calls
   // |callback| with the result.
@@ -162,6 +160,9 @@ class ASH_EXPORT SystemTray : public TrayBackgroundView {
   bool ShouldEnableExtraKeyboardAccessibility() override;
   void HideBubble(const views::TrayBubbleView* bubble_view) override;
 
+  // TimeToClickRecorder::Delegate:
+  void RecordTimeToClick() override;
+
   ScreenTrayItem* GetScreenShareItem() { return screen_share_tray_item_; }
   ScreenTrayItem* GetScreenCaptureItem() { return screen_capture_tray_item_; }
 
@@ -256,22 +257,6 @@ class ASH_EXPORT SystemTray : public TrayBackgroundView {
   DISALLOW_COPY_AND_ASSIGN(SystemTray);
 };
 
-// An event handler that will be installed as system tray view PreTargetHandler
-// to record TimeToClick metrics.
-class TimeToClickRecorder : public ui::EventHandler {
- public:
-  TimeToClickRecorder(SystemTray* tray);
-  ~TimeToClickRecorder() override = default;
-
- private:
-  // ui::EventHandler:
-  void OnEvent(ui::Event* event) override;
-
-  SystemTray* const tray_;
-
-  DISALLOW_COPY_AND_ASSIGN(TimeToClickRecorder);
-};
-
 }  // namespace ash
 
 #endif  // ASH_SYSTEM_TRAY_SYSTEM_TRAY_H_

ash/system/tray/system_tray_view.cc

@@ -51,11 +51,10 @@ SystemTrayView::SystemTrayView(SystemTray* system_tray,
                                SystemTrayType system_tray_type,
                                const std::vector<ash::SystemTrayItem*>& items)
     : time_to_click_recorder_(
-          std::make_unique<TimeToClickRecorder>(system_tray)),
+          std::make_unique<TimeToClickRecorder>(system_tray, this)),
       items_(items),
       system_tray_type_(system_tray_type) {
   SetLayoutManager(std::make_unique<BottomAlignedBoxLayout>());
-  AddPreTargetHandler(time_to_click_recorder_.get());
 }
 
 SystemTrayView::~SystemTrayView() {

ash/system/tray/time_to_click_recorder.cc

+// Copyright 2018 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.
+
+#include "ash/system/tray/time_to_click_recorder.h"
+
+#include "ui/events/event.h"
+#include "ui/views/view.h"
+
+namespace ash {
+
+TimeToClickRecorder::TimeToClickRecorder(Delegate* delegate,
+                                         views::View* target_view)
+    : delegate_(delegate) {
+  target_view->AddPreTargetHandler(this);
+}
+
+void TimeToClickRecorder::OnEvent(ui::Event* event) {
+  // Ignore if the event is neither click nor tap.
+  if (event->type() != ui::ET_MOUSE_PRESSED &&
+      event->type() != ui::ET_GESTURE_TAP) {
+    return;
+  }
+
+  delegate_->RecordTimeToClick();
+}
+
+}  // namespace ash

ash/system/tray/time_to_click_recorder.h

+// Copyright 2018 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 ASH_SYSTEM_TRAY_TIME_TO_CLICK_RECORDER_H_
+#define ASH_SYSTEM_TRAY_TIME_TO_CLICK_RECORDER_H_
+
+#include "ui/events/event_handler.h"
+
+namespace views {
+class View;
+}  // namespace views
+
+namespace ash {
+
+// An event handler that will be installed as PreTargetHandler of |target_view|
+// to record TimeToClick metrics.
+class TimeToClickRecorder : public ui::EventHandler {
+ public:
+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+
+    // Record TimeToClick metrics. Called by TimeToClickRecorder which is a
+    // PreTargetHandler of |target_view|.
+    virtual void RecordTimeToClick() = 0;
+  };
+
+  TimeToClickRecorder(Delegate* delegate, views::View* target_view);
+  ~TimeToClickRecorder() override = default;
+
+ private:
+  // ui::EventHandler:
+  void OnEvent(ui::Event* event) override;
+
+  Delegate* const delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(TimeToClickRecorder);
+};
+
+}  // namespace ash
+
+#endif  // ASH_SYSTEM_TRAY_TIME_TO_CLICK_RECORDER_H_

ash/system/unified/unified_system_tray.cc

@@ -80,7 +80,7 @@ bool UnifiedSystemTray::UiDelegate::ShowMessageCenter(bool show_by_click) {
   if (owner_->IsBubbleShown())
     return false;
 
-  owner_->ShowBubbleInternal();
+  owner_->ShowBubbleInternal(show_by_click);
   return true;
 }
 
@@ -138,8 +138,8 @@ void UnifiedSystemTray::HideBubbleWithView(
 
 void UnifiedSystemTray::ClickedOutsideBubble() {}
 
-void UnifiedSystemTray::ShowBubbleInternal() {
-  bubble_ = std::make_unique<UnifiedSystemTrayBubble>(this);
+void UnifiedSystemTray::ShowBubbleInternal(bool show_by_click) {
+  bubble_ = std::make_unique<UnifiedSystemTrayBubble>(this, show_by_click);
   // TODO(tetsui): Call its own SetIsActive. See the comment in the ctor.
   shelf()->GetStatusAreaWidget()->system_tray()->SetIsActive(true);
 }

ash/system/unified/unified_system_tray.h

@@ -51,7 +51,7 @@ class UnifiedSystemTray : public TrayBackgroundView {
   class UiDelegate;
 
   // Forwarded from UiDelegate.
-  void ShowBubbleInternal();
+  void ShowBubbleInternal(bool show_by_click);
   void HideBubbleInternal();
 
   std::unique_ptr<UiDelegate> ui_delegate_;

ash/system/unified/unified_system_tray_bubble.cc

@@ -10,6 +10,7 @@
 #include "ash/system/unified/unified_system_tray.h"
 #include "ash/system/unified/unified_system_tray_controller.h"
 #include "ash/system/unified/unified_system_tray_view.h"
+#include "base/metrics/histogram_macros.h"
 
 namespace ash {
 
@@ -19,11 +20,15 @@ constexpr int kPaddingFromScreenTop = 8;
 
 }  // namespace
 
-UnifiedSystemTrayBubble::UnifiedSystemTrayBubble(UnifiedSystemTray* tray)
+UnifiedSystemTrayBubble::UnifiedSystemTrayBubble(UnifiedSystemTray* tray,
+                                                 bool show_by_click)
     : controller_(std::make_unique<UnifiedSystemTrayController>(
           tray->model(),
           tray->shelf()->GetStatusAreaWidget()->system_tray())),
       tray_(tray) {
+  if (show_by_click)
+    time_shown_by_click_ = base::TimeTicks::Now();
+
   views::TrayBubbleView::InitParams init_params;
   init_params.anchor_alignment = views::TrayBubbleView::ANCHOR_ALIGNMENT_BOTTOM;
   init_params.min_width = kTrayMenuWidth;
@@ -40,6 +45,8 @@ UnifiedSystemTrayBubble::UnifiedSystemTrayBubble(UnifiedSystemTray* tray)
                    kPaddingFromScreenTop -
                    bubble_view->GetBorderInsets().height();
   auto* unified_view = controller_->CreateView();
+  time_to_click_recorder_ =
+      std::make_unique<TimeToClickRecorder>(this, unified_view);
   unified_view->SetMaxHeight(max_height);
   bubble_view->SetMaxHeight(max_height);
   bubble_view->AddChildView(unified_view);
@@ -77,4 +84,15 @@ void UnifiedSystemTrayBubble::OnWidgetDestroying(views::Widget* widget) {
   tray_->CloseBubble();
 }
 
+void UnifiedSystemTrayBubble::RecordTimeToClick() {
+  // Ignore if the tray bubble is not opened by click.
+  if (!time_shown_by_click_)
+    return;
+
+  UMA_HISTOGRAM_TIMES("ChromeOS.SystemTray.TimeToClick",
+                      base::TimeTicks::Now() - time_shown_by_click_.value());
+
+  time_shown_by_click_.reset();
+}
+
 }  // namespace ash

ash/system/unified/unified_system_tray_bubble.h

@@ -7,7 +7,10 @@
 
 #include <memory>
 
+#include "ash/system/tray/time_to_click_recorder.h"
 #include "base/macros.h"
+#include "base/optional.h"
+#include "base/time/time.h"
 #include "ui/views/widget/widget_observer.h"
 
 namespace views {
@@ -23,14 +26,18 @@ class UnifiedSystemTrayController;
 // Shows the bubble on the constructor, and closes the bubble on the destructor.
 // It is possible that the bubble widget is closed on deactivation. In such
 // case, this class calls UnifiedSystemTray::CloseBubble() to delete itself.
-class UnifiedSystemTrayBubble : public views::WidgetObserver {
+class UnifiedSystemTrayBubble : public views::WidgetObserver,
+                                public TimeToClickRecorder::Delegate {
  public:
-  explicit UnifiedSystemTrayBubble(UnifiedSystemTray* tray);
+  explicit UnifiedSystemTrayBubble(UnifiedSystemTray* tray, bool show_by_click);
   ~UnifiedSystemTrayBubble() override;
 
   // views::WidgetObserver:
   void OnWidgetDestroying(views::Widget* widget) override;
 
+  // TimeToClickRecorder::Delegate:
+  void RecordTimeToClick() override;
+
  private:
   // Controller of UnifiedSystemTrayView. As the view is owned by views
   // hierarchy, we have to own the controller here.
@@ -46,6 +53,13 @@ class UnifiedSystemTrayBubble : public views::WidgetObserver {
   // In order to do this, we observe OnWidgetDestroying().
   views::Widget* bubble_widget_ = nullptr;
 
+  // PreTargetHandler of |unified_view_| to record TimeToClick metrics. Owned.
+  std::unique_ptr<TimeToClickRecorder> time_to_click_recorder_;
+
+  // The time the bubble is created. If the bubble is not created by button
+  // click (|show_by_click| in ctor is false), it is not set.
+  base::Optional<base::TimeTicks> time_shown_by_click_;
+
   DISALLOW_COPY_AND_ASSIGN(UnifiedSystemTrayBubble);
 };
 

ash/system/unified/unified_system_tray_controller.cc

@@ -73,9 +73,6 @@ UnifiedSystemTrayView* UnifiedSystemTrayController::CreateView() {
       std::make_unique<UnifiedBrightnessSliderController>(model_);
   unified_view_->AddSliderView(brightness_slider_controller_->CreateView());
 
-  time_to_click_recorder_ = std::make_unique<TimeToClickRecorder>(system_tray_);
-  unified_view_->AddPreTargetHandler(time_to_click_recorder_.get());
-
   return unified_view_;
 }
 

ash/system/unified/unified_system_tray_controller.h

@@ -22,7 +22,6 @@ namespace ash {
 class FeaturePodControllerBase;
 class SystemTray;
 class SystemTrayItem;
-class TimeToClickRecorder;
 class UnifiedBrightnessSliderController;
 class UnifiedVolumeSliderController;
 class UnifiedSystemTrayModel;
@@ -130,9 +129,6 @@ class ASH_EXPORT UnifiedSystemTrayController : public gfx::AnimationDelegate {
   std::unique_ptr<UnifiedBrightnessSliderController>
       brightness_slider_controller_;
 
-  // PreTargetHandler of |unified_view_| to record TimeToClick metrics. Owned.
-  std::unique_ptr<TimeToClickRecorder> time_to_click_recorder_;
-
   // If the previous state is expanded or not. Only valid during dragging (from
   // BeginDrag to EndDrag).
   bool was_expanded_ = true;