Revert "Fixed *.TestGroupDetachedAndReInserted upstream test failure" (#29021)

This reverts commit dac0b60482.

fix https://github.com/brave/brave-browser/issues/45335

Revert reason: This code was added as a workaround for upstream's bug but
that fix below is arrived to our master.
https://chromium-review.googlesource.com/c/chromium/src/+/6438165

browser/ui/views/brave_actions/brave_shields_action_view.cc

@@ -10,7 +10,6 @@
 #include <utility>
 
 #include "base/check_deref.h"
-#include "base/check_is_test.h"
 #include "base/memory/weak_ptr.h"
 #include "brave/browser/ui/brave_icon_with_badge_image_source.h"
 #include "brave/browser/ui/webui/brave_shields/shields_panel_ui.h"
@@ -328,39 +327,9 @@ void BraveShieldsActionView::OnTabStripModelChanged(
     const TabStripSelectionChange& selection) {
   if (selection.active_tab_changed()) {
     if (selection.new_contents) {
-      auto* helper = brave_shields::BraveShieldsTabHelper::FromWebContents(
-          selection.new_contents);
-      // Some upstream tests (ex, *.TestGroupDetachedAndReInserted)
-      // do tab group detach & re-attach by raw api w/o updating active tab
-      // state that happens in product. In production, let say we have 4 tabs
-      // (tab A, B, C and D) in window A. made a tab group with tab A and B. and
-      // Current active tab is D. When that tab group is dragged and detached,
-      // tab A becomes active tab during the dragging(before detach) and active
-      // tab could be tab C or D in window A after detached. Then, new window B
-      // is created after tab group is detached. And tab A(or B) could become
-      // active tab in window B. If that tab group in window B is detached, tab
-      // A(in window B) becomes as inactive tab and it becomes active tab in
-      // window A after that tab group is attached to window A. This is a tab
-      // activation flow in production. In the test(ex,
-      // TabGroupsApiTest.TestGroupDetachedAndReInserted), that tab group is
-      // detached by calling DetachTabGroupForInsertion(group). During that
-      // detaching, any tab activation change signal is not delivered because
-      // this test omits tab group dragging step before detaching. So, tab D is
-      // still active Tab after calling that api. And then, this tab group is
-      // re-inserted by calling InsertDetachedTabGroupAt(). When this happens,
-      // window A gets active tab changed signal via OnTabStripModelChanged().
-      // and |selection| args delivered by OnTabStripModelChanged() gives true
-      // for `active_tab_changed()`. and |selection.new_contents| points to D.
-      // Because of this tab D gets activated signal twice. Or
-      // `active_tab_changed()` should give false if active tab is still D.
-      // In production, tab A is active tab instead of tab D. IMO, that upstream
-      // test should be improved.
-      if (helper->HasObserver(this)) {
-        // To avoid "NOTREACHED hit. Observers can only be added once!"
-        CHECK_IS_TEST();
-        helper->RemoveObserver(this);
-      }
-      helper->AddObserver(this);
+      brave_shields::BraveShieldsTabHelper::FromWebContents(
+          selection.new_contents)
+          ->AddObserver(this);
     }
 
     if (selection.old_contents) {
@@ -372,37 +341,5 @@ void BraveShieldsActionView::OnTabStripModelChanged(
   }
 }
 
-void BraveShieldsActionView::OnTabGroupChanged(const TabGroupChange& change) {
-  if (change.type != TabGroupChange::kCreated ||
-      change.GetCreateChange()->reason() !=
-          TabGroupChange::TabGroupCreationReason::
-              kInsertedFromAnotherTabstrip) {
-    return;
-  }
-
-  const int active_index = tab_strip_model_->active_index();
-  if (tab_strip_model_->empty() || active_index == TabStripModel::kNoTab) {
-    return;
-  }
-
-  // Why we have to find previous active web contents and reset callback here?
-  // We remove observer when it becomes inactive tab via
-  // OnTabStripModelChanged(). However, it doesn't work as expected when active
-  // tab is changed by tab group re-attaching. When it's re-attached, new tab
-  // from tab group is activated but |selection.old_contents| is null when
-  // OnTabStripModelChanged(). So can't have change to do it from
-  // OnTabStripModelChanged(). Curious why it's null. I think it should point to
-  // previous active web contents.
-  const int tab_count = tab_strip_model_->count();
-  for (int i = 0; i < tab_count; ++i) {
-    if (i == active_index) {
-      continue;
-    }
-
-    auto* web_contents = tab_strip_model_->GetWebContentsAt(i);
-    brave_shields::BraveShieldsTabHelper::FromWebContents(web_contents)
-        ->RemoveObserver(this);
-  }
-}
 BEGIN_METADATA(BraveShieldsActionView)
 END_METADATA

browser/ui/views/brave_actions/brave_shields_action_view.h

@@ -65,7 +65,6 @@ class BraveShieldsActionView
       TabStripModel* tab_strip_model,
       const TabStripModelChange& change,
       const TabStripSelectionChange& selection) override;
-  void OnTabGroupChanged(const TabGroupChange& change) override;
 
   raw_ptr<views::MenuButtonController> menu_button_controller_ = nullptr;
   raw_ref<Profile> profile_;

browser/ui/views/page_action/wayback_machine_state_manager.cc

@@ -72,42 +72,6 @@ void WaybackMachineStateManager::OnTabStripModelChanged(
   }
 }
 
-void WaybackMachineStateManager::OnTabGroupChanged(
-    const TabGroupChange& change) {
-  if (change.type != TabGroupChange::kCreated ||
-      change.GetCreateChange()->reason() !=
-          TabGroupChange::TabGroupCreationReason::
-              kInsertedFromAnotherTabstrip) {
-    return;
-  }
-
-  auto* model = browser_->tab_strip_model();
-  const int active_index = model->active_index();
-  if (model->empty() || active_index == TabStripModel::kNoTab) {
-    return;
-  }
-
-  // Why we have to find previous active web contents and reset callback here?
-  // We clear callback when it becomes inactive tab via
-  // OnTabStripModelChanged(). However, it doesn't work as expected when active
-  // tab is changed by tab group re-attaching. When it's re-attached, new tab
-  // from tab group is activated but |selection.old_contents| is null when
-  // OnTabStripModelChanged(). Curious why it's null. I think it should point to
-  // previous active web contents.
-  const int tab_count = model->count();
-  for (int i = 0; i < tab_count; ++i) {
-    if (i == active_index) {
-      continue;
-    }
-
-    auto* web_contents = model->GetWebContentsAt(i);
-    auto* tab_helper =
-        BraveWaybackMachineTabHelper::FromWebContents(web_contents);
-    CHECK(tab_helper);
-    tab_helper->SetWaybackStateChangedCallback(base::NullCallback());
-  }
-}
-
 void WaybackMachineStateManager::OnWaybackStateChanged(WaybackState state) {
   icon_->Update();
 }

browser/ui/views/page_action/wayback_machine_state_manager.h

@@ -33,7 +33,6 @@ class WaybackMachineStateManager : public TabStripModelObserver {
       TabStripModel* tab_strip_model,
       const TabStripModelChange& change,
       const TabStripSelectionChange& selection) override;
-  void OnTabGroupChanged(const TabGroupChange& change) override;
 
  private:
   void OnWaybackStateChanged(WaybackState state);

components/brave_wayback_machine/brave_wayback_machine_tab_helper.cc

@@ -7,7 +7,6 @@
 
 #include <utility>
 
-#include "base/check_is_test.h"
 #include "base/command_line.h"
 #include "base/containers/fixed_flat_set.h"
 #include "base/containers/flat_set.h"
@@ -68,35 +67,11 @@ void BraveWaybackMachineTabHelper::FetchWaybackURL() {
 void BraveWaybackMachineTabHelper::SetWaybackStateChangedCallback(
     WaybackStateChangedCallback callback) {
   // callback should be set only once.
+  // And it should be cleared only when there is existing one.
   if (callback) {
-    // Some upstream tests (ex, *.TestGroupDetachedAndReInserted)
-    // do tab group detach & re-attach by raw api w/o updating active tab state
-    // that happens in product.
-    // In production, let say we have 4 tabs (tab A, B, C and D) in window A.
-    // made a tab group with tab A and B. and Current active tab is D.
-    // When that tab group is dragged and detached, tab A becomes active tab
-    // during the dragging(before detach) and active tab could be
-    // tab C or D in window A after detached. Then, new window B is created
-    // after tab group is detached. And tab A(or B) could become active tab in
-    // window B. If that tab group in window B is detached, tab A(in window B)
-    // becomes as inactive tab and it becomes active tab in window A after that
-    // tab group is attached to window A. This is a tab activation flow in
-    // production. In the test(ex,
-    // TabGroupsApiTest.TestGroupDetachedAndReInserted), that tab group is
-    // detached by calling DetachTabGroupForInsertion(group). During that
-    // detaching, any tab activation change signal is not delivered because this
-    // test omits tab group dragging step before detaching. So, tab D is still
-    // active Tab after calling that api. And then, this tab group is
-    // re-inserted by calling InsertDetachedTabGroupAt(). When this happens,
-    // window A gets active tab changed signal via OnTabStripModelChanged(). and
-    // |selection| args delivered by OnTabStripModelChanged() gives true for
-    // `active_tab_changed()`. and |selection.new_contents| points to D. Because
-    // of this tab D gets activated signal twice. Or `active_tab_changed()`
-    // should give false if active tab is still D. In production, tab A is
-    // active tab instead of tab D. IMO, that upstream test should be improved.
-    if (wayback_state_changed_callback_) {
-      CHECK_IS_TEST();
-    }
+    CHECK(!wayback_state_changed_callback_);
+  } else {
+    CHECK(wayback_state_changed_callback_);
   }
 
   wayback_state_changed_callback_ = std::move(callback);