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: promisify dialog.showCertificateTrustDialog() #17181

Merged
merged 2 commits into from Mar 15, 2019

Conversation

Projects
None yet
4 participants
@codebytere
Copy link
Member

codebytere commented Mar 1, 2019

Description of Change

Promisify dialog.showCertificateTrustDialog().

Checklist

Release Notes

Notes: Converted dialog.showCertificateTrustDialog() to return a Promise instead of taking a callback.

@electron-cation electron-cation bot added new-pr 🌱 and removed new-pr 🌱 labels Mar 1, 2019

@codebytere codebytere force-pushed the promisify-showcerttrust branch from e0f6179 to b25f303 Mar 8, 2019

@codebytere codebytere marked this pull request as ready for review Mar 8, 2019

@codebytere codebytere requested review from miniak , zcbenz and deepak1556 Mar 8, 2019

@codebytere codebytere force-pushed the promisify-showcerttrust branch from b25f303 to aff0bc2 Mar 8, 2019

@@ -79,7 +80,8 @@ void ShowCertificateTrust(atom::NativeWindow* parent_window,
CertFreeCertificateChain(chain_context);
}

callback.Run();
promise.Resolve();

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Mar 8, 2019

Member

Is this method is synchronous / blocking on windows? Might be worth documenting / possibly making async somehow 🤔

@codebytere codebytere force-pushed the promisify-showcerttrust branch from aff0bc2 to defd7ae Mar 11, 2019

@codebytere codebytere force-pushed the promisify-showcerttrust branch 3 times, most recently from 7fda78f to 1e042da Mar 12, 2019

@codebytere codebytere requested a review from zcbenz Mar 12, 2019

@codebytere codebytere force-pushed the promisify-showcerttrust branch from 1e042da to 092b494 Mar 12, 2019

@miniak

miniak approved these changes Mar 13, 2019

@codebytere codebytere force-pushed the promisify-showcerttrust branch from 092b494 to a80719a Mar 13, 2019

@zcbenz

zcbenz approved these changes Mar 14, 2019

if ((self = [super init])) {
callback_ = callback;
promise_.reset(new atom::util::Promise(std::move(promise)));

This comment has been minimized.

@zcbenz

zcbenz Mar 14, 2019

Member

There is no need keeping promise_ in a std::unique_ptr. It is only needed when the stored promise may be empty, but in this class since the promise is guaranteed to be always available, it can just be stored as normal value.

@interface TrustDelegate : NSObject {
 @private
  atom::util::Promise promise_;
}

promise_ = std::move(promise);

This comment has been minimized.

@zcbenz

zcbenz Mar 14, 2019

Member

I just realized there is no constructor in objective-c classes so std::unique_ptr is the only way to go. This PR should be ready to go!

@codebytere codebytere force-pushed the promisify-showcerttrust branch from a80719a to 21a6f45 Mar 14, 2019

@codebytere codebytere force-pushed the promisify-showcerttrust branch from 21a6f45 to adfa72a Mar 14, 2019

@zcbenz zcbenz merged commit 961c9a8 into master Mar 15, 2019

5 of 6 checks passed

appveyor: win-ia32-testing AppVeyor build failed
Details
Semantic Pull Request ready to be squashed
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
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Mar 15, 2019

Release Notes Persisted

Converted dialog.showCertificateTrustDialog() to return a Promise instead of taking a callback.

@zcbenz zcbenz deleted the promisify-showcerttrust branch Mar 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.