Added header to bookmarks panel #20211
Conversation
github-actions
bot
added
the
potential-layer-violation-fixes
simonhong
force-pushed
the
bookmark_panel_header
branch
from
3c6ab32 to
ceb3140
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ui::ImageModel::FromVectorIcon(omnibox::kStarIcon, ui::kColorIcon), | ||
| base::BindRepeating( | ||
| &BraveBookmarksSidePanelCoordinator::CreateBookmarksPanelView, | ||
| base::Unretained(this)))); |
There was a problem hiding this comment.
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.
This BraveBookmarksSidePanelCoordinator and global_registry_ is per browser object. Both life cycle is tied with Browser object. So, both are destroyed together when Browser object is deleted. So, I think using Unretained() here would not cause any issue. Also all upstream coordinator uses this pattern.
| // 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 🐶
[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.
// Safe to use Unretained(this) here as |button| will be destroyed before
// this class.
fix brave/brave-browser#32952 Bookmarks panel's header includes icon, title and button for loading bookmarks manager.
simonhong
force-pushed
the
bookmark_panel_header
branch
from
78ca02d to
70255d1
Compare
fix brave/brave-browser#32952
Bookmarks panel's header includes icon, title and button for loading bookmarks manager.
BraveBookmarksSidePanelCoordinatorgives bookmaks panel's contents view and that contents viewhas our header view and upstream web view.
Resolves
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run lint,npm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: