[SidePanelPinning] Add a new toggle API to control the action callback for sidepanels. [chromium/src : main]

Shibalik Mohapatra (Gerrit)

unread,

Oct 23, 2023, 7:30:08 PM10/23/23

to Caroline Rising, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Caroline Rising.

Shibalik Mohapatra would like Caroline Rising to review this change.

[SidePanelPinning] Add a new toggle API to control the action callback for sidepanels.

Change-Id: I2b6c68691b0d426621ee42fe10ea5e6cf056b1ae
---
M chrome/browser/ui/side_panel/side_panel_ui.h
M chrome/browser/ui/views/frame/browser_actions.cc
M chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.cc
M chrome/browser/ui/views/side_panel/side_panel_coordinator.cc
M chrome/browser/ui/views/side_panel/side_panel_coordinator.h
5 files changed, 138 insertions(+), 31 deletions(-)

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Shibalik Mohapatra (Gerrit)

unread,

Oct 23, 2023, 7:30:13 PM10/23/23

to chromium-a...@chromium.org, extension...@chromium.org, Caroline Rising, chromium...@chromium.org

Attention is currently required from: Caroline Rising.

View Change

1 comment:

Patchset:
- Patch Set #1:
  WIP CL Relies on the pinnable cl to land. Manually tested with the CL though!

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Caroline Rising (Gerrit)

unread,

Oct 24, 2023, 10:54:02 AM10/24/23

to Shibalik Mohapatra, chromium-a...@chromium.org, extension...@chromium.org, chromium...@chromium.org

Attention is currently required from: Shibalik Mohapatra.

View Change

4 comments:

File chrome/browser/ui/views/side_panel/side_panel_coordinator.h:
- Patch Set #1, Line 14: #include "chrome/browser/ui/actions/chrome_action_id.h"
  is this needed?
- Patch Set #1, Line 32:
```
namespace actions {
class ActionItem;
}  // namespace actions
```
  nit: is this needed if you're including actions.h above?

File chrome/browser/ui/views/side_panel/side_panel_coordinator.cc:

Patch Set #1, Line 424:

if (IsSidePanelEntryShowing(key.value())) {
    Close();
    return;
  }

  if (IsSidePanelShowing()) {
    SidePanelContentSwappingContainer* content_wrapper =
        static_cast<SidePanelContentSwappingContainer*>(
            GetContentContainerView()->GetViewByID(
                kSidePanelContentWrapperViewId));

    if (content_wrapper->loading_entry() == GetEntryForKey(key.value())) {
      content_wrapper->ResetLoadingEntryIfNecessary();
      Close();
      return;
    }
  }

Could you add a comment in here to explain that we close if the entry is already showing or loading?

Patch Set #1, Line 442:
```
if (browser_view_->browser()->window()->IsFeaturePromoActive(
          feature_engagement::kIPHPowerBookmarksSidePanelFeature)) {
    Show(absl::make_optional(SidePanelEntry::Id::kBookmarks), open_trigger);
    return;
  }
```
This doesn't seem right anymore without the side panel button. With Mickey's changes does this even anchor to the button anymore? With this, if the promo was showing and a user had clicked the pinned reading list button wouldn't that mean we would then show bookmarks instead? That doesn't seem right.

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Shibalik Mohapatra (Gerrit)

unread,

Oct 24, 2023, 5:56:53 PM10/24/23

to chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Shibalik Mohapatra.

Shibalik Mohapatra uploaded patch set #2 to this change.

View Change

[SidePanelPinning] Add a new toggle API to control the action callback for sidepanels.

Change-Id: I2b6c68691b0d426621ee42fe10ea5e6cf056b1ae
---
M chrome/browser/ui/side_panel/side_panel_ui.h
M chrome/browser/ui/views/frame/browser_actions.cc
M chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.cc

M chrome/browser/ui/views/side_panel/search_companion/search_companion_side_panel_coordinator.cc
M chrome/browser/ui/views/side_panel/search_companion/search_companion_side_panel_coordinator.h
M chrome/browser/ui/views/side_panel/side_panel_coordinator.cc
M chrome/browser/ui/views/side_panel/side_panel_coordinator.h
M chrome/browser/ui/views/side_panel/side_panel_coordinator_unittest.cc
8 files changed, 68 insertions(+), 10 deletions(-)

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Shibalik Mohapatra (Gerrit)

