Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2366023002:
Add WebViewDatabaseAdapter#{get,set}HttpAuthUsernamePassword (Closed)
|
Created:
4 years, 2 months ago by Nate Fischer Modified:
4 years, 2 months ago CC:
android-webview-reviews_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd WebViewDatabaseAdapter#{get,set}HttpAuthUsernamePassword
Add these methods to WebViewDatabaseAdapter so that they can be moved to
Android's WebViewDatabase and the methods in WebView can be deprecated.
BUG=649816
Committed: https://crrev.com/a1d96e37797afcf6dec6e3967160e1f2f2ede198
Cr-Commit-Position: refs/heads/master@{#421029}
Patch Set 1 #Patch Set 2 : Removing methods in Awcontents #Patch Set 3 : Fixing errors #
Total comments: 4
Patch Set 4 : Modified TODOs #
Total comments: 1
Patch Set 5 : Removing overrides #Patch Set 6 : Add note about when to remove the old functions #
Total comments: 2
Patch Set 7 : Adding @Override back #Patch Set 8 : No-op for targeting O+ #
Total comments: 2
Patch Set 9 : Revert no-op #
Messages
Total messages: 24 (6 generated)
ntfschr@chromium.org changed reviewers: + boliu@chromium.org
ntfschr@chromium.org changed reviewers: + hush@chromium.org, sgurun@chromium.org - boliu@chromium.org
PTAL
Description was changed from
==========
Add WebViewDatabaseAdapter#{get,set}HttpAuthUsernamePassword
Add these methods to WebViewDatabaseAdapter so that they can be moved to
Android's WebViewDatabase and the methods in WebView can be deprecated.
BUG=616583
==========
to
==========
Add WebViewDatabaseAdapter#{get,set}HttpAuthUsernamePassword
Add these methods to WebViewDatabaseAdapter so that they can be moved to
Android's WebViewDatabase and the methods in WebView can be deprecated.
BUG=649816
==========
On 2016/09/23 22:43:27, Nate Fischer wrote: > PTAL it is generally a good idea to send some try jobs to verify what you did. I will let hush take a look too before stamping.
https://codereview.chromium.org/2366023002/diff/40001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewDatabaseAdapter.java (right): https://codereview.chromium.org/2366023002/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewDatabaseAdapter.java:85: // TODO(ntfschr): add @Override once this is added in Android (http://crbug.com/616583) Note that this can only happen after releasing the next major android version (O). https://codereview.chromium.org/2366023002/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (left): https://codereview.chromium.org/2366023002/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2021: if (TRACE) Log.i(TAG, "%s setHttpAuthUsernamePassword=%s", this, host); himm, so we are losing this trace. Not too important IMO. or we could add similar traces to WebViewDatabaseAdapter.
https://codereview.chromium.org/2366023002/diff/40001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewDatabaseAdapter.java (right): https://codereview.chromium.org/2366023002/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewDatabaseAdapter.java:85: // TODO(ntfschr): add @Override once this is added in Android (http://crbug.com/616583) On 2016/09/23 at 23:26:47, sgurun wrote: > Note that this can only happen after releasing the next major android version (O). I'll update that to be more explicit.
https://codereview.chromium.org/2366023002/diff/40001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/2366023002/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:376: ((WebViewDatabaseAdapter) mFactory.getWebViewDatabase(mContext)) why is casting necessary here?
On 2016/09/24 01:40:42, sgurun wrote: > https://codereview.chromium.org/2366023002/diff/40001/android_webview/glue/ja... > File > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java > (right): > > https://codereview.chromium.org/2366023002/diff/40001/android_webview/glue/ja... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:376: > ((WebViewDatabaseAdapter) mFactory.getWebViewDatabase(mContext)) > why is casting necessary here? Ah I see why :) yeah
https://codereview.chromium.org/2366023002/diff/60001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/2366023002/diff/60001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:364: @Override please remove override's here as we now can change the WebViewProvider interface and remove the not needed apis. Also add a note that these functions should continue to stay here as long as we support API 25 and below.
On 2016/09/24 at 01:45:18, sgurun wrote: > https://codereview.chromium.org/2366023002/diff/60001/android_webview/glue/ja... > File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): > > https://codereview.chromium.org/2366023002/diff/60001/android_webview/glue/ja... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:364: @Override > please remove override's here as we now can change the WebViewProvider interface and remove the not needed apis. Also add a note that these functions should continue to stay here as long as we support API 25 and below. Let me know how this note looks. If it needs to be modified, let me know and I can do that.
On 2016/09/24 02:04:55, Nate Fischer wrote: > On 2016/09/24 at 01:45:18, sgurun wrote: > > > https://codereview.chromium.org/2366023002/diff/60001/android_webview/glue/ja... > > File > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java > (right): > > > > > https://codereview.chromium.org/2366023002/diff/60001/android_webview/glue/ja... > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:364: > @Override > > please remove override's here as we now can change the WebViewProvider > interface and remove the not needed apis. Also add a note that these functions > should continue to stay here as long as we support API 25 and below. > > Let me know how this note looks. If it needs to be modified, let me know and I > can do that. that seems good. normally such a change in webviewchromium would break older Android versions since proguard would strip the methods after removing @Override. However we changed proguard to prevent it. lgtm please make sure to try your changes (good habit!) in both Android L and most recent Android master as we don't have super good test coverage (only tested by CTS tests). What you can do is write a simple webview app that uses these apis and then test it on a L device or run relevant CTS tests.. You should also modify the existing cts tests to use the new api (more on this later)
On 2016/09/24 02:16:23, sgurun wrote: > On 2016/09/24 02:04:55, Nate Fischer wrote: > > On 2016/09/24 at 01:45:18, sgurun wrote: > > > > > > https://codereview.chromium.org/2366023002/diff/60001/android_webview/glue/ja... > > > File > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java > > (right): > > > > > > > > > https://codereview.chromium.org/2366023002/diff/60001/android_webview/glue/ja... > > > > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:364: > > @Override > > > please remove override's here as we now can change the WebViewProvider > > interface and remove the not needed apis. Also add a note that these functions > > should continue to stay here as long as we support API 25 and below. > > > > Let me know how this note looks. If it needs to be modified, let me know and I > > can do that. > > that seems good. normally such a change in webviewchromium would break older > Android versions since proguard would strip the methods after removing > @Override. However we changed proguard to prevent it. > > lgtm > > please make sure to try your changes (good habit!) in both Android L and most > recent Android master as we don't have super good test coverage (only tested by > CTS tests). > What you can do is write a simple webview app that uses these apis and then test > it on a L device or run relevant CTS tests.. You should also modify the existing > cts tests to use the new api (more on this later) Actually, since we are not changing WebViewProvider, please add @Override again to WebViewChromium,
https://codereview.chromium.org/2366023002/diff/100001/android_webview/glue/j... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/2366023002/diff/100001/android_webview/glue/j... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:364: // TODO(ntfschr): remove these functions once API 25 and below is not supported I believe you can actually just remove this TODO, and add back @Override. It will be a very very long time before API 25 is not supported (probably never) We should, in principle, support all Android versions. https://codereview.chromium.org/2366023002/diff/100001/android_webview/glue/j... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:380: // TODO(ntfschr): remove these functions once API 25 and below is not supported same comment as above.
On 2016/09/26 at 17:29:20, hush wrote: > https://codereview.chromium.org/2366023002/diff/100001/android_webview/glue/j... > File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): > > https://codereview.chromium.org/2366023002/diff/100001/android_webview/glue/j... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:364: // TODO(ntfschr): remove these functions once API 25 and below is not supported > I believe you can actually just remove this TODO, and add back @Override. > It will be a very very long time before API 25 is not supported (probably never) > We should, in principle, support all Android versions. > > https://codereview.chromium.org/2366023002/diff/100001/android_webview/glue/j... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:380: // TODO(ntfschr): remove these functions once API 25 and below is not supported > same comment as above. I've put the @Overrides back in. PTAL
https://codereview.chromium.org/2366023002/diff/140001/android_webview/glue/j... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/2366023002/diff/140001/android_webview/glue/j... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:367: if (mAppTargetSdkVersion > Build.VERSION_CODES.N) return; I've implemented the no-op for O+ here. Please tell me if this is a mistake and I should change the approach. https://codereview.chromium.org/2366023002/diff/140001/android_webview/glue/j... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:384: if (mAppTargetSdkVersion > Build.VERSION_CODES.N) return null; Let me know if the no-op should return a non-null value (either here, or somewhere else), since this would likely crash the user app at runtime.
On 2016/09/26 at 20:29:38, Nate Fischer wrote: > https://codereview.chromium.org/2366023002/diff/140001/android_webview/glue/j... > File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): > > https://codereview.chromium.org/2366023002/diff/140001/android_webview/glue/j... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:367: if (mAppTargetSdkVersion > Build.VERSION_CODES.N) return; > I've implemented the no-op for O+ here. Please tell me if this is a mistake and I should change the approach. > > https://codereview.chromium.org/2366023002/diff/140001/android_webview/glue/j... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:384: if (mAppTargetSdkVersion > Build.VERSION_CODES.N) return null; > Let me know if the no-op should return a non-null value (either here, or somewhere else), since this would likely crash the user app at runtime. I've un-done the no-op. Let me know if there are any issues, otherwise I'll submit.
On 2016/09/26 21:18:56, Nate Fischer wrote: > On 2016/09/26 at 20:29:38, Nate Fischer wrote: > > > https://codereview.chromium.org/2366023002/diff/140001/android_webview/glue/j... > > File > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java > (right): > > > > > https://codereview.chromium.org/2366023002/diff/140001/android_webview/glue/j... > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:367: > if (mAppTargetSdkVersion > Build.VERSION_CODES.N) return; > > I've implemented the no-op for O+ here. Please tell me if this is a mistake > and I should change the approach. > > > > > https://codereview.chromium.org/2366023002/diff/140001/android_webview/glue/j... > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:384: > if (mAppTargetSdkVersion > Build.VERSION_CODES.N) return null; > > Let me know if the no-op should return a non-null value (either here, or > somewhere else), since this would likely crash the user app at runtime. > > I've un-done the no-op. Let me know if there are any issues, otherwise I'll > submit. you can submit
The CQ bit was checked by ntfschr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org Link to the patchset: https://codereview.chromium.org/2366023002/#ps160001 (title: "Revert no-op")
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 #9 (id:160001)
Message was sent while issue was closed.
Description was changed from
==========
Add WebViewDatabaseAdapter#{get,set}HttpAuthUsernamePassword
Add these methods to WebViewDatabaseAdapter so that they can be moved to
Android's WebViewDatabase and the methods in WebView can be deprecated.
BUG=649816
==========
to
==========
Add WebViewDatabaseAdapter#{get,set}HttpAuthUsernamePassword
Add these methods to WebViewDatabaseAdapter so that they can be moved to
Android's WebViewDatabase and the methods in WebView can be deprecated.
BUG=649816
Committed: https://crrev.com/a1d96e37797afcf6dec6e3967160e1f2f2ede198
Cr-Commit-Position: refs/heads/master@{#421029}
==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/a1d96e37797afcf6dec6e3967160e1f2f2ede198 Cr-Commit-Position: refs/heads/master@{#421029} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
