Skip to content

Commit

Permalink
Refactor SidebarContainerView
Browse files Browse the repository at this point in the history 
fix brave/brave-browser#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.
  • Loading branch information
@simonhong @emerick
simonhong authored and emerick committed Oct 25, 2024
1 parent 42cd9e4 commit 5b03a4e
Show file tree
Hide file tree
Showing 8 changed files with 110 additions and 131 deletions.
12 changes: 12 additions & 0 deletions browser/ui/sidebar/sidebar_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
8 changes: 8 additions & 0 deletions browser/ui/views/frame/brave_browser_view.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -779,6 +779,14 @@ WalletButton* BraveBrowserView::GetWalletButton() {
return static_cast<BraveToolbarView*>(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();
}
Expand Down
3 changes: 3 additions & 0 deletions browser/ui/views/frame/brave_browser_view.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class BraveBrowser;
class BraveHelpBubbleHostView;
class ContentsLayoutManager;
class SidebarContainerView;
class SidePanelEntry;
class SplitViewLocationBar;
class SplitViewSeparator;
class VerticalTabStripWidgetDelegateView;
Expand All @@ -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();
Expand Down
21 changes: 21 additions & 0 deletions browser/ui/views/side_panel/brave_side_panel_coordinator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<BraveBrowserView*>(browser_view_);
brave_browser_view->WillShowSidePanel();

SidePanelCoordinator::Show(entry_key, open_trigger);
}

Expand Down Expand Up @@ -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<BraveBrowserView*>(browser_view_);
brave_browser_view->WillShowSidePanel();

SidePanelCoordinator::Toggle(key, open_trigger);
}

Expand Down Expand Up @@ -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<BraveBrowserView*>(browser_view_);
brave_browser_view->WillDeregisterSidePanelEntry(entry);
}
2 changes: 2 additions & 0 deletions browser/ui/views/side_panel/brave_side_panel_coordinator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ class BraveSidePanelCoordinator : public SidePanelCoordinator {
const UniqueKey& unique_key,
SidePanelEntry* entry,
std::optional<std::unique_ptr<views::View>> content_view) override;
void OnEntryWillDeregister(SidePanelRegistry* registry,
SidePanelEntry* entry) override;

void NotifyPinnedContainerOfActiveStateChange(SidePanelEntryKey key,
bool is_active) override;
Expand Down
167 changes: 57 additions & 110 deletions browser/ui/views/sidebar/sidebar_container_view.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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() !=
Expand Down Expand Up @@ -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() {
Expand All @@ -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 =
Expand All @@ -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<BraveBrowser*>(browser_.get());
}
Expand Down
27 changes: 7 additions & 20 deletions browser/ui/views/sidebar/sidebar_container_view.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -222,9 +212,6 @@ class SidebarContainerView
sidebar_model_observation_{this};
base::ScopedMultiSourceObservation<SidePanelEntry, SidePanelEntryObserver>
panel_entry_observations_{this};
base::ScopedMultiSourceObservation<SidePanelRegistry,
SidePanelRegistryObserver>
panel_registry_observations_{this};
};

#endif // BRAVE_BROWSER_UI_VIEWS_SIDEBAR_SIDEBAR_CONTAINER_VIEW_H_
Loading

0 comments on commit 5b03a4e

Please sign in to comment.