unread,

Oct 24, 2023, 5:58:36 PM10/24/23

to Scott Violet, chromium-a...@chromium.org, extension...@chromium.org, Caroline Rising

Attention is currently required from: Caroline Rising, Scott Violet.

Shibalik Mohapatra would like Scott Violet to review this change.

View Change

[SidePanelPinning] Add a new toggle API to control the action callback for sidepanels.

Change-Id: I2b6c68691b0d426621ee42fe10ea5e6cf056b1ae
---
M chrome/browser/ui/side_panel/side_panel_ui.h
M chrome/browser/ui/views/frame/browser_actions.cc
M chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.cc
M chrome/browser/ui/views/side_panel/search_companion/search_companion_side_panel_coordinator.cc
M chrome/browser/ui/views/side_panel/search_companion/search_companion_side_panel_coordinator.h
M chrome/browser/ui/views/side_panel/side_panel_coordinator.cc
M chrome/browser/ui/views/side_panel/side_panel_coordinator.h
M chrome/browser/ui/views/side_panel/side_panel_coordinator_unittest.cc
8 files changed, 68 insertions(+), 10 deletions(-)

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Shibalik Mohapatra (Gerrit)

unread,

Oct 24, 2023, 5:58:42 PM10/24/23

to chromium-a...@chromium.org, extension...@chromium.org, Scott Violet, Chromium LUCI CQ, Caroline Rising, chromium...@chromium.org

Attention is currently required from: Caroline Rising, Scott Violet.

View Change

4 comments:

File chrome/browser/ui/views/side_panel/side_panel_coordinator.h:

- Patch Set #1, Line 14: #include "chrome/browser/ui/actions/chrome_action_id.h"
  is this needed?

- Done

Patch Set #1, Line 32:

namespace actions {
class ActionItem;
}  // namespace actions

- nit: is this needed if you're including actions. […]
  I think this was from the pinnable CL.
File chrome/browser/ui/views/side_panel/side_panel_coordinator.cc:

Patch Set #1, Line 424:

if (IsSidePanelEntryShowing(key.value())) {
    Close();
    return;
  }

  if (IsSidePanelShowing()) {
    SidePanelContentSwappingContainer* content_wrapper =
        static_cast<SidePanelContentSwappingContainer*>(
            GetContentContainerView()->GetViewByID(
                kSidePanelContentWrapperViewId));

    if (content_wrapper->loading_entry() == GetEntryForKey(key.value())) {
      content_wrapper->ResetLoadingEntryIfNecessary();
      Close();
      return;
    }
  }

Could you add a comment in here to explain that we close if the entry is already showing or loading?

- Done

Patch Set #1, Line 442:

if (browser_view_->browser()->window()->IsFeaturePromoActive(
          feature_engagement::kIPHPowerBookmarksSidePanelFeature)) {
    Show(absl::make_optional(SidePanelEntry::Id::kBookmarks), open_trigger);
    return;
  }

- This doesn't seem right anymore without the side panel button. […]
  yeah makes sense! Removed it.

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Shibalik Mohapatra (Gerrit)

unread,

Oct 24, 2023, 5:59:28 PM10/24/23

to chromium-a...@chromium.org, extension...@chromium.org, Scott Violet, Chromium LUCI CQ, Caroline Rising, chromium...@chromium.org

Attention is currently required from: Caroline Rising, Scott Violet.

View Change

1 comment:

Patchset:
- Patch Set #2:
  @sky can you take a look at side_panel_ui.h
  @corising can you take a look at the other files?
  Thanks!

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Oct 24, 2023, 7:19:15 PM10/24/23

to Shibalik Mohapatra, chromium-a...@chromium.org, extension...@chromium.org, Scott Violet, Chromium LUCI CQ, Caroline Rising, chromium...@chromium.org

Attention is currently required from: Caroline Rising, Scott Violet.

This change meets the code coverage requirements.

Patch set 2:Code-Coverage +1

View Change

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Caroline Rising (Gerrit)

unread,

Oct 25, 2023, 10:49:49 AM10/25/23

to Shibalik Mohapatra, chromium-a...@chromium.org, extension...@chromium.org, findit...@appspot.gserviceaccount.com, Scott Violet, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Scott Violet, Shibalik Mohapatra.

