From ea472a56b596cd0cbf9d15ec210e6e155179ec1c Mon Sep 17 00:00:00 2001 From: brave-builds Date: Fri, 11 Oct 2024 05:40:02 +0000 Subject: [PATCH 1/5] Uplift of #25679 (squashed) to beta --- browser/ui/sidebar/sidebar_browsertest.cc | 39 +++++++++++++++---- browser/ui/sidebar/sidebar_controller.cc | 12 ++++++ browser/ui/sidebar/sidebar_controller.h | 2 + browser/ui/sidebar/sidebar_utils.cc | 18 +++++++++ browser/ui/sidebar/sidebar_utils.h | 2 + .../views/sidebar/sidebar_container_view.cc | 23 ++++++++++- .../ui/views/sidebar/sidebar_container_view.h | 8 ++++ 7 files changed, 95 insertions(+), 9 deletions(-) diff --git a/browser/ui/sidebar/sidebar_browsertest.cc b/browser/ui/sidebar/sidebar_browsertest.cc index 45ab49e3283f..961ca1bcda2c 100644 --- a/browser/ui/sidebar/sidebar_browsertest.cc +++ b/browser/ui/sidebar/sidebar_browsertest.cc @@ -314,14 +314,14 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, BasicTest) { EXPECT_EQ(expected_count, model()->GetAllSidebarItems().size()); // Activate item that opens in panel. const size_t first_panel_item_index = GetFirstPanelItemIndex(); - controller()->ActivateItemAt(first_panel_item_index); + const auto& first_panel_item = + controller()->model()->GetAllSidebarItems()[first_panel_item_index]; + controller()->ActivatePanelItem(first_panel_item.built_in_item_type); + WaitUntil( + base::BindLambdaForTesting([&]() { return !!model()->active_index(); })); EXPECT_THAT(model()->active_index(), Optional(first_panel_item_index)); EXPECT_TRUE(controller()->IsActiveIndex(first_panel_item_index)); - // Try to activate item at index 1. - // Default item at index 1 opens in new tab. So, sidebar active index is not - // changed. Still active index is 2. - // Get first index of item that opens in panel. const size_t first_web_item_index = GetFirstWebItemIndex(); const auto item = model()->GetAllSidebarItems()[first_web_item_index]; @@ -331,10 +331,15 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, BasicTest) { EXPECT_THAT(model()->active_index(), Optional(active_item_index)); // Setting std::nullopt means deactivate current active tab. - controller()->ActivateItemAt(std::nullopt); + controller()->DeactivateCurrentPanel(); + WaitUntil( + base::BindLambdaForTesting([&]() { return !model()->active_index(); })); EXPECT_THAT(model()->active_index(), Eq(std::nullopt)); - controller()->ActivateItemAt(active_item_index); + controller()->ActivatePanelItem(first_panel_item.built_in_item_type); + WaitUntil( + base::BindLambdaForTesting([&]() { return !!model()->active_index(); })); + EXPECT_THAT(model()->active_index(), Optional(active_item_index)); auto* sidebar_service = SidebarServiceFactory::GetForProfile(browser()->profile()); @@ -1109,7 +1114,24 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTestWithAIChat, TabSpecificPanel) { ASSERT_TRUE(global_item_index.has_value()); auto tab_specific_item_index = model()->GetIndexOf(kTabSpecificItemType); ASSERT_TRUE(tab_specific_item_index.has_value()); - // Open 2 more tabs + + ASSERT_TRUE(ui_test_utils::NavigateToURLWithDisposition( + browser(), GURL("brave://newtab/"), + WindowOpenDisposition::NEW_FOREGROUND_TAB, + ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP)); + ASSERT_EQ(tab_model()->GetTabCount(), 2); + + // Open contextual panel from Tab 0. + tab_model()->ActivateTabAt(0); + SimulateSidebarItemClickAt(tab_specific_item_index.value()); + EXPECT_EQ(model()->active_index(), tab_specific_item_index); + + // Delete Tab 0 and check model doesn't have active index. + tab_model()->DetachAndDeleteWebContentsAt(0); + EXPECT_FALSE(!!model()->active_index()); + ASSERT_EQ(tab_model()->GetTabCount(), 1); + + // Create two more tab for test below. ASSERT_TRUE(ui_test_utils::NavigateToURLWithDisposition( browser(), GURL("brave://newtab/"), WindowOpenDisposition::NEW_FOREGROUND_TAB, @@ -1119,6 +1141,7 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTestWithAIChat, TabSpecificPanel) { WindowOpenDisposition::NEW_FOREGROUND_TAB, ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP)); ASSERT_EQ(tab_model()->GetTabCount(), 3); + // Open a "global" panel from Tab 0 tab_model()->ActivateTabAt(0); // Tab changed flag should be cleared after ActivateTabAt() executed. diff --git a/browser/ui/sidebar/sidebar_controller.cc b/browser/ui/sidebar/sidebar_controller.cc index feda646b53bc..e841d7365278 100644 --- a/browser/ui/sidebar/sidebar_controller.cc +++ b/browser/ui/sidebar/sidebar_controller.cc @@ -224,6 +224,18 @@ void SidebarController::AddItemWithCurrentTab() { SidebarItem::BuiltInItemType::kNone, false)); } +void SidebarController::UpdateActiveItemState( + std::optional active_panel_item) { + if (!active_panel_item) { + ActivateItemAt(std::nullopt); + return; + } + + if (auto index = sidebar_model_->GetIndexOf(*active_panel_item)) { + ActivateItemAt(*index); + } +} + void SidebarController::SetSidebar(Sidebar* sidebar) { DCHECK(!sidebar_); // |sidebar| can be null in unit test. diff --git a/browser/ui/sidebar/sidebar_controller.h b/browser/ui/sidebar/sidebar_controller.h index 64a6cc9a0329..77dcc13ac44f 100644 --- a/browser/ui/sidebar/sidebar_controller.h +++ b/browser/ui/sidebar/sidebar_controller.h @@ -52,6 +52,8 @@ class SidebarController : public SidebarService::Observer { std::optional index, WindowOpenDisposition disposition = WindowOpenDisposition::CURRENT_TAB); void AddItemWithCurrentTab(); + void UpdateActiveItemState(std::optional + active_panel_item = std::nullopt); // Ask panel item activation state change to SidePanelUI. void ActivatePanelItem(SidebarItem::BuiltInItemType panel_item); diff --git a/browser/ui/sidebar/sidebar_utils.cc b/browser/ui/sidebar/sidebar_utils.cc index 3efb59542fdd..58af87379838 100644 --- a/browser/ui/sidebar/sidebar_utils.cc +++ b/browser/ui/sidebar/sidebar_utils.cc @@ -149,6 +149,24 @@ SidePanelEntryId SidePanelIdFromSideBarItemType(BuiltInItemType type) { "not have a panel Id."; } +std::optional BuiltInItemTypeFromSidePanelId( + SidePanelEntryId id) { + switch (id) { + case SidePanelEntryId::kReadingList: + return BuiltInItemType::kReadingList; + case SidePanelEntryId::kBookmarks: + return BuiltInItemType::kBookmarks; + case SidePanelEntryId::kPlaylist: + return BuiltInItemType::kPlaylist; + case SidePanelEntryId::kChatUI: + return BuiltInItemType::kChatUI; + default: + break; + } + + return std::nullopt; +} + SidePanelEntryId SidePanelIdFromSideBarItem(const SidebarItem& item) { CHECK(item.open_in_panel) << static_cast(item.built_in_item_type); return SidePanelIdFromSideBarItemType(item.built_in_item_type); diff --git a/browser/ui/sidebar/sidebar_utils.h b/browser/ui/sidebar/sidebar_utils.h index cf25944c35ed..a58fce6aa498 100644 --- a/browser/ui/sidebar/sidebar_utils.h +++ b/browser/ui/sidebar/sidebar_utils.h @@ -29,6 +29,8 @@ GURL ConvertURLToBuiltInItemURL(const GURL& url); SidePanelEntryId SidePanelIdFromSideBarItemType( SidebarItem::BuiltInItemType type); SidePanelEntryId SidePanelIdFromSideBarItem(const SidebarItem& item); +std::optional BuiltInItemTypeFromSidePanelId( + SidePanelEntryId id); void SetLastUsedSidePanel(PrefService* prefs, std::optional id); std::optional GetLastUsedSidePanel(Browser* browser); diff --git a/browser/ui/views/sidebar/sidebar_container_view.cc b/browser/ui/views/sidebar/sidebar_container_view.cc index 47e050963485..099dad379204 100644 --- a/browser/ui/views/sidebar/sidebar_container_view.cc +++ b/browser/ui/views/sidebar/sidebar_container_view.cc @@ -127,6 +127,7 @@ SidebarContainerView::SidebarContainerView( SetNotifyEnterExitOnChild(true); side_panel_ = AddChildView(std::move(side_panel)); + side_panel_view_state_observation_.Observe(side_panel_coordinator_); } SidebarContainerView::~SidebarContainerView() = default; @@ -813,6 +814,26 @@ void SidebarContainerView::OnRegistryDestroying(SidePanelRegistry* registry) { } } +void SidebarContainerView::UpdateActiveItemState() { + DVLOG(1) << "Update active item state"; + + auto* controller = browser_->sidebar_controller(); + std::optional current_type; + if (auto entry_id = side_panel_coordinator_->GetCurrentEntryId()) { + current_type = sidebar::BuiltInItemTypeFromSidePanelId(*entry_id); + } + controller->UpdateActiveItemState(current_type); +} + +void SidebarContainerView::OnSidePanelDidClose() { + // As contextual registry is owned by TabFeatures, + // that registry is destroyed before coordinator notifies OnEntryHidden() when + // tab is closed. In this case, we should update sidebar ui(active item state) + // with this notification. If not, sidebar ui's active item state is not + // changed. + UpdateActiveItemState(); +} + void SidebarContainerView::OnTabStripModelChanged( TabStripModel* tab_strip_model, const TabStripModelChange& change, @@ -864,8 +885,8 @@ void SidebarContainerView::OnTabStripModelChanged( for (const auto& contents : change.GetRemove()->contents) { StopObservingContextualSidePanelRegistry(contents.contents); } - return; } + return; } } diff --git a/browser/ui/views/sidebar/sidebar_container_view.h b/browser/ui/views/sidebar/sidebar_container_view.h index aa090fafccd9..4779ac335095 100644 --- a/browser/ui/views/sidebar/sidebar_container_view.h +++ b/browser/ui/views/sidebar/sidebar_container_view.h @@ -23,6 +23,7 @@ #include "chrome/browser/ui/views/side_panel/side_panel_coordinator.h" #include "chrome/browser/ui/views/side_panel/side_panel_entry_observer.h" #include "chrome/browser/ui/views/side_panel/side_panel_registry_observer.h" +#include "chrome/browser/ui/views/side_panel/side_panel_view_state_observer.h" #include "components/prefs/pref_member.h" #include "ui/events/event_observer.h" #include "ui/gfx/animation/slide_animation.h" @@ -58,6 +59,7 @@ class SidebarContainerView public sidebar::SidebarModel::Observer, public SidePanelEntryObserver, public SidePanelRegistryObserver, + public SidePanelViewStateObserver, public TabStripModelObserver { METADATA_HEADER(SidebarContainerView, views::View) public: @@ -134,6 +136,9 @@ class SidebarContainerView const TabStripModelChange& change, const TabStripSelectionChange& selection) override; + // SidePanelViewStateObserver: + void OnSidePanelDidClose() override; + private: friend class sidebar::SidebarBrowserTest; @@ -186,6 +191,7 @@ class SidebarContainerView content::WebContents* contents); void StopObservingContextualSidePanelRegistry(content::WebContents* contents); void StopObservingContextualSidePanelRegistry(SidePanelRegistry* registry); + void UpdateActiveItemState(); raw_ptr browser_ = nullptr; raw_ptr side_panel_coordinator_ = nullptr; @@ -205,6 +211,8 @@ class SidebarContainerView std::unique_ptr browser_window_event_monitor_; std::unique_ptr show_options_widget_; BooleanPrefMember show_side_panel_button_; + base::ScopedObservation + side_panel_view_state_observation_{this}; base::ScopedObservation sidebar_model_observation_{this}; From ecdc01299661a9c911ab5432bb47cd6b630c3680 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Fri, 11 Oct 2024 14:19:14 +0900 Subject: [PATCH 2/5] Merge pull request #25771 from brave/refactor_sidebar_container_view Refactor SidebarContainerView --- browser/ui/sidebar/sidebar_browsertest.cc | 12 ++ browser/ui/views/frame/brave_browser_view.cc | 8 + browser/ui/views/frame/brave_browser_view.h | 3 + .../brave_side_panel_coordinator.cc | 28 ++++ .../side_panel/brave_side_panel_coordinator.h | 2 + .../views/sidebar/sidebar_container_view.cc | 137 ++++++++---------- .../ui/views/sidebar/sidebar_container_view.h | 23 +-- .../views/side_panel/side_panel_coordinator.h | 1 - 8 files changed, 119 insertions(+), 95 deletions(-) diff --git a/browser/ui/sidebar/sidebar_browsertest.cc b/browser/ui/sidebar/sidebar_browsertest.cc index 961ca1bcda2c..38319fa776cf 100644 --- a/browser/ui/sidebar/sidebar_browsertest.cc +++ b/browser/ui/sidebar/sidebar_browsertest.cc @@ -905,6 +905,18 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, TabSpecificAndGlobalPanelsTest) { WaitUntil(base::BindLambdaForTesting([&]() { return panel_ui->GetCurrentEntryId() == SidePanelEntryId::kBookmarks; })); + + // Check per-url contextual panel close. + // If current tab load another url, customize panel should be hidden. + tab_model()->ActivateTabAt(0); + WaitUntil(base::BindLambdaForTesting([&]() { + return panel_ui->GetCurrentEntryId() == SidePanelEntryId::kCustomizeChrome; + })); + + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), GURL("brave://newtab/"))); + WaitUntil(base::BindLambdaForTesting([&]() { + return panel_ui->GetCurrentEntryId() == SidePanelEntryId::kBookmarks; + })); } IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, DisabledItemsTest) { diff --git a/browser/ui/views/frame/brave_browser_view.cc b/browser/ui/views/frame/brave_browser_view.cc index 67bdb1036959..2931a2dc1ce3 100644 --- a/browser/ui/views/frame/brave_browser_view.cc +++ b/browser/ui/views/frame/brave_browser_view.cc @@ -779,6 +779,14 @@ WalletButton* BraveBrowserView::GetWalletButton() { return static_cast(toolbar())->wallet_button(); } +void BraveBrowserView::WillShowSidePanel() { + sidebar_container_view_->WillShowSidePanel(); +} + +void BraveBrowserView::WillDeregisterSidePanelEntry(SidePanelEntry* entry) { + sidebar_container_view_->WillDeregisterSidePanelEntry(entry); +} + void BraveBrowserView::NotifyDialogPositionRequiresUpdate() { GetBrowserViewLayout()->NotifyDialogPositionRequiresUpdate(); } diff --git a/browser/ui/views/frame/brave_browser_view.h b/browser/ui/views/frame/brave_browser_view.h index 59f5e042df31..ed60dccbb57a 100644 --- a/browser/ui/views/frame/brave_browser_view.h +++ b/browser/ui/views/frame/brave_browser_view.h @@ -57,6 +57,7 @@ class BraveBrowser; class BraveHelpBubbleHostView; class ContentsLayoutManager; class SidebarContainerView; +class SidePanelEntry; class SplitViewLocationBar; class SplitViewSeparator; class VerticalTabStripWidgetDelegateView; @@ -80,6 +81,8 @@ class BraveBrowserView : public BrowserView, void CloseWalletBubble(); WalletButton* GetWalletButton(); views::View* GetWalletButtonAnchorView(); + void WillShowSidePanel(); + void WillDeregisterSidePanelEntry(SidePanelEntry* entry); // Triggers layout of web modal dialogs void NotifyDialogPositionRequiresUpdate(); diff --git a/browser/ui/views/side_panel/brave_side_panel_coordinator.cc b/browser/ui/views/side_panel/brave_side_panel_coordinator.cc index dc3c5e23852f..38d0c31cfefb 100644 --- a/browser/ui/views/side_panel/brave_side_panel_coordinator.cc +++ b/browser/ui/views/side_panel/brave_side_panel_coordinator.cc @@ -42,6 +42,11 @@ void BraveSidePanelCoordinator::Show( sidebar::SetLastUsedSidePanel(browser_view_->GetProfile()->GetPrefs(), entry_key.id()); + // Notify to give opportunity to observe another panel entries from + // global or active tab's contextual registry. + auto* brave_browser_view = static_cast(browser_view_); + brave_browser_view->WillShowSidePanel(); + SidePanelCoordinator::Show(entry_key, open_trigger); } @@ -86,6 +91,13 @@ void BraveSidePanelCoordinator::Toggle() { void BraveSidePanelCoordinator::Toggle( SidePanelEntryKey key, SidePanelUtil::SidePanelOpenTrigger open_trigger) { + // Notify to give opportunity to observe another panel entries from + // global or active tab's contextual registry. + if (!IsSidePanelShowing()) { + auto* brave_browser_view = static_cast(browser_view_); + brave_browser_view->WillShowSidePanel(); + } + SidePanelCoordinator::Toggle(key, open_trigger); } @@ -155,6 +167,11 @@ void BraveSidePanelCoordinator::PopulateSidePanel( void BraveSidePanelCoordinator::NotifyPinnedContainerOfActiveStateChange( SidePanelEntryKey key, bool is_active) { + // // Notify to give opportunity to observe another panel entries from + // // global or active tab's contextual registry. + // auto* brave_browser_view = static_cast(browser_view_); + // brave_browser_view->WillShowSidePanel(); + if (!browser_view_->toolbar()->pinned_toolbar_actions_container()) { return; } @@ -162,3 +179,14 @@ void BraveSidePanelCoordinator::NotifyPinnedContainerOfActiveStateChange( SidePanelCoordinator::NotifyPinnedContainerOfActiveStateChange(key, is_active); } + +void BraveSidePanelCoordinator::OnEntryWillDeregister( + SidePanelRegistry* registry, + SidePanelEntry* entry) { + SidePanelCoordinator::OnEntryWillDeregister(registry, entry); + + // This could give the opportunity to stop observing from |entry| if + // this deregister happens while tab is still live. + auto* brave_browser_view = static_cast(browser_view_); + brave_browser_view->WillDeregisterSidePanelEntry(entry); +} diff --git a/browser/ui/views/side_panel/brave_side_panel_coordinator.h b/browser/ui/views/side_panel/brave_side_panel_coordinator.h index 642903b805b8..24347b804715 100644 --- a/browser/ui/views/side_panel/brave_side_panel_coordinator.h +++ b/browser/ui/views/side_panel/brave_side_panel_coordinator.h @@ -35,6 +35,8 @@ class BraveSidePanelCoordinator : public SidePanelCoordinator { const UniqueKey& unique_key, SidePanelEntry* entry, std::optional> content_view) override; + void OnEntryWillDeregister(SidePanelRegistry* registry, + SidePanelEntry* entry) override; void NotifyPinnedContainerOfActiveStateChange(SidePanelEntryKey key, bool is_active) override; diff --git a/browser/ui/views/sidebar/sidebar_container_view.cc b/browser/ui/views/sidebar/sidebar_container_view.cc index 099dad379204..a40af89ef92f 100644 --- a/browser/ui/views/sidebar/sidebar_container_view.cc +++ b/browser/ui/views/sidebar/sidebar_container_view.cc @@ -38,6 +38,7 @@ #include "chrome/browser/ui/exclusive_access/exclusive_access_manager.h" #include "chrome/browser/ui/exclusive_access/fullscreen_controller.h" #include "chrome/browser/ui/exclusive_access/fullscreen_within_tab_helper.h" +#include "chrome/browser/ui/tabs/public/tab_features.h" #include "chrome/browser/ui/tabs/tab_strip_model.h" #include "chrome/browser/ui/views/frame/browser_view.h" #include "chrome/browser/ui/views/side_panel/side_panel_entry.h" @@ -139,18 +140,6 @@ void SidebarContainerView::Init() { sidebar_model_observation_.Observe(sidebar_model_); browser_->tab_strip_model()->AddObserver(this); - auto* browser_view = BrowserView::GetBrowserViewForBrowser(browser_); - DCHECK(browser_view); - - auto* global_registry = side_panel_coordinator_->GetWindowRegistry(); - panel_registry_observations_.AddObservation(global_registry); - - for (const auto& entry : global_registry->entries()) { - DVLOG(1) << "Observing panel entry in ctor: " - << SidePanelEntryIdToString(entry->key().id()); - panel_entry_observations_.AddObservation(entry.get()); - } - show_side_panel_button_.Init( kShowSidePanelButton, browser_->profile()->GetPrefs(), base::BindRepeating(&SidebarContainerView::UpdateToolbarButtonVisibility, @@ -185,6 +174,29 @@ bool SidebarContainerView::IsSidebarVisible() const { return sidebar_control_view_ && sidebar_control_view_->GetVisible(); } +void SidebarContainerView::WillShowSidePanel() { + // It's good timing to start observing any panel entries + // from global and contextual if not yet observed. + auto* tab_model = browser_->tab_strip_model(); + auto* active_web_contents = tab_model->GetActiveWebContents(); + if (!active_web_contents) { + return; + } + StartObservingContextualSidePanelEntry(active_web_contents); + + auto* global_registry = side_panel_coordinator_->GetWindowRegistry(); + for (const auto& entry : global_registry->entries()) { + StartObservingForEntry(entry.get()); + } +} + +void SidebarContainerView::WillDeregisterSidePanelEntry(SidePanelEntry* entry) { + // If entry's life cycle is tied with tab, we stop observing from + // OnTabWillBeRemoved(). However, some entry could be deregistered while tab + // is live. In that case, we need to stop observing here explicitely. + StopObservingForEntry(entry); +} + bool SidebarContainerView::IsFullscreenForCurrentEntry() const { // For now, we only supports fullscreen from playlist. if (side_panel_coordinator_->GetCurrentEntryId() != @@ -792,26 +804,10 @@ void SidebarContainerView::OnEntryHidden(SidePanelEntry* entry) { } } -void SidebarContainerView::OnEntryRegistered(SidePanelRegistry* registry, - SidePanelEntry* entry) { - // Observe when it's shown or hidden - DVLOG(1) << "Observing panel entry in registry observer: " - << SidePanelEntryIdToString(entry->key().id()); - panel_entry_observations_.AddObservation(entry); -} - -void SidebarContainerView::OnEntryWillDeregister(SidePanelRegistry* registry, - SidePanelEntry* entry) { - // Stop observing - DVLOG(1) << "Unobserving panel entry in registry observer: " - << SidePanelEntryIdToString(entry->key().id()); - panel_entry_observations_.RemoveObservation(entry); -} - -void SidebarContainerView::OnRegistryDestroying(SidePanelRegistry* registry) { - if (panel_registry_observations_.IsObservingSource(registry)) { - StopObservingContextualSidePanelRegistry(registry); - } +void SidebarContainerView::OnTabWillBeRemoved(content::WebContents* contents, + int index) { + // At this time, we can stop observing as TabFeatures is available. + StopObservingContextualSidePanelEntry(contents); } void SidebarContainerView::UpdateActiveItemState() { @@ -866,70 +862,39 @@ void SidebarContainerView::OnTabStripModelChanged( } return; } - - if (change.type() == TabStripModelChange::kInserted) { - for (const auto& contents : change.GetInsert()->contents) { - StartObservingContextualSidePanelRegistry(contents.contents); - } - return; - } - - if (change.type() == TabStripModelChange::kRemoved) { - bool removed_for_deletion = - (change.GetRemove()->contents[0].remove_reason == - TabStripModelChange::RemoveReason::kDeleted); - // If the tab is removed for deletion the side panel registry has already - // been destroyed. We stop observing that registry in - // SidePanelRegistryObserver::OnRegistryDestroying override above. - if (!removed_for_deletion) { - for (const auto& contents : change.GetRemove()->contents) { - StopObservingContextualSidePanelRegistry(contents.contents); - } - } - return; - } } -void SidebarContainerView::StopObservingContextualSidePanelRegistry( +void SidebarContainerView::StopObservingContextualSidePanelEntry( content::WebContents* contents) { - auto* registry = SidePanelRegistry::GetDeprecated(contents); - StopObservingContextualSidePanelRegistry(registry); -} + auto* tab = tabs::TabInterface::GetFromContents(contents); + if (!tab->GetTabFeatures()) { + return; + } -void SidebarContainerView::StopObservingContextualSidePanelRegistry( - SidePanelRegistry* registry) { + auto* registry = tab->GetTabFeatures()->side_panel_registry(); if (!registry) { return; } - panel_registry_observations_.RemoveObservation(registry); - for (const auto& entry : registry->entries()) { - if (panel_entry_observations_.IsObservingSource(entry.get())) { - DVLOG(1) << "Removing panel entry observation from removed contextual " - "registry : " - << SidePanelEntryIdToString(entry->key().id()); - panel_entry_observations_.RemoveObservation(entry.get()); - } + StopObservingForEntry(entry.get()); } } -void SidebarContainerView::StartObservingContextualSidePanelRegistry( +void SidebarContainerView::StartObservingContextualSidePanelEntry( content::WebContents* contents) { - auto* registry = SidePanelRegistry::GetDeprecated(contents); - if (!registry) { + auto* tab = tabs::TabInterface::GetFromContents(contents); + if (!tab->GetTabFeatures()) { return; } - panel_registry_observations_.AddObservation(registry); + auto* registry = tab->GetTabFeatures()->side_panel_registry(); + if (!registry) { + return; + } for (const auto& entry : registry->entries()) { - if (!panel_entry_observations_.IsObservingSource(entry.get())) { - DVLOG(1) << "Observing existing panel entry from newly added contextual " - "registry : " - << SidePanelEntryIdToString(entry->key().id()); - panel_entry_observations_.AddObservation(entry.get()); - } + StartObservingForEntry(entry.get()); } SharedPinnedTabService* shared_pinned_tab_service = @@ -951,5 +916,21 @@ void SidebarContainerView::StartObservingContextualSidePanelRegistry( } } +void SidebarContainerView::StartObservingForEntry(SidePanelEntry* entry) { + if (!panel_entry_observations_.IsObservingSource(entry)) { + DVLOG(1) << "Observing panel entry: " + << SidePanelEntryIdToString(entry->key().id()); + panel_entry_observations_.AddObservation(entry); + } +} + +void SidebarContainerView::StopObservingForEntry(SidePanelEntry* entry) { + if (panel_entry_observations_.IsObservingSource(entry)) { + DVLOG(1) << "Removing panel entry observation: " + << SidePanelEntryIdToString(entry->key().id()); + panel_entry_observations_.RemoveObservation(entry); + } +} + BEGIN_METADATA(SidebarContainerView) END_METADATA diff --git a/browser/ui/views/sidebar/sidebar_container_view.h b/browser/ui/views/sidebar/sidebar_container_view.h index 4779ac335095..7a418bbce8b9 100644 --- a/browser/ui/views/sidebar/sidebar_container_view.h +++ b/browser/ui/views/sidebar/sidebar_container_view.h @@ -22,7 +22,6 @@ #include "chrome/browser/ui/tabs/tab_strip_model_observer.h" #include "chrome/browser/ui/views/side_panel/side_panel_coordinator.h" #include "chrome/browser/ui/views/side_panel/side_panel_entry_observer.h" -#include "chrome/browser/ui/views/side_panel/side_panel_registry_observer.h" #include "chrome/browser/ui/views/side_panel/side_panel_view_state_observer.h" #include "components/prefs/pref_member.h" #include "ui/events/event_observer.h" @@ -58,7 +57,6 @@ class SidebarContainerView public SidebarShowOptionsEventDetectWidget::Delegate, public sidebar::SidebarModel::Observer, public SidePanelEntryObserver, - public SidePanelRegistryObserver, public SidePanelViewStateObserver, public TabStripModelObserver { METADATA_HEADER(SidebarContainerView, views::View) @@ -80,6 +78,8 @@ class SidebarContainerView BraveSidePanel* side_panel() { return side_panel_; } + void WillShowSidePanel(); + void WillDeregisterSidePanelEntry(SidePanelEntry* entry); bool IsFullscreenForCurrentEntry() const; void set_operation_from_active_tab_change(bool tab_change) { @@ -123,18 +123,12 @@ class SidebarContainerView void OnEntryShown(SidePanelEntry* entry) override; void OnEntryHidden(SidePanelEntry* entry) override; - // SidePanelRegistryObserver: - void OnEntryRegistered(SidePanelRegistry* registry, - SidePanelEntry* entry) override; - void OnEntryWillDeregister(SidePanelRegistry* registry, - SidePanelEntry* entry) override; - void OnRegistryDestroying(SidePanelRegistry* registry) override; - // TabStripModelObserver: void OnTabStripModelChanged( TabStripModel* tab_strip_model, const TabStripModelChange& change, const TabStripSelectionChange& selection) override; + void OnTabWillBeRemoved(content::WebContents* contents, int index) override; // SidePanelViewStateObserver: void OnSidePanelDidClose() override; @@ -187,10 +181,10 @@ class SidebarContainerView void StartBrowserWindowEventMonitoring(); void StopBrowserWindowEventMonitoring(); - void StartObservingContextualSidePanelRegistry( - content::WebContents* contents); - void StopObservingContextualSidePanelRegistry(content::WebContents* contents); - void StopObservingContextualSidePanelRegistry(SidePanelRegistry* registry); + void StartObservingContextualSidePanelEntry(content::WebContents* contents); + void StopObservingContextualSidePanelEntry(content::WebContents* contents); + void StartObservingForEntry(SidePanelEntry* entry); + void StopObservingForEntry(SidePanelEntry* entry); void UpdateActiveItemState(); raw_ptr browser_ = nullptr; @@ -218,9 +212,6 @@ class SidebarContainerView sidebar_model_observation_{this}; base::ScopedMultiSourceObservation panel_entry_observations_{this}; - base::ScopedMultiSourceObservation - panel_registry_observations_{this}; }; #endif // BRAVE_BROWSER_UI_VIEWS_SIDEBAR_SIDEBAR_CONTAINER_VIEW_H_ diff --git a/chromium_src/chrome/browser/ui/views/side_panel/side_panel_coordinator.h b/chromium_src/chrome/browser/ui/views/side_panel/side_panel_coordinator.h index 4c102901ca2d..09cb0bdb9730 100644 --- a/chromium_src/chrome/browser/ui/views/side_panel/side_panel_coordinator.h +++ b/chromium_src/chrome/browser/ui/views/side_panel/side_panel_coordinator.h @@ -18,7 +18,6 @@ #include "chrome/browser/ui/tabs/tab_strip_model_observer.h" #include "chrome/browser/ui/views/side_panel/side_panel_entry.h" #include "chrome/browser/ui/views/side_panel/side_panel_registry.h" -#include "chrome/browser/ui/views/side_panel/side_panel_registry_observer.h" #include "chrome/browser/ui/views/side_panel/side_panel_ui.h" #include "chrome/browser/ui/views/side_panel/side_panel_util.h" #include "chrome/browser/ui/views/side_panel/side_panel_view_state_observer.h" From 0013246553b565e218f813001d058cb8e88dd7f1 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Tue, 22 Oct 2024 10:39:53 +0900 Subject: [PATCH 3/5] Merge pull request #26056 from brave/sidebar_deprecate_OnEntryWillDeregister Removed using SidePanelCoordinator::OnEntryWillDeregister --- browser/ui/views/frame/brave_browser_view.cc | 4 ---- browser/ui/views/frame/brave_browser_view.h | 1 - .../brave_side_panel_coordinator.cc | 11 --------- .../side_panel/brave_side_panel_coordinator.h | 3 --- .../views/sidebar/sidebar_container_view.cc | 23 +++++++++++++------ .../ui/views/sidebar/sidebar_container_view.h | 3 ++- 6 files changed, 18 insertions(+), 27 deletions(-) diff --git a/browser/ui/views/frame/brave_browser_view.cc b/browser/ui/views/frame/brave_browser_view.cc index 2931a2dc1ce3..28cd17d985a5 100644 --- a/browser/ui/views/frame/brave_browser_view.cc +++ b/browser/ui/views/frame/brave_browser_view.cc @@ -783,10 +783,6 @@ void BraveBrowserView::WillShowSidePanel() { sidebar_container_view_->WillShowSidePanel(); } -void BraveBrowserView::WillDeregisterSidePanelEntry(SidePanelEntry* entry) { - sidebar_container_view_->WillDeregisterSidePanelEntry(entry); -} - void BraveBrowserView::NotifyDialogPositionRequiresUpdate() { GetBrowserViewLayout()->NotifyDialogPositionRequiresUpdate(); } diff --git a/browser/ui/views/frame/brave_browser_view.h b/browser/ui/views/frame/brave_browser_view.h index ed60dccbb57a..d85ffbc2f233 100644 --- a/browser/ui/views/frame/brave_browser_view.h +++ b/browser/ui/views/frame/brave_browser_view.h @@ -82,7 +82,6 @@ class BraveBrowserView : public BrowserView, WalletButton* GetWalletButton(); views::View* GetWalletButtonAnchorView(); void WillShowSidePanel(); - void WillDeregisterSidePanelEntry(SidePanelEntry* entry); // Triggers layout of web modal dialogs void NotifyDialogPositionRequiresUpdate(); diff --git a/browser/ui/views/side_panel/brave_side_panel_coordinator.cc b/browser/ui/views/side_panel/brave_side_panel_coordinator.cc index 38d0c31cfefb..16295c36c53f 100644 --- a/browser/ui/views/side_panel/brave_side_panel_coordinator.cc +++ b/browser/ui/views/side_panel/brave_side_panel_coordinator.cc @@ -179,14 +179,3 @@ void BraveSidePanelCoordinator::NotifyPinnedContainerOfActiveStateChange( SidePanelCoordinator::NotifyPinnedContainerOfActiveStateChange(key, is_active); } - -void BraveSidePanelCoordinator::OnEntryWillDeregister( - SidePanelRegistry* registry, - SidePanelEntry* entry) { - SidePanelCoordinator::OnEntryWillDeregister(registry, entry); - - // This could give the opportunity to stop observing from |entry| if - // this deregister happens while tab is still live. - auto* brave_browser_view = static_cast(browser_view_); - brave_browser_view->WillDeregisterSidePanelEntry(entry); -} diff --git a/browser/ui/views/side_panel/brave_side_panel_coordinator.h b/browser/ui/views/side_panel/brave_side_panel_coordinator.h index 24347b804715..98fc1134fd4f 100644 --- a/browser/ui/views/side_panel/brave_side_panel_coordinator.h +++ b/browser/ui/views/side_panel/brave_side_panel_coordinator.h @@ -35,9 +35,6 @@ class BraveSidePanelCoordinator : public SidePanelCoordinator { const UniqueKey& unique_key, SidePanelEntry* entry, std::optional> content_view) override; - void OnEntryWillDeregister(SidePanelRegistry* registry, - SidePanelEntry* entry) override; - void NotifyPinnedContainerOfActiveStateChange(SidePanelEntryKey key, bool is_active) override; diff --git a/browser/ui/views/sidebar/sidebar_container_view.cc b/browser/ui/views/sidebar/sidebar_container_view.cc index a40af89ef92f..d34983f93b91 100644 --- a/browser/ui/views/sidebar/sidebar_container_view.cc +++ b/browser/ui/views/sidebar/sidebar_container_view.cc @@ -190,13 +190,6 @@ void SidebarContainerView::WillShowSidePanel() { } } -void SidebarContainerView::WillDeregisterSidePanelEntry(SidePanelEntry* entry) { - // If entry's life cycle is tied with tab, we stop observing from - // OnTabWillBeRemoved(). However, some entry could be deregistered while tab - // is live. In that case, we need to stop observing here explicitely. - StopObservingForEntry(entry); -} - bool SidebarContainerView::IsFullscreenForCurrentEntry() const { // For now, we only supports fullscreen from playlist. if (side_panel_coordinator_->GetCurrentEntryId() != @@ -804,6 +797,22 @@ void SidebarContainerView::OnEntryHidden(SidePanelEntry* entry) { } } +void SidebarContainerView::OnEntryWillHide(SidePanelEntry* entry, + SidePanelEntryHideReason reason) { + DVLOG(1) << "Panel will hide: " + << SidePanelEntryIdToString(entry->key().id()); + + // If |reason| is panel closing, we could deregister. And it'll be + // re-registered when panel is shown if that entry is still live in tab's + // registry. + // We only stop observing when |entry|'s panel is hidden by closing. + // If it's hidden by replacing with other panel, we shoudl not stop + // to know the timing that it's shown again. + if (reason == SidePanelEntryHideReason::kSidePanelClosed) { + StopObservingForEntry(entry); + } +} + void SidebarContainerView::OnTabWillBeRemoved(content::WebContents* contents, int index) { // At this time, we can stop observing as TabFeatures is available. diff --git a/browser/ui/views/sidebar/sidebar_container_view.h b/browser/ui/views/sidebar/sidebar_container_view.h index 7a418bbce8b9..bde960eafbb2 100644 --- a/browser/ui/views/sidebar/sidebar_container_view.h +++ b/browser/ui/views/sidebar/sidebar_container_view.h @@ -79,7 +79,6 @@ class SidebarContainerView BraveSidePanel* side_panel() { return side_panel_; } void WillShowSidePanel(); - void WillDeregisterSidePanelEntry(SidePanelEntry* entry); bool IsFullscreenForCurrentEntry() const; void set_operation_from_active_tab_change(bool tab_change) { @@ -122,6 +121,8 @@ class SidebarContainerView // SidePanelEntryObserver: void OnEntryShown(SidePanelEntry* entry) override; void OnEntryHidden(SidePanelEntry* entry) override; + void OnEntryWillHide(SidePanelEntry* entry, + SidePanelEntryHideReason reason) override; // TabStripModelObserver: void OnTabStripModelChanged( From 9e6b85ddea6ba99d0f646335d91ea4902d509f2b Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Thu, 24 Oct 2024 14:53:31 +0900 Subject: [PATCH 4/5] Merge pull request #26132 from brave/fix_sidebar_item_activation_state Fixed contextual panel item is not activated after attaching to window --- browser/ui/sidebar/sidebar_browsertest.cc | 15 +++++++++++++ .../brave_side_panel_coordinator.cc | 22 +++++-------------- .../side_panel/brave_side_panel_coordinator.h | 2 +- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/browser/ui/sidebar/sidebar_browsertest.cc b/browser/ui/sidebar/sidebar_browsertest.cc index 38319fa776cf..e885f391838d 100644 --- a/browser/ui/sidebar/sidebar_browsertest.cc +++ b/browser/ui/sidebar/sidebar_browsertest.cc @@ -1230,6 +1230,21 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTestWithAIChat, EXPECT_TRUE(model()->active_index().has_value()); EXPECT_EQ(model()->active_index(), model()->GetIndexOf(SidebarItem::BuiltInItemType::kBookmarks)); + + // Check tab specific panel item is still activated after moving to another + // browser. + tab_model()->ActivateTabAt(1); + EXPECT_EQ(model()->active_index(), tab_specific_item_index); + + auto* browser2 = CreateBrowser(browser()->profile()); + auto* browser2_model = + static_cast(browser2)->sidebar_controller()->model(); + auto* browser2_tab_model = browser2->tab_strip_model(); + + auto detached_tab = tab_model()->DetachTabAtForInsertion(1); + browser2_tab_model->AppendTab(std::move(detached_tab), /* foreground */ true); + EXPECT_EQ(browser2_model->active_index(), + browser2_model->GetIndexOf(SidebarItem::BuiltInItemType::kChatUI)); } IN_PROC_BROWSER_TEST_F(SidebarBrowserTestWithAIChat, diff --git a/browser/ui/views/side_panel/brave_side_panel_coordinator.cc b/browser/ui/views/side_panel/brave_side_panel_coordinator.cc index 16295c36c53f..a4fc9cade194 100644 --- a/browser/ui/views/side_panel/brave_side_panel_coordinator.cc +++ b/browser/ui/views/side_panel/brave_side_panel_coordinator.cc @@ -42,11 +42,6 @@ void BraveSidePanelCoordinator::Show( sidebar::SetLastUsedSidePanel(browser_view_->GetProfile()->GetPrefs(), entry_key.id()); - // Notify to give opportunity to observe another panel entries from - // global or active tab's contextual registry. - auto* brave_browser_view = static_cast(browser_view_); - brave_browser_view->WillShowSidePanel(); - SidePanelCoordinator::Show(entry_key, open_trigger); } @@ -91,13 +86,6 @@ void BraveSidePanelCoordinator::Toggle() { void BraveSidePanelCoordinator::Toggle( SidePanelEntryKey key, SidePanelUtil::SidePanelOpenTrigger open_trigger) { - // Notify to give opportunity to observe another panel entries from - // global or active tab's contextual registry. - if (!IsSidePanelShowing()) { - auto* brave_browser_view = static_cast(browser_view_); - brave_browser_view->WillShowSidePanel(); - } - SidePanelCoordinator::Toggle(key, open_trigger); } @@ -160,6 +148,11 @@ void BraveSidePanelCoordinator::PopulateSidePanel( return; } + // Notify to give opportunity to observe another panel entries from + // global or active tab's contextual registry. + auto* brave_browser_view = static_cast(browser_view_); + brave_browser_view->WillShowSidePanel(); + SidePanelCoordinator::PopulateSidePanel(supress_animations, unique_key, entry, std::move(content_view)); } @@ -167,11 +160,6 @@ void BraveSidePanelCoordinator::PopulateSidePanel( void BraveSidePanelCoordinator::NotifyPinnedContainerOfActiveStateChange( SidePanelEntryKey key, bool is_active) { - // // Notify to give opportunity to observe another panel entries from - // // global or active tab's contextual registry. - // auto* brave_browser_view = static_cast(browser_view_); - // brave_browser_view->WillShowSidePanel(); - if (!browser_view_->toolbar()->pinned_toolbar_actions_container()) { return; } diff --git a/browser/ui/views/side_panel/brave_side_panel_coordinator.h b/browser/ui/views/side_panel/brave_side_panel_coordinator.h index 98fc1134fd4f..a0f108b39d87 100644 --- a/browser/ui/views/side_panel/brave_side_panel_coordinator.h +++ b/browser/ui/views/side_panel/brave_side_panel_coordinator.h @@ -9,7 +9,7 @@ #include #include -#include "chrome/browser/ui/views/side_panel/side_panel_coordinator.h" // IWYU pragma: export +#include "chrome/browser/ui/views/side_panel/side_panel_coordinator.h" class BraveSidePanelCoordinator : public SidePanelCoordinator { public: From 68f54aa4dcc5ac8812e70a33cdf2897e8c447d8d Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Fri, 25 Oct 2024 13:46:26 +0900 Subject: [PATCH 5/5] Merge pull request #26194 from brave/fix_sidebar_asan_failure Fixed sidebar asan failure when closing browser window --- browser/ui/sidebar/sidebar_browsertest.cc | 16 ++++++++++++++++ browser/ui/views/frame/brave_browser_view.cc | 4 ++-- browser/ui/views/frame/brave_browser_view.h | 2 +- .../brave_side_panel_coordinator.cc | 19 +++++++++++++++++-- .../views/sidebar/sidebar_container_view.cc | 15 +++++++++++---- .../ui/views/sidebar/sidebar_container_view.h | 12 +++++++++++- 6 files changed, 58 insertions(+), 10 deletions(-) diff --git a/browser/ui/sidebar/sidebar_browsertest.cc b/browser/ui/sidebar/sidebar_browsertest.cc index e885f391838d..52be38c9cf64 100644 --- a/browser/ui/sidebar/sidebar_browsertest.cc +++ b/browser/ui/sidebar/sidebar_browsertest.cc @@ -284,6 +284,18 @@ class SidebarBrowserTest : public InProcessBrowserTest { return std::distance(items.cbegin(), iter); } + bool SidebarContainerObserving(SidePanelEntryId id) { + auto* sidebar_container_view = + static_cast(controller()->sidebar()); + for (const auto& entry : + sidebar_container_view->panel_entry_observations_.sources()) { + if (entry->key().id() == id) { + return true; + } + } + return false; + } + raw_ptr item_added_bubble_anchor_ = nullptr; std::unique_ptr run_loop_; base::WeakPtrFactory weak_factory_{this}; @@ -917,6 +929,10 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, TabSpecificAndGlobalPanelsTest) { WaitUntil(base::BindLambdaForTesting([&]() { return panel_ui->GetCurrentEntryId() == SidePanelEntryId::kBookmarks; })); + // As previous customize panel per-url panel, it's closed by deregistering + // after loading another url. Then, sidebar container should stop observing + // its entry. + EXPECT_FALSE(SidebarContainerObserving(SidePanelEntryId::kCustomizeChrome)); } IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, DisabledItemsTest) { diff --git a/browser/ui/views/frame/brave_browser_view.cc b/browser/ui/views/frame/brave_browser_view.cc index 28cd17d985a5..e252199152d6 100644 --- a/browser/ui/views/frame/brave_browser_view.cc +++ b/browser/ui/views/frame/brave_browser_view.cc @@ -779,8 +779,8 @@ WalletButton* BraveBrowserView::GetWalletButton() { return static_cast(toolbar())->wallet_button(); } -void BraveBrowserView::WillShowSidePanel() { - sidebar_container_view_->WillShowSidePanel(); +void BraveBrowserView::WillShowSidePanel(bool show_on_deregistered) { + sidebar_container_view_->WillShowSidePanel(show_on_deregistered); } void BraveBrowserView::NotifyDialogPositionRequiresUpdate() { diff --git a/browser/ui/views/frame/brave_browser_view.h b/browser/ui/views/frame/brave_browser_view.h index d85ffbc2f233..ec79310a5db5 100644 --- a/browser/ui/views/frame/brave_browser_view.h +++ b/browser/ui/views/frame/brave_browser_view.h @@ -81,7 +81,7 @@ class BraveBrowserView : public BrowserView, void CloseWalletBubble(); WalletButton* GetWalletButton(); views::View* GetWalletButtonAnchorView(); - void WillShowSidePanel(); + void WillShowSidePanel(bool show_on_deregistered); // Triggers layout of web modal dialogs void NotifyDialogPositionRequiresUpdate(); diff --git a/browser/ui/views/side_panel/brave_side_panel_coordinator.cc b/browser/ui/views/side_panel/brave_side_panel_coordinator.cc index a4fc9cade194..4750019371db 100644 --- a/browser/ui/views/side_panel/brave_side_panel_coordinator.cc +++ b/browser/ui/views/side_panel/brave_side_panel_coordinator.cc @@ -151,8 +151,23 @@ void BraveSidePanelCoordinator::PopulateSidePanel( // Notify to give opportunity to observe another panel entries from // global or active tab's contextual registry. auto* brave_browser_view = static_cast(browser_view_); - brave_browser_view->WillShowSidePanel(); - + CHECK(browser_view_->unified_side_panel()->children().size() == 1); + const auto content_wrapper = + browser_view_->unified_side_panel()->children()[0]; + bool show_on_deregistered = false; + // If current entry is not null and its content is already removed from + // wrapper when new panel is about to be shown, new panel is shown by + // deregistering current panel. See the comment of + // SidebarContainerView::WillShowSidePanel() about why |show_on_deregistered| + // is needed. + // Instead, we can override base class' Show(const UniqueKey& entry, ...) + // method as we can know whether it's shown by deregistering with + // |open_trigger| arg. However, need patching to make it virtual as base class + // already have overridden same name methods. + if (current_entry_.get() && content_wrapper->children().empty()) { + show_on_deregistered = true; + } + brave_browser_view->WillShowSidePanel(show_on_deregistered); SidePanelCoordinator::PopulateSidePanel(supress_animations, unique_key, entry, std::move(content_view)); } diff --git a/browser/ui/views/sidebar/sidebar_container_view.cc b/browser/ui/views/sidebar/sidebar_container_view.cc index d34983f93b91..d38f54f2887f 100644 --- a/browser/ui/views/sidebar/sidebar_container_view.cc +++ b/browser/ui/views/sidebar/sidebar_container_view.cc @@ -174,7 +174,9 @@ bool SidebarContainerView::IsSidebarVisible() const { return sidebar_control_view_ && sidebar_control_view_->GetVisible(); } -void SidebarContainerView::WillShowSidePanel() { +void SidebarContainerView::WillShowSidePanel(bool show_on_deregistered) { + deregister_when_hidden_ = show_on_deregistered; + // It's good timing to start observing any panel entries // from global and contextual if not yet observed. auto* tab_model = browser_->tab_strip_model(); @@ -764,8 +766,13 @@ void SidebarContainerView::OnEntryShown(SidePanelEntry* entry) { } void SidebarContainerView::OnEntryHidden(SidePanelEntry* entry) { - // Make sure item is deselected DVLOG(1) << "Panel hidden: " << SidePanelEntryIdToString(entry->key().id()); + + if (deregister_when_hidden_) { + StopObservingForEntry(entry); + deregister_when_hidden_ = false; + } + auto* controller = browser_->sidebar_controller(); // Handling if |entry| is managed one. @@ -806,10 +813,10 @@ void SidebarContainerView::OnEntryWillHide(SidePanelEntry* entry, // re-registered when panel is shown if that entry is still live in tab's // registry. // We only stop observing when |entry|'s panel is hidden by closing. - // If it's hidden by replacing with other panel, we shoudl not stop + // If it's hidden by replacing with other panel, we should not stop // to know the timing that it's shown again. if (reason == SidePanelEntryHideReason::kSidePanelClosed) { - StopObservingForEntry(entry); + deregister_when_hidden_ = true; } } diff --git a/browser/ui/views/sidebar/sidebar_container_view.h b/browser/ui/views/sidebar/sidebar_container_view.h index bde960eafbb2..21ef93847b29 100644 --- a/browser/ui/views/sidebar/sidebar_container_view.h +++ b/browser/ui/views/sidebar/sidebar_container_view.h @@ -78,7 +78,16 @@ class SidebarContainerView BraveSidePanel* side_panel() { return side_panel_; } - void WillShowSidePanel(); + // Need to know this showing comes from deregistering current entry + // or not. We're observing contextual/global panel entries when panel is + // shown. And stop observing when it's hidden by closing. Unfortunately, + // OnEntryWillHide() is not called when current entry is closed by + // deregistering. Only OnEntryHidden() is called. + // By using OnEntryWillHide()'s arg, we can know this hidden is from + // closing or not. Fortunately, we can know when WillShowSidePanel() + // is called whether it's closing from deregistration. + // With |show_on_deregistered|, we can stop observing OnEntryHidden(). + void WillShowSidePanel(bool show_on_deregistered); bool IsFullscreenForCurrentEntry() const; void set_operation_from_active_tab_change(bool tab_change) { @@ -196,6 +205,7 @@ class SidebarContainerView bool initialized_ = false; bool sidebar_on_left_ = true; bool operation_from_active_tab_change_ = false; + bool deregister_when_hidden_ = false; base::OneShotTimer sidebar_hide_timer_; sidebar::SidebarService::ShowSidebarOption show_sidebar_option_ = sidebar::SidebarService::ShowSidebarOption::kShowAlways;