Skip to content

Commit

Permalink
[Side Panel] Remove extension panel type checks in SidePanelCoordinator
Browse files Browse the repository at this point in the history 
This CL refactors SidePanelCoordinator to not explicitly check if a
SidePanelEntry is from an extension. Many of the behavior changes can
be reframed as "reasonable for panel types that are registered in both
global and contextual registries".

Change the fallback order of panels shown in OnTabStripModelChanged
(if a panel is showing in the old tab):
 - 1) the new tab's registry's active entry
 - if an entry with the same key as the active panel exists in the
   global registry:
   - 2) the new tab's registry's entry with the same key
   - 3) the global registry's entry with the same key
 - 4) the global registry's active entry
 - 5) none (close the side panel)

One-pager (internal): https://docs.google.com/document/d/1VkRaJ17vpjEcHMnuvz55wQe6tm8s7vyetw941E307IM/edit?usp=sharing&resourcekey=0-LyJbbUCObtlrltN47iMYEQ

Bug: 1403745
Change-Id: I95ee855714c47ad8aec55bde655d2625869cde51
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4207328
Reviewed-by: Caroline Rising <corising@chromium.org>
Commit-Queue: Kelvin Jiang <kelvinjiang@chromium.org>
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1108735}
  • Loading branch information
@Celsius273
Celsius273 authored and Chromium LUCI CQ committed Feb 23, 2023
1 parent ee1f5db commit 40a4e45
Show file tree
Hide file tree
Showing 3 changed files with 237 additions and 127 deletions.
128 changes: 61 additions & 67 deletions chrome/browser/ui/views/side_panel/side_panel_coordinator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,6 @@ constexpr int kSidePanelContentWrapperViewId = 43;

constexpr SidePanelEntry::Id kDefaultEntry = SidePanelEntry::Id::kReadingList;

bool IsExtensionEntry(SidePanelEntry* entry) {
return entry->key().id() == SidePanelEntry::Id::kExtension;
}

