From ed498e39a6385bdc87d48ac793c4ade96196c137 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Thu, 17 Oct 2024 12:45:45 +0900 Subject: [PATCH] Removed using SidePanelCoordinator::OnEntryWillDeregister fix https://github.com/brave/brave-browser/issues/41748 In cr132, SidePanelCoordinator::OnEntryWillDeregister() is removed. We relied on it to deregister observed entry from SidebarContainerView. Instead, we'll use OnEntryWillHide() callback. It'll be called when side panel is closed. At that time, we could remove observing it safely, and will start to observe again if that entry is still live in tab's registry. --- 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 41045afdb238..b40c911bcb4a 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 d2aa2c3fffea..f0c8916c8369 100644 --- a/browser/ui/views/sidebar/sidebar_container_view.cc +++ b/browser/ui/views/sidebar/sidebar_container_view.cc @@ -191,13 +191,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() != @@ -809,6 +802,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 bac8d07555dc..c14b95343a34 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(