Patch set 2:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #2:
  lgtm!

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Scott Violet (Gerrit)

unread,

Oct 25, 2023, 12:59:55 PM10/25/23

to Shibalik Mohapatra, chromium-a...@chromium.org, extension...@chromium.org, Caroline Rising, findit...@appspot.gserviceaccount.com, Scott Violet, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Shibalik Mohapatra.

View Change

1 comment:

File chrome/browser/ui/side_panel/side_panel_ui.h:
- Patch Set #2, Line 53: absl::optional<SidePanelOpenTrigger> open_trigger = absl::nullopt) = 0;
  Style guide says you shouldn't use default arguments in virtual functions. Specifically go/cstyle#Default_Arguments .

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Eshwar Stalin (Gerrit)

unread,

Oct 26, 2023, 2:34:29 AM10/26/23

to Shibalik Mohapatra, chromium-a...@chromium.org, extension...@chromium.org, Caroline Rising, findit...@appspot.gserviceaccount.com, Scott Violet, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Shibalik Mohapatra.

View Change

3 comments:

File chrome/browser/ui/views/side_panel/search_companion/search_companion_side_panel_coordinator.h:
- Patch Set #2, Line 111: raw_ptr<PinnedToolbarActionsModel> pinned_toolbar_actions_model_;
  This looks like unrelated change, but also this has been reverted in subsequent CLs.
File chrome/browser/ui/views/side_panel/side_panel_coordinator.cc:
- Patch Set #2, Line 419:
```
if (!key.has_value()) {
    Toggle();
    return;
  }
```
  Why make key optional?
- Patch Set #2, Line 433:

if (IsSidePanelShowing()) {
    SidePanelContentSwappingContainer* content_wrapper =
        static_cast<SidePanelContentSwappingContainer*>(
            GetContentContainerView()->GetViewByID(
                kSidePanelContentWrapperViewId));

    if (content_wrapper->loading_entry() == GetEntryForKey(key.value())) {
      content_wrapper->ResetLoadingEntryIfNecessary();
      Close();
      return;
    }
  }

- Isn't this case already handled within Show?

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Eshwar Stalin (Gerrit)

unread,

Nov 2, 2023, 6:36:46 PM11/2/23

to Shibalik Mohapatra, chromium-a...@chromium.org, extension...@chromium.org, Caroline Rising, findit...@appspot.gserviceaccount.com, Scott Violet, Chromium LUCI CQ, chromium...@chromium.org

View Change

1 comment:

File chrome/browser/ui/views/side_panel/side_panel_coordinator.cc:
- Patch Set #2, Line 433:

if (IsSidePanelShowing()) {
    SidePanelContentSwappingContainer* content_wrapper =
        static_cast<SidePanelContentSwappingContainer*>(
            GetContentContainerView()->GetViewByID(
                kSidePanelContentWrapperViewId));

    if (content_wrapper->loading_entry() == GetEntryForKey(key.value())) {
      content_wrapper->ResetLoadingEntryIfNecessary();
      Close();
      return;
    }
  }

Isn't this case already handled within Show?

- Done

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Eshwar Stalin (Gerrit)

unread,

Nov 3, 2023, 4:40:33 PM11/3/23

to Shibalik Mohapatra, chromium-a...@chromium.org, extension...@chromium.org, Caroline Rising, findit...@appspot.gserviceaccount.com, Scott Violet, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Shibalik Mohapatra.

View Change

3 comments:

Patchset:
- Patch Set #2:
  Shib, when you are back next week please look into prioritizing this CL. Thanks.
File chrome/browser/ui/side_panel/side_panel_ui.h:

- Patch Set #2, Line 53: absl::optional<SidePanelOpenTrigger> open_trigger = absl::nullopt) = 0;

- Style guide says you shouldn't use default arguments in virtual functions. […]
  +1 to this. Going forward we shouldn't need to support Toggle with no key value specified. Additionally, we shouldn't make open_trigger optional either so that we can get accurate metrics.

File chrome/browser/ui/views/side_panel/search_companion/search_companion_side_panel_coordinator.cc:

Patch Set #2, Line 87:

if (base::FeatureList::IsEnabled(features::kSidePanelPinning)) {
    pinned_toolbar_actions_model_ =
        PinnedToolbarActionsModel::Get(browser->profile());
  }

Looks like a rebase issue.

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Shibalik Mohapatra (Gerrit)

unread,

Nov 7, 2023, 4:56:11 AM11/7/23

to chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Caroline Rising, Shibalik Mohapatra.

Shibalik Mohapatra uploaded patch set #3 to this change.

View Change

The following approvals got outdated and were removed: Code-Coverage+1 by findit...@appspot.gserviceaccount.com, Code-Review+1 by Caroline Rising

[SidePanelPinning] Add a new toggle API to control the action callback for sidepanels.

Change-Id: I2b6c68691b0d426621ee42fe10ea5e6cf056b1ae
---
M chrome/browser/ui/side_panel/side_panel_ui.h
M chrome/browser/ui/views/frame/browser_actions.cc
M chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.cc

M chrome/browser/ui/views/side_panel/side_panel_coordinator.cc
M chrome/browser/ui/views/side_panel/side_panel_coordinator.h
M chrome/browser/ui/views/side_panel/side_panel_coordinator_unittest.cc

6 files changed, 64 insertions(+), 5 deletions(-)

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Shibalik Mohapatra (Gerrit)

unread,

Nov 7, 2023, 5:23:51 AM11/7/23

to chromium-a...@chromium.org, extension...@chromium.org, Eshwar Stalin, Caroline Rising, findit...@appspot.gserviceaccount.com, Scott Violet, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Caroline Rising, Eshwar Stalin, Scott Violet.

View Change

4 comments:

File chrome/browser/ui/side_panel/side_panel_ui.h:

- Patch Set #2, Line 53: absl::optional<SidePanelOpenTrigger> open_trigger = absl::nullopt) = 0;

- +1 to this. Going forward we shouldn't need to support Toggle with no key value specified. […]
  Removed the default arguments and the optional value for the key. I think the optional value for the open trigger should stay though for consistency given that the action item trigger for extension is not used. So nullopt should be used in that case.
File chrome/browser/ui/views/side_panel/search_companion/search_companion_side_panel_coordinator.h:

- Patch Set #2, Line 111: raw_ptr<PinnedToolbarActionsModel> pinned_toolbar_actions_model_;
  This looks like unrelated change, but also this has been reverted in subsequent CLs.

- Done
File chrome/browser/ui/views/side_panel/search_companion/search_companion_side_panel_coordinator.cc:

Patch Set #2, Line 87:

if (base::FeatureList::IsEnabled(features::kSidePanelPinning)) {
    pinned_toolbar_actions_model_ =
        PinnedToolbarActionsModel::Get(browser->profile());
  }

Looks like a rebase issue.

- Done
File chrome/browser/ui/views/side_panel/side_panel_coordinator.cc:
- Patch Set #2, Line 419:

if (!key.has_value()) {
    Toggle();
    return;
  }

Why make key optional?

- Done

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Nov 7, 2023, 5:53:32 AM11/7/23

to Shibalik Mohapatra, chromium-a...@chromium.org, extension...@chromium.org, Eshwar Stalin, Caroline Rising, Scott Violet, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Caroline Rising, Eshwar Stalin, Scott Violet.

This change meets the code coverage requirements.

Patch set 3:Code-Coverage +1

View Change

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Shibalik Mohapatra (Gerrit)

unread,

Nov 7, 2023, 11:07:30 AM11/7/23

to chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Caroline Rising, Eshwar Stalin, Scott Violet.

Shibalik Mohapatra uploaded patch set #4 to this change.

View Change

[SidePanelPinning] Add a new toggle API to control the action callback for sidepanels.



Change-Id: I2b6c68691b0d426621ee42fe10ea5e6cf056b1ae
---
M chrome/browser/ui/side_panel/side_panel_ui.h
M chrome/browser/ui/views/frame/browser_actions.cc
M chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.cc
M chrome/browser/ui/views/side_panel/side_panel_coordinator.cc
M chrome/browser/ui/views/side_panel/side_panel_coordinator.h
M chrome/browser/ui/views/side_panel/side_panel_coordinator_unittest.cc

6 files changed, 68 insertions(+), 5 deletions(-)

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Caroline Rising (Gerrit)

unread,

Nov 7, 2023, 11:18:44 AM11/7/23

