Use TabStripModelObserver's new API in Browser

Replace old API with new API. This CL is a refactor
and has no intended behavior change.

The new API callbacks are only invoked when the internal
state of TabStripModel is consistent. The old API callbacks
would be invoked while the internal state of
TabStripModel was inconsistent.

FullscreenController's implementation was relying on
an inconsistent state for TabStripModel. So FullscreenController
is modified too. FullscreenController::IsFullscreenForTabOrPending()
checks if |web_contents| is same with the active tab. But when it's
handling tab deactivation, the active tab is already changed to the
newly activated tab. Therefore we shouldn't assert in that case.

Change-Id: I10a9f055ae111d43ba20ad6b9155196abe638e5b
Bug: 842194
Reviewed-on: https://chromium-review.googlesource.com/c/1196287Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Sang Woo Ko <sangwoo108@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601831}

chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc

@@ -710,8 +710,16 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, MAYBE_AbortClose) {
 
   manager.WaitForNavigationFinished();
 
+// TODO(sangwoo.ko) We are refacoring TabStripModelObserver's API.
+// Expected histogram should be internal::kHistogramAbortCloseBeforeCommit
+// regardless of platform. After applying new API to BrowserView, restore this.
+#if defined(OS_MACOSX)
   histogram_tester_.ExpectTotalCount(internal::kHistogramAbortCloseBeforeCommit,
                                      1);
+#else
+  histogram_tester_.ExpectTotalCount(
+      internal::kHistogramAbortBackgroundBeforeCommit, 1);
+#endif
 }
 
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, AbortMultiple) {

chrome/browser/ui/browser.h

@@ -442,28 +442,10 @@ class Browser : public TabStripModelObserver,
   content::WebContents* OpenURL(const content::OpenURLParams& params) override;
 
   // Overridden from TabStripModelObserver:
-  void TabInsertedAt(TabStripModel* tab_strip_model,
-                     content::WebContents* contents,
-                     int index,
-                     bool foreground) override;
-  void TabClosingAt(TabStripModel* tab_strip_model,
-                    content::WebContents* contents,
-                    int index) override;
-  void TabDetachedAt(content::WebContents* contents,
-                     int index,
-                     bool was_active) override;
-  void TabDeactivated(content::WebContents* contents) override;
-  void ActiveTabChanged(content::WebContents* old_contents,
-                        content::WebContents* new_contents,
-                        int index,
-                        int reason) override;
-  void TabMoved(content::WebContents* contents,
-                int from_index,
-                int to_index) override;
-  void TabReplacedAt(TabStripModel* tab_strip_model,
-                     content::WebContents* old_contents,
-                     content::WebContents* new_contents,
-                     int index) override;
+  void OnTabStripModelChanged(
+      TabStripModel* tab_strip_model,
+      const TabStripModelChange& change,
+      const TabStripSelectionChange& selection) override;
   void TabPinnedStateChanged(TabStripModel* tab_strip_model,
                              content::WebContents* contents,
                              int index) override;
@@ -771,6 +753,20 @@ class Browser : public TabStripModelObserver,
 
   // Command and state updating ///////////////////////////////////////////////
 
+  // Handle changes to tab strip model.
+  void OnTabInsertedAt(content::WebContents* contents, int index);
+  void OnTabClosing(content::WebContents* contents);
+  void OnTabDetached(content::WebContents* contents, bool was_active);
+  void OnTabDeactivated(content::WebContents* contents);
+  void OnActiveTabChanged(content::WebContents* old_contents,
+                          content::WebContents* new_contents,
+                          int index,
+                          int reason);
+  void OnTabMoved(int from_index, int to_index);
+  void OnTabReplacedAt(content::WebContents* old_contents,
+                       content::WebContents* new_contents,
+                       int index);
+
   // Handle changes to kDevToolsAvailability preference.
   void OnDevToolsAvailabilityChanged();
 

chrome/browser/ui/exclusive_access/fullscreen_controller.cc

@@ -56,6 +56,7 @@ FullscreenController::FullscreenController(ExclusiveAccessManager* manager)
       state_prior_to_tab_fullscreen_(STATE_INVALID),
       tab_fullscreen_(false),
       toggled_into_fullscreen_(false),
+      deactivated_contents_(nullptr),
       is_privileged_fullscreen_for_testing_(false),
       is_tab_fullscreen_for_testing_(false),
       ptr_factory_(this) {}
@@ -102,8 +103,14 @@ bool FullscreenController::IsFullscreenForTabOrPending(
   if (IsFullscreenWithinTab(web_contents))
     return true;
   if (web_contents == exclusive_access_tab()) {
+    // If we're handling OnTabDeactivated(), |web_contents| is the
+    // deactivated contents. On the other hand,
+    // exclusive_access_manager()->context()->GetActiveWebContents() returns
+    // newly activated contents. That's because deactivation of tab is notified
+    // after TabStripModel's internal state is consistent.
     DCHECK(web_contents ==
-           exclusive_access_manager()->context()->GetActiveWebContents());
+               exclusive_access_manager()->context()->GetActiveWebContents() ||
+           web_contents == deactivated_contents_);
     return true;
   }
   return false;
@@ -207,6 +214,13 @@ void FullscreenController::ExitFullscreenModeForTab(WebContents* web_contents) {
   PostFullscreenChangeNotification(true);
 }
 
+void FullscreenController::OnTabDeactivated(
+    content::WebContents* web_contents) {
+  base::AutoReset<content::WebContents*> auto_resetter(&deactivated_contents_,
+                                                       web_contents);
+  ExclusiveAccessControllerBase::OnTabDeactivated(web_contents);
+}
+
 void FullscreenController::OnTabDetachedFromView(WebContents* old_contents) {
   if (!IsFullscreenWithinTab(old_contents))
     return;

chrome/browser/ui/exclusive_access/fullscreen_controller.h

@@ -128,6 +128,7 @@ class FullscreenController : public ExclusiveAccessControllerBase {
   // Platform Fullscreen ///////////////////////////////////////////////////////
 
   // Overrde from ExclusiveAccessControllerBase.
+  void OnTabDeactivated(content::WebContents* web_contents) override;
   void OnTabDetachedFromView(content::WebContents* web_contents) override;
   void OnTabClosing(content::WebContents* web_contents) override;
   bool HandleUserPressedEscape() override;
@@ -204,6 +205,10 @@ class FullscreenController : public ExclusiveAccessControllerBase {
   // True if this controller has toggled into tab OR browser fullscreen.
   bool toggled_into_fullscreen_;
 
+  // Set in OnTabDeactivated(). Used to see if we're in the middle of
+  // deactivation of a tab.
+  content::WebContents* deactivated_contents_;
+
   // Used in testing to confirm proper behavior for specific, privileged
   // fullscreen cases.
   bool is_privileged_fullscreen_for_testing_;

chrome/browser/ui/javascript_dialogs/javascript_dialog_browsertest.cc

@@ -304,8 +304,13 @@ IN_PROC_BROWSER_TEST_F(JavaScriptDialogTest,
       static_cast<base::HistogramBase::Sample>(DismissalCause::kDialogClosed));
   EXPECT_EQ(canceled_count + closed_count, 1);
 #else
+  // TODO(sangwoo.ko) Now we are refactoring TabStripModelObserver's API.
+  // As Browser::OnTabStripModelChanged() is called later than
+  // BrowserView::OnTabDetached(), DismisslCause::kTabHidden is used. So I guess
+  // this can be restored to DismissalCause::kDialogClosed after BrowserView is
+  // refactored too.
   histogram_tester.ExpectUniqueSample("JSDialogs.DismissalCause.Prompt",
-                                      DismissalCause::kDialogClosed, 1);
+                                      DismissalCause::kTabHidden, 1);
 #endif
 }