Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2330013002:
Save and restore state in the webview shell. (Closed)
|
Created:
4 years, 3 months ago by Tobias Sargeant Modified:
4 years, 3 months ago Reviewers:
timvolodine CC:
chromium-reviews, android-webview-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSave and restore state in the webview shell.
This exercises webview state save/restore, and also makes using the
shell as a testing/demonstration tool easier, because the current URL
is retained and reloaded when you switch webview providers.
BUG=
NOPRESUBMIT=true
Committed: https://crrev.com/12092c50879312843eec3282f506d33f8385e969
Cr-Commit-Position: refs/heads/master@{#418825}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Restructure #
Total comments: 3
Patch Set 3 : hide keyboard, just to be safe #
Total comments: 2
Patch Set 4 : Comment reloading code. #Messages
Total messages: 26 (8 generated)
tobiasjs@chromium.org changed reviewers: + timvolodine@chromium.org
https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:194: return; is it possible to just re-use the current code-path in this case? i.e. seems like it might be sufficient to have if (url == null) { mWebView.restoreState(savedInstanceState); url = mWebView.getUrl(); } and keep the rest the same..
https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:194: return; On 2016/09/12 14:34:21, timvolodine wrote: > is it possible to just re-use the current code-path in this case? > i.e. seems like it might be sufficient to have > > if (url == null) { > mWebView.restoreState(savedInstanceState); > url = mWebView.getUrl(); > } > > and keep the rest the same.. Although I uploaded a new PS, I just remembered what I was initially thinking: I think reload() has a couple of different properties to loadUrl(): 1) it preserves scroll position. 2) it doesn't add another history entry. Assuming I'm right, then I think that it might be better to go with PS1. WDYT?
https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:191: setUrlFail(false); this assumes the url did not fail previously.. https://codereview.chromium.org/2330013002/diff/20001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2330013002/diff/20001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:195: navigateTo(url); looks like this would break the case when there is a URISyntaxException later on. probably best to keep as before.
On 2016/09/12 14:59:49, Tobias Sargeant wrote: > https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... > File > android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java > (right): > > https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... > android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:194: > return; > On 2016/09/12 14:34:21, timvolodine wrote: > > is it possible to just re-use the current code-path in this case? > > i.e. seems like it might be sufficient to have > > > > if (url == null) { > > mWebView.restoreState(savedInstanceState); > > url = mWebView.getUrl(); > > } > > > > and keep the rest the same.. > > Although I uploaded a new PS, I just remembered what I was initially thinking: I > think reload() has a couple of different properties to loadUrl(): > > 1) it preserves scroll position. > 2) it doesn't add another history entry. > > Assuming I'm right, then I think that it might be better to go with PS1. WDYT? I think it would be good to keep the scroll position etc.. however in the code we have two special cases (maybe more) which would be skipped by patch1: - we check for FilePermissionRequest under some circumstances - case when there was a URISyntaxException or the page failed for some reason also, when changing providers seems 'safer' to reload the page?
https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:191: setUrlFail(false); On 2016/09/12 17:11:17, timvolodine wrote: > this assumes the url did not fail previously.. If the URL was badly formed, then loadUrl will not have been called. If it's well formatted, but some other error occurred, then presumably that error will be generated again on reload. https://codereview.chromium.org/2330013002/diff/20001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2330013002/diff/20001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:195: navigateTo(url); On 2016/09/12 17:11:17, timvolodine wrote: > looks like this would break the case when there is a URISyntaxException later > on. probably best to keep as before. I don't follow your logic here. The setUrlBarText(url); setUrlFail(false); is going to be done by navigateTo(url) if the URL is well formed, and if there's a URL parsing error, then setUrlFail(true) will be called. We miss the setUrlBarText(url) call in this case, but onPage{Started|Finished} set the url bar text to the loaded URL, so it's about to be set to a data: URL anyway.
https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:191: setUrlFail(false); On 2016/09/12 21:13:23, Tobias Sargeant wrote: > On 2016/09/12 17:11:17, timvolodine wrote: > > this assumes the url did not fail previously.. > > If the URL was badly formed, then loadUrl will not have been called. If it's > well formatted, but some other error occurred, then presumably that error will > be generated again on reload. I think my concern here was that if there was a URISyntaxException previously this would not set the setUrlFail to true on 'restore'. However it looks like onReceivedError would be called in some cases of malformed URIs, don't know if this covers all the malformed cases though.. (it probably should) https://codereview.chromium.org/2330013002/diff/20001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2330013002/diff/20001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:195: navigateTo(url); On 2016/09/12 21:13:23, Tobias Sargeant wrote: > On 2016/09/12 17:11:17, timvolodine wrote: > > looks like this would break the case when there is a URISyntaxException later > > on. probably best to keep as before. > > I don't follow your logic here. The setUrlBarText(url); setUrlFail(false); is > going to be done by navigateTo(url) if the URL is well formed, and if there's a > URL parsing error, then setUrlFail(true) will be called. We miss the > setUrlBarText(url) call in this case, but onPage{Started|Finished} set the url > bar text to the loaded URL, so it's about to be set to a data: URL anyway. right yes setUrlBarText was the issue here. it does look to be more 'spagetti' as that case with URIException relies on WebView.loadData being called, but since that case actually doesn't do what I intended it to do anymore (the url get overwritten by data:..) it doesn't seem to really matter. Don't know if it makes sense to maybe just add an extra setUrlBarText(url) in the catch URISyntaxException below for consistency.
https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:191: setUrlFail(false); On 2016/09/13 12:58:02, timvolodine wrote: > On 2016/09/12 21:13:23, Tobias Sargeant wrote: > > On 2016/09/12 17:11:17, timvolodine wrote: > > > this assumes the url did not fail previously.. > > > > If the URL was badly formed, then loadUrl will not have been called. If it's > > well formatted, but some other error occurred, then presumably that error will > > be generated again on reload. > > I think my concern here was that if there was a URISyntaxException previously > this would not set the setUrlFail to true on 'restore'. However it looks like > onReceivedError would be called in some cases of malformed URIs, don't know if > this covers all the malformed cases though.. (it probably should) Yeah, the behaviour here will be that it will restore the last valid URL. If you type something that's not a valid URL then that will be forgotten. But it seems like that's acceptable. Do you know what circumstances the hideKeyboard code is necessary? It seemed like just mWebView.requestFocus() was enough, but maybe that's not the case all the time?
https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2330013002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:191: setUrlFail(false); On 2016/09/13 13:24:53, Tobias Sargeant wrote: > On 2016/09/13 12:58:02, timvolodine wrote: > > On 2016/09/12 21:13:23, Tobias Sargeant wrote: > > > On 2016/09/12 17:11:17, timvolodine wrote: > > > > this assumes the url did not fail previously.. > > > > > > If the URL was badly formed, then loadUrl will not have been called. If it's > > > well formatted, but some other error occurred, then presumably that error > will > > > be generated again on reload. > > > > I think my concern here was that if there was a URISyntaxException previously > > this would not set the setUrlFail to true on 'restore'. However it looks like > > onReceivedError would be called in some cases of malformed URIs, don't know if > > this covers all the malformed cases though.. (it probably should) > > Yeah, the behaviour here will be that it will restore the last valid URL. If you > type something that's not a valid URL then that will be forgotten. But it seems > like that's acceptable. Do you know what circumstances the hideKeyboard code is > necessary? It seemed like just mWebView.requestFocus() was enough, but maybe > that's not the case all the time? It appears that without the hideKeyboard it will hang around for longer (which is counter-intuitive) and seems to get stuck on some websites e.g. cnn.com. In this case though it seems there would be no keyboard in the first place so maybe not relevant here.
> It appears that without the hideKeyboard it will hang around for longer (which > is counter-intuitive) and seems to get stuck on some websites e.g. http://cnn.com. In > this case though it seems there would be no keyboard in the first place so maybe > not relevant here. Ok. I think it's worth doing in that case, if it's possible for a website to (presumably) reject focus. Before I added the mWebView.requestFocus() call, the reload would end up with the url bar focussed and the keyboard displayed, so it's definitely necessary to do something in this case. Anyway. Going back to PS1 (so we get the reload behaviour) with the addition of hideKeyboard. Are you ok with this?
https://codereview.chromium.org/2330013002/diff/40001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2330013002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:193: mWebView.reload(); nit: maybe add some comments here regarding why this is a 'special' case, e.g. you mentioned scrollposition, history etc..
On 2016/09/14 10:08:56, Tobias Sargeant wrote: > > It appears that without the hideKeyboard it will hang around for longer (which > > is counter-intuitive) and seems to get stuck on some websites e.g. > http://cnn.com. In > > this case though it seems there would be no keyboard in the first place so > maybe > > not relevant here. > > Ok. I think it's worth doing in that case, if it's possible for a website to > (presumably) reject focus. Before I added the mWebView.requestFocus() call, the > reload would end up with the url bar focussed and the keyboard displayed, so > it's definitely necessary to do something in this case. > > Anyway. Going back to PS1 (so we get the reload behaviour) with the addition of > hideKeyboard. Are you ok with this? sounds good, lgtm for PS3 (which is the return to PS1). thanks!
https://codereview.chromium.org/2330013002/diff/40001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java (right): https://codereview.chromium.org/2330013002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:193: mWebView.reload(); On 2016/09/14 11:40:47, timvolodine wrote: > nit: maybe add some comments here regarding why this is a 'special' case, e.g. > you mentioned scrollposition, history etc.. Done.
The CQ bit was checked by tobiasjs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timvolodine@chromium.org Link to the patchset: https://codereview.chromium.org/2330013002/#ps60001 (title: "Comment reloading code.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Save and restore state in the webview shell. This exercises webview state save/restore, and also makes using the shell as a testing/demonstration tool easier, because the current URL is retained and reloaded when you switch webview providers. BUG= ========== to ========== Save and restore state in the webview shell. This exercises webview state save/restore, and also makes using the shell as a testing/demonstration tool easier, because the current URL is retained and reloaded when you switch webview providers. BUG= NOPRESUBMIT=true ==========
The CQ bit was checked by tobiasjs@chromium.org
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.
Description was changed from ========== Save and restore state in the webview shell. This exercises webview state save/restore, and also makes using the shell as a testing/demonstration tool easier, because the current URL is retained and reloaded when you switch webview providers. BUG= NOPRESUBMIT=true ========== to ========== Save and restore state in the webview shell. This exercises webview state save/restore, and also makes using the shell as a testing/demonstration tool easier, because the current URL is retained and reloaded when you switch webview providers. BUG= NOPRESUBMIT=true ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Save and restore state in the webview shell. This exercises webview state save/restore, and also makes using the shell as a testing/demonstration tool easier, because the current URL is retained and reloaded when you switch webview providers. BUG= NOPRESUBMIT=true ========== to ========== Save and restore state in the webview shell. This exercises webview state save/restore, and also makes using the shell as a testing/demonstration tool easier, because the current URL is retained and reloaded when you switch webview providers. BUG= NOPRESUBMIT=true Committed: https://crrev.com/12092c50879312843eec3282f506d33f8385e969 Cr-Commit-Position: refs/heads/master@{#418825} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/12092c50879312843eec3282f506d33f8385e969 Cr-Commit-Position: refs/heads/master@{#418825} |
