diff --git a/browser/ui/views/brave_actions/brave_shields_action_view.cc b/browser/ui/views/brave_actions/brave_shields_action_view.cc index 321efcef53e..76adbf340c1 100644 --- a/browser/ui/views/brave_actions/brave_shields_action_view.cc +++ b/browser/ui/views/brave_actions/brave_shields_action_view.cc @@ -10,7 +10,6 @@ #include #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 diff --git a/browser/ui/views/brave_actions/brave_shields_action_view.h b/browser/ui/views/brave_actions/brave_shields_action_view.h index 762cdd6b69e..efdd136f8cf 100644 --- a/browser/ui/views/brave_actions/brave_shields_action_view.h +++ b/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 menu_button_controller_ = nullptr; raw_ref profile_; diff --git a/browser/ui/views/page_action/wayback_machine_state_manager.cc b/browser/ui/views/page_action/wayback_machine_state_manager.cc index bef406417ba..6183c672f07 100644 --- a/browser/ui/views/page_action/wayback_machine_state_manager.cc +++ b/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(); } diff --git a/browser/ui/views/page_action/wayback_machine_state_manager.h b/browser/ui/views/page_action/wayback_machine_state_manager.h index 60460d86fb2..36c6cced7d5 100644 --- a/browser/ui/views/page_action/wayback_machine_state_manager.h +++ b/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); diff --git a/components/brave_wayback_machine/brave_wayback_machine_tab_helper.cc b/components/brave_wayback_machine/brave_wayback_machine_tab_helper.cc index 7e9f9612b78..b480ba5100d 100644 --- a/components/brave_wayback_machine/brave_wayback_machine_tab_helper.cc +++ b/components/brave_wayback_machine/brave_wayback_machine_tab_helper.cc @@ -7,7 +7,6 @@ #include -#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);