Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12374020:
Add Android support for SSL client authentication to the browser layer. (Closed)
|
Created:
7 years, 9 months ago by digit1 Modified:
7 years, 9 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@client-cert-test Visibility:
Public. |
DescriptionAdd Android support for SSL client authentication to the browser layer.
- Add an Android-specific implementation for the
chrome::ShowSSLClientCertificateSelector() function that calls
native APIs to let the user select a client certificate, and
return the result back to the caller when it is received from
the system dialog/application.
BUG=166642, 172902, 134418
TEST=Manual testing with run-forwarded-test-server.sh (browser).
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187507
Patch Set 1 #
Total comments: 16
Patch Set 2 : try something different #Patch Set 3 : simple rebasing #Patch Set 4 : a few more nits #
Total comments: 21
Patch Set 5 : optimizations #
Total comments: 11
Patch Set 6 : git cl try #Patch Set 7 : ensure callback(NULL) is posted as a task in case of error. #Patch Set 8 : Remove un-necessary refcounting #Patch Set 9 : use base::Passed() to get rid of helper struct. #Patch Set 10 : more simplification #
Total comments: 20
Patch Set 11 : nits #
Total comments: 14
Patch Set 12 : remove obsolete declarations in header. #
Total comments: 21
Patch Set 13 : address joth's nits #
Total comments: 4
Patch Set 14 : Address marcus' nits. #
Total comments: 2
Patch Set 15 : reorder Java sources properly in chrome_browser.gypi #Messages
Total messages: 34 (0 generated)
This is an initial version of the patch that completes client certificate support for Android. Ryan, please do not look at style issues in this patch. On the other hand, I would really appreciate if you could look at the lifecycle/ownership handling for the C++ SSLClientCertificateRequest object (in ssl_client_certificate_request.cc). I believe the code is correct, but you know all uses of base::Bind() much better than I do. Marcus, I had to put the RegisterSSLClientCertificateRequestAndroid() declaration in its own header to avoid bringing openssl as an Android dependency into chrome_browser.gypi. Maybe there is a way to achive that without doing so, but it looks like the auto-generated JNI code in jni/SSLClientCertificateRequest_jni.h will not compile if there isn't a full declaration for the SSLClientCertRequest class before it is included. If you have an alternative way to do the same, I'd be glad to hear it. I don't think the single-declaration header is too much of an issue here though. I'm still working on a way to unit-test this. This requires writing a very different implementation of chrome/browser/ssl/ssl_client_certificate_selector_test.cc but should be doable without touching any non-Android sources. More on this later. Thanks.
thanks digit! quick drive-by, I'm very interested in the JNI problems you mention, let me try to help! :) more questions below: https://codereview.chromium.org/12374020/diff/1/chrome/android/java/src/org/c... File chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java (right): https://codereview.chromium.org/12374020/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: 13 :) https://codereview.chromium.org/12374020/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:27: public class SSLClientCertificateRequest extends AsyncTask<Void, Void, Void> question (apologies if stupid :) is it correct to say that this class is only going to be accessed from native? if so, make private everything that is not an @Override.. "public" tend to be for java visibility, specially across packages, not much JNI visibility (which can access everything anyways...) https://codereview.chromium.org/12374020/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:124: // Called to pass request results to native side. nit: native stubs tend to go last https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... File chrome/browser/ui/android/ssl_client_certificate_request_jni.h (right): https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... chrome/browser/ui/android/ssl_client_certificate_request_jni.h:13: // path dependencies into chrome_browser.gypi. sorry, I'm not quite sure I understood this... would you have any error message? I think it's doable to just add this a function in ssl_client_certificate_request.h, or maybe even more scoped, a static function in SSLClientCertificateRequest itself? it seems that the ssl_client_certificate_request.h provides all forward declaration necessary to that the generated JNI code can bind everything correctly, fairly similar to content_view_core_impl.cc and many other classes, no? let me know what is broken, I'll be glad to fix it!
https://codereview.chromium.org/12374020/diff/1/chrome/android/java/src/org/c... File chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java (right): https://codereview.chromium.org/12374020/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:27: public class SSLClientCertificateRequest extends AsyncTask<Void, Void, Void> On 2013/02/28 16:34:18, bulach wrote: > question (apologies if stupid :) > > is it correct to say that this class is only going to be accessed from native? > if so, make private everything that is not an @Override.. "public" tend to be > for java visibility, specially across packages, not much JNI visibility (which > can access everything anyways...) Currently yes, but I plan to have a helper unit testing class that would use this class. Your point is duly noted for the next patch thoughm thanks. https://codereview.chromium.org/12374020/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:124: // Called to pass request results to native side. On 2013/02/28 16:34:18, bulach wrote: > nit: native stubs tend to go last ok, I'll move it https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... File chrome/browser/ui/android/ssl_client_certificate_request_jni.h (right): https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... chrome/browser/ui/android/ssl_client_certificate_request_jni.h:13: // path dependencies into chrome_browser.gypi. On 2013/02/28 16:34:18, bulach wrote: > sorry, I'm not qu4ite sure I understood this... would you have any error message? > I think it's doable to just add this a function in > ssl_client_certificate_request.h, or maybe even more scoped, a static function > in SSLClientCertificateRequest itself? > > it seems that the ssl_client_certificate_request.h provides all forward > declaration necessary to that the generated JNI code can bind everything > correctly, fairly similar to content_view_core_impl.cc and many other classes, > no? > let me know what is broken, I'll be glad to fix it! Yes, I know it's a bit confusing. So this function is used only in chrome/browser/android/chrome_jni_registrar.cc. In a previous version of this patch, this declaration was in ssl_client_certificate_request.h, which was included by chrome_jni_registrar.cc. But that header includes OpenSSL-specific headers, which themselves have to include OpenSSL headers (e.g. <openssl/evp.h>). And this is a requirement of using scoped ref-counted OpenSSL types in the class definition. chrome_jni_registrar.cc would not compile unless I add openssl as a Android-specific dependency to the 'browser' target in chrome_browser.gypi. Note that openssl is already a dependency for the 'browser_ui' target, which doesn't seem to have its own JNI registration stuff. By placing the declaration here, I avoid this problem entirely, but if you have a better solution, or you just think that 'browser' should depend on OpenSSL, just let me know.
Note that the android bots are failing because they don't seem to first download/apply the patch this one depends on. I'm not sure if it's normal or a problem with the bots though.
"git try master" to try the full patch sequence. The git-cl commands typically take an optional branch for the diff-head. E.g. "git cl upload branch_a && git try master" On Feb 28, 2013 8:50 AM, <digit@google.com> wrote: > Note that the android bots are failing because they don't seem to first > download/apply the patch this one depends on. I'm not sure if it's normal > or a > problem with the bots though. > > https://codereview.chromium.**org/12374020/<https://codereview.chromium.org/1... >
ahn, I think I see the issue with the jni here, it's bridging too many levels (different chrome targets, openssl, java and cc+) :) my proposal, not quite sure how much better that would be: - given that the JNI itself seems to be just using byte[][] and the java.security.PrivateKey besides PODs, would it be possible to make it in a way that there's a super thin C++ side just doing "chrome c++ <-> java", and then another level wrapping that one with the OpenSSL types? that is, a C++ class that just bridges the @CalledByNative and the "native", and then defines a delegate to pass through the data types up which will then be implemented by the full-blown scoped ref counted ssl types etc..? this would potentially make it easier to test as well, as the underlying java bits would be easier to mock out from the c++ side, wdyt?
https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... File chrome/browser/ui/android/ssl_client_certificate_request.cc (right): https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... chrome/browser/ui/android/ssl_client_certificate_request.cc:49: if (!callback_.is_null()) { Under what situations is this possible? I understand that it's a catch for when DoSendClientCertificate() has not been called (which would run and reset the callback), but it's not clear when that can actually happen. I'd also recommend you use a temporary for your .Run/.Reset() paradigm. I'm concerned about a situation where a .Run of a callback causes the current SSLClientCertificateRequest to be deleted. In the existing paradigm (.Run() then .Reset()), it's possible for the callback to be run twice (DoSendClientCertificate -> Run() -> dtor -> Run()) A safer pattern would be to use base::ResetAndReturn(&callback_).Run(NULL), which will swap out with a temporary before running. However, you should only need that in the DoSend path, AFAICT. https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... chrome/browser/ui/android/ssl_client_certificate_request.cc:199: this)); Because this object is RefCounted, you're forcing it to continue living on the multiple threads (IO and UI). Was that intentional? It seems like you could use a PostTaskAndReply idiom with a helper for DoRecordClientCertificateKey in the implementation, rather than having this be both ref-counted and living on multiple threads. https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... File chrome/browser/ui/android/ssl_client_certificate_request.h (right): https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... chrome/browser/ui/android/ssl_client_certificate_request.h:33: const chrome::SelectCertificateCallback& callback); nit: indent to 4 spaces
https://codereview.chromium.org/12374020/diff/1/chrome/android/java/src/org/c... File chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java (right): https://codereview.chromium.org/12374020/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/02/28 16:34:18, bulach wrote: > nit: 13 :) Done. https://codereview.chromium.org/12374020/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:27: public class SSLClientCertificateRequest extends AsyncTask<Void, Void, Void> ok, the unit test didn't make it, so I've changed everything back to private for now. :-/ https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... File chrome/browser/ui/android/ssl_client_certificate_request.cc (right): https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... chrome/browser/ui/android/ssl_client_certificate_request.cc:49: if (!callback_.is_null()) { On 2013/02/28 19:42:42, Ryan Sleevi wrote: > Under what situations is this possible? > > I understand that it's a catch for when DoSendClientCertificate() has not been > called (which would run and reset the callback), but it's not clear when that > can actually happen. > This can happen when an error exit happened in OnRequestCompletion(). Note that this is not part of the destructor in the newest version of the code. > I'd also recommend you use a temporary for your .Run/.Reset() paradigm. I'm > concerned about a situation where a .Run of a callback causes the current > SSLClientCertificateRequest to be deleted. In the existing paradigm (.Run() then > .Reset()), it's possible for the callback to be run twice > (DoSendClientCertificate -> Run() -> dtor -> Run()) > > A safer pattern would be to use base::ResetAndReturn(&callback_).Run(NULL), > which will swap out with a temporary before running. > > However, you should only need that in the DoSend path, AFAICT. I don't think this is totally necessary, because the callback object don't know anything about the request object, but I'll add that to the code for safety. https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... chrome/browser/ui/android/ssl_client_certificate_request.cc:199: this)); Indeed, the object doesn't have to live in multiple threads. I've changed the code to use PostTaskAndReply() which allows it to live exclusively in the UI thread. Only the certificate and the private key need to be sent to the I/O thread, so I've created a small helper class to do that (ScopedEVP_PKEY is not a real scoped_refptr<> so cannot be passed directly in a base::Bind() call). https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... File chrome/browser/ui/android/ssl_client_certificate_request.h (right): https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... chrome/browser/ui/android/ssl_client_certificate_request.h:33: const chrome::SelectCertificateCallback& callback); On 2013/02/28 19:42:42, Ryan Sleevi wrote: > nit: indent to 4 spaces Done. https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... File chrome/browser/ui/android/ssl_client_certificate_request_jni.h (right): https://codereview.chromium.org/12374020/diff/1/chrome/browser/ui/android/ssl... chrome/browser/ui/android/ssl_client_certificate_request_jni.h:13: // path dependencies into chrome_browser.gypi. I've reworked the code to get rid of this header. Now, SSLClientCertificateRequest is now a simpler base class, and the RegisterXXX function can be placed in ssl_client_certificate_request.h properly. ssl_client_cetificate_selector_android.cc provides a subclass that extends it with the necessary missing bits. The end result is also a bit cleaner, i.e. SSLClientCertiicateRequest only deals with JNI calls and objects, while the subclass deals with the tricky thread ping-pong and callback calls.
So, I've tried to write a unit test for this recently, but this doesn't seem possible. I started extending chrome/browser/ssl/ssl_browser_tests.cc, but it turns out that it's impossible to build the 'browser_tests' target for Android, so this didn't work at all :-/ Another try was to add a new ChromiumTestShell test, but it seems impossible to start a test remote server and the appropriate network redirections with them. Looking at solutions for this now, but I'm rather skeptical. I've started manually testing the current code too.
https://codereview.chromium.org/12374020/diff/3003/chrome/browser/ui/android/... File chrome/browser/ui/android/ssl_client_certificate_request.h (right): https://codereview.chromium.org/12374020/diff/3003/chrome/browser/ui/android/... chrome/browser/ui/android/ssl_client_certificate_request.h:86: jobject private_key_ref); Method naming: It's not at all clear how SSLClientCertificateRequest::OnCompletion differs from SSLClientCertificateRequest::OnRequestCompletion - can you find better names here? OnCertificateSelected? OnPrivateKeyLocated? I honestly don't know the better names, but it hopefully there's something clearer here. https://codereview.chromium.org/12374020/diff/3003/chrome/browser/ui/android/... chrome/browser/ui/android/ssl_client_certificate_request.h:101: bool RegisterSSLClientCertificateRequestAndroid(JNIEnv* env); why does this have to be in the global namespace? https://codereview.chromium.org/12374020/diff/3003/chrome/browser/ui/android/... File chrome/browser/ui/android/ssl_client_certificate_selector.cc (right): https://codereview.chromium.org/12374020/diff/3003/chrome/browser/ui/android/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:60: class CallbackGuard { We already have this, in src/base/bind_helpers.h See ScopedClosureRunner ScopedClosureRunner guard(base::Bind(callback_, NULL)); .... if (success) { guard.Release(); callback_.Run(SomeResult); } https://codereview.chromium.org/12374020/diff/3003/chrome/browser/ui/android/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:62: explicit CallbackGuard(chrome::SelectCertificateCallback& callback) I've mentioned it in past reviews, but in Google C++ & in Chrome, we do not pass non-const references. https://codereview.chromium.org/12374020/diff/3003/chrome/browser/ui/android/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:84: : public base::RefCountedThreadSafe<CertificateAndKey> { 1) We have RefCountedData<T> for this 2) You shouldn't need to RefCount - a simple struct with a scoped_ptr<> should be sufficient to pass between threads using base::Passed() https://codereview.chromium.org/12374020/diff/3003/chrome/browser/ui/android/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:101: class Request : public SSLClientCertificateRequest { I'm not sure why you've got the inheritance route. It seems like added overhead of a vtable for what seems like a build system issue? https://codereview.chromium.org/12374020/diff/3003/chrome/browser/ui/android/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:120: scoped_refptr<CertificateAndKey> pair); I've pointed out in past reviews, only scoped_ptr<> is passed by-value. https://codereview.chromium.org/12374020/diff/3003/chrome/browser/ui/android/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:174: base::Bind(&Request::DoSendCertificate, this, pair->client_cert)); Why have a DoSendCertificate helper? Why not simply base::Bind(callback_, pair->client_cert) ? You've already .Reset the guard (line 165), so you no longer have to worry about callback_ triggering on dtor, so I'm not sure why you loop back through this object. https://codereview.chromium.org/12374020/diff/3003/chrome/browser/ui/android/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:198: scoped_refptr<Request> request(new Request(callback)); Not sure why the RefCounting
https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... File chrome/browser/ui/android/ssl_client_certificate_request.h (right): https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... chrome/browser/ui/android/ssl_client_certificate_request.h:101: bool RegisterSSLClientCertificateRequestAndroid(JNIEnv* env); Most of similar functions that are called from chrome/browser/android/chrome_jni_registrar.cc are in the global namespace. I'll move this to something different. https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... File chrome/browser/ui/android/ssl_client_certificate_selector.cc (right): https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... chrome/browser/ui/android/ssl_client_certificate_selector.cc:60: class CallbackGuard { I'm not sure I didn't already mentioned already, but your proposal doesn't compile at all (even adding the trivial missing base:: prefix to ScopedClosureRunner). The error message is unreadable, even with Clang, so I had to design this helper instead. Let me know if you have a better proposal though. https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... chrome/browser/ui/android/ssl_client_certificate_selector.cc:62: explicit CallbackGuard(chrome::SelectCertificateCallback& callback) Sorry about that, I've fixed it. https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... chrome/browser/ui/android/ssl_client_certificate_selector.cc:84: : public base::RefCountedThreadSafe<CertificateAndKey> { 1) It looks like RefCountedData<> would be appropriate if there was only a single member here. Unless you recommend using RefCountedData<std::pair< scoped_refptr<net::X509Certificate>, ScopedEVP_PKEY> >, which seems less readable to me (especially at use sites). 2) Thanks, I wasn't sure about that, the PostTaskAndReply() documentation uses a RefCountedThreadSafe example, so I erred on the side of caution. https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... chrome/browser/ui/android/ssl_client_certificate_selector.cc:101: class Request : public SSLClientCertificateRequest { On 2013/03/04 23:47:21, Ryan Sleevi wrote: > I'm not sure why you've got the inheritance route. It seems like added overhead > of a vtable for what seems like a build system issue? This change reflect's Marcus' recommendation. He mentioned using a delegate but this would require the same number of vtables anyway. Would you prefer to add openssl as a dependency to the 'browser_test' target instead? i.e. go back to the old design, or do you have something different in mind? https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... chrome/browser/ui/android/ssl_client_certificate_selector.cc:120: scoped_refptr<CertificateAndKey> pair); On 2013/03/04 23:47:21, Ryan Sleevi wrote: > I've pointed out in past reviews, only scoped_ptr<> is passed by-value. The above line comes from the fact that a scoped_ptr<> couldn't be used with a refcounted type, but since CertificateAndKey no longer requires this constraint, I've changed the code to use scoped_ptr<>. https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... chrome/browser/ui/android/ssl_client_certificate_selector.cc:174: base::Bind(&Request::DoSendCertificate, this, pair->client_cert)); That's an excellent suggestion, I didn't think about it but I've implemented it in the latest patch. Thanks. https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... chrome/browser/ui/android/ssl_client_certificate_selector.cc:198: scoped_refptr<Request> request(new Request(callback)); The refcounting is used to ensure that the request is destroy through Release() when Start() call fails (if it succeeds, it increments the request's reference count). A simple scoped_ptr<> would not compile since the class is refcounted.
https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... File chrome/browser/ui/android/ssl_client_certificate_selector.cc (right): https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... chrome/browser/ui/android/ssl_client_certificate_selector.cc:60: class CallbackGuard { On 2013/03/05 16:54:00, digit1 wrote: > I'm not sure I didn't already mentioned already, but your proposal doesn't > compile at all (even adding the trivial missing base:: prefix to > ScopedClosureRunner). The error message is unreadable, even with Clang, so I had > to design this helper instead. Let me know if you have a better proposal though. Can you paste the error? I really want to avoid creating another instance of an existing class. https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... chrome/browser/ui/android/ssl_client_certificate_selector.cc:101: class Request : public SSLClientCertificateRequest { On 2013/03/05 16:54:00, digit1 wrote: > On 2013/03/04 23:47:21, Ryan Sleevi wrote: > > I'm not sure why you've got the inheritance route. It seems like added > overhead > > of a vtable for what seems like a build system issue? > > This change reflect's Marcus' recommendation. He mentioned using a delegate but > this would require the same number of vtables anyway. > > Would you prefer to add openssl as a dependency to the 'browser_test' target > instead? i.e. go back to the old design, or do you have something different in > mind? Can you not just pass a base::Closure/Callback in the ctor of the SSLClientCertRequest, and use that. It avoids the vtable of passing a delegate, and avoids the vtable overhead of subclassing. https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... chrome/browser/ui/android/ssl_client_certificate_selector.cc:120: scoped_refptr<CertificateAndKey> pair); On 2013/03/05 16:54:00, digit1 wrote: > On 2013/03/04 23:47:21, Ryan Sleevi wrote: > > I've pointed out in past reviews, only scoped_ptr<> is passed by-value. > > The above line comes from the fact that a scoped_ptr<> couldn't be used with a > refcounted type, but since CertificateAndKey no longer requires this constraint, > I've changed the code to use scoped_ptr<>. I think you missed what I was saying: scoped_refptr<>'s are passed as const-ref or pointers, not as values. scoped_ptr<>s are the only one we pass by value (in conjunction with Pass()) https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_request.h (right): https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.h:59: : public base::RefCounted<SSLClientCertificateRequest> { It's still not clear to me at all that you need RefCounting to protect the lifetime of this class. It's much clearer, and harder to misuse (since deriving from RefCounting makes it part of the *public* interface), to simply have the object clean up after itself (eg: "delete this") https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_selector.cc (right): https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:84: class CertificateAndKey { Why is this a class and not a struct https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:87: ~CertificateAndKey() { } no spaces in braces https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:89: ScopedEVP_PKEY private_key; It's still not clear to me why you need this class - ScopedEVP_PKEY derives from scoped_ptr<>, so you should get .Pass() (and base::Passed) for free. You shouldn't need to wrap this in a helper at all. https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:196: callback.Run(NULL); DESIGN: In general, we avoid recursing directly into callbacks that have asynchronous lifetimes, to avoid infinite stack recursion. This is why we follow the pattern of "Return a pending error or result". Since this API is defined as purely asynchronous (eg: no immediate result) - which, to clarify, **is fine** - then the pattern should be that we don't immediately run |callback|, but instead defer it (eg: via PostTask).
https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... File chrome/browser/ui/android/ssl_client_certificate_selector.cc (right): https://chromiumcodereview.appspot.com/12374020/diff/3003/chrome/browser/ui/a... chrome/browser/ui/android/ssl_client_certificate_selector.cc:60: class CallbackGuard { Ok, the error was due to the NULL inside the base::Bind() call, the compiler complained that it couldn't cast or match a net::X509Certificate* into an int. I could solve this with the following: base::ScopedClosureRunner guard( base::Bind(callback_, scoped_refptr<net::X509Certificate>())); I guess that's why nullptr would be nice in the future. I'll upload a new patch soon, I'm still experimenting with your other suggestions. https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_request.h (right): https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.h:59: : public base::RefCounted<SSLClientCertificateRequest> { I've experimented a little and could get rid of the refcounting entirely. I also got rid of the SSLClientCertificateRequest class. See latest patch for details. https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_selector.cc (right): https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:84: class CertificateAndKey { It used to require to be a class when if was refcounted. I'll simplify further. https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:89: ScopedEVP_PKEY private_key; I've originally tried passing a ScopedEVP_PKEY directly, but this fails to compile with a cryptic error message, i.e.: .... ../../base/bind_internal.h:2590:9: error: no matching constructor for initialization of 'scoped_ptr<evp_pkey_st, net::OpenSSLClientKeyStore::EVP_PKEY_Deleter>' p2_(p2) { It looks like the problem is that ScopedEVP_PKEY doesn't have a copy-constructor, but it's hard to understand why. Hence, the use of a helper class/struct to wrap it. https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:196: callback.Run(NULL); This has been addressed in the last patch, thanks.
https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_selector.cc (right): https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:89: ScopedEVP_PKEY private_key; On 2013/03/06 01:48:33, digit1 wrote: > I've originally tried passing a ScopedEVP_PKEY directly, but this fails to > compile with a cryptic error message, i.e.: > > .... > ../../base/bind_internal.h:2590:9: error: no matching constructor for > initialization of 'scoped_ptr<evp_pkey_st, > net::OpenSSLClientKeyStore::EVP_PKEY_Deleter>' > p2_(p2) { > > It looks like the problem is that ScopedEVP_PKEY doesn't have a > copy-constructor, but it's hard to understand why. Hence, the use of a helper > class/struct to wrap it. You have to use base::Passed() here, which will create an object that will use scoped_ptr<>::Pass() [which is our C++11 move emulation - aka, almost-as-good-as a copy ctor) base::Bind( &DoRecordClientCertificateKey, certificate, base::Passed(private_key)); // private_key is now NULL, ownership is transferred to the callback This is covered in http://dev.chromium.org/developers/coding-style/important-abstractions-and-da...
https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_selector.cc (right): https://chromiumcodereview.appspot.com/12374020/diff/17001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_selector.cc:89: ScopedEVP_PKEY private_key; thanks, this works and has been implemented in the latest patch. The latter also contains multiple simplifications that gets rid of the helper struct, or any extra C++ classes (it's now purely based on callbacks).
Looking much much cleaner and much much better - thanks! https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_request.cc (right): https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:32: scoped_refptr<net::X509Certificate> client_cert, I mentioned in previous reviews: Do not pass scoped_refptr<> by value - *only* scoped_ptr (or, in this case, ScopedEVP_PKEY). const scoped_refptr<net::X509Certificate>& client_cert https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:43: JNIEnv* env = base::android::AttachCurrentThread(); Should this be moved to line 73 - it seems unused until then. https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:46: // in case of error exit. // in case of an error. (I find "error exit" less clear) https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:107: } This port number check seems superflous - it's not user input, it's part of the API contract. If you want to DCHECK, great, but I'd recommend moving this if() altogether - it's an API violation to call with a junk cert_request_info, not a runtime error. https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:109: // Create a new req object and convert its address "req object" is not defined, contextually or externally. // Create a copy of the callback on the heap, so that its // address (and ownership) can be passed through and returned // from the Java method via JNI. https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:127: (void) dummy; Why are you following this pattern? Trying to avoid an unused variable report? See https://code.google.com/p/chromium/codesearch#chromium/src/base/compiler_spec... for the Chromium way of dealing with this pattern. https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_request.h (right): https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.h:34: CertificateSelectionCallback; Suggestion on reflow: typedef base::Callback<void(const std::vector<base::StringPiece>*, jobject)> CertificateSelectionCallback; Note that you've duplicated lines 29-31 and 51-53 (effectively). I'd suggest documenting in one place the error handling. https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.h:43: // Note the following limitations, coming from the platform APIS: Suggestion on reword: // Due to platform APIs, note the following limitations: https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.h:49: // style: suggest deleting the blank lines on 44, 46, and 49. https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.h:54: // nit: delete blank line
https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_request.cc (right): https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:32: scoped_refptr<net::X509Certificate> client_cert, Ah I misunderstood what you meant. For the record, the documentation in base/callback.h uses scoped_refptr<> passed by value and says that it should "just work", that' why I tended to use this. I've changed it. https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:43: JNIEnv* env = base::android::AttachCurrentThread(); On 2013/03/06 21:50:43, Ryan Sleevi wrote: > Should this be moved to line 73 - it seems unused until then. Not sure what you mean, it's used at least three times in the function. Moved anyway. https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:46: // in case of error exit. On 2013/03/06 21:50:43, Ryan Sleevi wrote: > // in case of an error. > > (I find "error exit" less clear) Done. https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:107: } Removed it. https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:109: // Create a new req object and convert its address On 2013/03/06 21:50:43, Ryan Sleevi wrote: > "req object" is not defined, contextually or externally. > > // Create a copy of the callback on the heap, so that its > // address (and ownership) can be passed through and returned > // from the Java method via JNI. Done. https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:127: (void) dummy; yes, thanks for the pointer. https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_request.h (right): https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.h:34: CertificateSelectionCallback; On 2013/03/06 21:50:43, Ryan Sleevi wrote: > Suggestion on reflow: > > typedef base::Callback<void(const std::vector<base::StringPiece>*, > jobject)> > CertificateSelectionCallback; > > Note that you've duplicated lines 29-31 and 51-53 (effectively). I'd suggest > documenting in one place the error handling. Done. https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.h:43: // Note the following limitations, coming from the platform APIS: On 2013/03/06 21:50:43, Ryan Sleevi wrote: > Suggestion on reword: > > // Due to platform APIs, note the following limitations: Done. https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.h:49: // On 2013/03/06 21:50:43, Ryan Sleevi wrote: > style: suggest deleting the blank lines on 44, 46, and 49. Done. https://chromiumcodereview.appspot.com/12374020/diff/38001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.h:54: // On 2013/03/06 21:50:43, Ryan Sleevi wrote: > nit: delete blank line Done.
https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_request.cc (right): https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:120: // Ownership was transfered to Java. nit: speeling/transfered/transferred/ https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:122: request.release(); Actually, in looking at https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... , why is this necessary? It's not set to WARN_UNUSED_RESULT, so you should just be able to // Ownership was transferred to Java. request.release(); https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:150: // Ensure that callback(NULL) is called on error exit. same error exit remark as before. https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:157: // to the callback immediately. This comment is a little misleading, in that it's not sending NULL to the callback explicitly. You don't describe the error returns on line 179/187, so it seems odd to document it here. https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:216: StartClientCertificateRequest(cert_request_info, callback); chrome::SelectCertificateCallback is not compatible with StartClientCertificateRequest - which StartClientCertificateRequest expects. How is this actually compiling? https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_request.h (right): https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.h:33: typedef base::Callback<void(const std::vector<base::StringPiece>*, On second glance, I'm not at all sure how this is safe: 1) You're passing a pointer, but no clear semantics about who owns the pointer 2) You're passing StringPiece's, which are non-copied - so when their underlying string goes away, they're invalidated. Perhaps you meant to pass a "const std::vector<std::string>&" Or is this just broken/unused? https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.h:49: void StartClientCertificateRequest( maybe I'm blind, but why does this need to be in the public header? Only seems to be called in the .cc
https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_request.cc (right): https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:120: // Ownership was transfered to Java. On 2013/03/06 23:24:22, Ryan Sleevi wrote: > nit: speeling/transfered/transferred/ Done. https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:122: request.release(); This is the release() method for the scoped_ptr_impl, the one for the actual scoped_ptr<> class is different and has the WARN_UNUSED_RESULT. See https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:150: // Ensure that callback(NULL) is called on error exit. On 2013/03/06 23:24:22, Ryan Sleevi wrote: > same error exit remark as before. Done. https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:157: // to the callback immediately. I've removed the comment https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:216: StartClientCertificateRequest(cert_request_info, callback); It does compile (cue the buildbots), because it actually calls the function by the same name in the anonymous namespace above. The function declaration in the header is the one that is incompatible (and unused), and just got removed. Sorry about that. https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_request.h (right): https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.h:33: typedef base::Callback<void(const std::vector<base::StringPiece>*, Actually this section of the head if obsolete, there is no reason to expose this anymore and I've removed it. I actually meant to pass a pointer here, because it could be NULL to indicate a request cancellation. Apart from that, the idea was that the data was owned by the caller and would disappear when the callback returns (so the data would have to be copied into something else, which the callback implementation actually did). https://chromiumcodereview.appspot.com/12374020/diff/44001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.h:49: void StartClientCertificateRequest( It doesn't need to be here, it's just that I forgot to remove it when I got rid of the ssl_client_certificate_selector.cc file which used to reference it.
ping?
LGTM, but someone will need to review the Java side.
On 2013/03/08 18:46:05, Ryan Sleevi wrote: > LGTM, but someone will need to review the Java side. Sure. Marcus & Jonathan, can take a look at the Java source file here? Thanks in advance.
lgtm comments are just nits. https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... File chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java (right): https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:27: public class SSLClientCertificateRequest extends AsyncTask<Void, Void, Void> nit: don't think there's any need for this class to be public? (could also comment it's an internal impl detail of the c++ side, not intended for use from other java code) https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:30: static final String TAG = "SSLClientCertificateRequest"; use private static final String TAG = SSLClientCertificateRequest.class.getSimpleName(); (crbug.com/146559) https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:33: // on the Dalvik side. There is a matching C++ ClientCertRequest nit - think we normally talk about Java rather than dalvik side (but I don't really care). https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:37: // system sends its answer to the activity). Add a sentence on the ownership? Most important thing is knowing if java owns native (yes?) or if native holds any refs to java. (sounds like not). https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:41: // private key alias string, which can be used to call nit: "which can be used" sounds like it's an instruction to the user of this class as to how you might make use of the result of SSLClientCertificateRequest. Actually, this is describing the internal implementation, so I'd switch "can be used" to "is then used" https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:128: * @return true on success. note nativeOnSystemRequestComplete will be called iff this method returns true https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:135: int port) { use java style param formatting. (not paren aligned) https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:164: principals, host_name, port, null); nit: indent (2 tabs in java, not paren align) https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_request.cc (right): https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:145: jobject private_key_ref) { DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); (useful for documentation as much as anything) https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:185: return; nit - wrong indent
lgtm, just nits, thanks! https://chromiumcodereview.appspot.com/12374020/diff/57001/chrome/android/jav... File chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java (right): https://chromiumcodereview.appspot.com/12374020/diff/57001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:120: * @param request_id The unique numerical id for the request. nit: nativePtr: the native object responsible for this request. Ownership is taken when this method returns true. https://chromiumcodereview.appspot.com/12374020/diff/57001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:131: int nativePtr, String[] key_types, byte[][] encoded_principals, nit: javaCaseStyle: keyTypes, encodedPrincipals, hostName (and also on the doc string above)
https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... File chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java (right): https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:27: public class SSLClientCertificateRequest extends AsyncTask<Void, Void, Void> On 2013/03/09 01:17:22, joth wrote: > nit: don't think there's any need for this class to be public? (could also > comment it's an internal impl detail of the c++ side, not intended for use from > other java code) No, not right now. I have plans to make it public for a later unit testing related patch (which I'm unsure I'll be able to land), but I've changed this back to package visibility in the meantime. https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:30: static final String TAG = "SSLClientCertificateRequest"; This is better discussed in the crbug, but using .getSimpleName() makes loading the code slower (it adds costly code paths at load time). Given that this value is only used for debugging, I prefer to stick to simple string constants instead. (Note: the whole Android platform tree uses direct strings for this very reason). https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:33: // on the Dalvik side. There is a matching C++ ClientCertRequest I've switched it to Java. https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:37: // system sends its answer to the activity). Actually, this comment is obsolete since there is no longer a matching C++ class, so I'll simplify/remove it. https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:41: // private key alias string, which can be used to call Sure. https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:128: * @return true on success. On 2013/03/09 01:17:22, joth wrote: > note nativeOnSystemRequestComplete will be called iff this method returns true Done. https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:135: int port) { On 2013/03/09 01:17:22, joth wrote: > use java style param formatting. (not paren aligned) Done. https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:135: int port) { On 2013/03/09 01:17:22, joth wrote: > use java style param formatting. (not paren aligned) Done. https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/browser/ui/... File chrome/browser/ui/android/ssl_client_certificate_request.cc (right): https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:145: jobject private_key_ref) { On 2013/03/09 01:17:22, joth wrote: > DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); > > (useful for documentation as much as anything) Done. https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/browser/ui/... chrome/browser/ui/android/ssl_client_certificate_request.cc:185: return; On 2013/03/09 01:17:22, joth wrote: > nit - wrong indent Done. https://chromiumcodereview.appspot.com/12374020/diff/57001/chrome/android/jav... File chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java (right): https://chromiumcodereview.appspot.com/12374020/diff/57001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:120: * @param request_id The unique numerical id for the request. On 2013/03/11 11:20:09, bulach wrote: > nit: nativePtr: the native object responsible for this request. Ownership is > taken when this method returns true. Done. https://chromiumcodereview.appspot.com/12374020/diff/57001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:131: int nativePtr, String[] key_types, byte[][] encoded_principals, On 2013/03/11 11:20:09, bulach wrote: > nit: javaCaseStyle: keyTypes, encodedPrincipals, hostName (and also on the doc > string above) Done.
I just added yfriedman and jochen as suggested owners. Guys, could you have a look at the chrome_browser.gypi and chrome_browser_ui.gypi changes in this CL? Thanks in advance.
gypi changes lgtm https://chromiumcodereview.appspot.com/12374020/diff/39011/chrome/chrome_brow... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/12374020/diff/39011/chrome/chrome_brow... chrome/chrome_browser.gypi:3135: 'android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java', usually we sort A-Z < a-z. However, the whole block ignores this, so I guess this is ok.
https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... File chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java (right): https://chromiumcodereview.appspot.com/12374020/diff/47001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:30: static final String TAG = "SSLClientCertificateRequest"; On 2013/03/11 21:41:21, digit1 wrote: > This is better discussed in the crbug, but using .getSimpleName() makes loading > the code slower (it adds costly code paths at load time). Given that this value > is only used for debugging, I prefer to stick to simple string constants > instead. > > (Note: the whole Android platform tree uses direct strings for this very > reason). SGTM. I migrated your comment to https://code.google.com/p/chromium/issues/detail?id=146559#c6
https://chromiumcodereview.appspot.com/12374020/diff/39011/chrome/chrome_brow... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/12374020/diff/39011/chrome/chrome_brow... chrome/chrome_browser.gypi:3135: 'android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java', Thanks, I've reordered the whole block since only database/DevToolsServer where mis-ordered previously. fwiw, I've also ran android_gyp + ninja, and this didn't trigger any rebuild ("no work to do"), confirming that this do something unexpected.
gypi changes lgtm. Was that all you wanted me to look at?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/digit@chromium.org/12374020/70001
Yes yaron, the other files have pretty much been covered by Ryan, Marcus and Jonathan. Feel free to have a look if you want, but I think it's time to submit this :)
Message was sent while issue was closed.
Change committed as 187507 |
