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

feat: Allow WebContentsView to accept webContents object. #42086

Merged

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 in the last 24 hours 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 in the last 24 hours 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
22 of 23 checks passed
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popup handling in BrowserViews doesn't work with WebContentsView
6 participants