to Shibalik Mohapatra, chromium-a...@chromium.org, extension...@chromium.org, findit...@appspot.gserviceaccount.com, Eshwar Stalin, Scott Violet, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Eshwar Stalin, Scott Violet, Shibalik Mohapatra.

Patch set 4:Code-Review +1

View Change

3 comments:

Patchset:
- Patch Set #4:
  lgtm with small nits
File chrome/browser/ui/side_panel/side_panel_ui.h:
- Patch Set #4, Line 46:
```
// Open side panel when it's close or close side panel when it's only
  virtual void Toggle() = 0;
```
  Could you add a todo that this should be removed after launch?
File chrome/browser/ui/views/side_panel/side_panel_coordinator_unittest.cc:
- Patch Set #4, Line 393: SidePanelToggleWithEntriesTest
  tiny nit: can we also test that calling toggle reading list then toggle bookmarks shows reading list then bookmarks without closing?

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Scott Violet (Gerrit)

unread,

Nov 7, 2023, 11:34:43 AM11/7/23

to Shibalik Mohapatra, chromium-a...@chromium.org, extension...@chromium.org, Scott Violet, Caroline Rising, findit...@appspot.gserviceaccount.com, Eshwar Stalin, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Eshwar Stalin, Shibalik Mohapatra.

Patch set 4:Code-Review +1

View Change

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Eshwar Stalin (Gerrit)

unread,

Nov 7, 2023, 11:45:33 AM11/7/23

to Shibalik Mohapatra, chromium-a...@chromium.org, extension...@chromium.org, Scott Violet, Caroline Rising, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Shibalik Mohapatra.

Patch set 4:Code-Review +1

View Change

2 comments:

File chrome/browser/ui/views/side_panel/side_panel_coordinator.cc:
- Patch Set #4, Line 442:
```
Also if the entry is the loading entry and is
  // toggled, it should also be closed. This handles quick double clicks
  // to close the sidepanel.
```
  nit: Could you move this comment next to the 2nd if statement?
File chrome/browser/ui/views/side_panel/side_panel_coordinator_unittest.cc:
- Patch Set #4, Line 402: Toggle
  You will need to pass in the 2nd parameter.

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Eshwar Stalin (Gerrit)

unread,

Nov 7, 2023, 11:59:05 AM11/7/23

to Shibalik Mohapatra, chromium-a...@chromium.org, extension...@chromium.org, Scott Violet, Caroline Rising, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Shibalik Mohapatra.

View Change

1 comment:

File chrome/browser/ui/side_panel/side_panel_ui.h:
- Patch Set #4, Line 49: sidepanel
  nit: Please use "side panel" instead of "sidepanel" for consistency.

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Shibalik Mohapatra (Gerrit)

unread,

Nov 7, 2023, 12:09:11 PM11/7/23

to chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Shibalik Mohapatra.

Shibalik Mohapatra uploaded patch set #5 to this change.

View Change

[SidePanelPinning] Add a new toggle API to control the action callback for sidepanels.

Change-Id: I2b6c68691b0d426621ee42fe10ea5e6cf056b1ae
---
M chrome/browser/ui/side_panel/side_panel_ui.h
M chrome/browser/ui/views/frame/browser_actions.cc
M chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.cc
M chrome/browser/ui/views/side_panel/side_panel_coordinator.cc
M chrome/browser/ui/views/side_panel/side_panel_coordinator.h
M chrome/browser/ui/views/side_panel/side_panel_coordinator_unittest.cc

6 files changed, 88 insertions(+), 6 deletions(-)

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Shibalik Mohapatra (Gerrit)

unread,

Nov 7, 2023, 12:11:48 PM11/7/23

to chromium-a...@chromium.org, extension...@chromium.org, Eshwar Stalin, Scott Violet, Caroline Rising, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org

View Change

4 comments:

File chrome/browser/ui/side_panel/side_panel_ui.h:
- Patch Set #4, Line 46:

// Open side panel when it's close or close side panel when it's only
  virtual void Toggle() = 0;

Could you add a todo that this should be removed after launch?

- Done
File chrome/browser/ui/views/side_panel/side_panel_coordinator.cc:

Patch Set #4, Line 442:

Also if the entry is the loading entry and is
  // toggled, it should also be closed. This handles quick double clicks
  // to close the sidepanel.

nit: Could you move this comment next to the 2nd if statement?

