Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use chromium's side panel inside our side bar container, and insert Reading List #13769

Merged
merged 3 commits into from
Aug 22, 2022

Conversation

petemill
Copy link
Member

@petemill petemill commented Jun 14, 2022

Chromium's side panel becomes controlled by Brave's side bar buttons, which uses chromium's side panel coordinator.

The goal of this PR is to get the best of both worlds - use Chromium's SidePanel related views, models, coordinators, WebContents management, and WebUI resources, whilst still using the awesome Brave SideBar from @simonhong.

There are many things we and chromium would like to use side panels for, and this could set us up to scale well for that.

Another change is that using the toolbar button to open side panel will result in the sidebar (and panel) staying open, even if the sidebar prefs are set to open on mouseover or never. Once the panel closes, then the sidebar will to if the prefs have either of these values.

Note: One consideration is to instead do the inverse of what I have done in this PR: Perhaps we move our SideBar to be a child of browser_view_->right_aligned_side_panel() (like the ComboBox is for Chromium) and have the toolbar button turn on SideBar + SidePanel, with no separation between the two. Some complication will need to be catered to where we want the SideBar to show even when there is no active SidePanel.

TODO

  • Use Chromium's SidePanelCoordinator to manage WebContents and panel open / close
  • Reading list works
  • Bookmarks works, and remove Brave's clone of the panel
  • Panel opens and closes when clicking SideBar item
  • Design team feedback on padding and border colors
  • Panel gets focus when opening
  • Toolbar button controls SideBar, not just panel inside it
  • Replace toolbar button icon so that sidebar is on left
  • Use Brave design styles for Reading list panel, not chromium (for follow-up)
  • Have side panel show on right for RTL UI (for follow-up)

image

Resolves brave/brave-browser#17959

Future considerations:

  • Side search
  • Interaction with vertical tabs, and perhaps Side Bar + Side Panel moving to the right

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

On brave/brave-browser#17959

@petemill petemill requested review from a team as code owners June 14, 2022 06:37
@petemill petemill self-assigned this Jun 14, 2022
@github-actions github-actions bot added CI/run-network-audit Run network-audit CI/storybook-url Deploy storybook and provide a unique URL for each build potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false labels Jun 14, 2022
@petemill petemill force-pushed the chromium-side-panel branch 2 times, most recently from 6b46214 to 1efc150 Compare June 17, 2022 04:23
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to reuse upstream's side panel UI 👍🏼

common/extensions/api/_api_features.json Show resolved Hide resolved
components/sidebar/sidebar_service.cc Outdated Show resolved Hide resolved
components/sidebar/constants.h Show resolved Hide resolved
@@ -4,24 +4,34 @@
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#include "brave/components/sidebar/sidebar_item.h"
#include "build/build_config.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this seems not needed here?

constexpr size_t kPanelWidthForBuiltIn = 260;
constexpr size_t kPanelWidthForNonBuiltIn = 360;
return IsBuiltInType(item) ? kPanelWidthForBuiltIn : kPanelWidthForNonBuiltIn;
SidePanelEntry::Id SidePanelIdFromSideBarItem(const sidebar::SidebarItem item) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: const sidebar::SidebarItem& item

bool IsSidebarHasAllBuiltiInItems() const;
int GetIndexOf(const SidebarItem& item) const;

// Don't cache web_contents. It can be deleted during the runtime.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From now, this SidebarModel's main role is managing active index and fetching favicon :)

sidebar_panel_webview_->SetVisible(true);
// When panel is opened, it will get focused.
sidebar_panel_webview_->RequestFocus();
if (!item.open_in_panel) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it would be more readable if positive condition comes first - if (item.open_in_panel).

}
// if (item.open_in_panel) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover

browser/ui/views/frame/brave_browser_view.cc Show resolved Hide resolved
sidebar_panel_webview_ =
AddChildView(std::make_unique<SidebarPanelWebView>(browser_->profile()));

