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

fix: NativeImage serialization of <webview>.capturePage() result #20825

Merged
merged 1 commit into from Nov 12, 2019

Conversation

@miniak
Copy link
Contributor

miniak commented Oct 29, 2019

Description of Change

Fixes #8151
Fixes #16241

Checklist

Release Notes

Notes: Fixed <webview>.capturePage() resolving with an empty object instead of NativeImage instance.

@zcbenz
zcbenz approved these changes Oct 30, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 label Oct 30, 2019
@miniak miniak requested a review from deepak1556 Nov 1, 2019
@miniak miniak force-pushed the miniak/fix-webview-capture-page branch 3 times, most recently from 0388424 to 872ead0 Nov 3, 2019
@miniak

This comment has been minimized.

Copy link
Contributor Author

miniak commented Nov 5, 2019

@nornagon, @MarshallOfSound can you please review?

Copy link
Member

nornagon left a comment

Let's apply the serialize / deserialize operation only where it's needed, i.e. this one capturePage operation. Otherwise we add a bunch of extra useless CPU work for no reason.

@miniak miniak force-pushed the miniak/fix-webview-capture-page branch from 872ead0 to 1dfe5ee Nov 6, 2019
@miniak miniak requested a review from nornagon Nov 6, 2019
@miniak

This comment has been minimized.

Copy link
Contributor Author

miniak commented Nov 6, 2019

@nornagon done, I've created a separate IPC for capturePage, which does the serialization.

@miniak miniak force-pushed the miniak/fix-webview-capture-page branch 2 times, most recently from f8799ee to e6a1912 Nov 6, 2019
@miniak miniak added wip and removed wip labels Nov 11, 2019
@miniak miniak force-pushed the miniak/fix-webview-capture-page branch from e6a1912 to 526485a Nov 12, 2019
Copy link
Contributor

alexeykuzmin left a comment

isPromise looks weird though.

@nornagon

This comment has been minimized.

Copy link
Member

nornagon commented Nov 12, 2019

@alexeykuzmin i agree, but it was there before, so 🤷‍♂ i guess?

@alexeykuzmin

This comment has been minimized.

Copy link
Contributor

alexeykuzmin commented Nov 12, 2019

@nornagon yeah, I was going to write a comment about it but then checked contents of the removed file =) The PR looks good otherwise.

@alexeykuzmin

This comment has been minimized.

Copy link
Contributor

alexeykuzmin commented Nov 12, 2019

The win-x64-testing failure is not related to the change. Merging.

@miniak miniak deleted the miniak/fix-webview-capture-page branch Nov 13, 2019
miniak added a commit that referenced this pull request Nov 13, 2019
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 13, 2019

@miniak has manually backported this PR to "8-x-y", please check out #21103

miniak added a commit that referenced this pull request Nov 13, 2019
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 13, 2019

@miniak has manually backported this PR to "7-1-x", please check out #21104

miniak added a commit that referenced this pull request Nov 13, 2019
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 13, 2019

@miniak has manually backported this PR to "6-1-x", please check out #21105

miniak added a commit that referenced this pull request Nov 13, 2019
codebytere added a commit that referenced this pull request Nov 14, 2019
codebytere added a commit that referenced this pull request Nov 14, 2019
codebytere added a commit that referenced this pull request Nov 14, 2019
)

* refactor: add Error to isSerializableObject() (#20886)

* fix: NativeImage serialization of <webview>.capturePage() result (#20825)
@trop trop bot added merged/8-x-y and removed in-flight/8-x-y labels Nov 14, 2019
@miniak miniak mentioned this pull request Nov 18, 2019
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.