Skip to content

Conversation

@khalwa
Copy link
Contributor

@khalwa khalwa commented May 8, 2024

Description of Change

Closes #42054.

This PR enables WebContentsView to accept existing webContents object in order to be able to properly handle popups. This issue is described here: #42054.

Checklist

Release Notes

Notes: Extended WebContentsView to accept pre-existing webContents object.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label May 8, 2024
@ckerr ckerr requested a review from nornagon May 8, 2024 15:58
@khalwa khalwa force-pushed the webContentsView-accepting-webcontents-option branch from 07cc3ac to bcf4199 Compare May 13, 2024 09:11
@khalwa khalwa requested a review from nornagon May 13, 2024 09:17
@codebytere codebytere added the semver/minor backwards-compatible functionality label May 13, 2024
@electron-cation electron-cation bot added api-review/requested 🗳 and removed new-pr 🌱 PR opened recently labels May 13, 2024
Copy link
Member

@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

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

API LGTM

Comment on lines +167 to +172
if (existing_web_contents->owner_window() != nullptr) {
args->ThrowError(
"options.webContents is already attached to a window");
return nullptr;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: It'd be preferred to move this check inside of the block on line 182

@samuelmaddock samuelmaddock added the target/31-x-y PR should also be added to the "31-x-y" branch. label May 28, 2024
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

API LGTM

@VerteDinde VerteDinde dismissed nornagon’s stale review May 30, 2024 19:45

Changes addressed above

@VerteDinde VerteDinde merged commit 85df2a8 into electron:main May 30, 2024
@release-clerk
Copy link

release-clerk bot commented May 30, 2024

Release Notes Persisted

Extended WebContentsView to accept pre-existing webContents object.

@trop
Copy link
Contributor

trop bot commented May 30, 2024

I have automatically backported this PR to "31-x-y", please check out #42319

@trop trop bot added in-flight/31-x-y and removed target/31-x-y PR should also be added to the "31-x-y" branch. labels May 30, 2024
@trop trop bot added merged/31-x-y PR was merged to the "31-x-y" branch. and removed in-flight/31-x-y labels Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-review/approved ✅ merged/31-x-y PR was merged to the "31-x-y" branch. semver/minor backwards-compatible functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Popup handling in BrowserViews doesn't work with WebContentsView

6 participants