- Done
File chrome/browser/ui/views/side_panel/side_panel_coordinator_unittest.cc:
- Patch Set #4, Line 393: SidePanelToggleWithEntriesTest
  tiny nit: can we also test that calling toggle reading list then toggle bookmarks shows reading list […]
  Done

- Patch Set #4, Line 402: Toggle
  You will need to pass in the 2nd parameter.

- Done

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Shibalik Mohapatra (Gerrit)

unread,

Nov 7, 2023, 12:18:41 PM11/7/23

to chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Shibalik Mohapatra.

Shibalik Mohapatra uploaded patch set #6 to this change.

View Change

The following approvals got outdated and were removed: Commit-Queue+1 by Shibalik Mohapatra

[SidePanelPinning] Add a new toggle API to control the action callback for sidepanels.

Change-Id: I2b6c68691b0d426621ee42fe10ea5e6cf056b1ae
---
M chrome/browser/ui/side_panel/side_panel_ui.h
M chrome/browser/ui/views/frame/browser_actions.cc
M chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.cc
M chrome/browser/ui/views/side_panel/side_panel_coordinator.cc
M chrome/browser/ui/views/side_panel/side_panel_coordinator.h
M chrome/browser/ui/views/side_panel/side_panel_coordinator_unittest.cc
6 files changed, 88 insertions(+), 6 deletions(-)

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Nov 7, 2023, 1:05:00 PM11/7/23

to Shibalik Mohapatra, chromium-a...@chromium.org, extension...@chromium.org, Eshwar Stalin, Scott Violet, Caroline Rising, Chromium LUCI CQ, chromium...@chromium.org

This change meets the code coverage requirements.

Patch set 6:Code-Coverage +1

View Change

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Shibalik Mohapatra (Gerrit)

unread,

Nov 7, 2023, 1:07:24 PM11/7/23

to chromium-a...@chromium.org, extension...@chromium.org, Eshwar Stalin, Scott Violet, Caroline Rising, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org

Patch set 6:Commit-Queue +2

View Change

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Nov 7, 2023, 1:26:21 PM11/7/23

to Shibalik Mohapatra, chromium-a...@chromium.org, extension...@chromium.org, Eshwar Stalin, Scott Violet, Caroline Rising, findit...@appspot.gserviceaccount.com, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change



4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: chrome/browser/ui/side_panel/side_panel_ui.h
Insertions: 4, Deletions: 3.

@@ -43,11 +43,12 @@
   // Close the side panel.
   virtual void Close() = 0;
 
-  // Open side panel when it's close or close side panel when it's only
+  // Open side panel when it's close or close side panel when it's open.
+  // TODO(shibalik): Remove after SidePanelPinning launch.


   virtual void Toggle() = 0;

-  // Open the sidepanel for a key. If sidepanel for the key is already opened
-  // then close the sidepanel.
+  // Open the side panel for a key. If side panel for the key is already opened
+  // then close the side panel.
   virtual void Toggle(SidePanelEntryKey key,
                       SidePanelOpenTrigger open_trigger) = 0;
 
```
```
The name of the file: chrome/browser/ui/views/side_panel/side_panel_coordinator.cc
Insertions: 4, Deletions: 3.

