Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2386513002:
Implement getWebViewClient and getWebChromeClient (Closed)
|
Created:
4 years, 2 months ago by Nate Fischer Modified:
4 years, 2 months ago CC:
agrieve+watch_chromium.org, android-webview-reviews_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement getWebViewClient and getWebChromeClient
Implement getWebViewClient and getWebChromeClient for new APIs in Android
BUG=627248
Committed: https://crrev.com/ea0215bc645133a998e8b3e0f8cf49e609e7aaf3
Cr-Commit-Position: refs/heads/master@{#422183}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Return null instead of sNullWebViewClient #
Total comments: 2
Patch Set 3 : Fixing error #
Messages
Total messages: 23 (6 generated)
Description was changed from ========== Implement getWebViewClient and getWebChromeClient Implement getWebViewClient and getWebChromeClient for new APIs in Android BUG= ========== to ========== Implement getWebViewClient and getWebChromeClient Implement getWebViewClient and getWebChromeClient for new APIs in Android BUG=627248 ==========
ntfschr@chromium.org changed reviewers: + hush@chromium.org
PTAL
ntfschr@chromium.org changed reviewers: + sgurun@chromium.org
sgurun@: Here's the implementation of the changes. If you could review for owners approval, that would be awesome. hush@: I'm tagging you on this so that you can take a look at the implementation, since I'll add you as the reviewer for the API changes on the Android side.
lgtm
On 2016/09/30 at 01:57:08, hush wrote: > lgtm sgurun@: does this look good to you?
https://codereview.chromium.org/2386513002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java (right): https://codereview.chromium.org/2386513002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:175: return mWebViewClient; what does this return if no webviewclient is set?
https://codereview.chromium.org/2386513002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java (right): https://codereview.chromium.org/2386513002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:175: return mWebViewClient; On 2016/09/30 at 17:55:23, sgurun wrote: > what does this return if no webviewclient is set? It returns the sNullWebViewClient
https://codereview.chromium.org/2386513002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java (right): https://codereview.chromium.org/2386513002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:175: return mWebViewClient; On 2016/09/30 at 17:55:23, sgurun wrote: > what does this return if no webviewclient is set? This one gets initialized to sNullWebViewClient (a client created by the default constructor), and mWebChromeClient is initialized to null. Should we change these to be more consistent, or adjust documentation to emphasize the differences?
On 2016/09/30 18:02:09, Nate Fischer wrote: > https://codereview.chromium.org/2386513002/diff/1/android_webview/glue/java/s... > File > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java > (right): > > https://codereview.chromium.org/2386513002/diff/1/android_webview/glue/java/s... > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:175: > return mWebViewClient; > On 2016/09/30 at 17:55:23, sgurun wrote: > > what does this return if no webviewclient is set? > > This one gets initialized to sNullWebViewClient (a client created by the default > constructor), and mWebChromeClient is initialized to null. Should we change > these to be more consistent, or adjust documentation to emphasize the > differences? using sNullWebViewClient is an internal implementation detail. We should not expose this in a public API. another programmer could easily use a null value there and implement the default behavior by null checking.
On 2016/09/30 18:11:48, sgurun wrote: > On 2016/09/30 18:02:09, Nate Fischer wrote: > > > https://codereview.chromium.org/2386513002/diff/1/android_webview/glue/java/s... > > File > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java > > (right): > > > > > https://codereview.chromium.org/2386513002/diff/1/android_webview/glue/java/s... > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:175: > > return mWebViewClient; > > On 2016/09/30 at 17:55:23, sgurun wrote: > > > what does this return if no webviewclient is set? > > > > This one gets initialized to sNullWebViewClient (a client created by the > default > > constructor), and mWebChromeClient is initialized to null. Should we change > > these to be more consistent, or adjust documentation to emphasize the > > differences? > > using sNullWebViewClient is an internal implementation detail. We should not > expose this in a public API. another programmer could easily use a null value > there and implement the default behavior by null checking. basically return null if mwebviewclient is snullwebviewclient.
On 2016/09/30 at 18:12:52, sgurun wrote: > On 2016/09/30 18:11:48, sgurun wrote: > > On 2016/09/30 18:02:09, Nate Fischer wrote: > > > > > https://codereview.chromium.org/2386513002/diff/1/android_webview/glue/java/s... > > > File > > > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java > > > (right): > > > > > > > > https://codereview.chromium.org/2386513002/diff/1/android_webview/glue/java/s... > > > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:175: > > > return mWebViewClient; > > > On 2016/09/30 at 17:55:23, sgurun wrote: > > > > what does this return if no webviewclient is set? > > > > > > This one gets initialized to sNullWebViewClient (a client created by the > > default > > > constructor), and mWebChromeClient is initialized to null. Should we change > > > these to be more consistent, or adjust documentation to emphasize the > > > differences? > > > > using sNullWebViewClient is an internal implementation detail. We should not > > expose this in a public API. another programmer could easily use a null value > > there and implement the default behavior by null checking. > > basically return null if mwebviewclient is snullwebviewclient. Ok sounds good. I'll make the change here then.
On 2016/09/30 at 18:13:42, Nate Fischer wrote: > On 2016/09/30 at 18:12:52, sgurun wrote: > > On 2016/09/30 18:11:48, sgurun wrote: > > > On 2016/09/30 18:02:09, Nate Fischer wrote: > > > > > > > https://codereview.chromium.org/2386513002/diff/1/android_webview/glue/java/s... > > > > File > > > > > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java > > > > (right): > > > > > > > > > > > https://codereview.chromium.org/2386513002/diff/1/android_webview/glue/java/s... > > > > > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:175: > > > > return mWebViewClient; > > > > On 2016/09/30 at 17:55:23, sgurun wrote: > > > > > what does this return if no webviewclient is set? > > > > > > > > This one gets initialized to sNullWebViewClient (a client created by the > > > default > > > > constructor), and mWebChromeClient is initialized to null. Should we change > > > > these to be more consistent, or adjust documentation to emphasize the > > > > differences? > > > > > > using sNullWebViewClient is an internal implementation detail. We should not > > > expose this in a public API. another programmer could easily use a null value > > > there and implement the default behavior by null checking. > > > > basically return null if mwebviewclient is snullwebviewclient. > > Ok sounds good. I'll make the change here then. PTAL
https://codereview.chromium.org/2386513002/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java (right): https://codereview.chromium.org/2386513002/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:176: return mWebViewClient == sNullWebViewClient ? null : mWebView; you meant to return mWebViewCient?
https://codereview.chromium.org/2386513002/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java (right): https://codereview.chromium.org/2386513002/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:176: return mWebViewClient == sNullWebViewClient ? null : mWebView; On 2016/09/30 at 18:33:10, hush wrote: > you meant to return mWebViewCient? Whoops, fixed it
On 2016/09/30 18:40:21, Nate Fischer wrote: > https://codereview.chromium.org/2386513002/diff/20001/android_webview/glue/ja... > File > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java > (right): > > https://codereview.chromium.org/2386513002/diff/20001/android_webview/glue/ja... > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:176: > return mWebViewClient == sNullWebViewClient ? null : mWebView; > On 2016/09/30 at 18:33:10, hush wrote: > > you meant to return mWebViewCient? > > Whoops, fixed it lgtm
The CQ bit was checked by ntfschr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hush@chromium.org Link to the patchset: https://codereview.chromium.org/2386513002/#ps40001 (title: "Fixing error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Implement getWebViewClient and getWebChromeClient Implement getWebViewClient and getWebChromeClient for new APIs in Android BUG=627248 ========== to ========== Implement getWebViewClient and getWebChromeClient Implement getWebViewClient and getWebChromeClient for new APIs in Android BUG=627248 Committed: https://crrev.com/ea0215bc645133a998e8b3e0f8cf49e609e7aaf3 Cr-Commit-Position: refs/heads/master@{#422183} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ea0215bc645133a998e8b3e0f8cf49e609e7aaf3 Cr-Commit-Position: refs/heads/master@{#422183} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