std::unique_ptr<views::ImageButton> CreateControlButton(
views::View* host,
base::RepeatingClosure pressed_callback,
Expand Down Expand Up @@ -621,79 +617,72 @@ void SidePanelCoordinator::SetSelectedEntryInCombobox(
header_combobox_->SchedulePaint();
}

bool SidePanelCoordinator::ShouldRemoveExtensionFromComboboxOnDeregister(
SidePanelRegistry* registry,
bool SidePanelCoordinator::ShouldRemoveFromComboboxOnDeregister(
SidePanelRegistry* deregistering_registry,
const SidePanelEntry::Key& key) {
// Remove the extension entry from the combobox if one of these conditions
// are met:
// Remove the entry from the combobox if one of these conditions are met:
// - The entry will be deregistered from the global registry and there's no
// entry for the extension in the active contextual registry.
// entry with the same key in the active contextual registry.
// - The entry will be deregistered from a contextual registry and there's
// no entry for the extension in the global registry.
bool remove_if_global =
registry == global_registry_ && !GetActiveContextualEntryForKey(key);
bool remove_if_contextual = registry == GetActiveContextualRegistry() &&
!global_registry_->GetEntryForKey(key);
// no entry with the same key in the global registry.
bool remove_if_global = deregistering_registry == global_registry_ &&
!GetActiveContextualEntryForKey(key);
bool remove_if_contextual =
deregistering_registry == GetActiveContextualRegistry() &&
!global_registry_->GetEntryForKey(key);

return remove_if_global || remove_if_contextual;
}

void SidePanelCoordinator::OnActiveExtensionEntryWillDeregister(
SidePanelRegistry* registry,
SidePanelEntry* SidePanelCoordinator::GetNewActiveEntryOnDeregister(
SidePanelRegistry* deregistering_registry,
const SidePanelEntry::Key& key) {
if (registry == global_registry_) {
if (auto* contextual_entry = GetActiveContextualEntryForKey(key)) {
// If the global extension entry is being deregistered and there
// exists an entry for the current tab, check that the active
// contextual entry for the extension is being shown.
DCHECK_EQ(current_entry_.get(), contextual_entry);
} else {
// Otherwise, close the side panel.
Close();
}
} else {
if (SidePanelEntry* global_extension_entry =
global_registry_->GetEntryForKey(key)) {
// If the contextual extension entry is being deregistered and there
// exists a global entry for this extension. Show this extension's
// global entry.
Show(global_extension_entry,
SidePanelUtil::SidePanelOpenTrigger::kSidePanelEntryDeregistered);
} else if (global_registry_->active_entry().has_value()) {
Show(GetLastActiveEntryKey().value_or(SidePanelEntry::Key(kDefaultEntry)),
SidePanelUtil::SidePanelOpenTrigger::kSidePanelEntryDeregistered);
} else {
Close();
}
// This function should only be called when the side panel view is shown.
DCHECK(GetContentView());

// Attempt to return an entry in the following fallback order: global entry
// for `key` if a contextual entry is deregistered > active global entry >
// null.
if (deregistering_registry == GetActiveContextualRegistry() &&
global_registry_->GetEntryForKey(key)) {
return global_registry_->GetEntryForKey(key);
}

return global_registry_->active_entry().value_or(nullptr);
}

SidePanelEntry* SidePanelCoordinator::GetNewActiveEntryOnTabChanged() {
// This function should only be called when the side panel view is shown.
DCHECK(GetContentView());

// If the current entry is an extension entry, attempt to return an entry in
// the following fallback order: extension's contextual entry for the new tab
// > extension's global entry. If neither exist, continue with the default
// fallback order.
if (current_entry_ && IsExtensionEntry(current_entry_.get())) {
if (auto* new_contextual_or_global_extension_entry =
GetEntryForKey(current_entry_->key())) {
return new_contextual_or_global_extension_entry;
}
}

// Attempt to return an entry in the following fallback order: new tab's
// active contextual entry > active global entry > null.
// Attempt to return an entry in the following fallback order:
// - the new tab's registry's active entry
// - if the active entry's key is registered in the global registry:
// - the new tab's registry's entry with the same key
// - the global registry's entry with the same key (note that
// GetEntryForKey will return this fallback order)
// - if there is an active entry in the global registry:
// - the new tab's registry's entry with the same key
// - the global registry's active entry (note that GetEntryForKey will
// return this fallback order)
// - no entry (this closes the side panel)
// Note: GetActiveContextualRegistry() returns the registry for the new tab in
// this function.
if (GetActiveContextualRegistry() &&
GetActiveContextualRegistry()->active_entry().has_value()) {
return GetActiveContextualRegistry()->active_entry().value();
// Note: If Show() is called with an entry returned by this function, then
// that entry will be active in its owning registry.
auto* active_contextual_registry = GetActiveContextualRegistry();
if (active_contextual_registry &&
active_contextual_registry->active_entry()) {
return *active_contextual_registry->active_entry();
}

if (current_entry_ &&
global_registry_->GetEntryForKey(current_entry_->key())) {
return GetEntryForKey(current_entry_->key());
}

return global_registry_->active_entry()
? global_registry_->active_entry().value()
? GetEntryForKey((*global_registry_->active_entry())->key())
: nullptr;
}

Expand All @@ -705,9 +694,9 @@ void SidePanelCoordinator::OnEntryRegistered(SidePanelRegistry* registry,
GetLastActiveEntryKey().value_or(SidePanelEntry::Key(kDefaultEntry)));
}

// If `entry` is a contextual extension entry and the global entry for the
// same extension is currently being shown, show the new `entry`.
if (IsExtensionEntry(entry) && registry == GetActiveContextualRegistry() &&
// If `entry` is a contextual entry and the global entry with the same key is
// currently being shown, show the new `entry`.
if (registry == GetActiveContextualRegistry() &&
IsGlobalEntryShowing(entry->key())) {
Show(entry, SidePanelUtil::SidePanelOpenTrigger::kExtensionEntryRegistered);
}
Expand All @@ -716,8 +705,7 @@ void SidePanelCoordinator::OnEntryRegistered(SidePanelRegistry* registry,
void SidePanelCoordinator::OnEntryWillDeregister(SidePanelRegistry* registry,
SidePanelEntry* entry) {
absl::optional<SidePanelEntry::Key> selected_key = GetSelectedKey();
if (!IsExtensionEntry(entry) ||
ShouldRemoveExtensionFromComboboxOnDeregister(registry, entry->key())) {
if (ShouldRemoveFromComboboxOnDeregister(registry, entry->key())) {
combobox_model_->RemoveItem(entry->key());

if (GetContentView()) {
Expand All @@ -739,10 +727,16 @@ void SidePanelCoordinator::OnEntryWillDeregister(SidePanelRegistry* registry,
// that has been visible.
if (GetContentView() && selected_key.has_value() &&
selected_key.value() == entry->key()) {
if (IsExtensionEntry(entry)) {
OnActiveExtensionEntryWillDeregister(registry, entry->key());
} else if (global_registry_->active_entry().has_value()) {
Show(GetLastActiveEntryKey().value_or(SidePanelEntry::Key(kDefaultEntry)),
// If a global entry is deregistered but a contextual entry with the same
// key is shown, do nothing.
if (registry == global_registry_ &&
GetActiveContextualEntryForKey(entry->key())) {
return;
}

if (auto* new_active_entry =
GetNewActiveEntryOnDeregister(registry, entry->key())) {
Show(new_active_entry,
SidePanelUtil::SidePanelOpenTrigger::kSidePanelEntryDeregistered);
} else {
Close();
Expand Down Expand Up @@ -819,7 +813,7 @@ void SidePanelCoordinator::OnTabStripModelChanged(
}
} else if (new_contextual_registry &&
new_contextual_registry->active_entry().has_value()) {
Show(new_contextual_registry->active_entry().value()->key().id(),
Show(new_contextual_registry->active_entry().value(),
SidePanelUtil::SidePanelOpenTrigger::kTabChanged);
}
}
Expand Down
32 changes: 17 additions & 15 deletions chrome/browser/ui/views/side_panel/side_panel_coordinator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ class SidePanelCoordinator final : public SidePanelRegistryObserver,
FRIEND_TEST_ALL_PREFIXES(UserNoteUICoordinatorTest,
PopulateUserNoteSidePanel);

// Unlike Show() which takes in a SidePanelEntry's id or key, this version
// Unlike `Show()` which takes in a SidePanelEntry's id or key, this version
// should only be used for the rare case when we need to show a particular
// entry instead of letting GetEntryForKey() decide for us.
void Show(SidePanelEntry* entry,
Expand Down Expand Up @@ -129,7 +129,7 @@ class SidePanelCoordinator final : public SidePanelRegistryObserver,

// Removes existing SidePanelEntry contents from the side panel if any exist
// and populates the side panel with the provided SidePanelEntry and
// |content_view| if provided, otherwise get the content_view from the
// `content_view` if provided, otherwise get the content_view from the
// provided SidePanelEntry.
void PopulateSidePanel(
SidePanelEntry* entry,
Expand Down Expand Up @@ -160,22 +160,24 @@ class SidePanelCoordinator final : public SidePanelRegistryObserver,
// Sets the entry corresponding to `entry_key` as selected in the combobox.
void SetSelectedEntryInCombobox(const SidePanelEntry::Key& entry_key);

// Determines if the extension's entry in the combobox should be removed when
// one of its entries is deregistered. Called from OnEntryWillDeregister().
bool ShouldRemoveExtensionFromComboboxOnDeregister(
SidePanelRegistry* registry,
// Determines if the entry in the combobox should be removed when it is
// deregistered. Called from `OnEntryWillDeregister()`.
bool ShouldRemoveFromComboboxOnDeregister(
SidePanelRegistry* deregistering_registry,
const SidePanelEntry::Key& entry_key);

// Finds and shows an appropriate fallback entry or closes the side panel.
// Called if the entry being deregistered is an active extension entry in
// OnEntryWillDeregister().
void OnActiveExtensionEntryWillDeregister(
SidePanelRegistry* registry,
const SidePanelEntry::Key& entry_key);
// Returns the new entry to be shown after the active entry is deregistered,
// or nullptr if no suitable entry is found. Called from
// `OnEntryWillDeregister()` when there's an active entry being shown in the
// side panel.
SidePanelEntry* GetNewActiveEntryOnDeregister(
SidePanelRegistry* deregistering_registry,
const SidePanelEntry::Key& key);

// Returns the new entry to be shown after the active tab has changed, or
// nullptr if no suitable entry is found. Called from OnTabStripModelChanged()
// when there's an active entry being shown in the side panel.
// nullptr if no suitable entry is found. Called from
// `OnTabStripModelChanged()` when there's an active entry being shown in the
// side panel.
SidePanelEntry* GetNewActiveEntryOnTabChanged();

// SidePanelRegistryObserver:
Expand Down Expand Up @@ -215,7 +217,7 @@ class SidePanelCoordinator final : public SidePanelRegistryObserver,
// automatically if the entry is destroyed.
base::WeakPtr<SidePanelEntry> current_entry_;

// Used to update SidePanelEntry options in the header_combobox_ based on
// Used to update SidePanelEntry options in the `header_combobox_` based on
// their availability in the observed side panel registries.
std::unique_ptr<SidePanelComboboxModel> combobox_model_;
raw_ptr<views::Combobox, DanglingUntriaged> header_combobox_ = nullptr;
Expand Down

0 comments on commit 40a4e45

Please sign in to comment.