Fix several back gesture nudge issues.

- Fix a bug that back gesture nudge dismiss metrics can be recorded even
  before back gesture nudge is shown.
- Fix a bug in BackGestureContextualNudgeControllerTest class, there are
  two BackGestureContextualNudgeControllerImpl instances.
- Fix several other tests in BackGestureContextualNudgeControllerTest.
- Add contextual_tooltip::DismissNudgeReason::kTimeout metrics back,
  which is logged after back nudge animation sequence is finished.
- Modify the timing to record contextual_tooltip::DismissNudgeReason::
  kPerformedGesture. Previously it's recorded after back gesture is
  finished, now it's recorded when back gesture starts.

Bug: 1101105
Change-Id: I6cd2b59bb0091892f9c24511c57d692e03272265
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2345705
Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796914}

ash/shelf/contextual_nudge_status_tracker.cc

@@ -53,7 +53,7 @@ void ContextualNudgeStatusTracker::HandleNudgeShown(
     base::TimeTicks shown_time) {
   nudge_shown_time_ = shown_time;
   gesture_time_recorded_ = false;
-  dismissal_reason_recorded_ = false;
+  can_record_dismiss_metrics_ = true;
 }
 
 void ContextualNudgeStatusTracker::HandleGesturePerformed(
@@ -61,7 +61,7 @@ void ContextualNudgeStatusTracker::HandleGesturePerformed(
   if (gesture_time_recorded_)
     return;
 
-  if (!dismissal_reason_recorded_) {
+  if (can_record_dismiss_metrics_) {
     MaybeLogNudgeDismissedMetrics(
         contextual_tooltip::DismissNudgeReason::kPerformedGesture);
   }
@@ -76,10 +76,10 @@ void ContextualNudgeStatusTracker::HandleGesturePerformed(
 
 void ContextualNudgeStatusTracker::MaybeLogNudgeDismissedMetrics(
     contextual_tooltip::DismissNudgeReason reason) {
-  if (dismissal_reason_recorded_)
+  if (!can_record_dismiss_metrics_)
     return;
   base::UmaHistogramEnumeration(GetEnumHistogramName(type_), reason);
-  dismissal_reason_recorded_ = true;
+  can_record_dismiss_metrics_ = false;
 }
 
 }  // namespace ash

ash/shelf/contextual_nudge_status_tracker.h

@@ -36,10 +36,8 @@ class ASH_EXPORT ContextualNudgeStatusTracker {
 
   bool gesture_time_recorded() const { return gesture_time_recorded_; }
 
-  bool dismissal_reason_recorded() const { return dismissal_reason_recorded_; }
-
   bool can_record_dismiss_metrics() const {
-    return !gesture_time_recorded_ && !dismissal_reason_recorded_;
+    return can_record_dismiss_metrics_;
   }
 
  private:
@@ -49,13 +47,16 @@ class ASH_EXPORT ContextualNudgeStatusTracker {
   // The tooltip type tracked by this object.
   const ash::contextual_tooltip::TooltipType type_;
 
-  // Tracks whether the tooltip dismiss time has been recorded.
-  // Set when the tooltip is shown. Resets after gesture is  performed.
+  // Tracks whether the tooltip dismiss time has been recorded because of the
+  // gesture is performed. Sets after gesture is performed. Resets when the
+  // tooltip is shown.
   bool gesture_time_recorded_ = false;
 
-  // Tracks whether the tooltip dismiss time has been recorded.
-  // Set when the tooltip is shown. Resets after tooltip is hidden.
-  bool dismissal_reason_recorded_ = false;
+  // Tracks whether the nudge dismiss metrics can be recorded. Only after the
+  // tooltip is shown, the tooltip can be dismissed and the dismiss reason
+  // metrics can be logged. Otherwise, calling MaybeLogNudgeDismissedMetrics()
+  // is a no-op.
+  bool can_record_dismiss_metrics_ = false;
 };
 
 }  // namespace ash

ash/wm/gestures/back_gesture/back_gesture_contextual_nudge.cc

@@ -5,6 +5,8 @@
 #include "ash/wm/gestures/back_gesture/back_gesture_contextual_nudge.h"
 
 #include "ash/public/cpp/shell_window_ids.h"
+#include "ash/session/session_controller_impl.h"
+#include "ash/shelf/contextual_tooltip.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "base/callback.h"
@@ -124,7 +126,7 @@ class BackGestureContextualNudge::ContextualNudgeView
     : public views::View,
       public ui::ImplicitAnimationObserver {
  public:
-  explicit ContextualNudgeView(base::OnceClosure callback)
+  explicit ContextualNudgeView(base::OnceCallback<void(bool)> callback)
       : callback_(std::move(callback)) {
     SetPaintToLayer();
     layer()->SetFillsBoundsOpaquely(false);
@@ -151,7 +153,7 @@ class BackGestureContextualNudge::ContextualNudgeView
       animation_stage_ = AnimationStage::kWaitingCancelled;
       DCHECK(show_timer_.IsRunning());
       show_timer_.AbandonAndStop();
-      std::move(callback_).Run();
+      std::move(callback_).Run(/*animation_completed=*/false);
     } else if (animation_stage_ == AnimationStage::kSlidingIn ||
                animation_stage_ == AnimationStage::kBouncing ||
                animation_stage_ == AnimationStage::kSlidingOut) {
@@ -164,6 +166,8 @@ class BackGestureContextualNudge::ContextualNudgeView
     }
   }
 
+  void SetNudgeShownForTesting() { SetNudgeCountsAsShown(); }
+
   bool count_as_shown() const { return count_as_shown_; }
 
  private:
@@ -330,6 +334,14 @@ class BackGestureContextualNudge::ContextualNudgeView
     layer()->SetTransform(transform);
   }
 
+  void SetNudgeCountsAsShown() {
+    count_as_shown_ = true;
+    // Log nudge metrics right after it's shown.
+    contextual_tooltip::HandleNudgeShown(
+        Shell::Get()->session_controller()->GetActivePrefService(),
+        contextual_tooltip::TooltipType::kBackGesture);
+  }
+
   // views::View:
   void Layout() override { suggestion_view_->SetBoundsRect(GetLocalBounds()); }
 
@@ -339,13 +351,16 @@ class BackGestureContextualNudge::ContextualNudgeView
         (animation_stage_ == AnimationStage::kSlidingOut &&
          !WasAnimationAbortedForProperty(
              ui::LayerAnimationElement::TRANSFORM))) {
-      std::move(callback_).Run();
+      std::move(callback_).Run(/*animation_completed=*/animation_stage_ ==
+                               AnimationStage::kSlidingOut);
       return;
     }
 
     if (animation_stage_ == AnimationStage::kSlidingIn &&
         !WasAnimationAbortedForProperty(ui::LayerAnimationElement::TRANSFORM)) {
-      count_as_shown_ = true;
+      // Only after the back nudge finishes sliding in animation, it counts as
+      // a successful shown.
+      SetNudgeCountsAsShown();
       animation_stage_ = AnimationStage::kBouncing;
       suggestion_view_->ScheduleBounceAnimation();
     }
@@ -366,12 +381,12 @@ class BackGestureContextualNudge::ContextualNudgeView
   bool count_as_shown_ = false;
 
   // Callback function to be called after animation is cancelled or completed.
-  // Count the nudge as shown successfully if |success| is true.
-  base::OnceClosure callback_;
+  // Count the nudge as shown successfully if |count_as_shown_| is true.
+  base::OnceCallback<void(bool)> callback_;
 };
 
 BackGestureContextualNudge::BackGestureContextualNudge(
-    base::OnceClosure callback) {
+    base::OnceCallback<void(bool)> callback) {
   widget_ = CreateWidget();
   nudge_view_ = new ContextualNudgeView(std::move(callback));
   widget_->SetContentsView(nudge_view_);
@@ -388,4 +403,8 @@ bool BackGestureContextualNudge::ShouldNudgeCountAsShown() const {
   return nudge_view_->count_as_shown();
 }
 
+void BackGestureContextualNudge::SetNudgeShownForTesting() {
+  nudge_view_->SetNudgeShownForTesting();
+}
+
 }  // namespace ash