@@ -439,14 +439,15 @@
     SidePanelEntryKey key,
     SidePanelUtil::SidePanelOpenTrigger open_trigger) {
   // If an entry is already showing in the sidepanel, the sidepanel
-  // should be closed. Also if the entry is the loading entry and is
-  // toggled, it should also be closed. This handles quick double clicks
-  // to close the sidepanel.
+  // should be closed.
   if (IsSidePanelEntryShowing(key)) {
     Close();
     return;
   }
 
+  // If the entry is the loading entry and is toggled,
+  // it should also be closed. This handles quick double clicks
+  // to close the sidepanel.


   if (IsSidePanelShowing()) {
     SidePanelContentSwappingContainer* content_wrapper =
         static_cast<SidePanelContentSwappingContainer*>(

```
```
The name of the file: chrome/browser/ui/views/side_panel/side_panel_coordinator_unittest.cc
Insertions: 21, Deletions: 4.

@@ -392,23 +392,40 @@
 
 TEST_F(SidePanelCoordinatorTest, SidePanelToggleWithEntriesTest) {
   // Show reading list sidepanel.
-  coordinator_->Toggle(SidePanelEntry::Key(SidePanelEntry::Id::kReadingList));
+  coordinator_->Toggle(SidePanelEntry::Key(SidePanelEntry::Id::kReadingList),
+                       SidePanelOpenTrigger::kPinnedEntryToolbarButton);
   EXPECT_TRUE(browser_view()->unified_side_panel()->GetVisible());
   EXPECT_TRUE(GetLastActiveEntryKey().has_value());
   EXPECT_EQ(GetLastActiveEntryKey().value().id(),
             SidePanelEntry::Id::kReadingList);
 
   // Toggle reading list sidepanel to close.
-  coordinator_->Toggle(SidePanelEntry::Key(SidePanelEntry::Id::kReadingList));
+  coordinator_->Toggle(SidePanelEntry::Key(SidePanelEntry::Id::kReadingList),
+                       SidePanelOpenTrigger::kPinnedEntryToolbarButton);
   EXPECT_FALSE(browser_view()->unified_side_panel()->GetVisible());
 
   // If the same entry is loading, close the sidepanel.
   coordinator_->SetNoDelaysForTesting(false);
-  coordinator_->Toggle(SidePanelEntry::Key(SidePanelEntry::Id::kBookmarks));
+  coordinator_->Toggle(SidePanelEntry::Key(SidePanelEntry::Id::kBookmarks),
+                       SidePanelOpenTrigger::kPinnedEntryToolbarButton);
   EXPECT_FALSE(browser_view()->unified_side_panel()->GetVisible());
   coordinator_->SetNoDelaysForTesting(true);
-  coordinator_->Toggle(SidePanelEntry::Key(SidePanelEntry::Id::kBookmarks));
+  coordinator_->Toggle(SidePanelEntry::Key(SidePanelEntry::Id::kBookmarks),
+                       SidePanelOpenTrigger::kPinnedEntryToolbarButton);
   EXPECT_FALSE(browser_view()->unified_side_panel()->GetVisible());
+
+  // Toggling reading list followed by bookmarks shows the reading list side
+  // panel followed by the bookmarks side panel.
+  coordinator_->Toggle(SidePanelEntry::Key(SidePanelEntry::Id::kReadingList),
+                       SidePanelOpenTrigger::kPinnedEntryToolbarButton);
+  EXPECT_TRUE(browser_view()->unified_side_panel()->GetVisible());
+  EXPECT_EQ(GetLastActiveEntryKey().value().id(),
+            SidePanelEntry::Id::kReadingList);
+  coordinator_->Toggle(SidePanelEntry::Key(SidePanelEntry::Id::kBookmarks),
+                       SidePanelOpenTrigger::kPinnedEntryToolbarButton);
+  EXPECT_TRUE(browser_view()->unified_side_panel()->GetVisible());
+  EXPECT_EQ(GetLastActiveEntryKey().value().id(),
+            SidePanelEntry::Id::kBookmarks);
 }
 
 TEST_F(SidePanelCoordinatorTest, ShowOpensSidePanel) {
```

Approvals:
  findit...@appspot.gserviceaccount.com: Ok
  Scott Violet: Looks good to me
  Shibalik Mohapatra: Commit
  Eshwar Stalin: Looks good to me
  Caroline Rising: Looks good to me

[SidePanelPinning] Add a new toggle API to control the action callback for sidepanels.

Change-Id: I2b6c68691b0d426621ee42fe10ea5e6cf056b1ae

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4969028
Reviewed-by: Eshwar Stalin <est...@chromium.org>
Reviewed-by: Scott Violet <s...@chromium.org>
Commit-Queue: Shibalik Mohapatra <shib...@chromium.org>
Reviewed-by: Caroline Rising <cori...@chromium.org>
Code-Coverage: findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1221042}


---
M chrome/browser/ui/side_panel/side_panel_ui.h
M chrome/browser/ui/views/frame/browser_actions.cc
M chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.cc
M chrome/browser/ui/views/side_panel/side_panel_coordinator.cc
M chrome/browser/ui/views/side_panel/side_panel_coordinator.h
M chrome/browser/ui/views/side_panel/side_panel_coordinator_unittest.cc
6 files changed, 88 insertions(+), 6 deletions(-)

To view, visit change 4969028. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward