Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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
spec/api-web-contents-view-spec.ts Show resolved Hide resolved
shell/browser/api/electron_api_web_contents_view.cc Outdated Show resolved Hide resolved
spec/api-web-contents-view-spec.ts Show resolved Hide resolved
@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

spec/api-web-contents-view-spec.ts Show resolved Hide resolved
@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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.