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

refactor: remove InspectableWebContentsViewMac in favor of the Views version #41326

Merged
merged 13 commits into from Mar 4, 2024

Conversation

nornagon
Copy link
Member

Description of Change

I just deleted the Mac version and things seem to work okay? A bit surprising.
Probably I missed something?

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes

Release Notes

Notes: none

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Feb 13, 2024
@nornagon nornagon added semver/patch backwards-compatible bug fixes no-backport labels Feb 13, 2024
@nornagon
Copy link
Member Author

nornagon commented Feb 13, 2024

Looks like this regresses #37301, which was fixed by #37386. Fixed.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Feb 14, 2024
@nornagon
Copy link
Member Author

Spent a day tracking down why the frame subscriber tests were failing. It looks like there's logic to skip rendering when the window is hidden unless headless mode is enabled: https://source.chromium.org/chromium/chromium/src/+/main:ui/views/cocoa/native_widget_mac_ns_window_host.mm;l=634-635;drc=3eb9fd518875bee685ad2e8c2b699822a8dfd5d9

  if (is_visible_ || is_headless_mode_window_)
    compositor_->Unsuspend();

i.e., if the window is hidden, the compositor won't unsuspend unless the headless bit is set.

The effects of setting the headless bit appear (currently) to be only this and one other thing: the frame size won't be constrained to a screen (https://source.chromium.org/chromium/chromium/src/+/main:components/remote_cocoa/app_shim/native_widget_mac_nswindow.mm;l=322-326;drc=2245eaf45d235260c18959d54844d6acc9fb4556). So I added some extra logic to unset the headless bit if we're visible in that function.

Prevents the window from being placed behind the menu bar.
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

This puts Electron's WebContents in a views::WebView, which I think might cause some behavior changes, but this refactoring surely worths the little risk.

@nornagon
Copy link
Member Author

nornagon commented Mar 4, 2024

Let's find out :D

@nornagon nornagon merged commit e67ab9a into main Mar 4, 2024
14 checks passed
@nornagon nornagon deleted the remove-iwcv-mac branch March 4, 2024 17:32
Copy link

release-clerk bot commented Mar 4, 2024

No Release Notes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants