[reland] back gesture nudge: bugs fixing.

Fix a few bugs related to back gesture:
- If we have a top active window that can perform back before entering
tablet mode, then when entering tablet mode, we should show the nudge
above the window.
- Fix an issue that if switching to an existing tab that can perform go
back operation, the nudge does not show up. We should trigger the
navigation status change when active tab changes.
- Also fix a crash happens when shutting down chrome when nudge is in
animation.
- Also use DidFinishNavigation() instead of NavigationEntryCommitted()
to inform observers when a navigation happens inside of the current
active webcontent.

Bug: 1009005b
Change-Id: I3caf166e6681cad7b3d9b88fa9fe32f2482f62bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2081518Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746040}

ash/wm/gestures/back_gesture/back_gesture_contextual_nudge_controller_impl.cc

@@ -12,6 +12,7 @@
 #include "ash/wm/gestures/back_gesture/back_gesture_contextual_nudge.h"
 #include "ash/wm/window_util.h"
 #include "components/prefs/pref_service.h"
+#include "ui/aura/client/window_types.h"
 #include "ui/wm/public/activation_client.h"
 
 namespace ash {
@@ -31,10 +32,7 @@ BackGestureContextualNudgeControllerImpl::
 
 BackGestureContextualNudgeControllerImpl::
     ~BackGestureContextualNudgeControllerImpl() {
-  if (is_monitoring_windows_) {
-    nudge_delegate_.reset();
-    Shell::Get()->activation_client()->RemoveObserver(this);
-  }
+  DoCleanUp();
 }
 
 void BackGestureContextualNudgeControllerImpl::OnActiveUserSessionChanged(
@@ -53,14 +51,10 @@ void BackGestureContextualNudgeControllerImpl::OnTabletModeStarted() {
 
 void BackGestureContextualNudgeControllerImpl::OnTabletModeEnded() {
   UpdateWindowMonitoring();
-
-  // Cancel in-waiting animation or in-progress animation.
-  if (nudge_)
-    nudge_->CancelAnimationOrFadeOutToHide();
 }
 
 void BackGestureContextualNudgeControllerImpl::OnTabletControllerDestroyed() {
-  tablet_mode_observer_.RemoveAll();
+  DoCleanUp();
 }
 
 void BackGestureContextualNudgeControllerImpl::OnWindowActivated(
@@ -89,10 +83,7 @@ void BackGestureContextualNudgeControllerImpl::NavigationEntryChanged(
   if (nudge_)
     nudge_->CancelAnimationOrFadeOutToHide();
 
-  if ((!nudge_ || !nudge_->ShouldNudgeCountAsShown()) &&
-      Shell::Get()->shell_delegate()->CanGoBack(window) && CanShowNudge()) {
-    ShowNudgeUi();
-  }
+  MaybeShowNudgeUi(window);
 }
 
 bool BackGestureContextualNudgeControllerImpl::CanShowNudge() const {
@@ -108,10 +99,16 @@ bool BackGestureContextualNudgeControllerImpl::CanShowNudge() const {
       GetActivePrefService(), contextual_tooltip::TooltipType::kBackGesture);
 }
 
-void BackGestureContextualNudgeControllerImpl::ShowNudgeUi() {
-  nudge_ = std::make_unique<BackGestureContextualNudge>(base::BindOnce(
-      &BackGestureContextualNudgeControllerImpl::OnNudgeAnimationFinished,
-      weak_ptr_factory_.GetWeakPtr()));
+void BackGestureContextualNudgeControllerImpl::MaybeShowNudgeUi(
+    aura::Window* window) {
+  if ((!nudge_ || !nudge_->ShouldNudgeCountAsShown()) &&
+      window->type() == aura::client::WINDOW_TYPE_NORMAL &&
+      !window->is_destroying() &&
+      Shell::Get()->shell_delegate()->CanGoBack(window) && CanShowNudge()) {
+    nudge_ = std::make_unique<BackGestureContextualNudge>(base::BindOnce(
+        &BackGestureContextualNudgeControllerImpl::OnNudgeAnimationFinished,
+        weak_ptr_factory_.GetWeakPtr()));
+  }
 }
 
 void BackGestureContextualNudgeControllerImpl::UpdateWindowMonitoring() {
@@ -128,8 +125,10 @@ void BackGestureContextualNudgeControllerImpl::UpdateWindowMonitoring() {
     // If there is an active window at this moment and we should monitor its
     // navigation status, start monitoring it now.
     aura::Window* active_window = window_util::GetActiveWindow();
-    if (active_window)
+    if (active_window) {
+      MaybeShowNudgeUi(active_window);
       nudge_delegate_->MaybeStartTrackingNavigation(active_window);
+    }
 
     Shell::Get()->activation_client()->AddObserver(this);
     return;
@@ -138,6 +137,9 @@ void BackGestureContextualNudgeControllerImpl::UpdateWindowMonitoring() {
   // Stop monitoring window.
   nudge_delegate_.reset();
   Shell::Get()->activation_client()->RemoveObserver(this);
+  // Cancel any in-waiting animation or in-progress animation.
+  if (nudge_)
+    nudge_->CancelAnimationOrFadeOutToHide();
 }
 
 void BackGestureContextualNudgeControllerImpl::OnNudgeAnimationFinished() {
@@ -154,4 +156,15 @@ void BackGestureContextualNudgeControllerImpl::OnNudgeAnimationFinished() {
   nudge_.reset();
 }
 
+void BackGestureContextualNudgeControllerImpl::DoCleanUp() {
+  tablet_mode_observer_.RemoveAll();
+
+  if (is_monitoring_windows_) {
+    Shell::Get()->activation_client()->RemoveObserver(this);
+    nudge_delegate_.reset();
+    nudge_.reset();
+    is_monitoring_windows_ = false;
+  }
+}
+
 }  // namespace ash

ash/wm/gestures/back_gesture/back_gesture_contextual_nudge_controller_impl.h

@@ -66,7 +66,8 @@ class ASH_EXPORT BackGestureContextualNudgeControllerImpl
   // Returns true if we can show back gesture contextual nudge ui in current
   // configuration.
   bool CanShowNudge() const;
-  void ShowNudgeUi();
+  // Maybe show nudge ui on top of |window|.
+  void MaybeShowNudgeUi(aura::Window* window);
 
   // Starts or stops monitoring windows activation changes to decide if and when
   // to show up the contextual nudge ui.
@@ -76,6 +77,9 @@ class ASH_EXPORT BackGestureContextualNudgeControllerImpl
   // completed.
   void OnNudgeAnimationFinished();
 
+  // Do necessary cleanup when |this| is destroyed or system is shutdown.
+  void DoCleanUp();
+
   ScopedSessionObserver session_observer_{this};
   ScopedObserver<TabletModeController, TabletModeObserver>
       tablet_mode_observer_{this};

ash/wm/gestures/back_gesture/back_gesture_contextual_nudge_controller_impl_unittest.cc

@@ -180,4 +180,15 @@ TEST_F(BackGestureContextualNudgeControllerTest, CanNotGoBackWindowTest) {
       user1_perf_service(), contextual_tooltip::TooltipType::kBackGesture));
 }
 
+TEST_F(BackGestureContextualNudgeControllerTest, ShowNudgeOnExistingWindow) {
+  TabletModeControllerTestApi tablet_mode_api;
+  tablet_mode_api.LeaveTabletMode();
+  EXPECT_FALSE(nudge());
+  std::unique_ptr<aura::Window> window = CreateTestWindow();
+  EXPECT_FALSE(nudge());
+
+  tablet_mode_api.EnterTabletMode();
+  EXPECT_TRUE(nudge());
+}
+
 }  // namespace ash

chrome/browser/ui/ash/back_gesture_contextual_nudge_delegate.cc

@@ -9,6 +9,7 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "content/public/browser/navigation_details.h"
+#include "content/public/browser/navigation_handle.h"
 #include "ui/aura/window.h"
 
 BackGestureContextualNudgeDelegate::BackGestureContextualNudgeDelegate(
@@ -42,9 +43,10 @@ void BackGestureContextualNudgeDelegate::MaybeStartTrackingNavigation(
   Observe(contents);
 }
 
-void BackGestureContextualNudgeDelegate::NavigationEntryCommitted(
-    const content::LoadCommittedDetails& load_details) {
-  if (window_)
+void BackGestureContextualNudgeDelegate::DidFinishNavigation(
+    content::NavigationHandle* navigation_handle) {
+  DCHECK(window_);
+  if (navigation_handle->HasCommitted())
     controller_->NavigationEntryChanged(window_);
 }
 
@@ -54,6 +56,8 @@ void BackGestureContextualNudgeDelegate::OnTabStripModelChanged(
     const TabStripSelectionChange& selection) {
   const bool active_tab_changed = selection.active_tab_changed();
   if (active_tab_changed) {
+    DCHECK(window_);
+    controller_->NavigationEntryChanged(window_);
     content::WebContents* contents = tab_strip_model->GetActiveWebContents();
     Observe(contents);
   }

chrome/browser/ui/ash/back_gesture_contextual_nudge_delegate.h

@@ -19,7 +19,8 @@ class BackGestureContextualNudgeController;
 }
 
 // BackGestureContextualNudgeDelegate observes |window_|'s active webcontent and
-// notify when |window_|'s navigation entry changes.
+// notify when |window_|'s navigation status changes (either the active
+// webcontent changed or a navigation happens in the active webcontent.).
 class BackGestureContextualNudgeDelegate
     : public ash::BackGestureContextualNudgeDelegate,
       public content::WebContentsObserver,
@@ -39,8 +40,8 @@ class BackGestureContextualNudgeDelegate
   void MaybeStartTrackingNavigation(aura::Window* window) override;
 
   // content::WebContentsObserver:
-  void NavigationEntryCommitted(
-      const content::LoadCommittedDetails& load_details) override;
+  void DidFinishNavigation(
+      content::NavigationHandle* navigation_handle) override;
 
   // TabStripModelObserver:
   void OnTabStripModelChanged(