Skip to content

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

Merged
codebytere merged 2 commits into35-x-yfrom
trop/35-x-y-bp-refactor-remove-inspectablewebcontentsviewmac-in-favor-of-the-views-version-1737127277340
Jan 23, 2025
Merged

refactor: remove InspectableWebContentsViewMac in favor of the Views version#45238
codebytere merged 2 commits into35-x-yfrom
trop/35-x-y-bp-refactor-remove-inspectablewebcontentsviewmac-in-favor-of-the-views-version-1737127277340

Conversation

@trop
Copy link
Contributor

@trop trop bot commented Jan 17, 2025

Backport of #44628

See that PR for details.

Notes: none

trop bot and others added 2 commits January 17, 2025 15:21
…version

* cherry-pick: refactor: remove InspectableWebContentsViewMac in favor of the Views version (#41326)

commit e67ab9a

Confilcts not resolved, except removal of the files removed
by the original commit.

* resolved conflicts and build issues after cherry-pick

* cherry-picked: fix: add method allowing to disable headless mode in native widget

#42996
fixing
#42995

* fix: displaying select popup in window created as fullscreen window

`constrainFrameRect:toScreen:` is not being call for windows created
with `fullscreen: true` therefore `headless` mode was not being removed
and `RenderWidgetHostNSViewBridge::DisplayPopupMenu` ignored displaying
popup.

Issue could be fixed by placing additional removal of `headless` mode
in the `toggleFullScreen:`, but `orderWindow:relativeTo:` is called
both for a regular and a fullscreen window, therefore there will be
a single place fixing both cases.

Because `electron::NativeWindowMac` lifetime may be shorter than
`ElectronNSWindow` on which macOS may execute `orderWindow:relativeTo:`
we need to clear `shell_` when `NativeWindow` is being closed.

Fixes #43010.

* fix: Content visibility when using `vibrancy`

We need to put `NSVisualEffectView` before `ViewsCompositorSuperview`
otherwise when using `vibrancy` in `BrowserWindow` `NSVisualEffectView`
will hide content displayed by the compositor.

Fixes #43003
Fixes #42336

In fact main issues reported in these tickets were not present after
cherry-picking original refactor switching to `views::WebView`, so
text could be selected and click event was properly generated. However
both issues testcases were using `vibrancy` and actual content was
invisible, because it was covered by the `NSVisualEffectView`.

* fix: EXCEPTION_ACCESS_VIOLATION crash on BrowserWindow.destroy()

Restored postponed deletion of the `NativeWindow`.

Restoration caused `DCHECK(new_parent_ui_layer->GetCompositor());` failure
in `BrowserCompositorMac::SetParentUiLayer` after the spec test:
`chrome extensions chrome.webRequest does not take precedence over Electron webRequest - http`
with stack:
```
7   Electron Framework 0x000000011fe07830 content::BrowserCompositorMac::SetParentUiLayer(ui::Layer*) + 628
8   Electron Framework 0x000000011fe0c154 content::RenderWidgetHostViewMac::SetParentUiLayer(ui::Layer*) + 220
9   Electron Framework 0x000000011fe226a8 content::WebContentsViewMac::CreateViewForWidget(content::RenderWidgetHost*) + 600
10  Electron Framework 0x000000011fd37e4c content::WebContentsImpl::CreateRenderWidgetHostViewForRenderManager(content::RenderViewHost*) + 164
11  Electron Framework 0x000000011fb32278 content::RenderFrameHostManager::CreateSpeculativeRenderFrame(content::SiteInstanceImpl*, bool, scoped_refptr<content::BrowsingContextState> const&) + 816
12  Electron Framework 0x000000011fb2ab8c content::RenderFrameHostManager::CreateSpeculativeRenderFrameHost(content::SiteInstanceImpl*, content::SiteInstanceImpl*, bool) + 1308
13  Electron Framework 0x000000011fb28598 content::RenderFrameHostManager::GetFrameHostForNavigation(content::NavigationRequest*, content::BrowsingContextGroupSwap*, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>*) + 1796
14  Electron Framework 0x000000011fa78660 content::NavigationRequest::SelectFrameHostForOnRequestFailedInternal(bool, bool, std::__Cr::optional<std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>> const&) + 280
15  Electron Framework 0x000000011fa6a994 content::NavigationRequest::OnRequestFailedInternal(network::URLLoaderCompletionStatus const&, bool, std::__Cr::optional<std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>> const&, bo
+ 1008
16  Electron Framework 0x000000011fa7772c content::NavigationRequest::OnRequestFailed(network::URLLoaderCompletionStatus const&) + 72
17  Electron Framework 0x000000011f8554ac content::NavigationURLLoaderImpl::NotifyRequestFailed(network::URLLoaderCompletionStatus const&) + 248
```
This was probably the reason of removing `NativeWindow` immediately
in order to cleanup `views_host_` in `WebContentsViewMac` to prevent
using layer without compositor in `WebContentsViewMac::CreateViewForWidget`.

`[ElectronNSWindowDelegate windowWillClose:]` is deleting window host
and the compositor used by the `NativeWindow` therefore detach `NativeWindow`
contents from parent. This will clear `views_host_` and prevent failing
mentioned `DCHECK`.

Fixes #42975

* chore: Applied review suggestions

Co-authored-by: Michał Pichliński <michal.pichlinski@here.io>
Co-authored-by: Samuel Maddock <smaddock@slack-corp.com>
@trop trop bot requested a review from a team as a code owner January 17, 2025 15:21
@trop trop bot requested a review from a team January 17, 2025 15:21
@trop trop bot added 35-x-y backport This is a backport PR semver/minor backwards-compatible functionality labels Jan 17, 2025
@samuelmaddock
Copy link
Member

@electron/wg-releases Previously when this landed prior, it led to many regressions. @michal-pichlinski-openfin has addressed each regression in order to re-land these changes. I think we should make this available in v35 so we can get more users testing this earlier if possible.

@VerteDinde
Copy link
Member

VerteDinde commented Jan 17, 2025

Adding a backport/requested, this was a large enough change that I think we should vote on this before merging (it's also a semver/minor, so policy says we have to vote anyway)

@codebytere codebytere merged commit 91bb748 into 35-x-y Jan 23, 2025
62 of 63 checks passed
@release-clerk
Copy link

release-clerk bot commented Jan 23, 2025

No Release Notes

@codebytere codebytere deleted the trop/35-x-y-bp-refactor-remove-inspectablewebcontentsviewmac-in-favor-of-the-views-version-1737127277340 branch January 23, 2025 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

35-x-y backport/approved ✅ backport This is a backport PR semver/minor backwards-compatible functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants