app: select-client-certificate event callback can accept certificate optionally #8134
Conversation
| if (skip) { | ||
| callback() | ||
| } else { | ||
| assert.equal(list.length, 1) |
There was a problem hiding this comment.
Just curious, could this still be done in the render process?
Assertion failures in the main process cause the specs to immediately terminate on CI, where as in the render process they will continue to run and finish.
| @@ -380,7 +380,7 @@ void OnClientCertificateSelected( | |||
| mate::Arguments* args) { | |||
| mate::Dictionary cert_data; | |||
| if (!args->GetNext(&cert_data)) { | |||
| args->ThrowError(); | |||
There was a problem hiding this comment.
So if you call the callback with an empty object, string, number, etc. now it will continue the request where previously it would error out.
Perhaps it would be better to explicitly check for the argument being null/undefined and still throw an error when an invalid parameter was specified to prevent accidental continuing.
What do you think?
There was a problem hiding this comment.
Sounds good to throw error for invalid arguments.
2573de0 to
cbddbdb
Compare
|
Have addressed the comments, thanks! |
| @@ -378,9 +378,21 @@ void OnClientCertificateSelected( | |||
| v8::Isolate* isolate, | |||
| std::shared_ptr<content::ClientCertificateDelegate> delegate, | |||
| mate::Arguments* args) { | |||
| if (args->Length() == 2) { | |||
There was a problem hiding this comment.
I'm not sure I understand this check for 2 here.
Calling callback(cert, null) would hit here and return early and not use the cert but calling callback(cert, null, null) would continue with the cert.
What case is this targeting?
There was a problem hiding this comment.
We want request to continue with nullptr when the callback is called with no arguments or null. Here callback has already two bound arguments on c++ side, so when callback() is called, the condition will be satisfied.
There was a problem hiding this comment.
Oh awesome, totally makes sense, thanks for explaining 👍
|
Thanks for this @deepak1556 🚢 |
Fixes #8088