ash/wm/gestures/back_gesture/back_gesture_contextual_nudge.h

@@ -8,7 +8,7 @@
 #include <memory>
 
 #include "ash/ash_export.h"
-#include "base/callback_forward.h"
+#include "base/callback.h"
 #include "ui/views/widget/widget.h"
 
 namespace ash {
@@ -19,7 +19,7 @@ class ASH_EXPORT BackGestureContextualNudge {
  public:
   // Constructor to create a BackGestureContextualNudge with |callback| to be
   // called after the animation is completed or cancelled.
-  explicit BackGestureContextualNudge(base::OnceClosure callback);
+  explicit BackGestureContextualNudge(base::OnceCallback<void(bool)> callback);
   BackGestureContextualNudge(const BackGestureContextualNudge&) = delete;
   BackGestureContextualNudge& operator=(const BackGestureContextualNudge&) =
       delete;
@@ -35,6 +35,11 @@ class ASH_EXPORT BackGestureContextualNudge {
   // not.
   bool ShouldNudgeCountAsShown() const;
 
+  // Set nudge as shown for testing. Only after nudge is counted as shown,
+  // the nudge dismiss metrics can be correctly logged. This is to simulate
+  // something happens in the middle of a nudge animation.
+  void SetNudgeShownForTesting();
+
   views::Widget* widget() { return widget_.get(); }
 
  private:

ash/wm/gestures/back_gesture/back_gesture_contextual_nudge_controller_impl.cc

@@ -123,6 +123,9 @@ void BackGestureContextualNudgeControllerImpl::OnShelfConfigUpdated() {
 
   shelf_control_visible_ = updated_shelf_control_visibility;
 
+  if (!nudge_ || !shelf_control_visible_)
+    return;
+
   // Metrics for hiding nudge when exiting tablet mode is handled by
   // OnTabletModeEnded.
   if (tablet_mode && shelf_control_visible_) {
@@ -130,9 +133,6 @@ void BackGestureContextualNudgeControllerImpl::OnShelfConfigUpdated() {
         contextual_tooltip::TooltipType::kBackGesture,
         contextual_tooltip::DismissNudgeReason::kOther);
   }
-
-  if (!nudge_ || !shelf_control_visible_)
-    return;
   nudge_->CancelAnimationOrFadeOutToHide();
 }
 
@@ -218,19 +218,23 @@ void BackGestureContextualNudgeControllerImpl::UpdateWindowMonitoring(
   }
 }
 
-void BackGestureContextualNudgeControllerImpl::OnNudgeAnimationFinished() {
-  // TODO(crbug/1101105) Reimplement kTimeout metric for back gesture nudge.
+void BackGestureContextualNudgeControllerImpl::OnNudgeAnimationFinished(
+    bool animation_completed) {
   const bool count_as_shown = nudge_->ShouldNudgeCountAsShown();
   // UpdateWindowMonitoring() might attempt to cancel any in-progress nudge,
   // which would switch the nudge into an invalid state. Reset the nudge before
   // window monitoring is updated.
   nudge_.reset();
 
+  if (animation_completed) {
+    DCHECK(count_as_shown);
+    contextual_tooltip::MaybeLogNudgeDismissedMetrics(
+        contextual_tooltip::TooltipType::kBackGesture,
+        contextual_tooltip::DismissNudgeReason::kTimeout);
+  }
   contextual_tooltip::SetBackGestureNudgeShowing(false);
 
   if (count_as_shown) {
-    contextual_tooltip::HandleNudgeShown(
-        GetActivePrefService(), contextual_tooltip::TooltipType::kBackGesture);
     UpdateWindowMonitoring(/*can_show_nudge_immediately=*/false);
 
     // Set a timer to monitoring windows and show nudge ui again.

ash/wm/gestures/back_gesture/back_gesture_contextual_nudge_controller_impl.h

@@ -96,8 +96,10 @@ class ASH_EXPORT BackGestureContextualNudgeControllerImpl
   void UpdateWindowMonitoring(bool can_show_nudge_immediately);
 
   // Callback function to be called after nudge animation is cancelled or
-  // completed.
-  void OnNudgeAnimationFinished();
+  // completed. |animation_completed| is true when the nudge animation has
+  // completed (i.e., finishes the entire animation sequence, which includes
+  // sliding in, bouncing and sliding out animation.)
+  void OnNudgeAnimationFinished(bool animation_completed);
 
   // Do necessary cleanup when |this| is destroyed or system is shutdown.
   void DoCleanUp();

ash/wm/gestures/back_gesture/back_gesture_contextual_nudge_controller_impl_unittest.cc

@@ -12,6 +12,7 @@
 #include "ash/test/ash_test_base.h"
 #include "ash/test_shell_delegate.h"
 #include "ash/wm/gestures/back_gesture/back_gesture_contextual_nudge.h"
+#include "ash/wm/gestures/back_gesture/back_gesture_event_handler.h"
 #include "ash/wm/gestures/back_gesture/test_back_gesture_contextual_nudge_delegate.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller_test_api.h"
 #include "base/test/metrics/histogram_tester.h"
@@ -51,8 +52,6 @@ class BackGestureContextualNudgeControllerTest : public NoSessionAshTestBase {
     scoped_feature_list_.InitWithFeatures(
         {features::kContextualNudges, features::kHideShelfControlsInTabletMode},
         {});
-    nudge_controller_ =
-        std::make_unique<BackGestureContextualNudgeControllerImpl>();
 
     GetSessionControllerClient()->AddUserSession(kUser1Email);
     GetSessionControllerClient()->AddUserSession(kUser2Email);
@@ -78,7 +77,6 @@ class BackGestureContextualNudgeControllerTest : public NoSessionAshTestBase {
   }
 
   void TearDown() override {
-    nudge_controller_.reset();
     contextual_tooltip::ClearClockOverrideForTesting();
     NoSessionAshTestBase::TearDown();
   }
@@ -98,6 +96,14 @@ class BackGestureContextualNudgeControllerTest : public NoSessionAshTestBase {
     }
   }
 
+  // Set nudge as shown for testing. Only after nudge is counted as shown,
+  // the nudge dismiss metrics can be correctly logged. This is to simulate
+  // something happens in the middle of nudge animation to dismiss the nudge.
+  void SetNudgeShownForTesting() {
+    if (nudge())
+      nudge()->SetNudgeShownForTesting();
+  }
+
   PrefService* user1_pref_service() {
     return Shell::Get()->session_controller()->GetUserPrefServiceForUser(
         AccountId::FromUserEmail(kUser1Email));
@@ -109,7 +115,9 @@ class BackGestureContextualNudgeControllerTest : public NoSessionAshTestBase {
   }
 
   BackGestureContextualNudgeControllerImpl* nudge_controller() {
-    return nudge_controller_.get();
+    return Shell::Get()
+        ->back_gesture_event_handler()
+        ->nudge_controller_for_testing();
   }
 
   BackGestureContextualNudge* nudge() { return nudge_controller()->nudge(); }
@@ -127,8 +135,6 @@ class BackGestureContextualNudgeControllerTest : public NoSessionAshTestBase {
   bool can_go_back_;
   base::SimpleTestClock test_clock_;
   base::test::ScopedFeatureList scoped_feature_list_;
-
-  std::unique_ptr<BackGestureContextualNudgeControllerImpl> nudge_controller_;
 };
 
 class BackGestureContextualNudgeControllerTestCantGoBack
@@ -320,10 +326,10 @@ TEST_F(BackGestureContextualNudgeControllerTest,
   ui::ScopedAnimationDurationScaleMode non_zero(
       ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
 
-  // Verify the nudge is shown and wait until nudge animation is finished.
+  // Verify the nudge is created and wait until nudge animation is shown.
   std::unique_ptr<aura::Window> window = CreateTestWindow();
   EXPECT_TRUE(nudge());
-  WaitNudgeAnimationDone();
+  SetNudgeShownForTesting();
 
   EXPECT_FALSE(contextual_tooltip::ShouldShowNudge(
       user1_pref_service(), contextual_tooltip::TooltipType::kBackGesture,
@@ -354,10 +360,12 @@ TEST_F(BackGestureContextualNudgeControllerTest,
 // Back gesture metrics should be recorded after performing gesture.
 TEST_F(BackGestureContextualNudgeControllerTest, GesturePerformedMetricTest) {
   base::HistogramTester histogram_tester;
-  // Verify the nudge is shown and wait until nudge animation is finished.
+  ui::ScopedAnimationDurationScaleMode non_zero(
+      ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
+  // Verify the nudge is created and wait until nudge animation is shown.
   std::unique_ptr<aura::Window> window = CreateTestWindow();
   EXPECT_TRUE(nudge());
-  WaitNudgeAnimationDone();
+  SetNudgeShownForTesting();
 
   GenerateBackSequence();
 
@@ -369,6 +377,55 @@ TEST_F(BackGestureContextualNudgeControllerTest, GesturePerformedMetricTest) {
       base::TimeDelta::FromSeconds(0), 1);
 }
 
+TEST_P(BackGestureContextualNudgeControllerTestA11yPrefs, TimeoutMetricsTest) {
+  base::HistogramTester histogram_tester;
+  ui::ScopedAnimationDurationScaleMode non_zero(
+      ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
+  std::unique_ptr<aura::Window> window = CreateTestWindow();
+  EXPECT_TRUE(nudge());
+  WaitNudgeAnimationDone();
+  EXPECT_FALSE(nudge());
+
+  histogram_tester.ExpectBucketCount(
+      "Ash.ContextualNudgeDismissContext.BackGesture",
+      contextual_tooltip::DismissNudgeReason::kTimeout, 1);
+}
+
+TEST_P(BackGestureContextualNudgeControllerTestA11yPrefs,
+       LogDismissMetricsAfterNudgeShown) {
+  base::HistogramTester histogram_tester;
+  ui::ScopedAnimationDurationScaleMode non_zero(
+      ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
+  std::unique_ptr<aura::Window> window = CreateTestWindow();
+  EXPECT_TRUE(nudge());
+  // Before nudge is still waiting to be shown, exit tablet mode. The nudge will
+  // be dismissed immediately.
+  TabletModeControllerTestApi tablet_mode_api;
+  tablet_mode_api.LeaveTabletMode();
+  EXPECT_FALSE(nudge());
+
+  // Because the nudge hasn't shown yet, the dismissal metrics should not be
+  // logged.
+  histogram_tester.ExpectBucketCount(
+      "Ash.ContextualNudgeDismissContext.BackGesture",
+      contextual_tooltip::DismissNudgeReason::kSwitchToClamshell, 0);
+
+  tablet_mode_api.EnterTabletMode();
+  std::unique_ptr<aura::Window> window2 = CreateTestWindow();
+  EXPECT_TRUE(nudge());
+  SetNudgeShownForTesting();
+
+  // Exit tablet mode in the middle of the animation, test the dismissmal
+  // metrics should be correctly logged.
+  tablet_mode_api.LeaveTabletMode();
+  WaitNudgeAnimationDone();
+  histogram_tester.ExpectBucketCount(
+      "Ash.ContextualNudgeDismissContext.BackGesture",
+      contextual_tooltip::DismissNudgeReason::kSwitchToClamshell, 1);
+  histogram_tester.ExpectTotalCount(
+      "Ash.ContextualNudgeDismissContext.BackGesture", 1);
+}
+
 // Back Gesture Nudge should be hidden when shelf controls are enabled.
 TEST_P(BackGestureContextualNudgeControllerTestA11yPrefs,
        HideNudgesForShelfControls) {
@@ -376,8 +433,7 @@ TEST_P(BackGestureContextualNudgeControllerTestA11yPrefs,
   SCOPED_TRACE(testing::Message() << "Pref=" << GetParam());
   std::unique_ptr<aura::Window> window = CreateTestWindow();
   EXPECT_TRUE(nudge());
-
-  WaitNudgeAnimationDone();
+  SetNudgeShownForTesting();
 
   // Turn on accessibility settings to enable shelf controls.
   Shell::Get()

ash/wm/gestures/back_gesture/back_gesture_event_handler.cc

@@ -314,6 +314,9 @@ bool BackGestureEventHandler::MaybeHandleBackGesture(
       if (features::AreContextualNudgesEnabled()) {
         // Cancel the in-waiting or in-progress back nudge animation.
         nudge_controller_->OnBackGestureStarted();
+        contextual_tooltip::HandleGesturePerformed(
+            Shell::Get()->session_controller()->GetActivePrefService(),
+            contextual_tooltip::TooltipType::kBackGesture);
       }
       return true;
     case ui::ET_GESTURE_SCROLL_BEGIN:
@@ -393,11 +396,6 @@ bool BackGestureEventHandler::MaybeHandleBackGesture(
           }
         }
         back_gesture_affordance_->Complete();
-        if (features::AreContextualNudgesEnabled()) {
-          contextual_tooltip::HandleGesturePerformed(
-              Shell::Get()->session_controller()->GetActivePrefService(),
-              contextual_tooltip::TooltipType::kBackGesture);
-        }
       } else {
         back_gesture_affordance_->Abort();
         RecordEndScenarioType(GetEndScenarioType(

ash/wm/gestures/back_gesture/back_gesture_event_handler.h

@@ -46,6 +46,10 @@ class BackGestureEventHandler : public display::DisplayObserver,
   void OnGestureEvent(GestureConsumer* consumer,
                       ui::GestureEvent* event) override;
 
+  BackGestureContextualNudgeControllerImpl* nudge_controller_for_testing() {
+    return nudge_controller_.get();
+  }
+
  private:
   // Returns true if |event| was handled as a go-back gesture. |event| is
   // generated by |gesture_provider_| from touch event, |screen_location| is