From 5b03a4e7aa6f35ebaf714db7d998b45ec43a7cda Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Wed, 2 Oct 2024 19:35:35 +0900 Subject: [PATCH] Refactor SidebarContainerView fix https://github.com/brave/brave-browser/issues/41342 As upstream deleted SidebarRegistryObserver in ToT, SidebarContainerView also should not rely on. Instead, SidePanelCoordinator notifies directly to SidebarContainerView when we can start/stop panel entry observing. --- 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 | 21 +++ .../side_panel/brave_side_panel_coordinator.h | 2 + .../views/sidebar/sidebar_container_view.cc | 167 ++++++------------ .../ui/views/sidebar/sidebar_container_view.h | 27 +-- .../views/side_panel/side_panel_coordinator.h | 1 - 8 files changed, 110 insertions(+), 131 deletions(-) diff --git a/browser/ui/sidebar/sidebar_browsertest.cc b/browser/ui/sidebar/sidebar_browsertest.cc index ee32db14655b..7f7dc9de032b 100644 --- a/browser/ui/sidebar/sidebar_browsertest.cc +++ b/browser/ui/sidebar/sidebar_browsertest.cc @@ -907,6 +907,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 c4c16b217aa2..41045afdb238 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..1e2196066b52 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,11 @@ 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. + auto* brave_browser_view = static_cast(browser_view_); + brave_browser_view->WillShowSidePanel(); + SidePanelCoordinator::Toggle(key, open_trigger); } @@ -162,3 +172,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 2c4785aee2b3..15685ddf0f9d 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, @@ -186,6 +175,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() != @@ -793,26 +805,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() { @@ -835,102 +831,37 @@ void SidebarContainerView::OnSidePanelDidClose() { UpdateActiveItemState(); } -void SidebarContainerView::OnTabStripModelChanged( - TabStripModel* tab_strip_model, - const TabStripModelChange& change, - const TabStripSelectionChange& selection) { - if ((change.type() == TabStripModelChange::kReplaced)) { - // Pre-cr129's change - // https://chromium.googlesource.com/chromium/src/+/2fd6b53ce, we would - // handle shared pinned tab moving from one window to another here by - // starting to observe the new contents registry and stoping observing the - // old contents registry. But since the registry is no longer associated - // with the contents and is now associated with the tab instead we don't - // need to do the swap here. However, we may need to take some action here - // to fix https://github.com/brave/brave-browser/issues/40681. - - // For AI Chat, if the contents got replaced then the AI Chat UI associated - // with that contetnts will no longer work, so just close it. - auto* replace = change.GetReplace(); - // old_contents is already removed from the tab, so use the new_contents to - // get the registry. - auto* registry = SidePanelRegistry::GetDeprecated(replace->new_contents); - if (registry) { - if (auto* entry = registry->GetEntryForKey( - SidePanelEntry::Key(SidePanelEntryId::kChatUI))) { - if (side_panel_coordinator_->IsSidePanelEntryShowing(entry->key())) { - side_panel_coordinator_->Close(); - } else { - entry->ClearCachedView(); - } - } - } +void SidebarContainerView::StopObservingContextualSidePanelEntry( + content::WebContents* contents) { + auto* tab = tabs::TabInterface::GetFromContents(contents); + if (!tab->GetTabFeatures()) { return; } - if (change.type() == TabStripModelChange::kInserted) { - for (const auto& contents : change.GetInsert()->contents) { - StartObservingContextualSidePanelRegistry(contents.contents); - } + auto* registry = tab->GetTabFeatures()->side_panel_registry(); + if (!registry) { 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; + for (const auto& entry : registry->entries()) { + StopObservingForEntry(entry.get()); } } -void SidebarContainerView::StopObservingContextualSidePanelRegistry( +void SidebarContainerView::StartObservingContextualSidePanelEntry( content::WebContents* contents) { - auto* registry = SidePanelRegistry::GetDeprecated(contents); - StopObservingContextualSidePanelRegistry(registry); -} - -void SidebarContainerView::StopObservingContextualSidePanelRegistry( - SidePanelRegistry* registry) { - if (!registry) { + auto* tab = tabs::TabInterface::GetFromContents(contents); + if (!tab->GetTabFeatures()) { 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()); - } - } -} - -void SidebarContainerView::StartObservingContextualSidePanelRegistry( - content::WebContents* contents) { - auto* registry = SidePanelRegistry::GetDeprecated(contents); + auto* registry = tab->GetTabFeatures()->side_panel_registry(); if (!registry) { return; } - panel_registry_observations_.AddObservation(registry); - 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 = @@ -952,6 +883,22 @@ 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); + } +} + BraveBrowser* SidebarContainerView::GetBraveBrowser() const { return static_cast(browser_.get()); } diff --git a/browser/ui/views/sidebar/sidebar_container_view.h b/browser/ui/views/sidebar/sidebar_container_view.h index 332a2b6454e7..f033f443a09d 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,8 @@ 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 +177,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(); // Casts |browser_| to BraveBrowser, as storing it as BraveBrowser would cause @@ -222,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 ab56f04fb831..9dbafde3487e 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"