-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Sidebar V2 - SidebarContainerViewNew #33655
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
base: master
Are you sure you want to change the base?
Conversation
| width_animation_.SetSlideDuration(base::Milliseconds(kAnimationDurationMS)); | ||
|
|
||
| SetNotifyEnterExitOnChild(true); | ||
| } |
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 🐶
[opengrep] There are several global functions that facilitate dependency inversion. It will not be possible to call them from modularized features (no dependency cycles), and their usage in non-modularized features is considered a red flag
Don't use Browser*. This is functionally a container of hundreds of other pointers. It is impossible to specify dependencies, since Browser* functionally depends on everything. Instead, pass in the relevant pointers, e.g. Profile*, FooFeatureController, etc
References:
- https://chromium.googlesource.com/chromium/src/+/main/docs/chrome_browser_design_principles.md#structure_modularity
Source: https://github.com/brave/security-action/blob/main/assets/opengrep_rules/client/browser-dependency-inversion.yaml
Cc @goodov @cdesouza-chromium @bridiver
| kShowSidePanelButton, browser_->profile()->GetPrefs(), | ||
| base::BindRepeating( | ||
| &SidebarContainerViewNew::UpdateToolbarButtonVisibility, | ||
| base::Unretained(this))); |
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 🐶
[opengrep] 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/opengrep_rules/client/chromium-uaf.yaml
Cc @thypon @goodov
b73f22e to
2be6739
Compare
| sidebar_hide_timer_.Start( | ||
| FROM_HERE, base::Milliseconds(kHideDelayInMS), | ||
| base::BindOnce(&SidebarContainerViewNew::HideSidebar, | ||
| base::Unretained(this))); |
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 🐶
[opengrep] 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/opengrep_rules/client/chromium-uaf.yaml
Cc @thypon @goodov
e1970dd to
45648c7
Compare
45648c7 to
922972b
Compare
09cf75a to
e39bb13
Compare
e39bb13 to
07343d4
Compare
When you say 'Once V2 is default, tests will be converted back to regular fixtures', does that mean adding functions like letting the sidebar apps sync across devices and all that? I thank you for working extremely hard on Web Panel also. |
Well.. this PR is not related with sync. This PR is for refactoring our sidebar implementation. |
Sorry mate. I misread it when I was getting excited about something else. |
Resolves brave/brave-browser#52536
Sidebar V2 is an architectural refactoring that separates the panel management from SidebarContainerView.
This allows Brave to reuse upstream's side panel instead of maintaining a custom brave side panel inside the SidebarContainerView.
No functional change from sidebar control UI.
All behavior should be same except panel open/close. For now, panel open/close doesn't work with V2.
This PR introduces SidebarContainerViewNew and establishes a testing framework to support both V1 and V2 during the migration.
SidebarContainerViewNew Implementation
Test Infrastructure for V1/V2 Coexistence
Migration Strategy:
Parameterized tests allow us to ensure V2 doesn't break existing V1 behavior during the transition. Once V2 is default, tests will be converted back to regular fixtures.
TEST=
SidebarBrowserTest*