Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2257743002:
Restore user state on ReloadwithoutSubResourceCacheRevalidation (Closed)
|
Created:
4 years, 4 months ago by Takashi Toyoshima Modified:
4 years, 3 months ago CC:
allada, blink-reviews, chromium-reviews, gavinp+loader_chromium.org, Nate Chapin, loading-reviews_chromium.org, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRestore user state on ReloadwithoutSubResourceCacheRevalidation
Currently, Chrome does not restore user state, scroll position
and scale factor for same page navigation regardless of
NonValidatingReloadOnNormalReload content feature flag.
This patch connects NonValidatingReloadOnNormalReload flag
to ReloadwithoutSubResourceCacheRevalidation blink feature flag,
and changes the behavior to restore user state even for the same
page navigation if ReloadwithoutSubResourceCacheRevalidation
flag is enabled.
BUG=632358
Committed: https://crrev.com/2a56f872fc3e82ccd819db4378ae2e1ac206c116
Cr-Commit-Position: refs/heads/master@{#416855}
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : Fix webkit_unit_tests #
Total comments: 1
Patch Set 4 : fix WebView test side not to rely on same page navigation behavior #Patch Set 5 : plumb a runtime flag #
Total comments: 11
Patch Set 6 : Addressing Torne's comments at #37 #
Total comments: 2
Patch Set 7 : remove TODO and rename blink flag #Patch Set 8 : [rebase] #
Messages
Total messages: 77 (32 generated)
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
IIRC, this will make some tests for WebView fail, and need some extra changes to submit. Let me check it by running git cl try.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Ok, this error of linux_android_rel_ng looks one I saw before.
I'd investigate when happens.
failures:
org.chromium.android_webview.test.AwSettingsTest#testLoadWithOverviewModeWithTwoViews
org.chromium.android_webview.test.AwSettingsTest#testLoadWithOverviewModeViewportTagWithTwoViews
org.chromium.android_webview.test.AwSettingsTest#testLoadWithOverviewModeWithTwoViews
with {--webview-sandboxed-renderer}
org.chromium.android_webview.test.AwSettingsTest#testLoadWithOverviewModeViewportTagWithTwoViews
with {--webview-sandboxed-renderer}
When I run these tests on my local machine, they fails with assertion failures, but on trybots it failed with a timeout. But, when I remove assertion checkes and run on my local machine, they also fails with a timeout. So, I guess tests are just broken from the beginning.
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Restore scroll position on NonValidatingReload Now, Chrome does not restore scroll positions for same page navigation and NonValidatingReload, or the new reload behavior in a field study. This patch changes the behavior to restore scroll positions even for the same navigation and NonValidatingReload if the feature is enabled. BUG=632358 ========== to ========== Restore scroll position on NonValidatingReload Now, Chrome does not restore scroll positions for same page navigation and NonValidatingReload, or the new reload behavior in a field study. This patch changes the behavior to restore scroll positions even for the same navigation and NonValidatingReload if the feature is enabled. BUG=632358 ==========
toyoshim@chromium.org changed reviewers: + bokan@chromium.org
bokan: can you review this change? you looks a viewport expert, and might be familiar with WebView's overview mode.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
ok, I should check the webkit unit tests first.
Overview mode is enabled by default in Blink https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Set... But Android document says it's disabled by default. https://developer.android.com/reference/android/webkit/WebSettings.html#setLo... Android WebView actually disabled it by default from here. https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/an... But for Chrome this is enabled by default from here. https://cs.chromium.org/chromium/src/content/public/common/web_preferences.cc So, I guess it's ok to flip the flag in Settings.in because it's always initialized explicitly by embedders.
Flipping flag makes more tests fail. I'll find another way.
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
toyoshim@chromium.org changed reviewers: + majidvp@chromium.org
+majidvp@chromium.org The Patch Set 3 passes all existing tests. I'd submit this if this is fine in terms of overview mode's expectation.
https://codereview.chromium.org/2257743002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2257743002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1200: bool shouldRestoreScale = m_currentItem->pageScaleFactor() && !(m_frame->page()->settings().loadWithOverviewMode() && isReloadLoadType(m_loadType)); In terms of UI design policy to handle scroll position and scale, this is a wrong fix. To achieve the project goal to remove minor reload variants, we may want to just restore the scale here even for overview mode, and to modify failed WebView test.
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
majidvp@, can you review the patch set 5? I discuss this at the linked crbug, but in short; - I am removing reload variants as possible - To achieve this goal, I will remove minor differences between reload and same page navigation. This means I will restore scale and scroll position even for the same page navigation - AwSettingsTest for overview mode relies on same page navigation's behavior, and should be modified to be free from noise
Description was changed from ========== Restore scroll position on NonValidatingReload Now, Chrome does not restore scroll positions for same page navigation and NonValidatingReload, or the new reload behavior in a field study. This patch changes the behavior to restore scroll positions even for the same navigation and NonValidatingReload if the feature is enabled. BUG=632358 ========== to ========== Restore scroll position on NonValidatingReload Now, Chrome does not restore scroll positions for same page navigation and NonValidatingReload if the feature is enabled. This patch changes the behavior to restore scroll positions even for the same navigation and NonValidatingReload if the feature is enabled. BUG=632358 ==========
Description was changed from ========== Restore scroll position on NonValidatingReload Now, Chrome does not restore scroll positions for same page navigation and NonValidatingReload if the feature is enabled. This patch changes the behavior to restore scroll positions even for the same navigation and NonValidatingReload if the feature is enabled. BUG=632358 ========== to ========== Restore user state on NonValidatingReload Now, Chrome does not restore user state, scroll position and scale factor for same page navigation. This is same for NonValidatingReload if the feature is enabled. This patch changes this behavior to restore user state even for the same navigation and NonValidatingReload if the feature is enabled. BUG=632358 ==========
Nit in description: "Now, Chrome does not restore..." replace with "Currently, Chrome does not restore...". Now can be read as "with this patch". https://codereview.chromium.org/2257743002/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java (right): https://codereview.chromium.org/2257743002/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2663: // on KitKat. It will work on SDK's emulator for both ARM and x86, too. Why does the Android version have an effect here? In general, I don't think we can say "run it on KitKat" since the waterfall builders will have a fixed version of Android.
Description was changed from ========== Restore user state on NonValidatingReload Now, Chrome does not restore user state, scroll position and scale factor for same page navigation. This is same for NonValidatingReload if the feature is enabled. This patch changes this behavior to restore user state even for the same navigation and NonValidatingReload if the feature is enabled. BUG=632358 ========== to ========== Restore user state on NonValidatingReload Currently, Chrome does not restore user state, scroll position and scale factor for same page navigation. This is same for NonValidatingReload if the feature is enabled. This patch changes this behavior to restore user state even for the same navigation and NonValidatingReload if the feature is enabled. BUG=632358 ==========
Description was fixed. https://codereview.chromium.org/2257743002/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java (right): https://codereview.chromium.org/2257743002/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2663: // on KitKat. It will work on SDK's emulator for both ARM and x86, too. I actually quite not sure why this can not run on other recent Android versions, but from the beginning, these test can run on M and N. I spent a long time to find a way to run these tests on local machine, and that's why I want to have this note for other developers to save their time. This is the thread I ask android-webview-dev@ about this; https://groups.google.com/a/chromium.org/d/topic/android-webview-dev/ZG7i6abn... In short, answer was: > Unfortunately android tests are quriky. Most of the time when things don't reproduce locally, it's due to some difference in configuration. So to start, the bot that failed use nexus 5 (hammerhead) devices, running kitkat build KTU84P. Can you try running your test again in that setup and see if it reproduces? And I figure out that these tests run only on KitKat.
toyoshim@chromium.org changed reviewers: + tkent@chromium.org, torne@chromium.org
+torne@ for android_webview OWNERS +tkent@ for WebKit OWNERS
It was hard for me to understand the CL description. Is the following correct? > Currently, Chrome does not restore user state, scroll position > and scale factor for same page navigation. > This is same for NonValidatingReload if the feature is enabled. ... same page navigation regardless of NonValidatingReloadOnNormalReload feature flag. > This patch changes this behavior to restore user state even for > the same navigation and NonValidatingReload if the feature is > enabled. ... same navigation if NonValidatingReloadOnNormalReload feature is enabled.
https://codereview.chromium.org/2257743002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2257743002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:133: NonValidatingReload -> NonValidatingReloadOnNormalReload https://codereview.chromium.org/2257743002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebRuntimeFeatures.h (right): https://codereview.chromium.org/2257743002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/web/WebRuntimeFeatures.h:89: BLINK_EXPORT static void enableNonValidatingReload(bool); We should use consistent feature name. enableNonValidatingReloadOnNormalReload(bool) is preferable.
https://codereview.chromium.org/2257743002/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java (right): https://codereview.chromium.org/2257743002/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1360: + " --></body></html>"; Could you explain why this is necessary? I don't entirely follow the context for this change. If you have to change the test this implies you're making a visible behaviour change for WebView and it's possible apps may rely on the old behaviour. It looks like the change here is going to cause a different data: URL to get loaded for the test each time - so presumably loading the same data: URL was triggering this logic you've added, but what's the actual observable effect there for the app? Just the scroll position not being reset? Or something else as well? https://codereview.chromium.org/2257743002/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2663: // on KitKat. It will work on SDK's emulator for both ARM and x86, too. This comment is liable to be outdated and generally it's not good that the tests don't work across all devices. We're in the process of testing WebView on more OS versions. If you have a reproducible failure, file a bug about it, don't just warn people off.
Description was changed from ========== Restore user state on NonValidatingReload Currently, Chrome does not restore user state, scroll position and scale factor for same page navigation. This is same for NonValidatingReload if the feature is enabled. This patch changes this behavior to restore user state even for the same navigation and NonValidatingReload if the feature is enabled. BUG=632358 ========== to ========== Restore user state on NonValidatingReload Currently, Chrome does not restore user state, scroll position and scale factor for same page navigation regardless of NonValidatingReloadOnNormalReload feature flag. This patch changes this behavior to restore user state even for the same navigation if NonValidatingReloadOnNormalReload feature is enabled. BUG=632358 ==========
Description was changed from ========== Restore user state on NonValidatingReload Currently, Chrome does not restore user state, scroll position and scale factor for same page navigation regardless of NonValidatingReloadOnNormalReload feature flag. This patch changes this behavior to restore user state even for the same navigation if NonValidatingReloadOnNormalReload feature is enabled. BUG=632358 ========== to ========== Restore user state on NonValidatingReload Currently, Chrome does not restore user state, scroll position and scale factor for same page navigation regardless of NonValidatingReloadOnNormalReload feature flag. This patch changes the behavior to restore user state even for the same navigation if NonValidatingReloadOnNormalReload feature is enabled. BUG=632358 ==========
Description is updated and reflect tkent's suggestion at #35.
This is answer for #36 by tkent. https://codereview.chromium.org/2257743002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2257743002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:133: NonValidatingReload I have some thoughts about this. Chrome has two feature flags around NonValidatingReload. One is NonValidatingReloadOnRefreshContent, the other is NonValidatingReloadOnNormalReload. They means a flag to enable NonValidatingReload against RefreshContent UI, and a flag to enable NonValidatingReload against NormalReload UI. Since Blink should not be aware of such embedder side user actions that triggers the feature, I chose NonValidatingReload for this flag name. Probably, I should also flip this flag for NonValidatingReloadOnRefreshContent. But checking the field study flags has complicated side effects and I need to be too careful to enable a Blink feature for two field trial flags to be statically correct. Here OnRefreshContent case does not matter so important. That's why I just ignore OnRefreshContent cases as a practical solution. Does this make sense? I do not stick this name, but I'd hear your opinion about this background.
https://codereview.chromium.org/2257743002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2257743002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:133: NonValidatingReload On 2016/08/31 at 05:57:02, toyoshim wrote: > I have some thoughts about this. > > Chrome has two feature flags around NonValidatingReload. > One is NonValidatingReloadOnRefreshContent, the other is NonValidatingReloadOnNormalReload. > They means a flag to enable NonValidatingReload against RefreshContent UI, and a flag to enable NonValidatingReload against NormalReload UI. > > Since Blink should not be aware of such embedder side user actions that triggers the feature, I chose NonValidatingReload for this flag name. > > Probably, I should also flip this flag for NonValidatingReloadOnRefreshContent. But checking the field study flags has complicated side effects and I need to be too careful to enable a Blink feature for two field trial flags to be statically correct. Here OnRefreshContent case does not matter so important. That's why I just ignore OnRefreshContent cases as a practical solution. > > Does this make sense? I do not stick this name, but I'd hear your opinion about this background. Ok, I understand you'd like to add a Blink flag different from the Chromium flags. However, the flag name 'NonValidatingReload' is still unclear. I haven't seen the word 'validate' in the Blink loader area. What is (not) validated in this context?
https://codereview.chromium.org/2257743002/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java (right): https://codereview.chromium.org/2257743002/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1360: + " --></body></html>"; Yes, my change intended to update the data URL for each request so that it always triggers a standard navigation instead of the same page navigation. When we turn on the new reload feature, the same page navigation will be changed to restore scroll position and scale factor those are defined as a 'user state'. This test checks if changing overview mode setting affects immediately in the next load. Tests work today because the same page navigation does not restore the user state. But once we turn on the new reload feature, scale factor is restored, and results in test failures. The same page navigation is a kind of weak variant of reloads, but scale factor and scroll position are reset today even though hard reload restores them. This is inconsistent behavior, and what I want to remove from Blink to simplify reload. https://codereview.chromium.org/2257743002/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2663: // on KitKat. It will work on SDK's emulator for both ARM and x86, too. Agreed. Filing a bug sounds a right solution here. https://bugs.chromium.org/p/chromium/issues/detail?id=642636 Also I will replace this note with a TODO(crbug.com/642636).
https://codereview.chromium.org/2257743002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2257743002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:133: NonValidatingReload On 2016/08/31 at 06:30:33, tkent wrote: > Ok, I understand you'd like to add a Blink flag different from the Chromium flags. > However, the flag name 'NonValidatingReload' is still unclear. I haven't seen the word 'validate' in the Blink loader area. What is (not) validated in this context? cache validation?
> Ok, I understand you'd like to add a Blink flag different from the Chromium > flags. > However, the flag name 'NonValidatingReload' is still unclear. I haven't seen > the word 'validate' in the Blink loader area. What is (not) validated in this > context? In the loader or fetch directory, we use 'revalidate' or 'revalidation' to mention this. On reload today, we revalidate all main and sub resources. This flag change this to revalidate only main resource. Sorry, it's super hard to use single consistent word from chrome through content and blink :/
On 2016/08/31 at 06:39:18, toyoshim wrote: > > Ok, I understand you'd like to add a Blink flag different from the Chromium > > flags. > > However, the flag name 'NonValidatingReload' is still unclear. I haven't seen > > the word 'validate' in the Blink loader area. What is (not) validated in this > > context? > > In the loader or fetch directory, we use 'revalidate' or 'revalidation' to mention this. > On reload today, we revalidate all main and sub resources. This flag change this to revalidate only main resource. > Sorry, it's super hard to use single consistent word from chrome through content and blink :/ Let's use Blink terms for a Blink flag. ReloadWithoutSubResourceRevalidation?
On 2016/08/31 06:31:19, toyoshim wrote: > https://codereview.chromium.org/2257743002/diff/80001/android_webview/javates... > File > android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java > (right): > > https://codereview.chromium.org/2257743002/diff/80001/android_webview/javates... > android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1360: > + " --></body></html>"; > Yes, my change intended to update the data URL for each request so that it > always triggers a standard navigation instead of the same page navigation. > > When we turn on the new reload feature, the same page navigation will be changed > to restore scroll position and scale factor those are defined as a 'user state'. > > This test checks if changing overview mode setting affects immediately in the > next load. Tests work today because the same page navigation does not restore > the user state. But once we turn on the new reload feature, scale factor is > restored, and results in test failures. > > The same page navigation is a kind of weak variant of reloads, but scale factor > and scroll position are reset today even though hard reload restores them. This > is inconsistent behavior, and what I want to remove from Blink to simplify > reload. OK, but my point is that WebView-using apps might be relying on this behaviour, and would be surprised to see the scale factor and scroll position not resetting when they load their page again. Is it just those two things that are affected? There's no way for a developer to explicitly reset the webview scale factor to the default, unfortunately (though they can scroll back to 0,0 just fine). I'd rather not diverge from Chrome's behaviour if possible, but if developers don't have an alternative way to accomplish the same thing this might be a problem (depending whether apps really are relying on this and whether it has a significant effect on their UI or not). > https://codereview.chromium.org/2257743002/diff/80001/android_webview/javates... > android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2663: > // on KitKat. It will work on SDK's emulator for both ARM and x86, too. > Agreed. Filing a bug sounds a right solution here. > https://bugs.chromium.org/p/chromium/issues/detail?id=642636 > > Also I will replace this note with a TODO(crbug.com/642636).
> Let's use Blink terms for a Blink flag. ReloadWithoutSubResourceRevalidation? OK. Also, I missed your last question. Yes, it's cache (re)validation in this context. So, probably, ReloadwithoutSubResourceCacheRevalidation would be better?
> OK. Also, I missed your last question. Yes, it's cache (re)validation in this context. > So, probably, ReloadwithoutSubResourceCacheRevalidation would be better? SGTM
> OK, but my point is that WebView-using apps might be relying on this behaviour, > and would be surprised to see the scale factor and scroll position not resetting > when they load their page again. Is it just those two things that are affected? We have ran a field trial for months that changes normal reload behavior to be same behavior with the same page navigation. That means I didn't change anything about the same page navigation until now, and this is the first change to modify the same page navigation side to be aligned with each other. That's everything I can say. I could miss something. But, yes, I believe this is only behavior change for the same page navigation. > There's no way for a developer to explicitly reset the webview scale factor to > the default, unfortunately (though they can scroll back to 0,0 just fine). In terms of whatwg spec, setting History.scrollRestoration to "manual" is the right way to be free from browser's user state restoration. The name is scrollRestoration for a historical reason, but it should affect for all users state including scale factor. If this does not work, I'm happy to try fixing it. What do you think? If this is clearly defined in the spec or API document, we may need to consider something more. Blink has a clear process to change or deprecate spec defined behavior. So if this is a spec matter, I'd follow the process. Otherwise, I'm not sure how much we should pay attention to contents that rely on undocumented behaviors. This change does not modify anything on the default path, but change alternative path that are controlled by our field trial infra. So probably, we can move this forward, and monitor what happens in the trial. My only concern about this is I'm not sure if the field trial also can run on WebView.
I checked scrollRestoration behavior at http://yuri.twintail.org/chrome/reload/scroll_restoration.html It works for scroll position, but does not for scale factor. Let me file a separate bug to discuss it.
WebView sounds like expecting navigating to the same URL just works as new page navigation, but spec does not expect it. Before I move the scrollRestoration approach forward, let me check the navigation spec again. I'll write updates here later.
On 2016/09/01 08:25:09, toyoshim wrote: > > OK, but my point is that WebView-using apps might be relying on this > behaviour, > > and would be surprised to see the scale factor and scroll position not > resetting > > when they load their page again. Is it just those two things that are > affected? > > We have ran a field trial for months that changes normal reload behavior to be > same behavior with the same page navigation. > That means I didn't change anything about the same page navigation until now, > and this is the first change to modify the same page navigation side to be > aligned with each other. That's everything I can say. I could miss something. > But, yes, I believe this is only behavior change for the same page navigation. Field trials don't apply to WebView.. > > There's no way for a developer to explicitly reset the webview scale factor to > > the default, unfortunately (though they can scroll back to 0,0 just fine). > > In terms of whatwg spec, setting History.scrollRestoration to "manual" is the > right way to be free from browser's user state restoration. > The name is scrollRestoration for a historical reason, but it should affect for > all users state including scale factor. > If this does not work, I'm happy to try fixing it. > > What do you think? I'm not sure how this works, but the app developer is not necessarily in control of the content; they can evaluate javascript in the webview, but it seems somewhat unfortunate to have to run JS code before a navigation to make the navigation have the intended effect :/ > If this is clearly defined in the spec or API document, we may need to consider > something more. > Blink has a clear process to change or deprecate spec defined behavior. So if > this is a spec matter, I'd follow the process. > Otherwise, I'm not sure how much we should pay attention to contents that rely > on undocumented behaviors. WebView does not have well-defined/documented behaviours, and we generally try to remain compatible with past behaviour where reasonable. When there's a good reason to change we can try, but we have to be prepared to back out changes and revisit things if they break a significant number of existing applications. > This change does not modify anything on the default path, but change alternative > path that are controlled by our field trial infra. > So probably, we can move this forward, and monitor what happens in the trial. My > only concern about this is I'm not sure if the field trial also can run on > WebView. WebView doesn't have field trials, so you won't get any information until this ships by default. We also have a very small beta population and effectively no dev/canary, so to really examine the impact of a change that may only affect a relatively small proportion of apps it often has to be shipped to stable, at which point reverting the change requires another full stable update (since we don't have the field trial framework to flip killswitches). I didn't realise this change wasn't actually turning the feature on, so we don't strictly need to block this on resolving the question here. The actual change to the test LGTM - I'm just concerned that the fact that one of our own tests just happened to rely on the current behaviour means that other apps might too.
On 2016/09/01 10:01:45, toyoshim wrote: > WebView sounds like expecting navigating to the same URL just works as new page > navigation, but spec does not expect it. WebView doesn't "expect" anything in particular. The documentation says nothing about this. Developers often assume that the behaviour they see now won't change. > Before I move the scrollRestoration approach forward, let me check the > navigation spec again. > I'll write updates here later. Could you explain a bit more how an app would actually use this for pages where it doesn't control the content?
> > There's no way for a developer to explicitly reset the webview scale factor to > > the default, unfortunately (though they can scroll back to 0,0 just fine). > > In terms of whatwg spec, setting History.scrollRestoration to "manual" is the > right way to be free from browser's user state restoration. > The name is scrollRestoration for a historical reason, but it should affect for > all users state including scale factor. > If this does not work, I'm happy to try fixing it. > > What do you think? This is actually not accurate. The spec is explicitly silent about if the scale needs to be restored or not so it is up to the user agent. In fact, in Blink implementation of history.scrollRestoration, we made a conscious choice that scrollRestoration='manual' should only disable scroll position restoration and should *not* disable scale restoration. (See https://codereview.chromium.org/1277583003). This was decided because there is currently no way for developers to restore scale themselves so not restoring scale would penalize developers which are using scrollRestoration='manual' to implement their own restoration logic which is the recommended approach. In any case, the spec wording is such that it does not prescribe anything on scale restoration and only talks about scroll so we can change our implementation if need be though I don't think you need/want to do that for you change so I think we should keep that behaviour as is. See full rationale here: https://groups.google.com/a/chromium.org/forum/#!msg/blink- dev/p1sy9aQDmtU/jndSickDCAAJ So history.scrollRestoration is not the solution if they need to reset scale! > I'm not sure how this works, but the app developer is not necessarily in control of the content; they can evaluate javascript in the webview, but it seems somewhat unfortunate to have to run JS code before a navigation to make the navigation have the intended effect :/ Well, even if they have the option to run JS before page reload it will not do what is desired. I don't have a lot of knowledge about webview but it seems that we want some way for webview users to indicate kind reload they are trying to have which will determine the scroll/scale restoration behaviour.
On 2016/09/01 19:20:26, majidvp wrote: > > I'm not sure how this works, but the app developer is not necessarily in > control > of the content; they can evaluate javascript in the webview, but it seems > somewhat unfortunate to have to run JS code before a navigation to make the > navigation have the intended effect :/ > > Well, even if they have the option to run JS before page reload it will not > do what is desired. > > I don't have a lot of knowledge about webview but it seems that we want > some way for webview users to indicate kind reload they are trying to have > which will determine the scroll/scale restoration behaviour. We also have no way to add WebView API surfaces retroactively - existing android OS versions have the API they have, and we can't change it. So if there's no way for the app to explicitly reset the scroll/scale correctly, then as far as WebView is concerned here, there's only two real options: 1) Go ahead and land/enable this feature as planned. Any WebView app which happens to load the same URL multiple times in a row will see the new behaviour where scale and scroll position persist across loads. This may or may not be a problem for them, depending what their app is actually doing. If they don't like the new behaviour, then the only suggestion we can really give them is to load about:blank inbetween to prevent it being treated as a reload. 2) Put in some kind of flag/pref to keep the old behaviour, and have WebView set this flag/pref. The behaviour will then diverge between WebView and Chrome. We could treat this as a compatibility flag, which would mean that we can set it up so that applications which target the next version of Android (since 7.0 was just released, it'd have to be whatever version is *after that*, in the future) will get the "new" chrome-compatible behaviour, and apps which target earlier versions will continue to get the compatibility behaviour. This would have to be preserved pretty much indefinitely; there'd be no particular time horizon where we could remove the compatibility behaviour. This doesn't mean we *have* to do 2). It's preferable for WebView to match Chrome's behaviour, in general, and not have weird compatibility quirks like this, but it always just depends on how many apps end up being impacted negatively by the change and how easy it is for the apps to fix/work around it in their code. Having to load about:blank is a bit of a gross workaround here - they might actually have to wait for the navigation to commit before they load the real page (not sure) and if so the code will be pretty fiddly. A better workaround would make this a better option..
majidvp: Thank you for sharing background around the scale factor restoration. If the spec delegates decision making of scale factor restoration to user agents, it sounds the best that blink has an API for embedders, but hum.... WebView cannot as Torne explained. My proposed workaround assumed only the case a developer used WebView to show contents under their control. I thought if they used WebView just to show open web contents, same behaviors with other agents would be preferable to minimize compatibility issues. I checked navigation spec carefully, but actually I could not find any explicit definition for the same page navigation. (I made a thread to ask about this; https://github.com/whatwg/html/issues/1742) But modern browsers handle it almost as a normal reload. For instance, user agents never create a new history entry for the same page navigation, and does not remove successor entries even if it happens in the middle of back/forward navigation. But scroll position isn't restored that is only difference from normal reload. Torne proposed two real options. But having a flag to keep this old behavior for the solution 2) does not help to simplify code to handle reload variants, and will need to keep another reload variant. So, from viewpoint of my goal, I prefer 1) unless this breaks the web. To make the option 1) more reasonable, let me propose another workaround that uses a query string. Similar to my test change, using unique URL can avoid the same page navigation. That's something like https://foo.bar/baz?seq=<sequence number>. Here I also assume only contents under developer's control. Also I checked other WebView APIs. But are zoomBy(), zoomIn(), zoomOut() APIs for managing scales, right? I may be wrong, but if they work, we do not need to discuss this difference seriously. Here is another note. I checked some browsers' behaviors about scale restoration, but unfortunately it's very inconsistent today. Chrome desktop - reset on each new navigation to a different origin, but keep it for others. restore on back/forward navigation. Chrome mobile - reset on each new navigation including the same page navigation, but restore on back/forward navigation. Firefox desktop - keep it as a session persistent configuration. never change it automatically on any navigation. Firefox mobile - reset always on each navigation including back/forward navigation (looks a little broken, touching does not work after scaling on tablet) Safari mobile - same with Chrome mobile Since there seems to be no consensus on scale restoration today, I prefer a simple way, 1). And probably we should explicitly declare this behavior in the spec if we can reach a consensus within browser vendors. Anyway the situation developers rely on undocumented behavior to reset scale factors is bad.
On 2016/09/05 11:11:39, toyoshim wrote: > majidvp: Thank you for sharing background around the scale factor restoration. > If the spec delegates decision making of scale factor restoration to user > agents, it sounds the best that blink has an API for embedders, but hum.... > WebView cannot as Torne explained. > > My proposed workaround assumed only the case a developer used WebView to show > contents under their control. I thought if they used WebView just to show open > web contents, same behaviors with other agents would be preferable to minimize > compatibility issues. Even if they're displaying regular web pages, they aren't necessarily exposing a UI like a browser; it's hard to guess what the user/developer expectations might be. > I checked navigation spec carefully, but actually I could not find any explicit > definition for the same page navigation. (I made a thread to ask about this; > https://github.com/whatwg/html/issues/1742) > But modern browsers handle it almost as a normal reload. For instance, user > agents never create a new history entry for the same page navigation, and does > not remove successor entries even if it happens in the middle of back/forward > navigation. But scroll position isn't restored that is only difference from > normal reload. > > Torne proposed two real options. But having a flag to keep this old behavior for > the solution 2) does not help to simplify code to handle reload variants, and > will need to keep another reload variant. So, from viewpoint of my goal, I > prefer 1) unless this breaks the web. To make the option 1) more reasonable, let > me propose another workaround that uses a query string. Similar to my test > change, using unique URL can avoid the same page navigation. That's something > like https://foo.bar/baz?seq=%3Csequence number>. Here I also assume only contents > under developer's control. Can you do that with a data: URI? That's a pretty common use case in webview.. > Also I checked other WebView APIs. But are zoomBy(), zoomIn(), zoomOut() APIs > for managing scales, right? I may be wrong, but if they work, we do not need to > discuss this difference seriously. zoomBy/zoomIn/zoomOut are all relative - there is no way to set the scale to any particular value, or to know what the current scale actually is, or what it was by default. These functions are not usable to actually control scale programmatically - only to wire up "zoom in/zoom out" buttons in a UI for the user, which is what they were intended for :( > Here is another note. I checked some browsers' behaviors about scale > restoration, but unfortunately it's very inconsistent today. > > Chrome desktop - reset on each new navigation to a different origin, but keep it > for others. restore on back/forward navigation. > Chrome mobile - reset on each new navigation including the same page navigation, > but restore on back/forward navigation. > Firefox desktop - keep it as a session persistent configuration. never change it > automatically on any navigation. > Firefox mobile - reset always on each navigation including back/forward > navigation (looks a little broken, touching does not work after scaling on > tablet) > Safari mobile - same with Chrome mobile > > Since there seems to be no consensus on scale restoration today, I prefer a > simple way, 1). And probably we should explicitly declare this behavior in the > spec if we can reach a consensus within browser vendors. Anyway the situation > developers rely on undocumented behavior to reset scale factors is bad. OK. Let's proceed assuming we can change this without any apps being affected in a particularly problematic way, and see what happens - it's just important to note that if this does cause problems for more than a few isolated apps we might have to reconsider, and we may not find this out until some time *after* the relevant changes go to stable due to webview's small beta population :/
> Can you do that with a data: URI? That's a pretty common use case in webview.. Data URL can not have a query part in the URI, so adding random string as a comment inside the content is a workaround as I did in tests. > OK. Let's proceed assuming we can change this without any apps being affected in > a particularly problematic way, and see what happens - it's just important to > note that if this does cause problems for more than a few isolated apps we might > have to reconsider, and we may not find this out until some time *after* the > relevant changes go to stable due to webview's small beta population :/ So how about this plan to roll out the feature step by step under a field trial control. 1. enabled by default at trunk, but keep the flag available until it goes to stable (as usually we do) 2. roll out this feature to desktops under a field trial control, there problem less likely happen. 3. roll out this feature to mobiles under a field trial control 4. wait until WebView with this feature goes to beta and stable, and see what happens. If serious problem happens, flip back the flag to disabled.
On 2016/09/05 12:24:16, toyoshim wrote: > > Can you do that with a data: URI? That's a pretty common use case in webview.. > > Data URL can not have a query part in the URI, so adding random string as a > comment inside the content is a workaround as I did in tests. > > > OK. Let's proceed assuming we can change this without any apps being affected > in > > a particularly problematic way, and see what happens - it's just important to > > note that if this does cause problems for more than a few isolated apps we > might > > have to reconsider, and we may not find this out until some time *after* the > > relevant changes go to stable due to webview's small beta population :/ > > So how about this plan to roll out the feature step by step under a field trial > control. > > 1. enabled by default at trunk, but keep the flag available until it goes to > stable (as usually we do) > 2. roll out this feature to desktops under a field trial control, there problem > less likely happen. > 3. roll out this feature to mobiles under a field trial control > 4. wait until WebView with this feature goes to beta and stable, and see what > happens. If serious problem happens, flip back the flag to disabled. Field trial controls don't work for WebView, so to actually turn the feature back off we have to do another release. If we hear about widespread problems early enough we may be able to flip a flag back off in a respin, but if it's not until after we've released to 100% of stable it's more difficult, as we don't generally want to respin a stable release for an app compatibility issue that only affects a small number of apps - we have to deal with this on a case-by-case basis. So.. just go ahead with whatever your plan would normally be, as long as there's a way to disable the feature for a while.
I see. Keeping the flag won't help binary update, but still help to get binaries with a simple patch to disable this feature, especially for later versions that will be beta or dev when we notice a problem on the stable release.
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/2257743002/diff/100001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java (right): https://codereview.chromium.org/2257743002/diff/100001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2661: // TODO(crbug.com/642636): This test does not work on Android M and laters. I'd remove this TODO in the next patch set because it seems running on trybots with M. This may be because of my local configuration issue, and will discuss at the crbug separately. https://codereview.chromium.org/2257743002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2257743002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:133: NonValidatingReload Rename to ReloadwithoutSubResourceCacheRevalidation in favor of #49
third_party/WebKit lgtm. Please update the CL description.
Description was changed from ========== Restore user state on NonValidatingReload Currently, Chrome does not restore user state, scroll position and scale factor for same page navigation regardless of NonValidatingReloadOnNormalReload feature flag. This patch changes the behavior to restore user state even for the same navigation if NonValidatingReloadOnNormalReload feature is enabled. BUG=632358 ========== to ========== Restore user state on ReloadwithoutSubResourceCacheRevalidation Currently, Chrome does not restore user state, scroll position and scale factor for same page navigation regardless of NonValidatingReloadOnNormalReload feature flag. This patch changes the behavior to restore user state even for the same navigation if NonValidatingReloadOnNormalReload feature is enabled. BUG=632358 ==========
Description was changed from ========== Restore user state on ReloadwithoutSubResourceCacheRevalidation Currently, Chrome does not restore user state, scroll position and scale factor for same page navigation regardless of NonValidatingReloadOnNormalReload feature flag. This patch changes the behavior to restore user state even for the same navigation if NonValidatingReloadOnNormalReload feature is enabled. BUG=632358 ========== to ========== Restore user state on ReloadwithoutSubResourceCacheRevalidation Currently, Chrome does not restore user state, scroll position and scale factor for same page navigation regardless of NonValidatingReloadOnNormalReload feature flag in Chrome. This patch connects NonValidatingReloadOnNormalReload chrome flag to ReloadwithoutSubResourceCacheRevalidation blink flag, and changes the behavior to restore user state even for the same navigation if ReloadwithoutSubResourceCacheRevalidation flag is enabled. BUG=632358 ==========
Description was changed from ========== Restore user state on ReloadwithoutSubResourceCacheRevalidation Currently, Chrome does not restore user state, scroll position and scale factor for same page navigation regardless of NonValidatingReloadOnNormalReload feature flag in Chrome. This patch connects NonValidatingReloadOnNormalReload chrome flag to ReloadwithoutSubResourceCacheRevalidation blink flag, and changes the behavior to restore user state even for the same navigation if ReloadwithoutSubResourceCacheRevalidation flag is enabled. BUG=632358 ========== to ========== Restore user state on ReloadwithoutSubResourceCacheRevalidation Currently, Chrome does not restore user state, scroll position and scale factor for same page navigation regardless of NonValidatingReloadOnNormalReload content feature flag. This patch connects NonValidatingReloadOnNormalReload flag to ReloadwithoutSubResourceCacheRevalidation blink feature flag, and changes the behavior to restore user state even for the same page navigation if ReloadwithoutSubResourceCacheRevalidation flag is enabled. BUG=632358 ==========
toyoshim@chromium.org changed reviewers: + jochen@chromium.org
+jochen for content/child/runtime_features.cc
lgtm
The CQ bit was checked by toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org, tkent@chromium.org Link to the patchset: https://codereview.chromium.org/2257743002/#ps160001 (title: "[rebase]")
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 ========== Restore user state on ReloadwithoutSubResourceCacheRevalidation Currently, Chrome does not restore user state, scroll position and scale factor for same page navigation regardless of NonValidatingReloadOnNormalReload content feature flag. This patch connects NonValidatingReloadOnNormalReload flag to ReloadwithoutSubResourceCacheRevalidation blink feature flag, and changes the behavior to restore user state even for the same page navigation if ReloadwithoutSubResourceCacheRevalidation flag is enabled. BUG=632358 ========== to ========== Restore user state on ReloadwithoutSubResourceCacheRevalidation Currently, Chrome does not restore user state, scroll position and scale factor for same page navigation regardless of NonValidatingReloadOnNormalReload content feature flag. This patch connects NonValidatingReloadOnNormalReload flag to ReloadwithoutSubResourceCacheRevalidation blink feature flag, and changes the behavior to restore user state even for the same page navigation if ReloadwithoutSubResourceCacheRevalidation flag is enabled. BUG=632358 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Restore user state on ReloadwithoutSubResourceCacheRevalidation Currently, Chrome does not restore user state, scroll position and scale factor for same page navigation regardless of NonValidatingReloadOnNormalReload content feature flag. This patch connects NonValidatingReloadOnNormalReload flag to ReloadwithoutSubResourceCacheRevalidation blink feature flag, and changes the behavior to restore user state even for the same page navigation if ReloadwithoutSubResourceCacheRevalidation flag is enabled. BUG=632358 ========== to ========== Restore user state on ReloadwithoutSubResourceCacheRevalidation Currently, Chrome does not restore user state, scroll position and scale factor for same page navigation regardless of NonValidatingReloadOnNormalReload content feature flag. This patch connects NonValidatingReloadOnNormalReload flag to ReloadwithoutSubResourceCacheRevalidation blink feature flag, and changes the behavior to restore user state even for the same page navigation if ReloadwithoutSubResourceCacheRevalidation flag is enabled. BUG=632358 Committed: https://crrev.com/2a56f872fc3e82ccd819db4378ae2e1ac206c116 Cr-Commit-Position: refs/heads/master@{#416855} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/2a56f872fc3e82ccd819db4378ae2e1ac206c116 Cr-Commit-Position: refs/heads/master@{#416855} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
