Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
235563005:
Add client cert support to android_webview (Closed)
|
Created:
6 years, 8 months ago by sgurun-gerrit only Modified:
6 years, 8 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd client cert support to android_webview
This CL implements client certs backend for android_webview. Most of the code path is similar to how chrome handles client certs. the callbacks are eventually plumbed to webview as APIs.
We still need to answer the question that if ClientCert cache is needed at the android_webview layer.
BUG=b/12983007
TBR=jcivelli@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265380
Patch Set 1 #Patch Set 2 : fix a clang issue #Patch Set 3 : temporary change to ease rolling to downstream #Patch Set 4 : fix a test compile issue #Patch Set 5 : fix findbugs issues #Patch Set 6 : revert unintended file addition #
Total comments: 36
Patch Set 7 : code review phase 1 #
Total comments: 18
Patch Set 8 : code review phase 2 #
Total comments: 22
Patch Set 9 : code review phase 3 #
Total comments: 18
Patch Set 10 : code review phase 4 #
Total comments: 4
Patch Set 11 : code review phase 5 #Patch Set 12 : provide a message as part of exception #Patch Set 13 : code review phase 6 #
Total comments: 5
Messages
Total messages: 36 (0 generated)
This is the client_cert chromium implementation for webview. I will add some more tests but potentially in a new CL. The AOSP CLs will follow (for needed API changes) digit: can you please review android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java android_webview/native/aw_contents_client_bridge.cc boliu: please review the rest
On 2014/04/14 18:54:29, sgurun wrote: > This is the client_cert chromium implementation for webview. I will add some > more tests but potentially in a new CL. > The AOSP CLs will follow (for needed API changes) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Please ignore this line. I will make the changes in internal tree. A momentary confusion about the state of AOSP. > > digit: can you please review > android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java > android_webview/native/aw_contents_client_bridge.cc > boliu: please review the rest
Didn't really look at tests yet https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1372: * @see android.webvkit.WebView#clearClientCertPreferences() nit: spelling https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:36: private static final DefaultAndroidKeyStore sLocalKeyStore = lazy? Make AwBrowserContext own this? https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:58: long mNativePtr; private? https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:63: mNativePtr = nativePtr; Do we leak if client ignores calblack and this object gets garbage collected? https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:149: nit: remove blank line https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:151: ClientCertLookupTable.Cert cert = table.getCertData(host, port); This doesn't need to be keyed by keyTypes/encodedPrincipals? (Honest question, I don't know certs) https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:153: // we have taken the ownership of the native object nit: Capitalize first word and period at end. https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ClientCertLookupTable.java (right): https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ClientCertLookupTable.java:21: public static class Cert { can this be private? https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ClientCertLookupTable.java:38: public static ClientCertLookupTable getInstance() { Let's avoid introducing more global singletons. Can you make AwBrowserContext (which is a singleton) own this object? https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ClientCertLookupTable.java:47: sLookupTable = null; This breaks the assumption that it's a singleton. But I guess it's ok if done in AwBrowserContext. https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ClientCertLookupTable.java:77: private String hostAndPort(String host, int port) { static https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:31: typedef net::OpenSSLClientKeyStore::ScopedEVP_PKEY ScopedEVP_PKEY; using? https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:108: // chrome/browser/ui/android/ssl_client_certificate_request.cc File a bug to componentize this? Do we have time to do this correctly this time? https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:117: base::IgnoreResult(&content::BrowserThread::PostTask), Do we really need to do the post? Can't we just call it back synchronously on failure? https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:138: // Ignore unknown types. This is ok because...? Should this be NOTREACHED? https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:174: if (obj.is_null()) Feels like this null check can be moved a lot higher. https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:191: SelectCertificateCallback* ALLOW_UNUSED dummy = request.release(); This transfer of ownership from native to java (which the app might leak) is super unsafe. Why not keep the native object in a map, give the key of the map to java, then there is no chance of leaks?
thanks for the quick review. There are some cases that I will add to the clientcertlookuptable. also will arrange a review from a SSL person. https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1372: * @see android.webvkit.WebView#clearClientCertPreferences() On 2014/04/14 23:56:04, boliu wrote: > nit: spelling Done. https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:36: private static final DefaultAndroidKeyStore sLocalKeyStore = On 2014/04/14 23:56:04, boliu wrote: > lazy? Make AwBrowserContext own this? Done. https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:58: long mNativePtr; On 2014/04/14 23:56:04, boliu wrote: > private? Done. https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:63: mNativePtr = nativePtr; On 2014/04/14 23:56:04, boliu wrote: > Do we leak if client ignores calblack and this object gets garbage collected? correct, but client will have a lot bigger problems to deal with. will change to a map. https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:149: On 2014/04/14 23:56:04, boliu wrote: > nit: remove blank line Done. https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:151: ClientCertLookupTable.Cert cert = table.getCertData(host, port); Encodedprincipals are not that important since they are generally not setup correctly, but I think keytypes are. I am still working on the cache, there are some corner case that needs to be addressed. On 2014/04/14 23:56:04, boliu wrote: > This doesn't need to be keyed by keyTypes/encodedPrincipals? (Honest question, I > don't know certs) https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:153: // we have taken the ownership of the native object On 2014/04/14 23:56:04, boliu wrote: > nit: Capitalize first word and period at end. Done. https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ClientCertLookupTable.java (right): https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ClientCertLookupTable.java:21: public static class Cert { This is accessed from another class. On 2014/04/14 23:56:04, boliu wrote: > can this be private? https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ClientCertLookupTable.java:38: public static ClientCertLookupTable getInstance() { On 2014/04/14 23:56:04, boliu wrote: > Let's avoid introducing more global singletons. Can you make AwBrowserContext > (which is a singleton) own this object? Done. https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ClientCertLookupTable.java:47: sLookupTable = null; On 2014/04/14 23:56:04, boliu wrote: > This breaks the assumption that it's a singleton. > > But I guess it's ok if done in AwBrowserContext. Done. https://codereview.chromium.org/235563005/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ClientCertLookupTable.java:77: private String hostAndPort(String host, int port) { On 2014/04/14 23:56:04, boliu wrote: > static Done. https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:31: typedef net::OpenSSLClientKeyStore::ScopedEVP_PKEY ScopedEVP_PKEY; On 2014/04/14 23:56:04, boliu wrote: > using? nop, this is a class member. won't work. https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:108: // chrome/browser/ui/android/ssl_client_certificate_request.cc opened an internal bug to track this. I think this makes sense but not sure where it should go now. On 2014/04/14 23:56:04, boliu wrote: > File a bug to componentize this? Do we have time to do this correctly this time? https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:117: base::IgnoreResult(&content::BrowserThread::PostTask), Taken verbatim from chrome. posting definitely does not hurt, however, it seems to my eyes that calling sync would also be ok. In any case I don't want to diverge from chrome here so I can easily combine the code later. Will put a todo to do it later while merging code. On 2014/04/14 23:56:04, boliu wrote: > Do we really need to do the post? Can't we just call it back synchronously on > failure? https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:138: // Ignore unknown types. We ignore the other types. This is almost advisory and user does not have to choose the provided type and things can even still work. this is fine. On 2014/04/14 23:56:04, boliu wrote: > This is ok because...? Should this be NOTREACHED? https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:174: if (obj.is_null()) On 2014/04/14 23:56:04, boliu wrote: > Feels like this null check can be moved a lot higher. Done. https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:191: SelectCertificateCallback* ALLOW_UNUSED dummy = request.release(); As we discussed I don't think it is super unsafe. The only leak condition that I am aware of is when a programmer makes a pretty grave mistake of overwriting this method without not calling one of the methods. There is nothing we can do then, and the careless soul will have bigger issues to worry about. Even in a map, if the user forgets calling, the method will be kept in the map indefinitely. so not much different. Are there any other concrete cases that you know that can cause a leak? On 2014/04/14 23:56:04, boliu wrote: > This transfer of ownership from native to java (which the app might leak) is > super unsafe. Why not keep the native object in a map, give the key of the map > to java, then there is no chance of leaks?
Should add one integration test to run through the java bits too. https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:108: // chrome/browser/ui/android/ssl_client_certificate_request.cc General track record is we never go back and clean stuff up. And this chrome code looks pretty bad already and doesn't entirely fit webview's case. Maybe for now we'll just fork and fix all the crap in webview? On 2014/04/15 23:40:03, sgurun wrote: > opened an internal bug to track this. I think this makes sense but not sure > where it should go now. > On 2014/04/14 23:56:04, boliu wrote: > > File a bug to componentize this? Do we have time to do this correctly this > time? > https://codereview.chromium.org/235563005/diff/110001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:191: SelectCertificateCallback* ALLOW_UNUSED dummy = request.release(); On 2014/04/15 23:40:03, sgurun wrote: > As we discussed I don't think it is super unsafe. The only leak condition that I > am aware of is when a programmer makes a pretty grave mistake of overwriting > this method without not calling one of the methods. There is nothing we can do > then, and the careless soul will have bigger issues to worry about. > Even in a map, if the user forgets calling, the method will be kept in the map > indefinitely. so not much different. If app holds onto the callback object, then it's an indication it's still not made the decision. If app takes forever to make the decision, then too bad and there's nothing we can do. But if app just drops the callback object and it gets garbage collected, we can just let the default behavior happen (or raise an exception) > > Are there any other concrete cases that you know that can cause a leak? If you put it in a map, the native object will be deleted when the webview is destroyed. > > On 2014/04/14 23:56:04, boliu wrote: > > This transfer of ownership from native to java (which the app might leak) is > > super unsafe. Why not keep the native object in a map, give the key of the map > > to java, then there is no chance of leaks? > https://codereview.chromium.org/235563005/diff/130001/android_webview/native/... File android_webview/native/aw_contents_client_bridge_unittest.cc (right): https://codereview.chromium.org/235563005/diff/130001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:19: unittests are usually put into its own anonymous namespace https://codereview.chromium.org/235563005/diff/130001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:98: EXPECT_EQ(0, cert_selected_callbacks_); Does the native object leak here? Maybe the mock can keep going and actually call the callback. https://codereview.chromium.org/235563005/diff/130001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:106: for(int i = 0; i<3; i++) { nit: space add space around "<" https://codereview.chromium.org/235563005/diff/130001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:144: jobjectArray array = env_->NewObjectArray(1, byteClass, 0); If mock uses ClientCertificateRequestCallback directly, can we continue the test above to cover these two cases (instead of doing crazy jni here) https://codereview.chromium.org/235563005/diff/130001/android_webview/unittes... File android_webview/unittestjava/src/org/chromium/android_webview/unittest/MockAwContentsClientBridge.java (right): https://codereview.chromium.org/235563005/diff/130001/android_webview/unittes... android_webview/unittestjava/src/org/chromium/android_webview/unittest/MockAwContentsClientBridge.java:14: class MockAwContentsClientBridge extends AwContentsClientBridge { Does this compile? Doesn't the class need to be public? Maybe I don't know java well enough... https://codereview.chromium.org/235563005/diff/130001/android_webview/unittes... android_webview/unittestjava/src/org/chromium/android_webview/unittest/MockAwContentsClientBridge.java:24: protected boolean selectClientCertificate(final long nativePtr, final String[] keyTypes, Hmm, interesting question, can you override a private method that's not marked final? https://codereview.chromium.org/235563005/diff/130001/android_webview/unittes... android_webview/unittestjava/src/org/chromium/android_webview/unittest/MockAwContentsClientBridge.java:32: private static MockAwContentsClientBridge getAwContentsClientBridge() { s/get/create/
@boliu: I will upload a new version today with fixes. Answers to some of your questions below. @wtc: this is to provide client cert support to android_webview. Can you review android_webview/native/aw_contents_client_bridge.cc One question: do we need a cache in android_webview layer to store user preferences (see android_webview/java/src/org/chromium/android_webview/ClientCertLookupTable.java) or is it redundant, since we have a OpenSSLClientKeyStore already. https://codereview.chromium.org/235563005/diff/130001/android_webview/native/... File android_webview/native/aw_contents_client_bridge_unittest.cc (right): https://codereview.chromium.org/235563005/diff/130001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:19: On 2014/04/16 02:57:46, boliu wrote: > unittests are usually put into its own anonymous namespace Done. https://codereview.chromium.org/235563005/diff/130001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:106: for(int i = 0; i<3; i++) { On 2014/04/16 02:57:46, boliu wrote: > nit: space add space around "<" Done. https://codereview.chromium.org/235563005/diff/130001/android_webview/unittes... File android_webview/unittestjava/src/org/chromium/android_webview/unittest/MockAwContentsClientBridge.java (right): https://codereview.chromium.org/235563005/diff/130001/android_webview/unittes... android_webview/unittestjava/src/org/chromium/android_webview/unittest/MockAwContentsClientBridge.java:14: class MockAwContentsClientBridge extends AwContentsClientBridge { Of course it does. A class not declared public can be accessed from the package (has package access). On 2014/04/16 02:57:46, boliu wrote: > Does this compile? Doesn't the class need to be public? Maybe I don't know java > well enough... https://codereview.chromium.org/235563005/diff/130001/android_webview/unittes... android_webview/unittestjava/src/org/chromium/android_webview/unittest/MockAwContentsClientBridge.java:24: protected boolean selectClientCertificate(final long nativePtr, final String[] keyTypes, 1. you cannot override a private method since its accessor is private. (of course you can have a method of same name in your class subtyping it, but this is not overriding). 2. making a private method final has no effect. On 2014/04/16 02:57:46, boliu wrote: > Hmm, interesting question, can you override a private method that's not marked > final?
addressed code review. Will add an integration test. May remove the webview layer cache depending on review from an ssl person. https://codereview.chromium.org/235563005/diff/130001/android_webview/native/... File android_webview/native/aw_contents_client_bridge_unittest.cc (right): https://codereview.chromium.org/235563005/diff/130001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:98: EXPECT_EQ(0, cert_selected_callbacks_); This does not apply anymore. On 2014/04/16 02:57:46, boliu wrote: > Does the native object leak here? > > Maybe the mock can keep going and actually call the callback. https://codereview.chromium.org/235563005/diff/130001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:144: jobjectArray array = env_->NewObjectArray(1, byteClass, 0); not doing crazy jni anymore. On 2014/04/16 02:57:46, boliu wrote: > If mock uses ClientCertificateRequestCallback directly, can we continue the test > above to cover these two cases (instead of doing crazy jni here) https://codereview.chromium.org/235563005/diff/130001/android_webview/unittes... File android_webview/unittestjava/src/org/chromium/android_webview/unittest/MockAwContentsClientBridge.java (right): https://codereview.chromium.org/235563005/diff/130001/android_webview/unittes... android_webview/unittestjava/src/org/chromium/android_webview/unittest/MockAwContentsClientBridge.java:32: private static MockAwContentsClientBridge getAwContentsClientBridge() { On 2014/04/16 02:57:46, boliu wrote: > s/get/create/ Done.
Didn't look at tests this round. https://codereview.chromium.org/235563005/diff/130001/android_webview/native/... File android_webview/native/aw_contents_client_bridge_unittest.cc (right): https://codereview.chromium.org/235563005/diff/130001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:19: On 2014/04/17 15:10:51, sgurun wrote: > On 2014/04/16 02:57:46, boliu wrote: > > unittests are usually put into its own anonymous namespace > > Done. Actually wrap the whole file in android_webview, then anonymous namespace. We are not being very consistent here, so feel free to fix all our other unit test files in a separate CL. https://codereview.chromium.org/235563005/diff/130001/android_webview/unittes... File android_webview/unittestjava/src/org/chromium/android_webview/unittest/MockAwContentsClientBridge.java (right): https://codereview.chromium.org/235563005/diff/130001/android_webview/unittes... android_webview/unittestjava/src/org/chromium/android_webview/unittest/MockAwContentsClientBridge.java:32: private static MockAwContentsClientBridge getAwContentsClientBridge() { On 2014/04/18 01:44:55, sgurun wrote: > On 2014/04/16 02:57:46, boliu wrote: > > s/get/create/ > > Done. Not done? https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:162: if (mLookupTable.isDenied(host, port)) { Put this above getCertData just to be safe https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:175: } catch (Exception e) { Is there a more specific exception you can catch? Docs say IllegalArgumentException. Only the code that can raise an exception (I assume X500Principal constructor), not the whole loop. https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:185: // We've taken the ownership of the native ssl request object. obsolete https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:187: return; Remove https://codereview.chromium.org/235563005/diff/150001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/150001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:110: // This method is almost same as SelectClientCertificate() in inspired by? https://codereview.chromium.org/235563005/diff/150001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:119: callback, scoped_refptr<net::X509Certificate>())); Insert the callback into the map here and re-use HandleErrorInClientCertificateResponse https://codereview.chromium.org/235563005/diff/150001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:172: ignore_result(guard.Release()); Keep this below the java call (to prevent someone slips in more code between this and the java call in the future) https://codereview.chromium.org/235563005/diff/150001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:245: ignore_result(guard.Release()); Again, move below PostTaskAndReply https://codereview.chromium.org/235563005/diff/150001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.h (right): https://codereview.chromium.org/235563005/diff/150001/android_webview/native/... android_webview/native/aw_contents_client_bridge.h:32: typedef base::Callback<void(net::X509Certificate*)> SelectCertificateCallback; Move this typedef to base class
https://codereview.chromium.org/235563005/diff/130001/android_webview/native/... File android_webview/native/aw_contents_client_bridge_unittest.cc (right): https://codereview.chromium.org/235563005/diff/130001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:19: That's something very loosely applied it seems -just checked 8 random unittest files and only two of them has it. anyway, it makes sense to have the unit test in the namespace, don't know why we are more strict about it. On 2014/04/18 17:10:12, boliu wrote: > On 2014/04/17 15:10:51, sgurun wrote: > > On 2014/04/16 02:57:46, boliu wrote: > > > unittests are usually put into its own anonymous namespace > > > > Done. > > Actually wrap the whole file in android_webview, then anonymous namespace. We > are not being very consistent here, so feel free to fix all our other unit test > files in a separate CL. https://codereview.chromium.org/235563005/diff/130001/android_webview/unittes... File android_webview/unittestjava/src/org/chromium/android_webview/unittest/MockAwContentsClientBridge.java (right): https://codereview.chromium.org/235563005/diff/130001/android_webview/unittes... android_webview/unittestjava/src/org/chromium/android_webview/unittest/MockAwContentsClientBridge.java:32: private static MockAwContentsClientBridge getAwContentsClientBridge() { forgot uploading the patch that has it. On 2014/04/18 17:10:12, boliu wrote: > On 2014/04/18 01:44:55, sgurun wrote: > > On 2014/04/16 02:57:46, boliu wrote: > > > s/get/create/ > > > > Done. > > Not done? https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:162: if (mLookupTable.isDenied(host, port)) { On 2014/04/18 17:10:12, boliu wrote: > Put this above getCertData just to be safe Done. https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:175: } catch (Exception e) { On 2014/04/18 17:10:12, boliu wrote: > Is there a more specific exception you can catch? Docs say > IllegalArgumentException. > > Only the code that can raise an exception (I assume X500Principal constructor), > not the whole loop. Done. https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:185: // We've taken the ownership of the native ssl request object. On 2014/04/18 17:10:12, boliu wrote: > obsolete Done. https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:187: return; On 2014/04/18 17:10:12, boliu wrote: > Remove Done. https://codereview.chromium.org/235563005/diff/150001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/150001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:110: // This method is almost same as SelectClientCertificate() in On 2014/04/18 17:10:12, boliu wrote: > inspired by? Done. https://codereview.chromium.org/235563005/diff/150001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:119: callback, scoped_refptr<net::X509Certificate>())); On 2014/04/18 17:10:12, boliu wrote: > Insert the callback into the map here and re-use > HandleErrorInClientCertificateResponse Done. https://codereview.chromium.org/235563005/diff/150001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:172: ignore_result(guard.Release()); On 2014/04/18 17:10:12, boliu wrote: > Keep this below the java call (to prevent someone slips in more code between > this and the java call in the future) Done. https://codereview.chromium.org/235563005/diff/150001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:245: ignore_result(guard.Release()); On 2014/04/18 17:10:12, boliu wrote: > Again, move below PostTaskAndReply Done. https://codereview.chromium.org/235563005/diff/150001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.h (right): https://codereview.chromium.org/235563005/diff/150001/android_webview/native/... android_webview/native/aw_contents_client_bridge.h:32: typedef base::Callback<void(net::X509Certificate*)> SelectCertificateCallback; On 2014/04/18 17:10:12, boliu wrote: > Move this typedef to base class Done.
@tommi: need lgtm for android_webview/native/DEPS
@tommi: need lgtm for android_webview/native/DEPS
I'm running out of nits to complain. Could a net owner take a look? https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:185: // We've taken the ownership of the native ssl request object. ? On 2014/04/19 01:28:50, sgurun wrote: > On 2014/04/18 17:10:12, boliu wrote: > > obsolete > > Done. https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:187: return; ? On 2014/04/19 01:28:50, sgurun wrote: > On 2014/04/18 17:10:12, boliu wrote: > > Remove > > Done. https://codereview.chromium.org/235563005/diff/170001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/235563005/diff/170001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:175: } catch (IllegalArgumentException e) { My second sentence was not complete. Can you also only put "new X500Principal(...)" in the try block, not the whole for loop? https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:197: if (!callback || callback->is_null()) { The only way I can think of this happening legitimately is if the client calls the callback twice. Anything else? Can we check for the called more than once behavior (maybe throw an exception in java), then make this DCHECK? https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... File android_webview/native/aw_contents_client_bridge_unittest.cc (right): https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:112: bridge_->ProvideClientCertificateResponse(env_, jbridge_.obj(), 1, Does bridge_ have the callback in the map? If not, what does the EXPECTs below get us? https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:138: EXPECT_EQ(NULL, selected_cert_); You can probably check here that there is something in the map in the bridge, and in test below. https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:145: EXPECT_EQ(NULL, selected_cert_); Why is this NULL? If it's supposed to be NULL, can we add a test where cert is not null?
seems wtc is OOO. I asked digit, and hopefully he will be able to take a look at it since they did the chrome side implementation. @digit can you please review aw_contents_client_bridge.cc and its java counterpart, AwContentsClientBridge.java. Can you also comment on whether SSLClientCertLookupTable is necessary or redundant (this table caches the user response). My understanding is when the user proceeds, the key is stored in lower layer OpenSslClientKeyStore, so maybe not necessary to store proceed decision.
thanks https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:185: // We've taken the ownership of the native ssl request object. On 2014/04/21 16:56:45, boliu wrote: > ? > > On 2014/04/19 01:28:50, sgurun wrote: > > On 2014/04/18 17:10:12, boliu wrote: > > > obsolete > > > > Done. > Done. https://codereview.chromium.org/235563005/diff/150001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:187: return; that's pretty weird. it has nine lives. On 2014/04/21 16:56:45, boliu wrote: > ? > > On 2014/04/19 01:28:50, sgurun wrote: > > On 2014/04/18 17:10:12, boliu wrote: > > > Remove > > > > Done. > https://codereview.chromium.org/235563005/diff/170001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/235563005/diff/170001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:175: } catch (IllegalArgumentException e) { On 2014/04/21 16:56:45, boliu wrote: > My second sentence was not complete. > > Can you also only put "new X500Principal(...)" in the try block, not the whole > for loop? Done. https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:197: if (!callback || callback->is_null()) { I think this is really safe and defensive programming. I don't see the value of changing it to a DCHECK. Having java side check for double calls is a good idea. Probably needs a test too. On 2014/04/21 16:56:45, boliu wrote: > The only way I can think of this happening legitimately is if the client calls > the callback twice. Anything else? > > Can we check for the called more than once behavior (maybe throw an exception in > java), then make this DCHECK? https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... File android_webview/native/aw_contents_client_bridge_unittest.cc (right): https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:112: bridge_->ProvideClientCertificateResponse(env_, jbridge_.obj(), 1, On 2014/04/21 16:56:45, boliu wrote: > Does bridge_ have the callback in the map? If not, what does the EXPECTs below > get us? Done. https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:138: EXPECT_EQ(NULL, selected_cert_); Even in a unittest, it is not a very good practice to test the internal state directly. makes the test and the class too tightly coupled. However, we are testing the same thing by verifying the callback is called, so no need to do such a invasive check anyway. On 2014/04/21 16:56:45, boliu wrote: > You can probably check here that there is something in the map in the bridge, > and in test below. https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge_unittest.cc:145: EXPECT_EQ(NULL, selected_cert_); On 2014/04/21 16:56:45, boliu wrote: > Why is this NULL? because the key is null. > > If it's supposed to be NULL, can we add a test where cert is not null? tried that already last week. This is way too complicated (due to the use of AndroidPrivateKey in the API) for little return at this level. A more proper layer is probably an integration or CTS test. I will likely do the tests at the CTS level since in android, we need API tests anyway for this functionality.
https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:197: if (!callback || callback->is_null()) { What if in a future change, we accidentally introduce this bug? It could go unnoticed for a long time due to this. You added isCalled on java side, which means we completely control how this method is called, so we should DCHECK the conditions we expect. Defensive programming only applies when dealing with code/data outside of our control imo. On 2014/04/21 23:53:29, sgurun wrote: > I think this is really safe and defensive programming. I don't see the value of > changing it to a DCHECK. > Having java side check for double calls is a good idea. Probably needs a test > too. > > > On 2014/04/21 16:56:45, boliu wrote: > > The only way I can think of this happening legitimately is if the client calls > > the callback twice. Anything else? > > > > Can we check for the called more than once behavior (maybe throw an exception > in > > java), then make this DCHECK? > https://codereview.chromium.org/235563005/diff/190001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/235563005/diff/190001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:70: private boolean isCalled; mIsCalled, or just mCalled? https://codereview.chromium.org/235563005/diff/190001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:83: isCalled = true; factor out these 4 lines into a helper method?
https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:197: if (!callback || callback->is_null()) { the best way to identify bugs is via tests, which I will add for this case in a subsequent CL. The important behavior is to maintain is that we do throw exceptions in subsequent calls. most likely a CTS test will be proper there. you want to be defensive not only against misuse of external APIs but also against internal APIs. On 2014/04/22 00:20:55, boliu wrote: > What if in a future change, we accidentally introduce this bug? It could go > unnoticed for a long time due to this. > > You added isCalled on java side, which means we completely control how this > method is called, so we should DCHECK the conditions we expect. Defensive > programming only applies when dealing with code/data outside of our control imo. > > On 2014/04/21 23:53:29, sgurun wrote: > > I think this is really safe and defensive programming. I don't see the value > of > > changing it to a DCHECK. > > Having java side check for double calls is a good idea. Probably needs a test > > too. > > > > > > On 2014/04/21 16:56:45, boliu wrote: > > > The only way I can think of this happening legitimately is if the client > calls > > > the callback twice. Anything else? > > > > > > Can we check for the called more than once behavior (maybe throw an > exception > > in > > > java), then make this DCHECK? > > > https://codereview.chromium.org/235563005/diff/190001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/235563005/diff/190001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:70: private boolean isCalled; On 2014/04/22 00:20:56, boliu wrote: > mIsCalled, or just mCalled? Done. https://codereview.chromium.org/235563005/diff/190001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:83: isCalled = true; On 2014/04/22 00:20:56, boliu wrote: > factor out these 4 lines into a helper method? Done.
https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:197: if (!callback || callback->is_null()) { On 2014/04/22 00:44:36, sgurun wrote: > you want to be defensive not only against misuse of external APIs but also > against internal APIs. Disagree. If you want to keep an invariant in code you control, why can't you assert that invariant? What are you defending against here?
https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:197: if (!callback || callback->is_null()) { Assertions and exceptions are very useful in debugging and correcting a problem. Your suggestion of throwing an exception was good, because it provides some information to embedder of webview some information that they can act on. In this case, however I don't see any value except making the code more brittle. The internal APIs always change, the tests get outdated, and zillions of things happen at once and this is a piece of code that is not in the common use case for most. Why do you want to crash loudly in the field if we have a bug there. Ignoring the duplicate code has no side effect and is the right behavior. We made a similar mistake before, leaving CHECKs in the code and introducing unnecessary crashes in autofill. it was pointless. On 2014/04/22 00:47:57, boliu wrote: > On 2014/04/22 00:44:36, sgurun wrote: > > you want to be defensive not only against misuse of external APIs but also > > against internal APIs. > > Disagree. If you want to keep an invariant in code you control, why can't you > assert that invariant? What are you defending against here?
https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:197: if (!callback || callback->is_null()) { On 2014/04/22 01:12:27, sgurun wrote: > Assertions and exceptions are very useful in debugging and correcting a problem. > Your suggestion of throwing an exception was good, because it provides some > information to embedder of webview some information that they can act on. > > In this case, however I don't see any value except making the code more brittle. > The internal APIs always change, the tests get outdated, and zillions of things > happen at once and this is a piece of code that is not in the common use case > for most. How do tests apply to this discussion? You can test it regardless. > Why do you want to crash loudly in the field if we have a bug there. It's a bug in *our code* if ever fall into this path. I want it to crash loudly so it's unlikely to have that bug slip out. > Ignoring the duplicate code has no side effect and is the right behavior. It's not the right behavior if we forget to check isCalled on a method in the future. > > We made a similar mistake before, leaving CHECKs in the code and introducing > unnecessary crashes in autofill. it was pointless. It was not pointless. Even if caught in production, (which I agree is costly,) it was caught. Without the CHECK, you would never know. The right solution is to have enough test coverage to never let these things slip out into production, not being lax about invariants in the code. > > > On 2014/04/22 00:47:57, boliu wrote: > > On 2014/04/22 00:44:36, sgurun wrote: > > > you want to be defensive not only against misuse of external APIs but also > > > against internal APIs. > > > > Disagree. If you want to keep an invariant in code you control, why can't you > > assert that invariant? What are you defending against here? >
https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:197: if (!callback || callback->is_null()) { On 2014/04/22 01:32:17, boliu wrote: > On 2014/04/22 01:12:27, sgurun wrote: > > Assertions and exceptions are very useful in debugging and correcting a > problem. > > Your suggestion of throwing an exception was good, because it provides some > > information to embedder of webview some information that they can act on. > > > > In this case, however I don't see any value except making the code more > brittle. > > The internal APIs always change, the tests get outdated, and zillions of > things > > happen at once and this is a piece of code that is not in the common use case > > for most. > > How do tests apply to this discussion? You can test it regardless. Of course you can. The point is test coverage is not perfect (or when covers even it can get outdated), and bugs in such rarely used code paths generally not get caught until somebody tries it in the field. > > > Why do you want to crash loudly in the field if we have a bug there. > > It's a bug in *our code* if ever fall into this path. I want it to crash loudly > so it's unlikely to have that bug slip out. > This is exactly the problem I am trying to communicate. Why do you want to crash when you have a good solution that does not crash? Crashing loudly is always a bad experience for users. > > Ignoring the duplicate code has no side effect and is the right behavior. > > It's not the right behavior if we forget to check isCalled on a method in the > future. > > > > > We made a similar mistake before, leaving CHECKs in the code and introducing > > unnecessary crashes in autofill. it was pointless. > > It was not pointless. Even if caught in production, (which I agree is costly,) > it was caught. Without the CHECK, you would never know. > > The right solution is to have enough test coverage to never let these things > slip out into production, not being lax about invariants in the code. > Of course it was pointless. This is why we removed such CHECKs afterwards. You want to crash loudly but if you start crashing in every little bug that you can handle via other types of defenses, then you are annoying your users. In any case, this started taking way too much of time. I don't agree with the change but I will do it tomorrow regardless. > > > > > > On 2014/04/22 00:47:57, boliu wrote: > > > On 2014/04/22 00:44:36, sgurun wrote: > > > > you want to be defensive not only against misuse of external APIs but also > > > > against internal APIs. > > > > > > Disagree. If you want to keep an invariant in code you control, why can't > you > > > assert that invariant? What are you defending against here? > >
https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:197: if (!callback || callback->is_null()) { On 2014/04/22 02:06:47, sgurun wrote: > On 2014/04/22 01:32:17, boliu wrote: > > On 2014/04/22 01:12:27, sgurun wrote: > > > Assertions and exceptions are very useful in debugging and correcting a > > problem. > > > Your suggestion of throwing an exception was good, because it provides some > > > information to embedder of webview some information that they can act on. > > > > > > In this case, however I don't see any value except making the code more > > brittle. > > > The internal APIs always change, the tests get outdated, and zillions of > > things > > > happen at once and this is a piece of code that is not in the common use > case > > > for most. > > > > How do tests apply to this discussion? You can test it regardless. > > Of course you can. The point is test coverage is not perfect (or when covers > even it can get outdated), and bugs in such rarely used code paths generally not > get caught until somebody tries it in the field. > > > > > > Why do you want to crash loudly in the field if we have a bug there. > > > > It's a bug in *our code* if ever fall into this path. I want it to crash > loudly > > so it's unlikely to have that bug slip out. > > > > This is exactly the problem I am trying to communicate. Why do you want to crash > when you have a good solution that does not crash? Crashing loudly is always a > bad experience for users. > > > > Ignoring the duplicate code has no side effect and is the right behavior. > > > > It's not the right behavior if we forget to check isCalled on a method in the > > future. > > > > > > > > We made a similar mistake before, leaving CHECKs in the code and introducing > > > unnecessary crashes in autofill. it was pointless. > > > > It was not pointless. Even if caught in production, (which I agree is costly,) > > it was caught. Without the CHECK, you would never know. > > > > The right solution is to have enough test coverage to never let these things > > slip out into production, not being lax about invariants in the code. > > > Of course it was pointless. This is why we removed such CHECKs afterwards. > You want to crash loudly but if you start crashing in every little bug that you > can handle via other types of defenses, then you are annoying your users. > > In any case, this started taking way too much of time. I don't agree with the > change but I will do it tomorrow regardless. > > > > > > > > > > On 2014/04/22 00:47:57, boliu wrote: > > > > On 2014/04/22 00:44:36, sgurun wrote: > > > > > you want to be defensive not only against misuse of external APIs but > also > > > > > against internal APIs. > > > > > > > > Disagree. If you want to keep an invariant in code you control, why can't > > you > > > > assert that invariant? What are you defending against here? > > > > Done.
lgtm (for the C++ part in aw_contents_client_bridge). Thanks. https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/170001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:197: if (!callback || callback->is_null()) { I think Bo's point is that this should be a DCHECK() so this can be detected during testing, only a CHECK() will provide horrible experience to users :-) I agree with him, better catch when the assertion fails as soon as possible. Sometimes, hiding the bug with a soft-fallback just makes things more difficult to debug.
lgtm
The CQ bit was checked by sgurun@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sgurun@chromium.org/235563005/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
Ah. git cl upload suggested tommi but I see that he is only owner for webrtc. @jcivelli can you please LGTM the DEPS file for depending to content/public/test
Ah. git cl upload suggested tommi but I see that he is only owner for webrtc. @jcivelli can you please LGTM the DEPS file for depending to content/public/test
The CQ bit was checked by sgurun@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sgurun@chromium.org/235563005/270001
LGTM for DEPS
Message was sent while issue was closed.
Change committed as 265380
Message was sent while issue was closed.
Review comments on patch set 13: Selim: yes, I was on vacation. digit is a better review than me anyway. I still took a quick look at android_webview/native/aw_contents_client_bridge.cc but I'm afraid that the only comments I have are all coding style nits. Sorry. https://codereview.chromium.org/235563005/diff/270001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/270001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:31: typedef net::OpenSSLClientKeyStore::ScopedEVP_PKEY ScopedEVP_PKEY; Nit: we have crypto/scoped_nss_types.h for similar scoped NSS types. Perhaps we should also create a similar header for scoped OpenSSL types. https://codereview.chromium.org/235563005/diff/270001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:33: namespace { Nit: add a blank line after this line. https://codereview.chromium.org/235563005/diff/270001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:133: for (size_t n = 0; n < cert_request_info->cert_key_types.size(); ++n) { Nit: the for loop index is usually named |i| by convention. (|n| is often the variable for the iteration count.) Also on line 216.
Message was sent while issue was closed.
On 2014/04/22 22:47:16, wtc wrote: > Review comments on patch set 13: > > Selim: yes, I was on vacation. digit is a better review than me anyway. I still > took a quick look at android_webview/native/aw_contents_client_bridge.cc but I'm > afraid that the only comments I have are all coding style nits. Sorry. > > https://codereview.chromium.org/235563005/diff/270001/android_webview/native/... > File android_webview/native/aw_contents_client_bridge.cc (right): > > https://codereview.chromium.org/235563005/diff/270001/android_webview/native/... > android_webview/native/aw_contents_client_bridge.cc:31: typedef > net::OpenSSLClientKeyStore::ScopedEVP_PKEY ScopedEVP_PKEY; > > Nit: we have crypto/scoped_nss_types.h for similar scoped NSS types. Perhaps we > should also create a similar header for scoped OpenSSL types. > > https://codereview.chromium.org/235563005/diff/270001/android_webview/native/... > android_webview/native/aw_contents_client_bridge.cc:33: namespace { > > Nit: add a blank line after this line. > > https://codereview.chromium.org/235563005/diff/270001/android_webview/native/... > android_webview/native/aw_contents_client_bridge.cc:133: for (size_t n = 0; n < > cert_request_info->cert_key_types.size(); ++n) { > > Nit: the for loop index is usually named |i| by convention. (|n| is often the > variable for the iteration count.) > > Also on line 216. Thank you, I will incorporate these in the next CL.
Message was sent while issue was closed.
https://codereview.chromium.org/235563005/diff/270001/android_webview/native/... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/235563005/diff/270001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:33: namespace { On 2014/04/22 22:47:16, wtc wrote: > > Nit: add a blank line after this line. Done. https://codereview.chromium.org/235563005/diff/270001/android_webview/native/... android_webview/native/aw_contents_client_bridge.cc:133: for (size_t n = 0; n < cert_request_info->cert_key_types.size(); ++n) { On 2014/04/22 22:47:16, wtc wrote: > > Nit: the for loop index is usually named |i| by convention. (|n| is often the > variable for the iteration count.) > > Also on line 216. Done. |