ReorderChildView(side_panel_, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add sidebar_control_view_ at index 0, then maybe we don't need to reorder side_pane_?

  sidebar_control_view_ =
      AddChildViewAt(std::make_unique<SidebarControlView>(browser_), 0);
@petemill petemill force-pushed the chromium-side-panel branch 2 times, most recently from cfc808a to e942a31 Compare July 12, 2022 15:21
@petemill
Copy link
Member Author

petemill commented Jul 12, 2022

I have now pushed all functionality working well with the new toolbar icon. The Sidebar correctly shows active states when the panel is made active from external means (i.e. the side panel toolbar icon).

Another change is that using the toolbar button to open side panel will result in the sidebar (and panel) staying open, even if the sidebar prefs are set to open on mouseover or never. Once the panel closes, then the sidebar will to if the prefs have either of these values.

The only item left for me at the moment is to update the vector icon from figma.

@petemill petemill requested a review from simonhong July 12, 2022 15:24
@petemill petemill force-pushed the chromium-side-panel branch 3 times, most recently from 2413b2f to aa7ef88 Compare July 12, 2022 23:16
@petemill
Copy link
Member Author

Test plans added to brave/brave-browser#17959

@petemill
Copy link
Member Author

All feedback addressed @simonhong

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@simonhong
Copy link
Member

simonhong commented Jul 13, 2022

I realized that SidebarService doesn't show newly introduced default item.
So existing user should add reading list item via add item bubble.
It should be handled as a separated issue.

@simonhong
Copy link
Member

simonhong commented Jul 13, 2022

When any item is not activated after launching, clicking toolbar button opens always reading list (even it's removed item).
When readling list item is removed, I think another (bookmark) item should be activated.
If there are no visible items for panel, what should we do when clicks toolbar button?

also there is no border between web contens and panel when panel is opened via toolbar button after launching.
image

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@petemill petemill force-pushed the chromium-side-panel branch 2 times, most recently from 4a89ca6 to 691bc97 Compare July 25, 2022 21:24
@petemill
Copy link
Member Author

@simonhong I've addressed feedback again and added / amended some tests as appropriate. Please re-review when you can.

I realized that SidebarService doesn't show newly introduced default item.
So existing user should add reading list item via add item bubble.
It should be handled as a separated issue.

I've handled that in this PR now. I migrated the prefs such that the existing pref only stores user items (not built-in). And I made a new pref which simply contains a list of IDs of built-in items which the user has hidden.

When any item is not activated after launching, clicking toolbar button opens always reading list (even it's removed item).
When readling list item is removed, I think another (bookmark) item should be activated.
If there are no visible items for panel, what should we do when clicks toolbar button?

This should now work as you describe. And if the user removes all panel items, then the toolbar icon will hide.

also there is no border between web contens and panel when panel is opened via toolbar button after launching.

This is fixed. The issue was the delay between sidebar item getting activated and sidepanel view being shown (due to waiting for load event).

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

Copy link
Contributor

@sangwoo108 sangwoo108 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!🙌

Sorry for being picky. Please feel free to give me feedbacks for my comments.

@@ -0,0 +1,77 @@
CANVAS_DIMENSIONS, 16,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have copyright here?

ASSERT_TRUE(
ui_test_utils::NavigateToURL(browser(), GURL("brave://settings/")));

EXPECT_TRUE(CanAddCurrentActiveTabToSidebar(browser()));
controller()->AddItemWithCurrentTab();
// have 4 items.
EXPECT_EQ(4UL, model()->GetAllSidebarItems().size());
// Has 1 item.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Has 1 item.
// Add 1 item.

or

Suggested change
// Has 1 item.
// Has 5 items.
browser/ui/sidebar/sidebar_controller.cc Show resolved Hide resolved
Comment on lines 122 to 129
if (active_index_ == from) {
new_active_index = to;
}
if (active_index_ == to) {
new_active_index = (to < from) ? active_index_ + 1 : active_index_ - 1;
}
if (from > active_index_ && to < active_index_) {
new_active_index = active_index_ + 1;
}
UpdateActiveIndexAndNotify(new_active_index);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems from < active_index < to case isn't handled, where we should decrease active index by one.

How about collapsing conditions like this?

  if (active_index_ == from) {
    new_active_index = to;
  } else {
    new_active_index = (to < from) ? active_index_ + 1 : active_index_ - 1;
  }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks - for some reason this was still working fine though 🤷

Comment on lines 195 to 196
std::find_if(items.begin(), items.end(), [item](const auto& i) {
return (item.built_in_item_type == i.built_in_item_type &&
item.url == i.url);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

  • Let's use ranges
  • capturing reference can be better I think.
Suggested change
std::find_if(items.begin(), items.end(), [item](const auto& i) {
return (item.built_in_item_type == i.built_in_item_type &&
item.url == i.url);
});
base::ranges::find_if(items, [&item](const auto& i) {
return (item.built_in_item_type == i.built_in_item_type &&
item.url == i.url);
});
// Remember not to hide this item
auto iter = std::find_if(
built_in_items_to_hide.begin(), built_in_items_to_hide.end(),
[item_id](const auto& default_item) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same with comment above, & capture and ranges.

@@ -225,75 +350,133 @@ SidebarService::ShowSidebarOption SidebarService::GetSidebarShowOption() const {
return static_cast<ShowSidebarOption>(prefs_->GetInteger(kSidebarShowOption));
}

absl::optional<SidebarItem> SidebarService::GetDefaultPanelItem() const {
const std::vector<SidebarItem::BuiltInItemType> preferred_item_types{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto: NoDestructor

SidebarItem::BuiltInItemType::kBookmarks};
absl::optional<SidebarItem> default_item;
for (const auto& type : preferred_item_types) {
auto found_item_iter = std::find_if(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ranges?

if (type == SidebarItem::Type::kTypeBuiltIn) {
if (const auto value =
item.FindIntKey(kSidebarItemBuiltInItemTypeKey)) {
auto id = static_cast<SidebarItem::BuiltInItemType>(*value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
auto id = static_cast<SidebarItem::BuiltInItemType>(*value);
auto type = static_cast<SidebarItem::BuiltInItemType>(*value);
components/sidebar/sidebar_service.cc Show resolved Hide resolved
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// you can obtain one at http://mozilla.org/MPL/2.0/.

// Need to include this header here otherwise SidePanel macro gets undef'ed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically we include the corresponding header first to prevent the redefinitions. Should
#include "chrome/browser/ui/views/side_search/side_search_browser_controller.h" here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we also need to keep the browser_view.h include as that also has a macro called SidePanel

@@ -3,16 +3,24 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#include "brave/browser/ui/views/frame/brave_browser_view_layout.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should include "chrome/browser/ui/views/frame/browser_view.h" first?

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@petemill
Copy link
Member Author

petemill commented Aug 16, 2022

@sangwoo108 @mkarolin @simonhong thanks for the reviews - feedback is addressed again. Changes can be seen here: https://github.com/brave/brave-core/compare/45e5419e2c160c5a7cef1c0e5735a4329f2d670e..4ffd577477

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ it seems only network audit test should be fixed :)

@@ -63,34 +76,22 @@ SidebarItem GetBuiltInItemForType(SidebarItem::BuiltInItemType type) {
return SidebarItem();
}

SidebarItem::BuiltInItemType GetBuiltInItemTypeForURL(const std::string& url) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for deleting obsolete functions!

@petemill
Copy link
Member Author

Updated to fix network audit test, review changes can now be seen at https://github.com/brave/brave-core/compare/45e5419e2c160c5a7cef1c0e5735a4329f2d670e..4ffd577477

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

bool SidebarModel::IsSidebarHasAllBuiltiInItems() const {
return GetSidebarService(profile_)->GetHiddenDefaultSidebarItems().empty();
}

int SidebarModel::GetIndexOf(const SidebarItem& item) const {
const auto items = GetAllSidebarItems();
const auto iter =
std::find_if(items.begin(), items.end(),
[item](const auto& i) { return item.url == i.url; });
base::ranges::find_if(items.begin(), items.end(), [&item](const auto& i) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you use ranges, I think you can omit begin(), end() of container.

Suggested change
base::ranges::find_if(items.begin(), items.end(), [&item](const auto& i) {
base::ranges::find_if(items, [&item](const auto& i) {
}
// Remember not to hide this item
auto iter = base::ranges::find_if(
built_in_items_to_hide.begin(), built_in_items_to_hide.end(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto:

Suggested change
built_in_items_to_hide.begin(), built_in_items_to_hide.end(),
built_in_items_to_hide,
added_item.built_in_item_type;
});
auto iter =
base::ranges::find_if(default_items.begin(), default_items.end(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto:

Suggested change
base::ranges::find_if(default_items.begin(), default_items.end(),
base::ranges::find_if(default_items,
absl::optional<SidebarItem> default_item;
for (const auto& type : *preferred_item_types) {
auto found_item_iter = base::ranges::find_if(
items_.begin(), items_.end(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto:

Suggested change
items_.begin(), items_.end(),
items_,
void OnActiveIndexChanged(int old_index, int new_index) override;
void OnItemRemoved(int index) override;

// SidePanelEntryObserver:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about having base::ScopedObservation<SidePanelEntry, SidePanelEntryObserver> as member variable for this?

I think there's no RemoveObserver() call for SidebarPanelEntry and that could be guaranteed by this RAII object.


// SidePanelRegistryObserver:
void OnEntryRegistered(SidePanelEntry* entry) override;
void OnEntryWillDeregister(SidePanelEntry* entry) override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And how about having base::ScopedMultiSourceObservation? For similar reason.

@@ -85,6 +93,7 @@ class SidebarControlView : public views::View,
void UpdateSettingsButtonState();
void UpdateBackgroundAndBorder();

raw_ptr<Delegate> delegate_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for consistency

Suggested change
raw_ptr<Delegate> delegate_;
raw_ptr<Delegate> delegate_ = nullptr;
components/sidebar/sidebar_service.cc Show resolved Hide resolved

SidebarItem GetBuiltInItemForURL(const std::string& url) {
return GetBuiltInItemForType(GetBuiltInItemTypeForURL(url));
std::vector<SidebarItem::BuiltInItemType> GetDefaultBuiltInItemTypes() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about returning const& in order to avoid copying the entire vector?

Suggested change
std::vector<SidebarItem::BuiltInItemType> GetDefaultBuiltInItemTypes() {
const std::vector<SidebarItem::BuiltInItemType>& GetDefaultBuiltInItemTypes() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for this nuance!

// Add the items the user has never seen (or never persisted).
// Get the initial order of items so that we can attempt to
// insert at the intended order.
auto default_item_types = GetDefaultBuiltInItemTypes();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If GetDefaultBuiltInItemTypes returns const&, const auto& will be better.

Suggested change
auto default_item_types = GetDefaultBuiltInItemTypes();
const auto& default_item_types = GetDefaultBuiltInItemTypes();
Copy link
Contributor

@sangwoo108 sangwoo108 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits 👍

@petemill
Copy link
Member Author

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

…eading List

Chromium's side panel is controlled by Brave's side bar buttons, via chromium's side panel coordinator
…e" to "exclusive"

This allows new items to be easily added, and only store the item ref by ID instead of all item details
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/run-network-audit Run network-audit CI/storybook-url Deploy storybook and provide a unique URL for each build potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false
6 participants