Use TabStripModelObserver's new API in BrowserView

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

In the process, Introduce BrowserWindow::OnTabDetached()

When closing a tab with old API, expected order of calls are..

1. Browser::TabDetached
2. BrowserView::TabDetached -> this resets contents_web_view_.
3. Browser::ActiveTabChanged() -> This will call
BrowserWindow(BrowserView)::OnActiveTabChange() : Set new contents to
contents_web_view_

But, if we use the new observer API it would be

1. Browser::OnTabStripModelChanged()
    * handles things Browser::TabDetached() used to do.
    * handles things Browser::ActiveTabChanged() used to
2. BrowserView::OnTabStripModelChanged()
    * handles BrowserView:TabDetachedAt() used to do.

As a result, contents_web_view_ is cleared out with null
contents.

Repro step and the result would be,
1. Open browser
2. Create new tab
3. Close active tab.
-> Then new active tab's contents won't be visible.

Therefore, Introduce BrowserWindow::OnTabDetached() and
let the Browser control the flow.

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

chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc

@@ -710,16 +710,8 @@ 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.cc

@@ -2139,6 +2139,8 @@ void Browser::OnTabDetached(WebContents* contents, bool was_active) {
   }
 
   TabDetachedAtImpl(contents, was_active, DETACH_TYPE_DETACH);
+
+  window_->OnTabDetached(contents, was_active);
 }
 
 void Browser::OnTabDeactivated(WebContents* contents) {

chrome/browser/ui/browser_window.h

@@ -199,6 +199,12 @@ class BrowserWindow : public ui::BaseWindow {
                                   int index,
                                   int reason) = 0;
 
+  // Called when a tab is detached. Subclasses which implement
+  // TabStripModelObserver should implement this instead of processing this
+  // in OnTabStripModelChanged(); the Browser will call this method.
+  virtual void OnTabDetached(content::WebContents* contents,
+                             bool was_active) = 0;
+
   // Called to force the zoom state to for the active tab to be recalculated.
   // |can_show_bubble| is true when a user presses the zoom up or down keyboard
   // shortcuts and will be false in other cases (e.g. switching tabs, "clicking"

chrome/browser/ui/javascript_dialogs/javascript_dialog_browsertest.cc

@@ -304,13 +304,8 @@ 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::kTabHidden, 1);
+                                      DismissalCause::kDialogClosed, 1);
 #endif
 }
 

chrome/browser/ui/views/frame/browser_view.cc

