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

Encapsulate the split view in a new class #24478

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

sangwoo108
Copy link
Contributor

@sangwoo108 sangwoo108 commented Jul 3, 2024

Previously,

  • BrowserView was responsible for managing the split view.
  • ContentsContainer has two web view, and two dev tools, which would weird view hierarchy.

This PR introduces SplitView class and it encapsulate operations related to split view. And split view contains two ContentsContainer, so that we don't reuse upstream's contents container as is.

Also this change doesn't intend to change the behavior. The existing test like SplitViewBrowserTest and SplitViewLayoutManagerTest would check the regression.

Resolves brave/brave-browser#39542

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • 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 wiki
    • npm run presubmit wiki, 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:

No behavior changes.

@sangwoo108 sangwoo108 marked this pull request as ready for review July 3, 2024 04:38
@sangwoo108 sangwoo108 requested a review from a team as a code owner July 3, 2024 04:38
@sangwoo108 sangwoo108 requested a review from bridiver July 3, 2024 04:41
@sangwoo108 sangwoo108 force-pushed the sko/split-view-refactor branch from 0d74c08 to 480d308 Compare July 18, 2024 05:50
@sangwoo108 sangwoo108 force-pushed the sko/split-view-refactor branch from 480d308 to 1bc390c Compare July 29, 2024 23:07
@sangwoo108 sangwoo108 force-pushed the sko/split-view-refactor branch 3 times, most recently from 6804671 to fed9dad Compare August 9, 2024 04:49
@simonhong simonhong assigned simonhong and unassigned sangwoo108 Nov 11, 2024
@simonhong simonhong marked this pull request as draft November 11, 2024 07:13
@simonhong simonhong force-pushed the sko/split-view-refactor branch 3 times, most recently from e1291af to 9e0616c Compare November 26, 2024 04:06
@simonhong
Copy link
Member

simonhong commented Nov 26, 2024

Regressions from this PR.

  • Window resizing is too slow fixed
  • Flashing whenever change active tab in split view fixed

Whenever active tab changes in a same tile, we set different styled borfer to active/inactive tabs.
As each border's thickness are different, it causes re-layout in each tab container.
and this causes flashing whenever active tab changes.
When same thickness is used, not flickered. => Fixed by using same thickness.

@simonhong simonhong force-pushed the sko/split-view-refactor branch from 9e0616c to 5c52436 Compare December 9, 2024 02:26
@simonhong simonhong marked this pull request as ready for review December 10, 2024 04:40
@simonhong simonhong force-pushed the sko/split-view-refactor branch from 32ccddd to f01442c Compare December 10, 2024 06:18
@simonhong simonhong force-pushed the sko/split-view-refactor branch 4 times, most recently from f943ebf to e10df73 Compare December 11, 2024 12:49
@simonhong simonhong force-pushed the sko/split-view-refactor branch from e10df73 to 1cc501a Compare December 11, 2024 14:07
Copy link
Contributor

@fallaciousreasoning fallaciousreasoning left a comment

Choose a reason for hiding this comment

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

lgtm @simonhong

browser/ui/views/frame/brave_browser_view_layout.h Outdated Show resolved Hide resolved
sangwoo108 and others added 16 commits December 12, 2024 11:24
Previously,
* BrowserView was responsible for managing the split view.
* ContentsContainer has two web view, and two dev tools, which would
  weird view hierarchy.

This PR introduces `SplitView` class and it encapsulate operations
related to split view. And split view contains two `ContentsContainer`,
so that we don't reuse upstream's contents container as is.

Also this change doesn't intend to change the behavior. The existing
test like `SplitViewBrowserTest` and `SplitViewLayoutManagerTest` would
check the regression.
Use same border thickness for active/inactive tab in the split view.
Otherwise, it could cause web contents resizing whenever active tab
changes between the tab in the split view.
@simonhong simonhong force-pushed the sko/split-view-refactor branch from 1cc501a to 6ad8171 Compare December 12, 2024 02:52
Copy link
Contributor

[puLL-Merge] - brave/brave-core@24478

Description

This PR significantly refactors the split view functionality in the Brave browser. It introduces a new SplitView class and reorganizes related code, improving the structure and maintainability of the split view feature.

Changes

Changes

  1. browser/ui/views/split_view/split_view.cc and split_view.h:

    • New SplitView class introduced, encapsulating split view functionality
    • Handles layout, visibility, and management of secondary web views and devtools
  2. browser/ui/views/split_view/split_view_layout_manager.cc and split_view_layout_manager.h:

    • New SplitViewLayoutManager class for managing the layout of split view components
  3. browser/ui/views/frame/brave_browser_view.cc and brave_browser_view.h:

    • Removed direct management of split view components
    • Now uses SplitView class for split view functionality
  4. browser/ui/BUILD.gn:

    • Updated build configuration to include new split view files
    • Removed references to deleted files
  5. Various test files:

    • Updated to work with the new SplitView structure
    • Some tests moved to new locations
  6. Removed files:

    • brave_contents_layout_manager.cc and brave_contents_layout_manager.h
    • split_view_location_bar.cc and split_view_location_bar.h (moved to new location)
sequenceDiagram
    participant BraveBrowserView
    participant SplitView
    participant SplitViewLayoutManager
    participant ContentsWebView
    participant SecondaryContentsWebView

    BraveBrowserView->>SplitView: Create
    SplitView->>SplitViewLayoutManager: Create
    SplitView->>ContentsWebView: Manage
    SplitView->>SecondaryContentsWebView: Manage
    BraveBrowserView->>SplitView: WillChangeActiveWebContents
    SplitView->>SplitViewLayoutManager: UpdateLayout
    BraveBrowserView->>SplitView: DidChangeActiveWebContents
    SplitView->>ContentsWebView: UpdateVisibility
    SplitView->>SecondaryContentsWebView: UpdateVisibility
Loading

Possible Issues

  • The refactoring may introduce temporary instability or bugs in the split view functionality.
  • Some existing tests may need further adjustment to work with the new structure.

Security Hotspots

No significant security hotspots identified in this refactoring.

@simonhong
Copy link
Member

@bridiver PTAL when you're available.

@simonhong
Copy link
Member

@bridiver Kindly ping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encapsulate split view
4 participants