Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12091111:
Implement Webviewclient.onReceivedSslError (Closed)
|
Created:
7 years, 10 months ago by sgurun-gerrit only Modified:
7 years, 10 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplement Webviewclient.onReceivedSslError
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184643
Patch Set 1 #Patch Set 2 : minor fix #
Total comments: 7
Patch Set 3 : updated copyrights #
Total comments: 20
Patch Set 4 : address code review #
Total comments: 26
Patch Set 5 : addressed code review #
Total comments: 24
Patch Set 6 : add a bridge for aw_contents_client #
Total comments: 22
Patch Set 7 : code review from joth #
Total comments: 10
Patch Set 8 : expand the base class. #
Total comments: 4
Patch Set 9 : rebased #Patch Set 10 : use valuecallback #Patch Set 11 : fix compile issues #
Total comments: 8
Patch Set 12 : address ben's code review #Patch Set 13 : rebased #Patch Set 14 : fix capitalization error #Patch Set 15 : wasting time... #Messages
Total messages: 39 (0 generated)
please take a look, thanks.
Update copyright to 2013 for all new files.
rough first pass https://codereview.chromium.org/12091111/diff/2001/android_webview/android_we... File android_webview/android_webview.gyp (right): https://codereview.chromium.org/12091111/diff/2001/android_webview/android_we... android_webview/android_webview.gyp:122: 'browser/aw_certificate_error_handler.h', _base? https://codereview.chromium.org/12091111/diff/2001/android_webview/browser/aw... File android_webview/browser/aw_certificate_error_handler_base.h (right): https://codereview.chromium.org/12091111/diff/2001/android_webview/browser/aw... android_webview/browser/aw_certificate_error_handler_base.h:20: class AwCertificateErrorHandlerBase { document copiously https://codereview.chromium.org/12091111/diff/2001/android_webview/browser/aw... android_webview/browser/aw_certificate_error_handler_base.h:31: }; Consider adding DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/12091111/diff/6001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/12091111/diff/6001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:824: private boolean allowCertificateError(int certError, byte[] derBytes, String url) { This return value is really weird in that it gets assigned to cancel_request which is a negative. So this method should be disallowCertificateError. https://codereview.chromium.org/12091111/diff/6001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:833: return false; If you can determine synchronously the result, then should just return true and cancel the request synchronously, no need to call the callback. https://codereview.chromium.org/12091111/diff/6001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:839: post(new Runnable() { oh SslErrorHandler inherents from Handler...so stupid...sorry for ranting https://codereview.chromium.org/12091111/diff/6001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java (right): https://codereview.chromium.org/12091111/diff/6001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:16: * This class is not threadsafe. It is used only on the WebCore thread. Also, it say UI thread, (same thing, but it's the chromium convention) https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... File android_webview/native/aw_certificate_error_handler_base.cc (right): https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... android_webview/native/aw_certificate_error_handler_base.cc:21: const content::RenderViewHost* host = DCHECK every step of the way for null values. https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... android_webview/native/aw_contents.cc:936: *cancel_request = (bool)Java_AwContents_allowCertificateError(env, This shouldn't require a cast (also if it does, use static_cast<>)
https://codereview.chromium.org/12091111/diff/6001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/12091111/diff/6001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:824: private boolean allowCertificateError(int certError, byte[] derBytes, String url) { On 2013/02/01 20:03:23, boliu wrote: > This return value is really weird in that it gets assigned to cancel_request > which is a negative. So this method should be disallowCertificateError. +1 https://codereview.chromium.org/12091111/diff/6001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java (right): https://codereview.chromium.org/12091111/diff/6001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:16: * This class is not threadsafe. It is used only on the WebCore thread. Also, it On 2013/02/01 20:03:23, boliu wrote: > say UI thread, (same thing, but it's the chromium convention) And remove reference to Chromium HTTP stack. https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... File android_webview/native/aw_certificate_error_handler_base.cc (right): https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... android_webview/native/aw_certificate_error_handler_base.cc:21: const content::RenderViewHost* host = On 2013/02/01 20:03:23, boliu wrote: > DCHECK every step of the way for null values. Just curious, what does that really add? It will crash with a line number for both. https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... android_webview/native/aw_contents.cc:935: ssl_error_callback_ = callback; Can there be more than one of these at the same time? If so, what if: -> error1 -> error2 -> error3 <- callback error2 <- callback error3 <- callback error1 https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... android_webview/native/aw_contents.cc:943: ssl_error_callback_.Run(proceed); Should probably check this is a valid callback pointer.
https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... File android_webview/native/aw_certificate_error_handler_base.cc (right): https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... android_webview/native/aw_certificate_error_handler_base.cc:21: const content::RenderViewHost* host = On 2013/02/01 20:51:12, Kristian Monsen wrote: > On 2013/02/01 20:03:23, boliu wrote: > > DCHECK every step of the way for null values. > Just curious, what does that really add? It will crash with a line number for > both. Benefit is that (especially with the crazy optimizations in a release build), the crash stack might not be here since it only segfaults if you try to dereference a null pointer, which makes debugging slightly harder. DCHECK will make it crash where it failed, at least in debug builds... (I think in release, it will print but not segfault, and in official, All D.. logging is just defined to be no-op, but then that's the desktop build)
these are good catches. Thanks guys. https://codereview.chromium.org/12091111/diff/2001/android_webview/android_we... File android_webview/android_webview.gyp (right): https://codereview.chromium.org/12091111/diff/2001/android_webview/android_we... android_webview/android_webview.gyp:122: 'browser/aw_certificate_error_handler.h', On 2013/02/01 20:03:23, boliu wrote: > _base? Done. https://codereview.chromium.org/12091111/diff/2001/android_webview/browser/aw... File android_webview/browser/aw_certificate_error_handler_base.h (right): https://codereview.chromium.org/12091111/diff/2001/android_webview/browser/aw... android_webview/browser/aw_certificate_error_handler_base.h:20: class AwCertificateErrorHandlerBase { On 2013/02/01 20:03:23, boliu wrote: > document copiously Done. https://codereview.chromium.org/12091111/diff/2001/android_webview/browser/aw... android_webview/browser/aw_certificate_error_handler_base.h:31: }; just curious. do we need this in a pure abstract class? On 2013/02/01 20:03:23, boliu wrote: > Consider adding DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/12091111/diff/6001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/12091111/diff/6001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:824: private boolean allowCertificateError(int certError, byte[] derBytes, String url) { Yeah, make sense. good catch. On 2013/02/01 20:51:12, Kristian Monsen wrote: > On 2013/02/01 20:03:23, boliu wrote: > > This return value is really weird in that it gets assigned to cancel_request > > which is a negative. So this method should be disallowCertificateError. > > +1 https://codereview.chromium.org/12091111/diff/6001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java (right): https://codereview.chromium.org/12091111/diff/6001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:16: * This class is not threadsafe. It is used only on the WebCore thread. Also, it Another good catch. I don't see any reason to talk about UI thread here (trivial class) but I will keep it anyway. On 2013/02/01 20:51:12, Kristian Monsen wrote: > On 2013/02/01 20:03:23, boliu wrote: > > say UI thread, (same thing, but it's the chromium convention) > > And remove reference to Chromium HTTP stack. https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... File android_webview/native/aw_certificate_error_handler_base.cc (right): https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... android_webview/native/aw_certificate_error_handler_base.cc:21: const content::RenderViewHost* host = On 2013/02/01 20:56:59, boliu wrote: > On 2013/02/01 20:51:12, Kristian Monsen wrote: > > On 2013/02/01 20:03:23, boliu wrote: > > > DCHECK every step of the way for null values. > > Just curious, what does that really add? It will crash with a line number for > > both. > > Benefit is that (especially with the crazy optimizations in a release build), > the crash stack might not be here since it only segfaults if you try to > dereference a null pointer, which makes debugging slightly harder. DCHECK will > make it crash where it failed, at least in debug builds... > > (I think in release, it will print but not segfault, and in official, All D.. > logging is just defined to be no-op, but then that's the desktop build) Done. https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... android_webview/native/aw_contents.cc:935: ssl_error_callback_ = callback; No, it seems AllowCertificateError is called only for main frame. (we privately chatted about it) On 2013/02/01 20:51:12, Kristian Monsen wrote: > Can there be more than one of these at the same time? If so, what if: > -> error1 > -> error2 > -> error3 > <- callback error2 > <- callback error3 > <- callback error1 https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... android_webview/native/aw_contents.cc:936: *cancel_request = (bool)Java_AwContents_allowCertificateError(env, On 2013/02/01 20:03:23, boliu wrote: > This shouldn't require a cast (also if it does, use static_cast<>) Done. https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... android_webview/native/aw_contents.cc:943: ssl_error_callback_.Run(proceed); good idea. On 2013/02/01 20:51:12, Kristian Monsen wrote: > Should probably check this is a valid callback pointer.
So overall the webview api lacks a global client (instead of per webview), where the ssl error callback should really be. So some questions arising from that. https://codereview.chromium.org/12091111/diff/2001/android_webview/browser/aw... File android_webview/browser/aw_certificate_error_handler_base.h (right): https://codereview.chromium.org/12091111/diff/2001/android_webview/browser/aw... android_webview/browser/aw_certificate_error_handler_base.h:31: }; On 2013/02/01 22:08:00, sgurun wrote: > just curious. do we need this in a pure abstract class? > Good point. No https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... android_webview/native/aw_contents.cc:935: ssl_error_callback_ = callback; Actually I just noticed this, can I get in the loop too? So my question is can this happen? -> page1 error1 -> calls client method -> navigate to page2, which has error2 -> calls client method <- client comes back with response for error1 Even if this does cannot happen, we should still store the callbacks individually. Easiest way is just put it in a map based on a static incrementing key, then you can store that key in the java side and get it back in ProceedSslError. (Good thing there are no threading issues) https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:836: return false; so we are not doing this optimization? https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:20: private final Bundle table; I don't know much about bundle. Is it better than treemap or hashmap? https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:33: public void setIsAllowed(SslError sslError) { Looks like this is not binary, so something like setMaxAllowedSslErrorLevel? https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:55: public void clear() { I don't think this is called anywhere. Should this class be per awcontents instead of global? (match old implementation) https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... File android_webview/native/aw_certificate_error_handler_base.cc (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... android_webview/native/aw_certificate_error_handler_base.cc:13: AwCertificateErrorHandlerBase::~AwCertificateErrorHandlerBase() { This needs to be in browser/ in the matching .cc file, but see comment below. https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... android_webview/native/aw_certificate_error_handler_base.cc:17: AwCertificateErrorHandlerBase* AwCertificateErrorHandlerBase::FromID( Total stylistic issue: Normally we would put this in the class where the rest of the class implementation is, which means there is no need for this file and this method implementation should live in aw_contents.cc But, I'm not sure if this is the best way to work around the layering issue, but I can't think of anything better. So maybe wait around for joth to review before doing this. https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... android_webview/native/aw_certificate_error_handler_base.cc:23: DCHECK(host); Sorry to turn this around on you again, but now that I think about it, I wonder if this can happen in production, like say an ssl error happens while shutdown is happening. (DCHECKs are supposed to be for conditions that never happens) I don't know enough to answer that question, so maybe either wait for joth to review this, or do it more defensively and return NULL if either of these steps fail, and handle null case appropriately from the caller (probably LOG(WARNING) and cancel) https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... android_webview/native/aw_contents.cc:936: *cancel_request = Java_AwContents_cancelCertificateError(env, Would the old name but with a negation here be better...(I keep going back on what I suggestion...sorry) https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... File android_webview/native/aw_contents.h (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... android_webview/native/aw_contents.h:156: bool* cancel_request) OVERRIDE; blank line between methods
I'm very unsure about this. It will add substantial overhead to do it "right", but this seems a bit fragile to me. https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/6001/android_webview/native/aw_... android_webview/native/aw_contents.cc:935: ssl_error_callback_ = callback; On 2013/02/01 23:07:13, boliu wrote: > Actually I just noticed this, can I get in the loop too? > > So my question is can this happen? > > -> page1 error1 > -> calls client method > -> navigate to page2, which has error2 > -> calls client method > <- client comes back with response for error1 > > Even if this does cannot happen, we should still store the callbacks > individually. Easiest way is just put it in a map based on a static incrementing > key, then you can store that key in the java side and get it back in > ProceedSslError. (Good thing there are no threading issues) Right, I think this can happen. And if we add: -> page1 error1 -> calls client method -> navigate to page2, which has error2 -> calls client method <- client comes back with response for error1 <- client comes back with response for error2 -- this will be on the reset callback. Not sure what uninitialized state means, but don't think it is smart to call anything on it. Also this is an implementation detail that might change, or? I feel there should at least be a comment explaining this.
I think the navigation situation is a real problem. will work on it. Replies to other comments below. https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:836: return false; I think you mean why do we have to use the callback. We have to because returning true is "no-op". :) see the method level comments. We cannot change the semantics to mean such that returning false would mean "proceed". On 2013/02/01 23:07:13, boliu wrote: > so we are not doing this optimization? https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:20: private final Bundle table; I don't know if this is better, but this is from original code and I am not eager to change it unless there is a bug or some performance numbers that show it makes a difference. On 2013/02/01 23:07:13, boliu wrote: > I don't know much about bundle. Is it better than treemap or hashmap? https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:33: public void setIsAllowed(SslError sslError) { Please clarify, not sure what you mean. On 2013/02/01 23:07:13, boliu wrote: > Looks like this is not binary, so something like setMaxAllowedSslErrorLevel? https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:55: public void clear() { No not called right now. But soon will be used (reviving clearSslPreferences is the second part of bug, so I am not putting a TODO). On 2013/02/01 23:07:13, boliu wrote: > I don't think this is called anywhere. > > Should this class be per awcontents instead of global? (match old > implementation) https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... File android_webview/native/aw_certificate_error_handler_base.cc (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... android_webview/native/aw_certificate_error_handler_base.cc:13: AwCertificateErrorHandlerBase::~AwCertificateErrorHandlerBase() { On 2013/02/01 23:07:13, boliu wrote: > This needs to be in browser/ in the matching .cc file, but see comment below. I have chatted with Martin this morning and we sketched the general design as this one to workaround the layering problem. If we have a policy to move cc file next to header and put the implementation of static method to some other cc file, this is also fine and easy to do. I will let Martin/Joth or anybody who feels strong about it speak to it then. https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... android_webview/native/aw_certificate_error_handler_base.cc:17: AwCertificateErrorHandlerBase* AwCertificateErrorHandlerBase::FromID( see above. On 2013/02/01 23:07:13, boliu wrote: > Total stylistic issue: > > Normally we would put this in the class where the rest of the class > implementation is, which means there is no need for this file and this method > implementation should live in aw_contents.cc > > But, I'm not sure if this is the best way to work around the layering issue, but > I can't think of anything better. So maybe wait around for joth to review before > doing this. https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... android_webview/native/aw_certificate_error_handler_base.cc:23: DCHECK(host); I have seen this pattern in multiple places, and I don't think we check for nulls/DCHECK. But I don't know enough about it. If there is a slight chance that it can happen, let's put a check now! On 2013/02/01 23:07:13, boliu wrote: > Sorry to turn this around on you again, but now that I think about it, I wonder > if this can happen in production, like say an ssl error happens while shutdown > is happening. (DCHECKs are supposed to be for conditions that never happens) > > I don't know enough to answer that question, so maybe either wait for joth to > review this, or do it more defensively and return NULL if either of these steps > fail, and handle null case appropriately from the caller (probably LOG(WARNING) > and cancel) https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... android_webview/native/aw_contents.cc:936: *cancel_request = Java_AwContents_cancelCertificateError(env, I think the name is very clear. Why make it complicated. On 2013/02/01 23:07:13, boliu wrote: > Would the old name but with a negation here be better...(I keep going back on > what I suggestion...sorry) https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... File android_webview/native/aw_contents.h (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... android_webview/native/aw_contents.h:156: bool* cancel_request) OVERRIDE; On 2013/02/01 23:07:13, boliu wrote: > blank line between methods Done.
https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... File android_webview/native/aw_certificate_error_handler_base.cc (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... android_webview/native/aw_certificate_error_handler_base.cc:13: AwCertificateErrorHandlerBase::~AwCertificateErrorHandlerBase() { actually thinking about this again, I think cc and header files should be together. I will do the change. On 2013/02/02 01:01:10, sgurun wrote: > On 2013/02/01 23:07:13, boliu wrote: > > This needs to be in browser/ in the matching .cc file, but see comment below. > I have chatted with Martin this morning and we sketched the general design as > this one to workaround the layering problem. If we have a policy to move cc file > next to header and put the implementation of static method to some other cc > file, this is also fine and easy to do. I will let Martin/Joth or anybody who > feels strong about it speak to it then.
https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:836: return false; On 2013/02/02 01:01:10, sgurun wrote: > I think you mean why do we have to use the callback. We have to because > returning true is "no-op". :) see the method level comments. We cannot change > the semantics to mean such that returning false would mean "proceed". > > On 2013/02/01 23:07:13, boliu wrote: > > so we are not doing this optimization? > Can you just return true here without calling proceedSslError? https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:33: public void setIsAllowed(SslError sslError) { On 2013/02/02 01:01:10, sgurun wrote: > Please clarify, not sure what you mean. > > On 2013/02/01 23:07:13, boliu wrote: > > Looks like this is not binary, so something like setMaxAllowedSslErrorLevel? > I mean setIsAllowed feels like it should take a boolean value. This method effectively takes an integer, so was suggesting rename to something else. https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... File android_webview/native/aw_certificate_error_handler_base.cc (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... android_webview/native/aw_certificate_error_handler_base.cc:17: AwCertificateErrorHandlerBase* AwCertificateErrorHandlerBase::FromID( On 2013/02/02 01:01:10, sgurun wrote: > see above. > > On 2013/02/01 23:07:13, boliu wrote: > > Total stylistic issue: > > > > Normally we would put this in the class where the rest of the class > > implementation is, which means there is no need for this file and this method > > implementation should live in aw_contents.cc > > > > But, I'm not sure if this is the best way to work around the layering issue, > but > > I can't think of anything better. So maybe wait around for joth to review > before > > doing this. > Oh, if Martin already looked over the layering problem, then yeah, move the destructor to a cc alongside the h file, then move all of this to aw_contents.cc https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... android_webview/native/aw_contents.cc:936: *cancel_request = Java_AwContents_cancelCertificateError(env, On 2013/02/02 01:01:10, sgurun wrote: > I think the name is very clear. Why make it complicated. > > On 2013/02/01 23:07:13, boliu wrote: > > Would the old name but with a negation here be better...(I keep going back on > > what I suggestion...sorry) > There is a convention to match the method name all the way from native to java...but I don't care that much in this case :)
https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:836: return false; Nop, it won't work (Please take a look at the place where AllowCertificateError method call originates, we can chat later if you want to). On 2013/02/02 01:10:42, boliu wrote: > On 2013/02/02 01:01:10, sgurun wrote: > > I think you mean why do we have to use the callback. We have to because > > returning true is "no-op". :) see the method level comments. We cannot change > > the semantics to mean such that returning false would mean "proceed". > > > > On 2013/02/01 23:07:13, boliu wrote: > > > so we are not doing this optimization? > > > > Can you just return true here without calling proceedSslError? https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:33: public void setIsAllowed(SslError sslError) { Obviously SslError is not int, but I see what you mean. We can change the name (I can't think of a good name now, but will find) On 2013/02/02 01:10:42, boliu wrote: > On 2013/02/02 01:01:10, sgurun wrote: > > Please clarify, not sure what you mean. > > > > On 2013/02/01 23:07:13, boliu wrote: > > > Looks like this is not binary, so something like setMaxAllowedSslErrorLevel? > > > > I mean setIsAllowed feels like it should take a boolean value. This method > effectively takes an integer, so was suggesting rename to something else. https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... File android_webview/native/aw_certificate_error_handler_base.cc (right): https://codereview.chromium.org/12091111/diff/14001/android_webview/native/aw... android_webview/native/aw_certificate_error_handler_base.cc:17: AwCertificateErrorHandlerBase* AwCertificateErrorHandlerBase::FromID( On 2013/02/02 01:10:42, boliu wrote: > On 2013/02/02 01:01:10, sgurun wrote: > > see above. > > > > On 2013/02/01 23:07:13, boliu wrote: > > > Total stylistic issue: > > > > > > Normally we would put this in the class where the rest of the class > > > implementation is, which means there is no need for this file and this > method > > > implementation should live in aw_contents.cc > > > > > > But, I'm not sure if this is the best way to work around the layering issue, > > but > > > I can't think of anything better. So maybe wait around for joth to review > > before > > > doing this. > > > > Oh, if Martin already looked over the layering problem, then yeah, move the > destructor to a cc alongside the h file, then move all of this to aw_contents.cc Done.
https://codereview.chromium.org/12091111/diff/23001/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/23001/android_webview/native/aw... android_webview/native/aw_contents.cc:953: ssl_error_callback_ = callback; Only save callback if cancel_request is false? But I guess you are already reworking this part of the code.
Good point. The function supports both partial synchronous and async. behavior which makes using it unnecessarily complicated. However, more than this, I am not very happy with the fact that AllowCertificateError does work only for the main frame. This means the webview app will not be able to control the loading of the resources (in case of ssl error) which I think is an important set back. We may want to change chromium code, and do some discussion around it. I will leave this fun to monday.
https://codereview.chromium.org/12091111/diff/23001/android_webview/browser/a... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/12091111/diff/23001/android_webview/browser/a... android_webview/browser/aw_content_browser_client.cc:214: DCHECK(error_handler); this is pointless, we'll crash in the next line anyway. It does seem like a more robust solution is easily available: if (error_handler) { ... } else { DCHECK(false); *cancel_request = true; } https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:827: private boolean cancelCertificateError(int certError, byte[] derBytes, String url) { would it be possible to move this and the other method out of AwContents and into a separate class (like I've done for AwLayoutSizer)? The main role of AwContents should be routing calls and callbacks. https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:976: private SslCertificate getCertificateFromDerBytes(byte[] derBytes) { this could be static, right? https://codereview.chromium.org/12091111/diff/23001/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/23001/android_webview/native/aw... android_webview/native/aw_contents.cc:126: const content::RenderViewHost* host = uber nit: normally this is called the rvh, not host :) https://codereview.chromium.org/12091111/diff/23001/android_webview/native/aw... android_webview/native/aw_contents.cc:128: DCHECK(host); please make this robust to NULLs. It's likely for an rvh to not exist in unittest code (in production it's rarer but still can happen if the user closes a tab at the "right" time). https://codereview.chromium.org/12091111/diff/23001/android_webview/native/aw... android_webview/native/aw_contents.cc:132: return static_cast<AwCertificateErrorHandlerBase*> nit: I think the common pattern is to leave the open paren on the previous line. https://codereview.chromium.org/12091111/diff/23001/android_webview/native/aw... android_webview/native/aw_contents.cc:953: ssl_error_callback_ = callback; IMO it would be helpful if we had a bridge from Chromium base::Callbacks to a Java object (something like Callable<?> maybe). We've been working around the lack of such a construct on multiple occasions (this one included) so I think it'd be a good investment. WDYT?
https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:986: // Intentional fall through exceptions don't fall through? https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java (right): https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:14: * Stores the user's decision of whether to allow or deny an invalid certificate. singleton https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:18: private static SslCertLookupTable sTable; sInstance? https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:20: private final Bundle table; mTable? But a table of what exactly? I think the name could be more descriptive.
https://codereview.chromium.org/12091111/diff/23001/android_webview/browser/a... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/12091111/diff/23001/android_webview/browser/a... android_webview/browser/aw_content_browser_client.cc:214: DCHECK(error_handler); himm, if there is a possibility of rvh not existing, I guess DCHECK would be wrong anyway. On 2013/02/04 12:51:31, Martin Kosiba wrote: > this is pointless, we'll crash in the next line anyway. It does seem like a more > robust solution is easily available: > > if (error_handler) { > ... > } else { > DCHECK(false); > *cancel_request = true; > } Done. https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:827: private boolean cancelCertificateError(int certError, byte[] derBytes, String url) { I think we can. I see that you are using a delegate mechanism so that AwLayoutSizer and AwContents are not tightly coupled. Looks reasonable. On 2013/02/04 12:51:31, Martin Kosiba wrote: > would it be possible to move this and the other method out of AwContents and > into a separate class (like I've done for AwLayoutSizer)? The main role of > AwContents should be routing calls and callbacks. https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:976: private SslCertificate getCertificateFromDerBytes(byte[] derBytes) { On 2013/02/04 12:51:31, Martin Kosiba wrote: > this could be static, right? Done. https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:986: // Intentional fall through I don't know what the author meant. Just moved the code out of the method. On 2013/02/04 17:27:46, benm wrote: > exceptions don't fall through? https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java (right): https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:14: * Stores the user's decision of whether to allow or deny an invalid certificate. On 2013/02/04 17:27:46, benm wrote: > singleton Done. https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:18: private static SslCertLookupTable sTable; On 2013/02/04 17:27:46, benm wrote: > sInstance? Done. https://codereview.chromium.org/12091111/diff/23001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslCertLookupTable.java:20: private final Bundle table; On 2013/02/04 17:27:46, benm wrote: > mTable? But a table of what exactly? I think the name could be more descriptive. Done. https://codereview.chromium.org/12091111/diff/23001/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/23001/android_webview/native/aw... android_webview/native/aw_contents.cc:126: const content::RenderViewHost* host = On 2013/02/04 12:51:31, Martin Kosiba wrote: > uber nit: normally this is called the rvh, not host :) Done. https://codereview.chromium.org/12091111/diff/23001/android_webview/native/aw... android_webview/native/aw_contents.cc:128: DCHECK(host); On 2013/02/04 12:51:31, Martin Kosiba wrote: > please make this robust to NULLs. It's likely for an rvh to not exist in > unittest code (in production it's rarer but still can happen if the user closes > a tab at the "right" time). Done. https://codereview.chromium.org/12091111/diff/23001/android_webview/native/aw... android_webview/native/aw_contents.cc:132: return static_cast<AwCertificateErrorHandlerBase*> On 2013/02/04 12:51:31, Martin Kosiba wrote: > nit: I think the common pattern is to leave the open paren on the previous line. Done. https://codereview.chromium.org/12091111/diff/23001/android_webview/native/aw... android_webview/native/aw_contents.cc:953: ssl_error_callback_ = callback; Makes sense. adding a bridge for the aw_content_client as we talked. It seems you are working on the native callback bridge. On 2013/02/04 12:51:31, Martin Kosiba wrote: > IMO it would be helpful if we had a bridge from Chromium base::Callbacks to a > Java object (something like Callable<?> maybe). We've been working around the > lack of such a construct on multiple occasions (this one included) so I think > it'd be a good investment. WDYT? https://codereview.chromium.org/12091111/diff/23001/android_webview/native/aw... android_webview/native/aw_contents.cc:953: ssl_error_callback_ = callback; we are now saving all the callbacks. On 2013/02/02 03:51:29, boliu wrote: > Only save callback if cancel_request is false? But I guess you are already > reworking this part of the code.
I browsed the overall structure more than detailed line-by-line review. I like having a native AwContentsClient object. Can't believe we've got this far without it. Main suggestion is on how to look it up from inside the aw/browser/ layer https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/a... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/a... android_webview/browser/aw_content_browser_client.cc:272: AwCertificateErrorHandlerBase::FromID(render_process_id, render_view_id); you're fighting your way up the layers in an awkward way here - declaring static function in lower layer but defining it in higher one is skirting around the issue (I think AwHttpAuthHandlerBase has the same snag, but not a reason to copy it now we have better things to work with) for singleton-like objects, Bo's JNI dependency factory maybe the cool new solution to all these problems: https://codereview.chromium.org/12253057/diff/23008/android_webview/browser/j... but I don't think that's the right fit here. you need the object to be per-webview (i.e. the AwContentsClient is a per webview relation) so I think the answer is, 1) expand AwCertificateErrorHandlerBase to be the browser/ base class for all of AwContentsClient. (we might call it AwContentsClient and have the native/ subclass be AwContentsClientImpl 2) use WebContents::SetUserData to store a AwContentsClient* inside the web contents instance. (see AwContents for an example). (be carefull to handle the AwContents::SetWebContents case too) 3) here, use WebContents::FromId()->GetUserData(kAwContentsClient)->allowCertificateError() (we can make a static method AwContentsClient::FromID(), and AwContentsClient::FromWebContents, to hide some of that) I can pair code that if it doesn't make sense. https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:27: public class AwContentsClientBridge { I see this is fairly logic heavy, so maybe useful to have its own class, but our normal approach would be to have AwContentsClient.java talk direct to aw_contents_client.cc no need for another Bridge class in between. Marcin - is this what you were thinking? The logic below maybe able to extract into some SslCertificateHelper class... which will avoid the need to have AwContents call method on AwContentsClient for the der->cert conversion utility (which feels a bit of a random interconnection) https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:42: mNativeAwContentsClientBridge = 0; I'd expect to see nativeDestroy(mNativeAwContentsClientBridge) here... the java object owns the native side? https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw... android_webview/native/aw_contents_client_bridge.cc:44: net::X509Certificate::GetDEREncoded(cert->os_cert_handle(),&der_string); nit: space after comma https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw... android_webview/native/aw_contents_client_bridge.cc:50: // as it may do an asynchronous callback prior to returning. I think you mean: an async -> a synchronous ? https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw... android_webview/native/aw_contents_client_bridge.cc:54: java_ref_.get(env).obj(), reinterpret_cast<jint>(cert_error), jcert.obj(), java_ref_ is a weak ref, so we need to always promote it to a ScopedJavaLocalRef and check it for null prior to calling obj() (same pattern as used throughout aw_contents.cc)
https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/a... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/a... android_webview/browser/aw_content_browser_client.cc:272: AwCertificateErrorHandlerBase::FromID(render_process_id, render_view_id); I talked to Martin and got his idea about this part since I am not too familiar with what patterns we use to deal with such inter layer access. I can do the change but I am not sure what you mean by "be careful to handle AwContents::SetWebContents case too" On 2013/02/22 01:15:45, joth wrote: > you're fighting your way up the layers in an awkward way here - declaring static > function in lower layer but defining it in higher one is skirting around the > issue (I think AwHttpAuthHandlerBase has the same snag, but not a reason to copy > it now we have better things to work with) > > for singleton-like objects, Bo's JNI dependency factory maybe the cool new > solution to all these problems: > https://codereview.chromium.org/12253057/diff/23008/android_webview/browser/j... > > but I don't think that's the right fit here. you need the object to be > per-webview (i.e. the AwContentsClient is a per webview relation) > > so I think the answer is, > 1) expand AwCertificateErrorHandlerBase to be the browser/ base class for all of > AwContentsClient. (we might call it AwContentsClient and have the native/ > subclass be AwContentsClientImpl > 2) use WebContents::SetUserData to store a AwContentsClient* inside the web > contents instance. (see AwContents for an example). (be carefull to handle the > AwContents::SetWebContents case too) > 3) here, use > WebContents::FromId()->GetUserData(kAwContentsClient)->allowCertificateError() > (we can make a static method AwContentsClient::FromID(), and > AwContentsClient::FromWebContents, to hide some of that) > > > I can pair code that if it doesn't make sense. > https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:27: public class AwContentsClientBridge { I started as part of AwContensClient.java but then found it already large enough so that I did not want to crowd it, had a quick chat with Marcin and decided to go this way. We could move ssl stuff to its own class too. However, I don't think offloading the JNI communication part to a bridge class is a terribly bad idea. Agreed with the final comment. On 2013/02/22 01:15:45, joth wrote: > I see this is fairly logic heavy, so maybe useful to have its own class, but our > normal approach would be to have AwContentsClient.java talk direct to > aw_contents_client.cc no need for another Bridge class in between. > > Marcin - is this what you were thinking? > > > The logic below maybe able to extract into some SslCertificateHelper class... > which will avoid the need to have AwContents call method on AwContentsClient for > the der->cert conversion utility (which feels a bit of a random interconnection) > https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:42: mNativeAwContentsClientBridge = 0; It is owned by AwContents, since we utilize it to access this class. We can come up with a different way if you want to. On 2013/02/22 01:15:45, joth wrote: > I'd expect to see nativeDestroy(mNativeAwContentsClientBridge) here... the java > object owns the native side? https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw... android_webview/native/aw_contents_client_bridge.cc:44: net::X509Certificate::GetDEREncoded(cert->os_cert_handle(),&der_string); will fix On 2013/02/22 01:15:45, joth wrote: > nit: space after comma https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw... android_webview/native/aw_contents_client_bridge.cc:50: // as it may do an asynchronous callback prior to returning. yeah. On 2013/02/22 01:15:45, joth wrote: > I think you mean: an async -> a synchronous ? https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw... android_webview/native/aw_contents_client_bridge.cc:54: java_ref_.get(env).obj(), reinterpret_cast<jint>(cert_error), jcert.obj(), Aha... right. But I think I was just looking at the HttpAuthRequest, which is not doing that! ScopedJavaLocalRef<jstring> jrealm = ConvertUTF8ToJavaString(env, realm); Java_AwContents_onReceivedHttpAuthRequest(env, java_ref_.get(env).obj(), handler.obj(), jhost.obj(), jrealm.obj()); On 2013/02/22 01:15:45, joth wrote: > java_ref_ is a weak ref, so we need to always promote it to a ScopedJavaLocalRef > and check it for null prior to calling obj() > > (same pattern as used throughout aw_contents.cc)
https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:36: mNativeAwContentsClientBridge = nativeInit(nativeAwContents); suggest doing the native construction in AwContents (.cc) to keep it symmetric (i.e. create own and destroy all from same class, use scoped_ptr) https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:42: mNativeAwContentsClientBridge = 0; On 2013/02/22 01:48:07, sgurun wrote: > It is owned by AwContents, since we utilize it to access this class. We can come > up with a different way if you want to. > > On 2013/02/22 01:15:45, joth wrote: > > I'd expect to see nativeDestroy(mNativeAwContentsClientBridge) here... the > java > > object owns the native side? > Gotcha. So lets initiate the mNativeAwContentsClientBridge = 0; from a CalledByNative method, invoked from the native side's destructor. That way it's sure to be zeroed however the native side dies. https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw... android_webview/native/aw_contents.cc:677: Java_AwContents_onReceivedHttpAuthRequest(env, java_ref_.get(env).obj(), stash result in ScopedLocalRef and do null check.
On 21 February 2013 17:48, <sgurun@chromium.org> wrote: > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/browser/aw_**content_browser_client.cc<https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/aw_content_browser_client.cc> > File android_webview/browser/aw_**content_browser_client.cc (right): > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/browser/aw_**content_browser_client.cc#**newcode272<https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/aw_content_browser_client.cc#newcode272> > android_webview/browser/aw_**content_browser_client.cc:272: > AwCertificateErrorHandlerBase:**:FromID(render_process_id, > render_view_id); > I talked to Martin and got his idea about this part since I am not too > familiar with what patterns we use to deal with such inter layer access. > > > I can do the change but I am not sure what you mean by "be careful to > handle AwContents::SetWebContents case too" > > > On 2013/02/22 01:15:45, joth wrote: > >> you're fighting your way up the layers in an awkward way here - >> > declaring static > >> function in lower layer but defining it in higher one is skirting >> > around the > >> issue (I think AwHttpAuthHandlerBase has the same snag, but not a >> > reason to copy > >> it now we have better things to work with) >> > > for singleton-like objects, Bo's JNI dependency factory maybe the >> > cool new > >> solution to all these problems: >> > > https://codereview.chromium.**org/12253057/diff/23008/** > android_webview/browser/jni_**dependency_factory.h<https://codereview.chromium.org/12253057/diff/23008/android_webview/browser/jni_dependency_factory.h> > > but I don't think that's the right fit here. you need the object to be >> per-webview (i.e. the AwContentsClient is a per webview relation) >> > > so I think the answer is, >> 1) expand AwCertificateErrorHandlerBase to be the browser/ base class >> > for all of > >> AwContentsClient. (we might call it AwContentsClient and have the >> > native/ > >> subclass be AwContentsClientImpl >> 2) use WebContents::SetUserData to store a AwContentsClient* inside >> > the web > >> contents instance. (see AwContents for an example). (be carefull to >> > handle the > >> AwContents::SetWebContents case too) >> 3) here, use >> > > WebContents::FromId()->**GetUserData(kAwContentsClient)** > ->allowCertificateError() > >> (we can make a static method AwContentsClient::FromID(), and >> AwContentsClient::**FromWebContents, to hide some of that) >> > > > I can pair code that if it doesn't make sense. >> > > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java<https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java> > File > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java > (right): > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java#**newcode27<https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java#newcode27> > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java:**27: > public class AwContentsClientBridge { > I started as part of AwContensClient.java but then found it already > large enough so that I did not want to crowd it, had a quick chat with > Marcin and decided to go this way. We could move ssl stuff to its own > class too. However, I don't think offloading the JNI communication part > to a bridge class is a terribly bad idea. > > > yeah you convinced me. FooBridge is (for whatever reason) not a naming scheme we've had until now, but I think it partitions it well enough here that we should roll with it then. > Agreed with the final comment. > > > On 2013/02/22 01:15:45, joth wrote: > >> I see this is fairly logic heavy, so maybe useful to have its own >> > class, but our > >> normal approach would be to have AwContentsClient.java talk direct to >> aw_contents_client.cc no need for another Bridge class in between. >> > > Marcin - is this what you were thinking? >> > > > The logic below maybe able to extract into some SslCertificateHelper >> > class... > >> which will avoid the need to have AwContents call method on >> > AwContentsClient for > >> the der->cert conversion utility (which feels a bit of a random >> > interconnection) > > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java#**newcode42<https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java#newcode42> > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java:**42: > mNativeAwContentsClientBridge = 0; > It is owned by AwContents, since we utilize it to access this class. We > can come up with a different way if you want to. > > > On 2013/02/22 01:15:45, joth wrote: > >> I'd expect to see nativeDestroy(**mNativeAwContentsClientBridge) here... >> > the java > >> object owns the native side? >> > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/native/aw_**contents_client_bridge.cc<https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw_contents_client_bridge.cc> > File android_webview/native/aw_**contents_client_bridge.cc (right): > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/native/aw_**contents_client_bridge.cc#**newcode44<https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw_contents_client_bridge.cc#newcode44> > android_webview/native/aw_**contents_client_bridge.cc:44: > net::X509Certificate::**GetDEREncoded(cert->os_cert_** > handle(),&der_string); > will fix > > > On 2013/02/22 01:15:45, joth wrote: > >> nit: space after comma >> > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/native/aw_**contents_client_bridge.cc#**newcode50<https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw_contents_client_bridge.cc#newcode50> > android_webview/native/aw_**contents_client_bridge.cc:50: // as it may do > an asynchronous callback prior to returning. > yeah. > > On 2013/02/22 01:15:45, joth wrote: > >> I think you mean: an async -> a synchronous ? >> > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/native/aw_**contents_client_bridge.cc#**newcode54<https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw_contents_client_bridge.cc#newcode54> > android_webview/native/aw_**contents_client_bridge.cc:54: > java_ref_.get(env).obj(), reinterpret_cast<jint>(cert_**error), > jcert.obj(), > Aha... right. But I think I was just looking at the HttpAuthRequest, > which is not doing that! > > ScopedJavaLocalRef<jstring> jrealm = ConvertUTF8ToJavaString(env, > realm); > Java_AwContents_**onReceivedHttpAuthRequest(env, > java_ref_.get(env).obj(), > handler.obj(), jhost.obj(), > jrealm.obj()); > > > > On 2013/02/22 01:15:45, joth wrote: > >> java_ref_ is a weak ref, so we need to always promote it to a >> > ScopedJavaLocalRef > >> and check it for null prior to calling obj() >> > > (same pattern as used throughout aw_contents.cc) >> > > https://codereview.chromium.**org/12091111/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/a... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/a... android_webview/browser/aw_content_browser_client.cc:272: AwCertificateErrorHandlerBase::FromID(render_process_id, render_view_id); On 2013/02/22 01:48:07, sgurun wrote: > I talked to Martin and got his idea about this part since I am not too familiar > with what patterns we use to deal with such inter layer access. > > I can do the change but I am not sure what you mean by "be careful to handle > AwContents::SetWebContents case too" > > On 2013/02/22 01:15:45, joth wrote: > > you're fighting your way up the layers in an awkward way here - declaring > static > > function in lower layer but defining it in higher one is skirting around the > > issue (I think AwHttpAuthHandlerBase has the same snag, but not a reason to > copy > > it now we have better things to work with) > > > > for singleton-like objects, Bo's JNI dependency factory maybe the cool new > > solution to all these problems: > > > https://codereview.chromium.org/12253057/diff/23008/android_webview/browser/j... > > > > but I don't think that's the right fit here. you need the object to be > > per-webview (i.e. the AwContentsClient is a per webview relation) > > > > so I think the answer is, > > 1) expand AwCertificateErrorHandlerBase to be the browser/ base class for all > of > > AwContentsClient. (we might call it AwContentsClient and have the native/ > > subclass be AwContentsClientImpl > > 2) use WebContents::SetUserData to store a AwContentsClient* inside the web > > contents instance. (see AwContents for an example). (be carefull to handle the > > AwContents::SetWebContents case too) > > 3) here, use > > WebContents::FromId()->GetUserData(kAwContentsClient)->allowCertificateError() > > (we can make a static method AwContentsClient::FromID(), and > > AwContentsClient::FromWebContents, to hide some of that) > > > > > > I can pair code that if it doesn't make sense. > > > The static method was my suggestion - I believe at that time of our conversation the AwHttpAuthHandlerBase pattern was the only thing we had. Sorry for misleading you there, but it looks like Joth's plan shouldn't be a big pain to apply. https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:81: if (mClient != null) { if the client is null then shouldn't we return false?
On 22 February 2013 07:35, <mkosiba@chromium.org> wrote: > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/browser/aw_**content_browser_client.cc<https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/aw_content_browser_client.cc> > File android_webview/browser/aw_**content_browser_client.cc (right): > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/browser/aw_**content_browser_client.cc#**newcode272<https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/aw_content_browser_client.cc#newcode272> > android_webview/browser/aw_**content_browser_client.cc:272: > AwCertificateErrorHandlerBase:**:FromID(render_process_id, > render_view_id); > On 2013/02/22 01:48:07, sgurun wrote: > >> I talked to Martin and got his idea about this part since I am not too >> > familiar > >> with what patterns we use to deal with such inter layer access. >> > > I can do the change but I am not sure what you mean by "be careful to >> > handle > >> AwContents::SetWebContents case too" >> > > On 2013/02/22 01:15:45, joth wrote: >> > you're fighting your way up the layers in an awkward way here - >> > declaring > >> static >> > function in lower layer but defining it in higher one is skirting >> > around the > >> > issue (I think AwHttpAuthHandlerBase has the same snag, but not a >> > reason to > >> copy >> > it now we have better things to work with) >> > >> > for singleton-like objects, Bo's JNI dependency factory maybe the >> > cool new > >> > solution to all these problems: >> > >> > > https://codereview.chromium.**org/12253057/diff/23008/** > android_webview/browser/jni_**dependency_factory.h<https://codereview.chromium.org/12253057/diff/23008/android_webview/browser/jni_dependency_factory.h> > >> > >> > but I don't think that's the right fit here. you need the object to >> > be > >> > per-webview (i.e. the AwContentsClient is a per webview relation) >> > >> > so I think the answer is, >> > 1) expand AwCertificateErrorHandlerBase to be the browser/ base >> > class for all > >> of >> > AwContentsClient. (we might call it AwContentsClient and have the >> > native/ > >> > subclass be AwContentsClientImpl >> > 2) use WebContents::SetUserData to store a AwContentsClient* inside >> > the web > >> > contents instance. (see AwContents for an example). (be carefull to >> > handle the > >> > AwContents::SetWebContents case too) >> > 3) here, use >> > >> > WebContents::FromId()->**GetUserData(kAwContentsClient)** > ->allowCertificateError() > >> > (we can make a static method AwContentsClient::FromID(), and >> > AwContentsClient::**FromWebContents, to hide some of that) >> > >> > >> > I can pair code that if it doesn't make sense. >> > >> > > > The static method was my suggestion - I believe at that time of our > conversation the AwHttpAuthHandlerBase pattern was the only thing we > had. Sorry for misleading you there, but it looks like Joth's plan > shouldn't be a big pain to apply. > > Ah, yes, no problem. I knew we'd used a static method escape hatch a few places in lieu of having a sensible abstraction for aw/browser -> embedder callbacks. As we're now adding that sensible abstraction, I thought we may as well do it the right way :-) > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java<https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java> > File > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java > (right): > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java#**newcode81<https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java#newcode81> > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java:**81: > if (mClient != null) { > if the client is null then shouldn't we return false? > > https://codereview.chromium.**org/12091111/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/a... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/a... android_webview/browser/aw_content_browser_client.cc:272: AwCertificateErrorHandlerBase::FromID(render_process_id, render_view_id); I am doing the change -- Maybe I did not understand it right, but I did not like this new solution. The UserData looks like a object registry where you register some objects for using later. So why did not I like it that much: first it takes the ownership of the object (unnecessary ownership concerns), second, as Joth pointed out webcontents can be set later, creating another opportunity to create bugs in future, third it makes me use another unnecessary reinterpret_cast (which unfortunately takes away the typesafety). It is very possible I am misunderstanding how to use it by a gross margin though. I am implementing this anyway... On 2013/02/22 15:35:56, Martin Kosiba wrote: > On 2013/02/22 01:48:07, sgurun wrote: > > I talked to Martin and got his idea about this part since I am not too > familiar > > with what patterns we use to deal with such inter layer access. > > > > I can do the change but I am not sure what you mean by "be careful to handle > > AwContents::SetWebContents case too" > > > > On 2013/02/22 01:15:45, joth wrote: > > > you're fighting your way up the layers in an awkward way here - declaring > > static > > > function in lower layer but defining it in higher one is skirting around the > > > issue (I think AwHttpAuthHandlerBase has the same snag, but not a reason to > > copy > > > it now we have better things to work with) > > > > > > for singleton-like objects, Bo's JNI dependency factory maybe the cool new > > > solution to all these problems: > > > > > > https://codereview.chromium.org/12253057/diff/23008/android_webview/browser/j... > > > > > > but I don't think that's the right fit here. you need the object to be > > > per-webview (i.e. the AwContentsClient is a per webview relation) > > > > > > so I think the answer is, > > > 1) expand AwCertificateErrorHandlerBase to be the browser/ base class for > all > > of > > > AwContentsClient. (we might call it AwContentsClient and have the native/ > > > subclass be AwContentsClientImpl > > > 2) use WebContents::SetUserData to store a AwContentsClient* inside the web > > > contents instance. (see AwContents for an example). (be carefull to handle > the > > > AwContents::SetWebContents case too) > > > 3) here, use > > > > WebContents::FromId()->GetUserData(kAwContentsClient)->allowCertificateError() > > > (we can make a static method AwContentsClient::FromID(), and > > > AwContentsClient::FromWebContents, to hide some of that) > > > > > > > > > I can pair code that if it doesn't make sense. > > > > > > > The static method was my suggestion - I believe at that time of our conversation > the AwHttpAuthHandlerBase pattern was the only thing we had. Sorry for > misleading you there, but it looks like Joth's plan shouldn't be a big pain to > apply. https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:36: mNativeAwContentsClientBridge = nativeInit(nativeAwContents); On 2013/02/22 02:39:44, joth wrote: > suggest doing the native construction in AwContents (.cc) to keep it symmetric > (i.e. create own and destroy all from same class, use scoped_ptr) Done. https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:42: mNativeAwContentsClientBridge = 0; On 2013/02/22 02:39:44, joth wrote: > On 2013/02/22 01:48:07, sgurun wrote: > > It is owned by AwContents, since we utilize it to access this class. We can > come > > up with a different way if you want to. > > > > On 2013/02/22 01:15:45, joth wrote: > > > I'd expect to see nativeDestroy(mNativeAwContentsClientBridge) here... the > > java > > > object owns the native side? > > > > Gotcha. So lets initiate the mNativeAwContentsClientBridge = 0; from a > CalledByNative method, invoked from the native side's destructor. That way it's > sure to be zeroed however the native side dies. Done. https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:81: if (mClient != null) { good catch. This is an unnecessary check. mClient can't be null actually since we asserting in the constructor and then keeping a ref. On 2013/02/22 15:35:56, Martin Kosiba wrote: > if the client is null then shouldn't we return false? https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw... android_webview/native/aw_contents.cc:677: Java_AwContents_onReceivedHttpAuthRequest(env, java_ref_.get(env).obj(), On 2013/02/22 02:39:44, joth wrote: > stash result in ScopedLocalRef and do null check. Done.
On 22 February 2013 11:31, <sgurun@chromium.org> wrote: > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/browser/aw_**content_browser_client.cc<https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/aw_content_browser_client.cc> > File android_webview/browser/aw_**content_browser_client.cc (right): > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/browser/aw_**content_browser_client.cc#**newcode272<https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/aw_content_browser_client.cc#newcode272> > android_webview/browser/aw_**content_browser_client.cc:272: > AwCertificateErrorHandlerBase:**:FromID(render_process_id, > render_view_id); > I am doing the change -- Maybe I did not understand it right, but I did > not like this new solution. The UserData looks like a object registry > where you register some objects for using later. So why did not I like > it that much: first it takes the ownership of the object (unnecessary > ownership concerns), second, as Joth pointed out webcontents can be set > later, creating another opportunity to create bugs in future, third it > makes me use another unnecessary reinterpret_cast (which unfortunately > takes away the typesafety). > It is very possible I am misunderstanding how to use it by a gross > margin though. > > Look for existing users of it - there's pattern for avoiding it taking ownership (basically, declare a new struct that it owns, that internally just has a raw (non-owning) pointer to your object). Wrap the setter and getter in static functions inside the AwContentClientBridge base class, which contains the casting and UserData thunking all in one place and so is cleaner. Your old solution had to down-cast in AwCertificateErrorHandlerBase::FromID anyway, so it's not all that different > I am implementing this anyway... > > You are a noble man. > > On 2013/02/22 15:35:56, Martin Kosiba wrote: > >> On 2013/02/22 01:48:07, sgurun wrote: >> > I talked to Martin and got his idea about this part since I am not >> > too > >> familiar >> > with what patterns we use to deal with such inter layer access. >> > >> > I can do the change but I am not sure what you mean by "be careful >> > to handle > >> > AwContents::SetWebContents case too" >> > >> > On 2013/02/22 01:15:45, joth wrote: >> > > you're fighting your way up the layers in an awkward way here - >> > declaring > >> > static >> > > function in lower layer but defining it in higher one is skirting >> > around the > >> > > issue (I think AwHttpAuthHandlerBase has the same snag, but not a >> > reason to > >> > copy >> > > it now we have better things to work with) >> > > >> > > for singleton-like objects, Bo's JNI dependency factory maybe the >> > cool new > >> > > solution to all these problems: >> > > >> > >> > > https://codereview.chromium.**org/12253057/diff/23008/** > android_webview/browser/jni_**dependency_factory.h<https://codereview.chromium.org/12253057/diff/23008/android_webview/browser/jni_dependency_factory.h> > >> > > >> > > but I don't think that's the right fit here. you need the object >> > to be > >> > > per-webview (i.e. the AwContentsClient is a per webview relation) >> > > >> > > so I think the answer is, >> > > 1) expand AwCertificateErrorHandlerBase to be the browser/ base >> > class for > >> all >> > of >> > > AwContentsClient. (we might call it AwContentsClient and have the >> > native/ > >> > > subclass be AwContentsClientImpl >> > > 2) use WebContents::SetUserData to store a AwContentsClient* >> > inside the web > >> > > contents instance. (see AwContents for an example). (be carefull >> > to handle > >> the >> > > AwContents::SetWebContents case too) >> > > 3) here, use >> > > >> > > WebContents::FromId()->**GetUserData(kAwContentsClient)** > ->allowCertificateError() > >> > > (we can make a static method AwContentsClient::FromID(), and >> > > AwContentsClient::**FromWebContents, to hide some of that) >> > > >> > > >> > > I can pair code that if it doesn't make sense. >> > > >> > >> > > The static method was my suggestion - I believe at that time of our >> > conversation > >> the AwHttpAuthHandlerBase pattern was the only thing we had. Sorry for >> misleading you there, but it looks like Joth's plan shouldn't be a big >> > pain to > >> apply. >> > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java<https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java> > File > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java > (right): > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java#**newcode36<https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java#newcode36> > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java:**36: > mNativeAwContentsClientBridge = nativeInit(nativeAwContents); > On 2013/02/22 02:39:44, joth wrote: > >> suggest doing the native construction in AwContents (.cc) to keep it >> > symmetric > >> (i.e. create own and destroy all from same class, use scoped_ptr) >> > > Done. > > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java#**newcode42<https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java#newcode42> > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java:**42: > mNativeAwContentsClientBridge = 0; > On 2013/02/22 02:39:44, joth wrote: > >> On 2013/02/22 01:48:07, sgurun wrote: >> > It is owned by AwContents, since we utilize it to access this class. >> > We can > >> come >> > up with a different way if you want to. >> > >> > On 2013/02/22 01:15:45, joth wrote: >> > > I'd expect to see nativeDestroy(**mNativeAwContentsClientBridge) >> > here... the > >> > java >> > > object owns the native side? >> > >> > > Gotcha. So lets initiate the mNativeAwContentsClientBridge = 0; from a >> CalledByNative method, invoked from the native side's destructor. That >> > way it's > >> sure to be zeroed however the native side dies. >> > > Done. > > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java#**newcode81<https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java#newcode81> > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClientBridge.java:**81: > if (mClient != null) { > good catch. This is an unnecessary check. mClient can't be null actually > since we asserting in the constructor and then keeping a ref. > > On 2013/02/22 15:35:56, Martin Kosiba wrote: > >> if the client is null then shouldn't we return false? >> > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/native/aw_**contents.cc<https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw_contents.cc> > File android_webview/native/aw_**contents.cc (right): > > https://codereview.chromium.**org/12091111/diff/31001/** > android_webview/native/aw_**contents.cc#newcode677<https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw_contents.cc#newcode677> > android_webview/native/aw_**contents.cc:677: > Java_AwContents_**onReceivedHttpAuthRequest(env, java_ref_.get(env).obj(), > On 2013/02/22 02:39:44, joth wrote: > >> stash result in ScopedLocalRef and do null check. >> > > Done. > > https://codereview.chromium.**org/12091111/<https://codereview.chromium.org/1... >
:) On Fri, Feb 22, 2013 at 12:42 PM, Jonathan Dixon <joth@chromium.org> wrote: > > > > On 22 February 2013 11:31, <sgurun@chromium.org> wrote: > >> >> https://codereview.chromium.**org/12091111/diff/31001/** >> android_webview/browser/aw_**content_browser_client.cc<https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/aw_content_browser_client.cc> >> File android_webview/browser/aw_**content_browser_client.cc (right): >> >> https://codereview.chromium.**org/12091111/diff/31001/** >> android_webview/browser/aw_**content_browser_client.cc#**newcode272<https://codereview.chromium.org/12091111/diff/31001/android_webview/browser/aw_content_browser_client.cc#newcode272> >> android_webview/browser/aw_**content_browser_client.cc:272: >> AwCertificateErrorHandlerBase:**:FromID(render_process_id, >> render_view_id); >> I am doing the change -- Maybe I did not understand it right, but I did >> not like this new solution. The UserData looks like a object registry >> where you register some objects for using later. So why did not I like >> it that much: first it takes the ownership of the object (unnecessary >> ownership concerns), second, as Joth pointed out webcontents can be set >> later, creating another opportunity to create bugs in future, third it >> makes me use another unnecessary reinterpret_cast (which unfortunately >> takes away the typesafety). >> It is very possible I am misunderstanding how to use it by a gross >> margin though. >> >> > Look for existing users of it - there's pattern for avoiding it taking > ownership (basically, declare a new struct that it owns, that internally > just has a raw (non-owning) pointer to your object). > Wrap the setter and getter in static functions inside the > AwContentClientBridge base class, which contains the casting and UserData > thunking all in one place and so is cleaner. > Your old solution had to down-cast in > AwCertificateErrorHandlerBase::FromID anyway, so it's not all that > different > > > >> I am implementing this anyway... >> >> You are a noble man. > > >> >> On 2013/02/22 15:35:56, Martin Kosiba wrote: >> >>> On 2013/02/22 01:48:07, sgurun wrote: >>> > I talked to Martin and got his idea about this part since I am not >>> >> too >> >>> familiar >>> > with what patterns we use to deal with such inter layer access. >>> > >>> > I can do the change but I am not sure what you mean by "be careful >>> >> to handle >> >>> > AwContents::SetWebContents case too" >>> > >>> > On 2013/02/22 01:15:45, joth wrote: >>> > > you're fighting your way up the layers in an awkward way here - >>> >> declaring >> >>> > static >>> > > function in lower layer but defining it in higher one is skirting >>> >> around the >> >>> > > issue (I think AwHttpAuthHandlerBase has the same snag, but not a >>> >> reason to >> >>> > copy >>> > > it now we have better things to work with) >>> > > >>> > > for singleton-like objects, Bo's JNI dependency factory maybe the >>> >> cool new >> >>> > > solution to all these problems: >>> > > >>> > >>> >> >> https://codereview.chromium.**org/12253057/diff/23008/** >> android_webview/browser/jni_**dependency_factory.h<https://codereview.chromium.org/12253057/diff/23008/android_webview/browser/jni_dependency_factory.h> >> >>> > > >>> > > but I don't think that's the right fit here. you need the object >>> >> to be >> >>> > > per-webview (i.e. the AwContentsClient is a per webview relation) >>> > > >>> > > so I think the answer is, >>> > > 1) expand AwCertificateErrorHandlerBase to be the browser/ base >>> >> class for >> >>> all >>> > of >>> > > AwContentsClient. (we might call it AwContentsClient and have the >>> >> native/ >> >>> > > subclass be AwContentsClientImpl >>> > > 2) use WebContents::SetUserData to store a AwContentsClient* >>> >> inside the web >> >>> > > contents instance. (see AwContents for an example). (be carefull >>> >> to handle >> >>> the >>> > > AwContents::SetWebContents case too) >>> > > 3) here, use >>> > > >>> >> >> WebContents::FromId()->**GetUserData(kAwContentsClient)** >> ->allowCertificateError() >> >>> > > (we can make a static method AwContentsClient::FromID(), and >>> > > AwContentsClient::**FromWebContents, to hide some of that) >>> > > >>> > > >>> > > I can pair code that if it doesn't make sense. >>> > > >>> > >>> >> >> The static method was my suggestion - I believe at that time of our >>> >> conversation >> >>> the AwHttpAuthHandlerBase pattern was the only thing we had. Sorry for >>> misleading you there, but it looks like Joth's plan shouldn't be a big >>> >> pain to >> >>> apply. >>> >> >> https://codereview.chromium.**org/12091111/diff/31001/** >> android_webview/java/src/org/**chromium/android_webview/** >> AwContentsClientBridge.java<https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java> >> File >> android_webview/java/src/org/**chromium/android_webview/** >> AwContentsClientBridge.java >> (right): >> >> https://codereview.chromium.**org/12091111/diff/31001/** >> android_webview/java/src/org/**chromium/android_webview/** >> AwContentsClientBridge.java#**newcode36<https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java#newcode36> >> android_webview/java/src/org/**chromium/android_webview/** >> AwContentsClientBridge.java:**36: >> mNativeAwContentsClientBridge = nativeInit(nativeAwContents); >> On 2013/02/22 02:39:44, joth wrote: >> >>> suggest doing the native construction in AwContents (.cc) to keep it >>> >> symmetric >> >>> (i.e. create own and destroy all from same class, use scoped_ptr) >>> >> >> Done. >> >> >> https://codereview.chromium.**org/12091111/diff/31001/** >> android_webview/java/src/org/**chromium/android_webview/** >> AwContentsClientBridge.java#**newcode42<https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java#newcode42> >> android_webview/java/src/org/**chromium/android_webview/** >> AwContentsClientBridge.java:**42: >> mNativeAwContentsClientBridge = 0; >> On 2013/02/22 02:39:44, joth wrote: >> >>> On 2013/02/22 01:48:07, sgurun wrote: >>> > It is owned by AwContents, since we utilize it to access this class. >>> >> We can >> >>> come >>> > up with a different way if you want to. >>> > >>> > On 2013/02/22 01:15:45, joth wrote: >>> > > I'd expect to see nativeDestroy(**mNativeAwContentsClientBridge) >>> >> here... the >> >>> > java >>> > > object owns the native side? >>> > >>> >> >> Gotcha. So lets initiate the mNativeAwContentsClientBridge = 0; from a >>> CalledByNative method, invoked from the native side's destructor. That >>> >> way it's >> >>> sure to be zeroed however the native side dies. >>> >> >> Done. >> >> >> https://codereview.chromium.**org/12091111/diff/31001/** >> android_webview/java/src/org/**chromium/android_webview/** >> AwContentsClientBridge.java#**newcode81<https://codereview.chromium.org/12091111/diff/31001/android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java#newcode81> >> android_webview/java/src/org/**chromium/android_webview/** >> AwContentsClientBridge.java:**81: >> if (mClient != null) { >> good catch. This is an unnecessary check. mClient can't be null actually >> since we asserting in the constructor and then keeping a ref. >> >> On 2013/02/22 15:35:56, Martin Kosiba wrote: >> >>> if the client is null then shouldn't we return false? >>> >> >> https://codereview.chromium.**org/12091111/diff/31001/** >> android_webview/native/aw_**contents.cc<https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw_contents.cc> >> File android_webview/native/aw_**contents.cc (right): >> >> https://codereview.chromium.**org/12091111/diff/31001/** >> android_webview/native/aw_**contents.cc#newcode677<https://codereview.chromium.org/12091111/diff/31001/android_webview/native/aw_contents.cc#newcode677> >> android_webview/native/aw_**contents.cc:677: >> Java_AwContents_**onReceivedHttpAuthRequest(env, >> java_ref_.get(env).obj(), >> On 2013/02/22 02:39:44, joth wrote: >> >>> stash result in ScopedLocalRef and do null check. >>> >> >> Done. >> >> https://codereview.chromium.**org/12091111/<https://codereview.chromium.org/1... >> > >
PTAL, thanks
quick comments... please move the UserData class into the .cc in anon namespace. https://codereview.chromium.org/12091111/diff/39001/android_webview/browser/a... File android_webview/browser/aw_certificate_error_handler_base.cc (right): https://codereview.chromium.org/12091111/diff/39001/android_webview/browser/a... android_webview/browser/aw_certificate_error_handler_base.cc:46: if (!web_contents) return NULL; nit: you already have this check into GetContents. Don't need it in both places. https://codereview.chromium.org/12091111/diff/39001/android_webview/browser/a... File android_webview/browser/aw_certificate_error_handler_base.h (right): https://codereview.chromium.org/12091111/diff/39001/android_webview/browser/a... android_webview/browser/aw_certificate_error_handler_base.h:27: class AwCertificateErrorHandlerBase { I thought we agreed to expand this to being a AwContentClientBridge base-class? i.e. share the userdata plumbing for all browser layer up-calls. https://codereview.chromium.org/12091111/diff/39001/android_webview/browser/a... android_webview/browser/aw_certificate_error_handler_base.h:34: class UserData : public base::SupportsUserData::Data { Lets make this class internal to the .cc file. Just put (static) methods on here - Associate(WebContents*, AwContentClientBridge*); AwContentClientBridge* ForWebContents(WebContents*) AwContentClientBridge* ForId(int, int) https://codereview.chromium.org/12091111/diff/39001/android_webview/browser/a... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/12091111/diff/39001/android_webview/browser/a... android_webview/browser/aw_content_browser_client.cc:275: callback, cancel_request); nit: in c++ align the wrapped params with the ( rather than double indent. (like line 367) https://codereview.chromium.org/12091111/diff/39001/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/39001/android_webview/native/aw... android_webview/native/aw_contents.cc:221: contents_client_bridge_.get())); yeah this will be much cleaner as: AwCertificateErrorHandlerBase::Associate(web_contents_.get(), contents_client_bridge_.get());
thanks for quick review. PTAL, thanks, https://codereview.chromium.org/12091111/diff/39001/android_webview/browser/a... File android_webview/browser/aw_certificate_error_handler_base.cc (right): https://codereview.chromium.org/12091111/diff/39001/android_webview/browser/a... android_webview/browser/aw_certificate_error_handler_base.cc:46: if (!web_contents) return NULL; On 2013/02/22 23:12:41, joth wrote: > nit: you already have this check into GetContents. Don't need it in both places. Done. https://codereview.chromium.org/12091111/diff/39001/android_webview/browser/a... File android_webview/browser/aw_certificate_error_handler_base.h (right): https://codereview.chromium.org/12091111/diff/39001/android_webview/browser/a... android_webview/browser/aw_certificate_error_handler_base.h:27: class AwCertificateErrorHandlerBase { unintentional. just forgotten. On 2013/02/22 23:12:41, joth wrote: > I thought we agreed to expand this to being a AwContentClientBridge base-class? > i.e. share the userdata plumbing for all browser layer up-calls. https://codereview.chromium.org/12091111/diff/39001/android_webview/browser/a... android_webview/browser/aw_certificate_error_handler_base.h:34: class UserData : public base::SupportsUserData::Data { Sure. FromID is a already established name though, let's not invent new names. On 2013/02/22 23:12:41, joth wrote: > Lets make this class internal to the .cc file. > > Just put (static) methods on here - > > Associate(WebContents*, AwContentClientBridge*); > AwContentClientBridge* ForWebContents(WebContents*) > AwContentClientBridge* ForId(int, int) https://codereview.chromium.org/12091111/diff/39001/android_webview/browser/a... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/12091111/diff/39001/android_webview/browser/a... android_webview/browser/aw_content_browser_client.cc:275: callback, cancel_request); I thought this was acceptable, but then looking at the code conventions, it seems not. duh. On 2013/02/22 23:12:41, joth wrote: > nit: in c++ align the wrapped params with the ( rather than double indent. (like > line 367) https://codereview.chromium.org/12091111/diff/39001/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/39001/android_webview/native/aw... android_webview/native/aw_contents.cc:221: contents_client_bridge_.get())); On 2013/02/22 23:12:41, joth wrote: > yeah this will be much cleaner as: > AwCertificateErrorHandlerBase::Associate(web_contents_.get(), > contents_client_bridge_.get()); Done.
lg -except- I don't think this will build upstream. (I sent a try job for you to find out... :) https://codereview.chromium.org/12091111/diff/35003/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/12091111/diff/35003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:235: public abstract void onReceivedSslError(SslErrorHandler handler, SslError error); either make this non-abstract, or plan to land the downstream implementation of this (even just empty) before this side lands. https://codereview.chromium.org/12091111/diff/35003/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/12091111/diff/35003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:55: SslErrorHandler handler = new SslErrorHandler() { Oh... I don't think this will compile upstream :-/ the SslErrorHandler constructor is hidden. https://codereview.chromium.org/12091111/diff/35003/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/35003/android_webview/native/aw... android_webview/native/aw_contents.cc:686: jhost.obj(), jrealm.obj()); nit: indent to ( https://codereview.chromium.org/12091111/diff/35003/android_webview/native/aw... android_webview/native/aw_contents.cc:731: contents_client_bridge); ditto.
probably not. I did not sync to latest, since it is becoming a moving target... I was trying to put it into a shape first.
https://codereview.chromium.org/12091111/diff/42003/android_webview/browser/a... File android_webview/browser/aw_contents_client_bridge_base.h (right): https://codereview.chromium.org/12091111/diff/42003/android_webview/browser/a... android_webview/browser/aw_contents_client_bridge_base.h:24: // in the native/ layer, due to layering. The implementor of the base class does "browser/ layer interface for AwContentsClientsBridge, as DEPS prevents this layer from depending on native/ where the actual implementation lives." flow a bit better? https://codereview.chromium.org/12091111/diff/42003/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/SslUtil.java (right): https://codereview.chromium.org/12091111/diff/42003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslUtil.java:34: return new SslError(SslError.SSL_INVALID, cert, url); nit: would a switch look neater? https://codereview.chromium.org/12091111/diff/42003/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/42003/android_webview/native/aw... android_webview/native/aw_contents.cc:299: return; mm, this should probably return false and cancel the auth request in the caller? https://codereview.chromium.org/12091111/diff/42003/android_webview/native/aw... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/12091111/diff/42003/android_webview/native/aw... android_webview/native/aw_contents_client_bridge.cc:77: jboolean proceed, jint id) { nit: indent looks wrong here
PTAL, https://codereview.chromium.org/12091111/diff/42003/android_webview/browser/a... File android_webview/browser/aw_contents_client_bridge_base.h (right): https://codereview.chromium.org/12091111/diff/42003/android_webview/browser/a... android_webview/browser/aw_contents_client_bridge_base.h:24: // in the native/ layer, due to layering. The implementor of the base class yep. done. On 2013/02/25 12:16:38, benm wrote: > does "browser/ layer interface for AwContentsClientsBridge, as DEPS prevents > this layer from depending on native/ where the actual implementation lives." > > flow a bit better? https://codereview.chromium.org/12091111/diff/42003/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/SslUtil.java (right): https://codereview.chromium.org/12091111/diff/42003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslUtil.java:34: return new SslError(SslError.SSL_INVALID, cert, url); On 2013/02/25 12:16:38, benm wrote: > nit: would a switch look neater? Done. https://codereview.chromium.org/12091111/diff/42003/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12091111/diff/42003/android_webview/native/aw... android_webview/native/aw_contents.cc:299: return; On 2013/02/25 12:16:38, benm wrote: > mm, this should probably return false and cancel the auth request in the caller? Done. https://codereview.chromium.org/12091111/diff/42003/android_webview/native/aw... File android_webview/native/aw_contents_client_bridge.cc (right): https://codereview.chromium.org/12091111/diff/42003/android_webview/native/aw... android_webview/native/aw_contents_client_bridge.cc:77: jboolean proceed, jint id) { On 2013/02/25 12:16:38, benm wrote: > nit: indent looks wrong here Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sgurun@chromium.org/12091111/42007
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sgurun@chromium.org/12091111/43004
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sgurun@chromium.org/12091111/45001
Message was sent while issue was closed.
Change committed as 184643 |
