Added header to bookmarks panel #20211
+367
−37
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| /* Copyright (c) 2023 The Brave Authors. All rights reserved. | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
| * You can obtain one at https://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| #include "brave/browser/ui/views/side_panel/bookmarks/brave_bookmarks_side_panel_coordinator.h" | ||
|
|
||
| #include "base/functional/bind.h" | ||
| #include "brave/browser/ui/views/side_panel/brave_bookmarks_side_panel_view.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/grit/generated_resources.h" | ||
| #include "components/omnibox/browser/vector_icons.h" | ||
| #include "ui/base/l10n/l10n_util.h" | ||
| #include "ui/base/models/image_model.h" | ||
| #include "ui/views/vector_icons.h" | ||
|
|
||
| BraveBookmarksSidePanelCoordinator::BraveBookmarksSidePanelCoordinator( | ||
| Browser* browser) | ||
| : BrowserUserData<BraveBookmarksSidePanelCoordinator>(*browser) {} | ||
|
|
||
| BraveBookmarksSidePanelCoordinator::~BraveBookmarksSidePanelCoordinator() = | ||
| default; | ||
|
|
||
| void BraveBookmarksSidePanelCoordinator::CreateAndRegisterEntry( | ||
| SidePanelRegistry* global_registry) { | ||
| global_registry->Register(std::make_unique<SidePanelEntry>( | ||
| SidePanelEntry::Id::kBookmarks, | ||
| l10n_util::GetStringUTF16(IDS_BOOKMARK_MANAGER_TITLE), | ||
| ui::ImageModel::FromVectorIcon(omnibox::kStarIcon, ui::kColorIcon), | ||
| base::BindRepeating( | ||
| &BraveBookmarksSidePanelCoordinator::CreateBookmarksPanelView, | ||
| base::Unretained(this)))); | ||
| } | ||
|
|
||
| std::unique_ptr<views::View> | ||
| BraveBookmarksSidePanelCoordinator::CreateBookmarksPanelView() { | ||
| return std::make_unique<BraveBookmarksSidePanelView>(&GetBrowser()); | ||
| } | ||
|
|
||
| BROWSER_USER_DATA_KEY_IMPL(BraveBookmarksSidePanelCoordinator); | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| /* Copyright (c) 2023 The Brave Authors. All rights reserved. | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
| * You can obtain one at https://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| #ifndef BRAVE_BROWSER_UI_VIEWS_SIDE_PANEL_BOOKMARKS_BRAVE_BOOKMARKS_SIDE_PANEL_COORDINATOR_H_ | ||
| #define BRAVE_BROWSER_UI_VIEWS_SIDE_PANEL_BOOKMARKS_BRAVE_BOOKMARKS_SIDE_PANEL_COORDINATOR_H_ | ||
|
|
||
| #include <memory> | ||
|
|
||
| #include "chrome/browser/ui/browser_user_data.h" | ||
|
|
||
| class Browser; | ||
| class SidePanelRegistry; | ||
|
|
||
| namespace views { | ||
| class View; | ||
| } // namespace views | ||
|
|
||
| // Introduced to give custom contents view(BraveBookmarksSidePanelView) for | ||
| // bookmarks panel entry. That contents view includes bookmarks panel specific | ||
| // header view and web view. | ||
| // Note that this is not the subclass of upstream BookmarksSidePanelCoordinator. | ||
| // As it inherits BrowserUserData, subclassig doesn't work well when its | ||
| // instance is created via BrowserUserData<>::CreateForBrowser(). So, new | ||
| // coordinator class is introduced and | ||
| // BookmarksSidePanelCoordinator::CreateBookmarksWebView() is reused from | ||
| // BraveBookmarksSidePanelView. That's why BraveBookmarksSidePanelView is set | ||
| // as BookmarksSidePanelCoordinator's friend. | ||
| class BraveBookmarksSidePanelCoordinator | ||
| : public BrowserUserData<BraveBookmarksSidePanelCoordinator> { | ||
| public: | ||
| explicit BraveBookmarksSidePanelCoordinator(Browser* browser); | ||
| BraveBookmarksSidePanelCoordinator( | ||
| const BraveBookmarksSidePanelCoordinator&) = delete; | ||
| BraveBookmarksSidePanelCoordinator& operator=( | ||
| const BraveBookmarksSidePanelCoordinator&) = delete; | ||
| ~BraveBookmarksSidePanelCoordinator() override; | ||
|
|
||
| void CreateAndRegisterEntry(SidePanelRegistry* global_registry); | ||
|
|
||
| private: | ||
| friend class BrowserUserData<BraveBookmarksSidePanelCoordinator>; | ||
|
|
||
| std::unique_ptr<views::View> CreateBookmarksPanelView(); | ||
|
|
||
| BROWSER_USER_DATA_KEY_DECL(); | ||
| }; | ||
|
|
||
| #endif // BRAVE_BROWSER_UI_VIEWS_SIDE_PANEL_BOOKMARKS_BRAVE_BOOKMARKS_SIDE_PANEL_COORDINATOR_H_ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| /* Copyright (c) 2023 The Brave Authors. All rights reserved. | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
| * You can obtain one at https://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| #include "brave/browser/ui/views/side_panel/brave_bookmarks_side_panel_view.h" | ||
|
|
||
| #include <memory> | ||
|
|
||
| #include "base/memory/raw_ref.h" | ||
| #include "brave/browser/ui/color/brave_color_id.h" | ||
| #include "brave/components/vector_icons/vector_icons.h" | ||
| #include "brave/grit/brave_generated_resources.h" | ||
| #include "brave/grit/brave_theme_resources.h" | ||
| #include "chrome/browser/ui/singleton_tabs.h" | ||
| #include "chrome/browser/ui/views/side_panel/bookmarks/bookmarks_side_panel_coordinator.h" | ||
| #include "chrome/browser/ui/views/side_panel/read_later_side_panel_web_view.h" | ||
| #include "chrome/common/webui_url_constants.h" | ||
| #include "chrome/grit/generated_resources.h" | ||
| #include "ui/base/l10n/l10n_util.h" | ||
| #include "ui/base/models/image_model.h" | ||
| #include "ui/base/resource/resource_bundle.h" | ||
| #include "ui/gfx/font_list.h" | ||
| #include "ui/gfx/geometry/insets.h" | ||
| #include "ui/views/background.h" | ||
| #include "ui/views/controls/button/image_button.h" | ||
| #include "ui/views/controls/image_view.h" | ||
| #include "ui/views/controls/label.h" | ||
| #include "ui/views/controls/separator.h" | ||
| #include "ui/views/layout/flex_layout.h" | ||
| #include "ui/views/layout/layout_types.h" | ||
|
|
||
| namespace { | ||
|
|
||
| // Renders icon, title and launch button. | ||
| class BookmarksSidePanelHeaderView : public views::View { | ||
| public: | ||
| explicit BookmarksSidePanelHeaderView(Browser* browser) { | ||
| constexpr int kHeaderInteriorMargin = 16; | ||
| SetLayoutManager(std::make_unique<views::FlexLayout>()) | ||
| ->SetOrientation(views::LayoutOrientation::kHorizontal) | ||
| .SetInteriorMargin(gfx::Insets(kHeaderInteriorMargin)) | ||
| .SetMainAxisAlignment(views::LayoutAlignment::kStart) | ||
| .SetCrossAxisAlignment(views::LayoutAlignment::kCenter); | ||
|
|
||
| ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance(); | ||
| auto* header_image = AddChildView( | ||
| std::make_unique<views::ImageView>(ui::ImageModel::FromImageSkia( | ||
| *rb.GetImageSkiaNamed(IDR_SIDEBAR_BOOKMARKS_PANEL_HEADER)))); | ||
| constexpr int kSpacingBetweenHeaderImageAndLabel = 8; | ||
| header_image->SetProperty( | ||
| views::kMarginsKey, | ||
| gfx::Insets::TLBR(0, 0, 0, kSpacingBetweenHeaderImageAndLabel)); | ||
| header_image->SetProperty( | ||
| views::kFlexBehaviorKey, | ||
| views::FlexSpecification(views::MinimumFlexSizeRule::kPreferred, | ||
| views::MaximumFlexSizeRule::kPreferred)); | ||
|
|
||
| auto* header_label = AddChildView(std::make_unique<views::Label>( | ||
| l10n_util::GetStringUTF16(IDS_BOOKMARK_MANAGER_TITLE))); | ||
| header_label->SetFontList(gfx::FontList("Poppins, Semi-Bold 16px")); | ||
| header_label->SetEnabledColorId(kColorSidebarPanelHeaderTitle); | ||
| header_label->SetProperty( | ||
| views::kFlexBehaviorKey, | ||
| views::FlexSpecification(views::MinimumFlexSizeRule::kPreferred, | ||
| views::MaximumFlexSizeRule::kPreferred)); | ||
| auto* spacer = AddChildView(std::make_unique<views::View>()); | ||
| spacer->SetProperty( | ||
| views::kFlexBehaviorKey, | ||
| views::FlexSpecification(views::MinimumFlexSizeRule::kScaleToZero, | ||
| views::MaximumFlexSizeRule::kUnbounded) | ||
| .WithOrder(2)); | ||
| // Safe to use Unretained(this) here as |button| will be destroyed before | ||
| // this class. | ||
| auto* button = AddChildView(std::make_unique<views::ImageButton>( | ||
| base::BindRepeating(&BookmarksSidePanelHeaderView::OnButtonPressed, | ||
| base::Unretained(this), browser))); | ||
|
There was a problem hiding this comment. reported by reviewdog 🐶 There was a problem hiding this comment. |
||
| button->SetTooltipText(l10n_util::GetStringUTF16( | ||
| IDS_SIDEBAR_READING_LIST_PANEL_HEADER_BOOKMARKS_BUTTON_TOOLTIP)); | ||
|
|
||
| constexpr int kHeaderButtonSize = 20; | ||
| button->SetImageModel( | ||
| views::Button::STATE_NORMAL, | ||
| ui::ImageModel::FromVectorIcon( | ||
| kLeoLaunchIcon, kColorSidebarPanelHeaderButton, kHeaderButtonSize)); | ||
| button->SetImageModel( | ||
| views::Button::STATE_HOVERED, | ||
| ui::ImageModel::FromVectorIcon(kLeoLaunchIcon, | ||
| kColorSidebarPanelHeaderButtonHovered, | ||
| kHeaderButtonSize)); | ||
|
|
||
| SetBackground( | ||
| views::CreateThemedSolidBackground(kColorSidebarPanelHeaderBackground)); | ||
| } | ||
|
|
||
| ~BookmarksSidePanelHeaderView() override = default; | ||
| BookmarksSidePanelHeaderView(const BookmarksSidePanelHeaderView&) = delete; | ||
| BookmarksSidePanelHeaderView& operator=(const BookmarksSidePanelHeaderView&) = | ||
| delete; | ||
|
|
||
| private: | ||
| void OnButtonPressed(Browser* browser, const ui::Event& event) { | ||
| ShowSingletonTab(browser, GURL(chrome::kChromeUIBookmarksURL)); | ||
| } | ||
| }; | ||
|
|
||
| } // namespace | ||
|
|
||
| BraveBookmarksSidePanelView::BraveBookmarksSidePanelView(Browser* browser) { | ||
| SetLayoutManager(std::make_unique<views::FlexLayout>()) | ||
| ->SetOrientation(views::LayoutOrientation::kVertical); | ||
| AddChildView(std::make_unique<BookmarksSidePanelHeaderView>(browser)); | ||
| AddChildView(std::make_unique<views::Separator>()) | ||
| ->SetColorId(kColorSidebarPanelHeaderSeparator); | ||
|
|
||
| // Reuse upstream's bookmarks panl nwebui. | ||
| auto* web_view = | ||
| AddChildView(BookmarksSidePanelCoordinator::GetOrCreateForBrowser(browser) | ||
| ->CreateBookmarksWebView()); | ||
| web_view->SetProperty( | ||
| views::kFlexBehaviorKey, | ||
| views::FlexSpecification(views::MinimumFlexSizeRule::kPreferred, | ||
| views::MaximumFlexSizeRule::kUnbounded)); | ||
|
|
||
| StartObservingWebWebViewVisibilityChange(web_view); | ||
| } | ||
|
|
||
| BraveBookmarksSidePanelView::~BraveBookmarksSidePanelView() = default; | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| /* Copyright (c) 2023 The Brave Authors. All rights reserved. | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
| * You can obtain one at https://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| #ifndef BRAVE_BROWSER_UI_VIEWS_SIDE_PANEL_BRAVE_BOOKMARKS_SIDE_PANEL_VIEW_H_ | ||
| #define BRAVE_BROWSER_UI_VIEWS_SIDE_PANEL_BRAVE_BOOKMARKS_SIDE_PANEL_VIEW_H_ | ||
|
|
||
| #include "brave/browser/ui/views/side_panel/brave_side_panel_view_base.h" | ||
|
|
||
| class Browser; | ||
|
|
||
| // Gives bookmarks panel specific header view with web view. | ||
| class BraveBookmarksSidePanelView : public BraveSidePanelViewBase { | ||
| public: | ||
| explicit BraveBookmarksSidePanelView(Browser* browser); | ||
| ~BraveBookmarksSidePanelView() override; | ||
| BraveBookmarksSidePanelView(const BraveBookmarksSidePanelView&) = delete; | ||
| BraveBookmarksSidePanelView& operator=(const BraveBookmarksSidePanelView&) = | ||
| delete; | ||
| }; | ||
|
|
||
| #endif // BRAVE_BROWSER_UI_VIEWS_SIDE_PANEL_BRAVE_BOOKMARKS_SIDE_PANEL_VIEW_H_ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reported by reviewdog 🐶
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:
- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml
Cc @thypon @goodov @iefremov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
BraveBookmarksSidePanelCoordinatorandglobal_registry_is per browser object. Both life cycle is tied with Browser object. So, both are destroyed together whenBrowserobject is deleted. So, I think usingUnretained()here would not cause any issue. Also all upstream coordinator uses this pattern.