@@ -839,6 +839,13 @@ void BrowserView::OnActiveTabChanged(content::WebContents* old_contents,
                                      int reason) {
   DCHECK(new_contents);
 
+  if (old_contents && !old_contents->IsBeingDestroyed()) {
+    // We do not store the focus when closing the tab to work-around bug 4633.
+    // Some reports seem to show that the focus manager and/or focused view can
+    // be garbage at that point, it is not clear why.
+    old_contents->StoreFocus();
+  }
+
   // If |contents_container_| already has the correct WebContents, we can save
   // some work.  This also prevents extra events from being reported by the
   // Visibility API under Windows, as ChangeWebContents will briefly hide
@@ -925,6 +932,19 @@ void BrowserView::OnActiveTabChanged(content::WebContents* old_contents,
   TranslateBubbleView::CloseCurrentBubble();
 }
 
+void BrowserView::OnTabDetached(content::WebContents* contents,
+                                bool was_active) {
+  if (was_active) {
+    // We need to reset the current tab contents to null before it gets
+    // freed. This is because the focus manager performs some operations
+    // on the selected WebContents when it is removed.
+    web_contents_close_handler_->ActiveTabChanged();
+    contents_web_view_->SetWebContents(nullptr);
+    infobar_container_->ChangeInfoBarManager(nullptr);
+    UpdateDevToolsForContents(nullptr, true);
+  }
+}
+
 void BrowserView::ZoomChangedForActiveTab(bool can_show_bubble) {
   const AppMenuButton* app_menu_button =
       toolbar_button_provider()->GetAppMenuButton();
@@ -1596,48 +1616,33 @@ LocationBarView* BrowserView::GetLocationBarView() const {
 ///////////////////////////////////////////////////////////////////////////////
 // BrowserView, TabStripModelObserver implementation:
 
-void BrowserView::TabInsertedAt(TabStripModel* tab_strip_model,
-                                WebContents* contents,
-                                int index,
-                                bool foreground) {
+void BrowserView::OnTabStripModelChanged(
+    TabStripModel* tab_strip_model,
+    const TabStripModelChange& change,
+    const TabStripSelectionChange& selection) {
+  if (change.type() != TabStripModelChange::kInserted)
+    return;
+
+  for (size_t i = 0; i < change.deltas().size(); i++) {
 #if defined(USE_AURA)
-  // WebContents inserted in tabs might not have been added to the root
-  // window yet. Per http://crbug/342672 add them now since drawing the
-  // WebContents requires root window specific data - information about
-  // the screen the WebContents is drawn on, for example.
-  if (!contents->GetNativeView()->GetRootWindow()) {
-    aura::Window* window = contents->GetNativeView();
-    aura::Window* root_window = GetNativeWindow()->GetRootWindow();
-    aura::client::ParentWindowWithContext(
-        window, root_window, root_window->GetBoundsInScreen());
-    DCHECK(contents->GetNativeView()->GetRootWindow());
-  }
+    // WebContents inserted in tabs might not have been added to the root
+    // window yet. Per http://crbug/342672 add them now since drawing the
+    // WebContents requires root window specific data - information about
+    // the screen the WebContents is drawn on, for example.
+    const auto& delta = change.deltas()[i];
+    content::WebContents* contents = delta.insert.contents;
+    if (!contents->GetNativeView()->GetRootWindow()) {
+      aura::Window* window = contents->GetNativeView();
+      aura::Window* root_window = GetNativeWindow()->GetRootWindow();
+      aura::client::ParentWindowWithContext(window, root_window,
+                                            root_window->GetBoundsInScreen());
+      DCHECK(contents->GetNativeView()->GetRootWindow());
+    }
 #endif
-  web_contents_close_handler_->TabInserted();
-}
-
-void BrowserView::TabDetachedAt(WebContents* contents,
-                                int index,
-                                bool was_active) {
-  if (was_active) {
-    // We need to reset the current tab contents to null before it gets
-    // freed. This is because the focus manager performs some operations
-    // on the selected WebContents when it is removed.
-    web_contents_close_handler_->ActiveTabChanged();
-    contents_web_view_->SetWebContents(nullptr);
-    infobar_container_->ChangeInfoBarManager(nullptr);
-    UpdateDevToolsForContents(nullptr, true);
+    web_contents_close_handler_->TabInserted();
   }
 }
 
-void BrowserView::TabDeactivated(WebContents* contents) {
-  // We do not store the focus when closing the tab to work-around bug 4633.
-  // Some reports seem to show that the focus manager and/or focused view can
-  // be garbage at that point, it is not clear why.
-  if (!contents->IsBeingDestroyed())
-    contents->StoreFocus();
-}
-
 void BrowserView::TabStripEmpty() {
   // Make sure all optional UI is removed before we are destroyed, otherwise
   // there will be consequences (since our view hierarchy will still have

chrome/browser/ui/views/frame/browser_view.h

@@ -308,6 +308,7 @@ class BrowserView : public BrowserWindow,
                           content::WebContents* new_contents,
                           int index,
                           int reason) override;
+  void OnTabDetached(content::WebContents* contents, bool was_active) override;
   void ZoomChangedForActiveTab(bool can_show_bubble) override;
   gfx::Rect GetRestoredBounds() const override;
   ui::WindowShowState GetRestoredState() const override;
@@ -416,14 +417,10 @@ class BrowserView : public BrowserWindow,
   LocationBarView* GetLocationBarView() const;
 
   // TabStripModelObserver:
-  void TabInsertedAt(TabStripModel* tab_strip_model,
-                     content::WebContents* contents,
-                     int index,
-                     bool foreground) override;
-  void TabDetachedAt(content::WebContents* contents,
-                     int index,
-                     bool was_active) override;
-  void TabDeactivated(content::WebContents* contents) override;
+  void OnTabStripModelChanged(
+      TabStripModel* tab_strip_model,
+      const TabStripModelChange& change,
+      const TabStripSelectionChange& selection) override;
   void TabStripEmpty() override;
   void WillCloseAllTabs(TabStripModel* tab_strip_model) override;
   void CloseAllTabsStopped(TabStripModel* tab_strip_model,

chrome/test/base/test_browser_window.h

@@ -70,6 +70,8 @@ class TestBrowserWindow : public BrowserWindow {
                           content::WebContents* new_contents,
                           int index,
                           int reason) override {}
+  void OnTabDetached(content::WebContents* contents, bool was_active) override {
+  }
   void ZoomChangedForActiveTab(bool can_show_bubble) override {}
   gfx::Rect GetRestoredBounds() const override;
   ui::WindowShowState GetRestoredState() const override;