Give main content area padding and a shadow #20526
Merged
Conversation
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
github-actions
bot
added
potential-layer-violation-fixes
zenparsing
force-pushed
the
ksmith-padded-content
branch
11 times, most recently
from
a4ab92a to
e203b05
Compare
sangwoo108
reviewed
Oct 20, 2023
There was a problem hiding this comment.
Vertical tab related stuff LGTM but deferring the final call to @simonhong as he would know better about contents layout and sidebar layout.
|
Got below crash while resizing window. WDP Infobar(#15822) was launched at that time. Only happened once and can't repro it again. |
|
@simonhong Ah, thanks! I see that layout is getting called recursively. STR:
|
zenparsing
force-pushed
the
ksmith-padded-content
branch
from
e203b05 to
9571224
Compare
simonhong
approved these changes
Oct 23, 2023
| @@ -262,10 +303,9 @@ void BraveBrowserView::UpdateSideBarHorizontalAlignment() { | |||
| prefs::kSidePanelHorizontalAlignment); | |||
|
|
|||
| sidebar_container_view_->SetSidebarOnLeft(on_left); | |||
| static_cast<BraveContentsLayoutManager*>(GetContentsLayoutManager()) | |||
| ->set_sidebar_on_left(on_left); | |||
| GetBrowserViewLayout()->set_sidebar_on_left(on_left); | |||
There was a problem hiding this comment.
nit: How about call sidebar_container_view_->SetSidebarOnLeft(on_left); from BraveBrowserViewLayout::SetSidebarOnLeft() as layout manager manages sidebar container?
There was a problem hiding this comment.
Good idea.
Another option would be to have BraveBrowserViewLayout just get the "on left" state from sidebar_container_view_, like sidebar_container_view_->is_sidebar_on_left(). That way, we wouldn't have to worry about the "on left" state ever getting out of sync. Would that work?
There was a problem hiding this comment.
I think it's good idea. It seems BraveBrowserView itself doesn't need left/right info
There was a problem hiding this comment.
Seems like BraveBrowserView still needs to listen to the pref change so that it can re-layout, though?
There was a problem hiding this comment.
Ah, I thought you'll use container view for listening prefs instead :)
IMO, current one seems fine but container view or layout manager also could listen prefs change if you want.
Comment on lines
9
to
15
| #define BRAVE_SIDE_PANEL_WEB_UI_VIEW_H \ | ||
| protected: \ | ||
| void AddedToWidget() override; | ||
|
|
||
| #include "src/chrome/browser/ui/views/side_panel/side_panel_web_ui_view.h" // IWYU pragma: export | ||
|
|
||
| #undef BRAVE_SIDE_PANEL_WEB_UI_VIEW_H |
There was a problem hiding this comment.
With this, we could avoid side_panel_web_ui_view.h patch file.
#include "ui/views/controls/webview/webview.h"
#define ViewHierarchyChanged \
AddedToWidget() override; \
void ViewHierarchyChanged
#include "src/chrome/browser/ui/views/side_panel/side_panel_web_ui_view.h" // IWYU pragma: export
#undef ViewHierarchyChanged
There was a problem hiding this comment.
Ah, I missed that we could include a dependency first to avoid interfering with parent class identifiers. Nice!
zenparsing
force-pushed
the
ksmith-padded-content
branch
from
9571224 to
cae6fe3
Compare
mkarolin
reviewed
Oct 23, 2023
| @@ -19,13 +19,11 @@ | |||
| #define BrowserWindow BraveBrowserWindow | |||
| #define BrowserViewLayout BraveBrowserViewLayout | |||
There was a problem hiding this comment.
Looks like this is a duplicate entry with L28 (similar to how #undef was duplicated).
goodov
approved these changes
Oct 23, 2023
| @@ -69,6 +69,8 @@ class SidebarContainerView | |||
| SidebarContainerView& operator=(const SidebarContainerView&) = delete; | |||
|
|
|||
| void Init(); | |||
|
|
|||
| bool sidebar_on_left() { return sidebar_on_left_; } | |||
mkarolin
approved these changes
Oct 23, 2023
zenparsing
force-pushed
the
ksmith-padded-content
branch
from
cae6fe3 to
63f645a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves brave/brave-browser#31645
A new feature flag has been created for this change:
With feature flag enabled:
Implementation Notes
In order to round the corners of the combined webview and devtools area, all other views have been moved out of the contents container view, matching upstream. The following views have been re-parented to the browser view:
Significant refactoring has been done in
brave_browser_view_layout.ccin order to accomplish this.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: