Use TabStripModelObserver's new API in chrome/browser/devtools/

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

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

chrome/browser/devtools/devtools_auto_opener.cc

@@ -15,10 +15,14 @@ DevToolsAutoOpener::DevToolsAutoOpener()
 DevToolsAutoOpener::~DevToolsAutoOpener() {
 }
 
-void DevToolsAutoOpener::TabInsertedAt(TabStripModel* tab_strip_model,
-                                       content::WebContents* contents,
-                                       int index,
-                                       bool foreground) {
-  if (!DevToolsWindow::IsDevToolsWindow(contents))
-    DevToolsWindow::OpenDevToolsWindow(contents);
+void DevToolsAutoOpener::OnTabStripModelChanged(
+    const TabStripModelChange& change,
+    const TabStripSelectionChange& selection) {
+  if (change.type() != TabStripModelChange::kInserted)
+    return;
+
+  for (const auto& delta : change.deltas()) {
+    if (!DevToolsWindow::IsDevToolsWindow(delta.insert.contents))
+      DevToolsWindow::OpenDevToolsWindow(delta.insert.contents);
+  }
 }

chrome/browser/devtools/devtools_auto_opener.h

@@ -15,10 +15,9 @@ class DevToolsAutoOpener : public TabStripModelObserver {
 
  private:
   // TabStripModelObserver overrides.
-  void TabInsertedAt(TabStripModel* tab_strip_model,
-                     content::WebContents* contents,
-                     int index,
-                     bool foreground) override;
+  void OnTabStripModelChanged(
+      const TabStripModelChange& change,
+      const TabStripSelectionChange& selection) override;
 
   BrowserTabStripTracker browser_tab_strip_tracker_;
 

chrome/browser/devtools/global_confirm_info_bar.cc

@@ -177,11 +177,14 @@ GlobalConfirmInfoBar::~GlobalConfirmInfoBar() {
   }
 }
 
-void GlobalConfirmInfoBar::TabInsertedAt(TabStripModel* tab_strip_model,
-                                         content::WebContents* web_contents,
-                                         int index,
-                                         bool foreground) {
-  MaybeAddInfoBar(web_contents);
+void GlobalConfirmInfoBar::OnTabStripModelChanged(
+    const TabStripModelChange& change,
+    const TabStripSelectionChange& selection) {
+  if (change.type() != TabStripModelChange::kInserted)
+    return;
+
+  for (const auto& delta : change.deltas())
+    MaybeAddInfoBar(delta.insert.contents);
 }
 
 void GlobalConfirmInfoBar::TabChangedAt(content::WebContents* web_contents,

chrome/browser/devtools/global_confirm_info_bar.h

@@ -43,10 +43,9 @@ class GlobalConfirmInfoBar : public TabStripModelObserver,
   class DelegateProxy;
 
   // TabStripModelObserver:
-  void TabInsertedAt(TabStripModel* tab_strip_model,
-                     content::WebContents* web_contents,
-                     int index,
-                     bool foreground) override;
+  void OnTabStripModelChanged(
+      const TabStripModelChange& change,
+      const TabStripSelectionChange& selection) override;
   void TabChangedAt(content::WebContents* web_contents,
                     int index,
                     TabChangeType change_type) override;

chrome/browser/ui/browser_tab_strip_tracker.cc

@@ -68,11 +68,22 @@ void BrowserTabStripTracker::MaybeTrackBrowser(Browser* browser) {
   TabStripModel* tab_strip_model = browser->tab_strip_model();
   tab_strip_model->AddObserver(tab_strip_model_observer_);
   const int active_index = tab_strip_model->active_index();
+
+  std::vector<TabStripModelChange::Delta> deltas;
   for (int i = 0; i < tab_strip_model->count(); ++i) {
+    // TODO(sangwoo108): Delete this. https://crbug.com/842194.
     tab_strip_model_observer_->TabInsertedAt(
         tab_strip_model, tab_strip_model->GetWebContentsAt(i), i,
         i == active_index);
+
+    deltas.push_back(TabStripModelChange::CreateInsertDelta(
+        tab_strip_model->GetWebContentsAt(i), i));
   }
+
+  TabStripModelChange change(TabStripModelChange::kInserted, deltas);
+  TabStripSelectionChange selection(tab_strip_model->GetActiveWebContents(),
+                                    tab_strip_model->selection_model());
+  tab_strip_model_observer_->OnTabStripModelChanged(change, selection);
 }
 
 void BrowserTabStripTracker::OnBrowserAdded(Browser* browser) {

chrome/browser/ui/tabs/tab_strip_model.cc

@@ -62,21 +62,6 @@ bool ShouldForgetOpenersForTransition(ui::PageTransition transition) {
                                       ui::PAGE_TRANSITION_AUTO_TOPLEVEL);
 }
 
-// Fill TabStripSelectionChange with given |contents| and |selection_model|.
-// note that |new_contents| and |new_model| will be filled too so that
-// selection_changed() and active_tab_changed() won't return true.
-TabStripSelectionChange GetDefaultSelection(
-    content::WebContents* contents,
-    const ui::ListSelectionModel& selection_model) {
-  TabStripSelectionChange selection;
-  selection.old_contents = contents;
-  selection.new_contents = contents;
-  selection.old_model = selection_model;
-  selection.new_model = selection_model;
-  selection.reason = 0;
-  return selection;
-}
-
 // This tracks (and reports via UMA and tracing) how long it takes before a
 // RenderWidgetHost is requested to become visible.
 class RenderWidgetHostVisibilityTracker
@@ -358,8 +343,7 @@ void TabStripModel::InsertWebContentsAt(int index,
   if (manager)
     data->set_blocked(manager->IsDialogActive());
 
-  TabStripSelectionChange selection =
-      GetDefaultSelection(GetActiveWebContents(), selection_model_);
+  TabStripSelectionChange selection(GetActiveWebContents(), selection_model_);
 
   contents_data_.insert(contents_data_.begin() + index, std::move(data));
 
@@ -376,9 +360,9 @@ void TabStripModel::InsertWebContentsAt(int index,
                              /*triggered_by_other_operation=*/true);
   }
 
-  base::Optional<TabStripModelChange> change(TabStripModelChange(
+  TabStripModelChange change(
       TabStripModelChange::kInserted,
-      TabStripModelChange::CreateInsertDelta(raw_contents, index)));
+      TabStripModelChange::CreateInsertDelta(raw_contents, index));
   for (auto& observer : observers_)
     observer.OnTabStripModelChanged(change, selection);
 }
@@ -392,8 +376,7 @@ std::unique_ptr<content::WebContents> TabStripModel::ReplaceWebContentsAt(
 
   FixOpenersAndGroupsReferencing(index);
 
-  TabStripSelectionChange selection =
-      GetDefaultSelection(GetActiveWebContents(), selection_model_);
+  TabStripSelectionChange selection(GetActiveWebContents(), selection_model_);
   WebContents* raw_new_contents = new_contents.get();
   std::unique_ptr<WebContents> old_contents =
       contents_data_[index]->ReplaceWebContents(std::move(new_contents));
@@ -414,10 +397,9 @@ std::unique_ptr<content::WebContents> TabStripModel::ReplaceWebContentsAt(
     }
   }
 
-  base::Optional<TabStripModelChange> change(
-      TabStripModelChange(TabStripModelChange::kReplaced,
-                          TabStripModelChange::CreateReplaceDelta(
-                              old_contents.get(), raw_new_contents, index)));
+  TabStripModelChange change(TabStripModelChange::kReplaced,
+                             TabStripModelChange::CreateReplaceDelta(
+                                 old_contents.get(), raw_new_contents, index));
   for (auto& observer : observers_)
     observer.OnTabStripModelChanged(change, selection);
 
@@ -573,8 +555,7 @@ void TabStripModel::SendDetachWebContentsNotifications(
       observer.TabStripEmpty();
   }
 
-  base::Optional<TabStripModelChange> change(
-      TabStripModelChange(TabStripModelChange::kRemoved, deltas));
+  TabStripModelChange change(TabStripModelChange::kRemoved, deltas);
   for (auto& observer : observers_)
     observer.OnTabStripModelChanged(change, selection);
 }
@@ -1553,8 +1534,9 @@ TabStripSelectionChange TabStripModel::SetSelection(
 
   if (!triggered_by_other_operation &&
       (selection.active_tab_changed() || selection.selection_changed())) {
+    TabStripModelChange change;
     for (auto& observer : observers_)
-      observer.OnTabStripModelChanged({}, selection);
+      observer.OnTabStripModelChanged(change, selection);
   }
 
   return selection;
@@ -1577,8 +1559,7 @@ void TabStripModel::MoveWebContentsAtImpl(int index,
                                           bool select_after_move) {
   FixOpenersAndGroupsReferencing(index);
 
-  TabStripSelectionChange selection =
-      GetDefaultSelection(GetActiveWebContents(), selection_model_);
+  TabStripSelectionChange selection(GetActiveWebContents(), selection_model_);
 
   std::unique_ptr<WebContentsData> moved_data =
       std::move(contents_data_[index]);
@@ -1597,9 +1578,9 @@ void TabStripModel::MoveWebContentsAtImpl(int index,
   for (auto& observer : observers_)
     observer.TabMoved(web_contents, index, to_position);
 
-  base::Optional<TabStripModelChange> change(TabStripModelChange(
+  TabStripModelChange change(
       TabStripModelChange::kMoved,
-      TabStripModelChange::CreateMoveDelta(web_contents, index, to_position)));
+      TabStripModelChange::CreateMoveDelta(web_contents, index, to_position));
   for (auto& observer : observers_)
     observer.OnTabStripModelChanged(change, selection);
 }

chrome/browser/ui/tabs/tab_strip_model_observer.cc

@@ -45,6 +45,8 @@ TabStripModelChange::Delta TabStripModelChange::CreateReplaceDelta(
   return delta;
 }
 
+TabStripModelChange::TabStripModelChange() = default;
+
 TabStripModelChange::TabStripModelChange(Type type, const Delta& delta)
     : type_(type), deltas_({delta}) {}
 
@@ -59,11 +61,20 @@ TabStripModelChange::TabStripModelChange(TabStripModelChange&& other) = default;
 
 TabStripSelectionChange::TabStripSelectionChange() = default;
 
+TabStripSelectionChange::TabStripSelectionChange(
+    content::WebContents* contents,
+    const ui::ListSelectionModel& selection_model)
+    : old_contents(contents),
+      new_contents(contents),
+      old_model(selection_model),
+      new_model(selection_model),
+      reason(0) {}
+
 TabStripModelObserver::TabStripModelObserver() {
 }
 
 void TabStripModelObserver::OnTabStripModelChanged(
-    const base::Optional<TabStripModelChange>& change,
+    const TabStripModelChange& change,
     const TabStripSelectionChange& selection) {}
 
 void TabStripModelObserver::TabInsertedAt(TabStripModel* tab_strip_model,

chrome/browser/ui/tabs/tab_strip_model_observer.h

@@ -8,7 +8,6 @@
 #include <vector>
 
 #include "base/macros.h"
-#include "base/optional.h"
 #include "chrome/browser/ui/tabs/tab_change_type.h"
 #include "ui/base/models/list_selection_model.h"
 
@@ -33,12 +32,7 @@ class WebContents;
 ////////////////////////////////////////////////////////////////////////////////
 class TabStripModelChange {
  public:
-  enum Type {
-    kInserted,
-    kRemoved,
-    kMoved,
-    kReplaced,
-  };
+  enum Type { kSelectionOnly, kInserted, kRemoved, kMoved, kReplaced };
 
   // A WebContents was inserted at |index|. This implicitly changes the existing
   // selection model by calling IncrementFrom(index).
@@ -97,6 +91,7 @@ class TabStripModelChange {
                                   content::WebContents* new_contents,
                                   int index);
 
+  TabStripModelChange();
   TabStripModelChange(Type type, const Delta& delta);
   TabStripModelChange(Type type, const std::vector<Delta>& deltas);
   ~TabStripModelChange();
@@ -107,7 +102,7 @@ class TabStripModelChange {
   const std::vector<Delta>& deltas() const { return deltas_; }
 
  private:
-  const Type type_;
+  const Type type_ = kSelectionOnly;
   const std::vector<Delta> deltas_;
 
   DISALLOW_COPY_AND_ASSIGN(TabStripModelChange);
@@ -117,6 +112,12 @@ class TabStripModelChange {
 struct TabStripSelectionChange {
   TabStripSelectionChange();
 
+  // Fill TabStripSelectionChange with given |contents| and |selection_model|.
+  // note that |new_contents| and |new_model| will be filled too so that
+  // selection_changed() and active_tab_changed() won't return true.
+  TabStripSelectionChange(content::WebContents* contents,
+                          const ui::ListSelectionModel& model);
+
   bool active_tab_changed() const { return old_contents != new_contents; }
 
   // TODO(sangwoo.ko) Do we need something to indicate that the change
@@ -172,9 +173,8 @@ class TabStripModelObserver {
   // TabStripModel before the |change| and after the |change| are applied.
   // When only selection/activation was changed without any change about
   // WebContents, |change| can be empty.
-  virtual void OnTabStripModelChanged(
-      const base::Optional<TabStripModelChange>& change,
-      const TabStripSelectionChange& selection);
+  virtual void OnTabStripModelChanged(const TabStripModelChange& change,
+                                      const TabStripSelectionChange& selection);
 
   // A new WebContents was inserted into the TabStripModel at the
   // specified index. |foreground| is whether or not it was opened in the

chrome/browser/ui/tabs/tab_strip_model_unittest.cc

@@ -377,48 +377,46 @@ class NewTabStripModelObserver : public MockTabStripModelObserver {
 
   // TabStripModelObserver implementation:
   void OnTabStripModelChanged(
-      const base::Optional<TabStripModelChange>& change,
+      const TabStripModelChange& change,
       const TabStripSelectionChange& selection) override {
-    if (change) {
-      switch (change->type()) {
-        case TabStripModelChange::kInserted: {
-          for (const auto& delta : change->deltas()) {
-            PushInsertState(delta.insert.contents, delta.insert.index,
-                            selection.new_contents == delta.insert.contents);
-          }
-          break;
+    switch (change.type()) {
+      case TabStripModelChange::kInserted: {
+        for (const auto& delta : change.deltas()) {
+          PushInsertState(delta.insert.contents, delta.insert.index,
+                          selection.new_contents == delta.insert.contents);
         }
-        case TabStripModelChange::kRemoved: {
-          for (const auto& delta : change->deltas()) {
-            if (delta.remove.will_be_deleted)
-              PushCloseState(delta.remove.contents, delta.remove.index);
-
-            PushDetachState(delta.remove.contents, delta.remove.index,
-                            selection.old_contents == delta.remove.contents);
-          }
-          break;
-        }
-        case TabStripModelChange::kReplaced: {
-          for (const auto& delta : change->deltas()) {
-            PushReplaceState(delta.replace.old_contents,
-                             delta.replace.new_contents, delta.replace.index);
-          }
-          break;
-        }
-        case TabStripModelChange::kMoved: {
-          for (const auto& delta : change->deltas()) {
-            PushMoveState(delta.move.contents, delta.move.from_index,
-                          delta.move.to_index);
-          }
-          // Selection change triggered by move shouldn't be counted as
-          // exsiting tests don't expect selection change in this case.
-          // TODO(sangwoo.ko): Update the tests in this class to not use the
-          // deprecated callbacks. https://crbug.com/842194
-          return;
-        }
-        default:
-          NOTREACHED();
+        break;
       }
+      case TabStripModelChange::kRemoved: {
+        for (const auto& delta : change.deltas()) {
+          if (delta.remove.will_be_deleted)
+            PushCloseState(delta.remove.contents, delta.remove.index);
+
+          PushDetachState(delta.remove.contents, delta.remove.index,
+                          selection.old_contents == delta.remove.contents);
+        }
+        break;
+      }
+      case TabStripModelChange::kReplaced: {
+        for (const auto& delta : change.deltas()) {
+          PushReplaceState(delta.replace.old_contents,
+                           delta.replace.new_contents, delta.replace.index);
+        }
+        break;
+      }
+      case TabStripModelChange::kMoved: {
+        for (const auto& delta : change.deltas()) {
+          PushMoveState(delta.move.contents, delta.move.from_index,
+                        delta.move.to_index);
+        }
+        // Selection change triggered by move shouldn't be counted as
+        // exsiting tests don't expect selection change in this case.
+        // TODO(sangwoo.ko): Update the tests in this class to not use the
+        // deprecated callbacks. https://crbug.com/842194
+        return;
+      }
+      default:
+        break;
     }
 
     if (selection.active_tab_changed()) {