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: add safer nativeImage.createFromBitmap(), which does not decode PNG/JPEG #17337

Merged
merged 1 commit into from Mar 14, 2019

Conversation

@miniak
Copy link
Contributor

commented Mar 11, 2019

Description of Change

Required to safely serialize/deserialize NativeImage instances to be sent over IPC in #17200

/cc @nornagon

Checklist

Release Notes

Notes: Added safer nativeImage.createFromBitmap(), which does not decode PNG/JPEG.

@miniak miniak force-pushed the miniak/native-image-bitmap branch 2 times, most recently from 8c4f6bb to de3e18f Mar 11, 2019

@miniak miniak self-assigned this Mar 11, 2019

@miniak miniak changed the title feat: add safer nativeImage.createFromBitmap(), which does not decode PNGs feat: add safer nativeImage.createFromBitmap(), which does not decode PNG/JPEG Mar 11, 2019

@nornagon
Copy link
Contributor

left a comment

IMO we should deprecate createFromBuffer, and replace it with createFromBitmap (or maybe createFromRGBA), createFromPNG and createFromJPEG. Just guessing at the image format isn't really a sane way to do it.

Show resolved Hide resolved docs/api/native-image.md Outdated
Show resolved Hide resolved docs/api/native-image.md Outdated

@miniak miniak force-pushed the miniak/native-image-bitmap branch 3 times, most recently from 3e03037 to df45afd Mar 11, 2019

@miniak

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

@nornagon I was just thinking about doing that. I agree that having separate createFromPNG and createFromJPEG is cleaner. I will do that in a follow up PR. What about addRepresentation? That one has the same issues.

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

I'm thinking we should probably have some kind of "guess what this is" method. I can see it being handy when you think a file is an image but don't know what format it is.

@nornagon

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

I think that falls outside the scope of Electron. See e.g. http://www.darwinsys.com/file/

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

@nornagon At least pointing people to that / an npm module for file type detection would be enough. I just don't want to say "figure it out have fun" 👍

Show resolved Hide resolved docs/api/native-image.md Outdated
Show resolved Hide resolved atom/common/api/atom_api_native_image.cc Outdated

@miniak miniak force-pushed the miniak/native-image-bitmap branch from db6d05a to ad99925 Mar 12, 2019

@miniak miniak requested a review from nornagon Mar 12, 2019

@sindresorhus

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

Just guessing at the image format isn't really a sane way to do it.

It's not guessing, it's detecting, and I think it should stay. It's a nice convenience. I agree there should also be explicit methods for JPEG and PNG.

@sindresorhus

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

At least pointing people to that / an npm module for file type detection would be enough. I just don't want to say "figure it out have fun" 👍

My file-type package is the most popular one.

@electron-cation electron-cation bot removed the new-pr 🌱 label Mar 12, 2019

@miniak miniak force-pushed the miniak/native-image-bitmap branch from ad99925 to 019fcf8 Mar 13, 2019

@nornagon nornagon merged commit 878538f into master Mar 14, 2019

12 of 13 checks passed

Artifact Comparison Changes Detected
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190313.8 succeeded
Details
electron-arm64-testing Build #20190313.7 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Mar 14, 2019

Release Notes Persisted

Added safer nativeImage.createFromBitmap(), which does not decode PNG/JPEG.

@miniak miniak deleted the miniak/native-image-bitmap branch Mar 14, 2019

Kiku-Reise added a commit to Kiku-Reise/electron that referenced this pull request May 16, 2019

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.