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

[MM-59823] Migrate BrowserView to WebContentsView #3177

Merged
merged 7 commits into from
Oct 30, 2024
Merged

Conversation

devinbinnie
Copy link
Member

Summary

A while back, Electron implemented BrowserView as an experimental way of nesting sandboxed Chromium views inside of a main windows, replacing webview. However, for the longest time that functionality remained experimental and not fully supported.

This PR migrates us over to the new WebContentsView, introduced in Electron 30 which should be more stable and supported. It should also simplify resizing logic.

Ticket Link

https://mattermost.atlassian.net/browse/MM-59823

NONE

@devinbinnie devinbinnie added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Oct 23, 2024
@devinbinnie devinbinnie requested review from pvev, yasserfaraazkhan, a team and enahum and removed request for a team October 23, 2024 19:43
@yasserfaraazkhan yasserfaraazkhan added the Run Desktop E2E Tests This label will trigger the workflow that runs e2e automation tests label Oct 24, 2024
Copy link

Here are the test results below:

Test Summary for Linux on commit a0c98b6

The following known failed tests have been fixed on Linux:
- menu/view MM-T820 should open Developer Tools For Application Wrapper for main window

Test Summary for macOS on commit a0c98b6

New failed tests found on macOS:

  • Add Server Modal MM-T1312 should focus the first text input

The following known failed tests have been fixed on macOS:
- popup MM-T2827_1 should be able to select all in popup windows

Test Summary for Windows on commit a0c98b6

All stable tests passed on Windows.

@github-actions github-actions bot removed the Run Desktop E2E Tests This label will trigger the workflow that runs e2e automation tests label Oct 24, 2024
@devinbinnie devinbinnie added this to the v5.11.0 milestone Oct 24, 2024
@devinbinnie devinbinnie removed the 3: QA Review Requires review by a QA tester label Oct 24, 2024
Copy link
Contributor

@pvev pvev left a comment

Choose a reason for hiding this comment

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

LGTM! nice change @devinbinnie 👍

@devinbinnie
Copy link
Member Author

/update-branch

@devinbinnie
Copy link
Member Author

@enahum Gentle ping for review :) let me know if I need to re-assign

@enahum
Copy link
Contributor

enahum commented Oct 29, 2024

@enahum Gentle ping for review :) let me know if I need to re-assign

Sorry was not in my radar, I can give it a review in about 12h or so

Copy link
Contributor

@enahum enahum left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. lgtm

@devinbinnie devinbinnie added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Oct 30, 2024
@devinbinnie devinbinnie merged commit 5fccd0f into master Oct 30, 2024
11 checks passed
@devinbinnie devinbinnie deleted the MM-59823 branch October 30, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request kind/refactor release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants