Skip to content

refactor: only create webContents after 'will-attach-webview' - #30311

Merged
nornagon merged 1 commit into
mainfrom
miniak/refactor-webview
Aug 3, 2021
Merged

refactor: only create webContents after 'will-attach-webview'#30311
nornagon merged 1 commit into
mainfrom
miniak/refactor-webview

Conversation

@miniak

@miniak miniak commented Jul 28, 2021

Copy link
Copy Markdown
Contributor

Description of Change

Merge createGuest and attachGuest handlers. Remove WebContentsPreferences::Merge.
Depends on #24344 and #30280.

Checklist

Release Notes

Notes: WebContents for <webview> are now only created after the 'will-attach-webview' event is emitted and preventDefault is not called.

@miniak miniak added the wip ⚒ label Jul 28, 2021
@miniak miniak self-assigned this Jul 28, 2021
@electron-cation electron-cation Bot added the new-pr 🌱 PR opened recently label Jul 28, 2021
@miniak miniak changed the title Miniak/refactor webview refactor: only create webContents after 'will-attach-webview' Jul 28, 2021
@miniak
miniak force-pushed the miniak/refactor-webview branch from 857d306 to 6e690a8 Compare July 28, 2021 22:41

@nornagon nornagon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks like a nice refactor! as a sanity check, as i'm not sure we have any tests for it, do things still work if you detach a webview from the dom, modify its parameters, and reattach it?

@ckerr ckerr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

➕ on the test question; but otherwise, this looks like a good patch

@miniak
miniak force-pushed the miniak/refactor-webview branch from 6e690a8 to 1d3e538 Compare August 2, 2021 15:54
@miniak
miniak marked this pull request as ready for review August 2, 2021 15:55
@miniak
miniak requested a review from a team as a code owner August 2, 2021 15:55
@miniak miniak added semver/patch backwards-compatible bug fixes and removed wip ⚒ labels Aug 2, 2021
@electron-cation electron-cation Bot removed the new-pr 🌱 PR opened recently label Aug 2, 2021
@miniak

miniak commented Aug 2, 2021

Copy link
Copy Markdown
Contributor Author

@nornagon It does work. btw web-contents-created is now emitted after will-attach-webview and not at all when the webview creation is prevented. Is that considered a breaking change?

@miniak

miniak commented Aug 2, 2021

Copy link
Copy Markdown
Contributor Author

@zcbenz and @deepak1556 can you please review?

@nornagon nornagon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice cleanup.

@nornagon
nornagon merged commit 6e43b0b into main Aug 3, 2021
@nornagon
nornagon deleted the miniak/refactor-webview branch August 3, 2021 17:08
@release-clerk

release-clerk Bot commented Aug 3, 2021

Copy link
Copy Markdown

Release Notes Persisted

WebContents for &lt;webview&gt; are now only created after the 'will-attach-webview' event is emitted and preventDefault is not called.

BlackHole1 pushed a commit to BlackHole1/electron that referenced this pull request Aug 30, 2021
ckerr added a commit that referenced this pull request Nov 10, 2021
jkleinsc pushed a commit that referenced this pull request Nov 11, 2021
ckerr added a commit that referenced this pull request Nov 15, 2021
VerteDinde pushed a commit that referenced this pull request Nov 15, 2021
…view'… (#31848)

* Revert "refactor: only create webContents after 'will-attach-webview' (#30311)" (#31785)

This reverts commit 6e43b0b.

* fix: backport 31785 to fix localStorage persistence regression
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.

5 participants