Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2898423002:
Reset scroll and scale while toggling "Request desktop site" (Closed)
|
Created:
3 years, 7 months ago by jaebaek Modified:
3 years, 6 months ago Reviewers:
aelias_OOO_until_Jul13 CC:
blink-reviews, chromium-reviews, Jinsuk Kim Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL resets scroll and scale when a reload occurs
because of toggling "Request desktop site".
BUG=693877
Review-Url: https://codereview.chromium.org/2898423002
Cr-Commit-Position: refs/heads/master@{#479276}
Committed: https://chromium.googlesource.com/chromium/src/+/4c2b2cf9b350fd155fd0c1b9159b5f5c9477ff94
Patch Set 1 #
Total comments: 2
Patch Set 2 : [WIP] Page is zoomed in while toggling "Request desktop site" #
Total comments: 1
Patch Set 3 : Reset page zoom while requesting desktop site #Patch Set 4 : unit test update #Patch Set 5 : WebView bug #Patch Set 6 : WebView bug #
Total comments: 1
Patch Set 7 : WebView bug #
Messages
Total messages: 30 (16 generated)
Description was changed from ========== [WIP] Page is zoomed in while toggling "Request desktop site" BUG=693877 ========== to ========== [WIP] Page is zoomed in while toggling "Request desktop site" If we zoom the mobile page a little, and change to "Request desktop site", the desktop version is zoomed very high. The expected result is that the page should always be fully zoomed out after a reload. When WebKit handles the post-behavior of reload, it determines if restoring scale and scroll is required (See FrameLoader::RestoreScrollPositionAndViewStateForLoadType). This CL checks if the reload is caused by changing the user agent. If yes, it just set those restoring scale and scroll as falses so that it does not call VisualViewport::SetScaleAndLocation(). I tried to check side effects caused by this CL and I cautiously think it would be fine. BUG=693877 ==========
jaebaek@chromium.org changed reviewers: + aelias@chromium.org, jinsukkim@chromium.org
jaebaek@chromium.org changed reviewers: - jinsukkim@chromium.org
PTAL
https://codereview.chromium.org/2898423002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2898423002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1108: if (!frame_->GetSettings()->GetViewportMetaEnabled()) { Unfortunately, there are indeed side effects: 1. On other Chrome platforms this setting is always false, because viewport meta is mobile-specific feature. So this is universally disabling scroll/scale restore on non-Android. 2. Even if this were wrapped in "#if OS(ANDROID)" (which is discouraged pattern anyway), we will also see the problem that further navigations starting from the desktop site will no longer restore scroll/scale properly. So this needs to be done conditionally that the *last* navigation was a special RDS reload.
https://codereview.chromium.org/2898423002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2898423002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1108: if (!frame_->GetSettings()->GetViewportMetaEnabled()) { On 2017/05/25 02:56:08, aelias wrote: > Unfortunately, there are indeed side effects: > 1. On other Chrome platforms this setting is always false, because viewport meta > is mobile-specific feature. So this is universally disabling scroll/scale > restore on non-Android. > 2. Even if this were wrapped in "#if OS(ANDROID)" (which is discouraged pattern > anyway), we will also see the problem that further navigations starting from the > desktop site will no longer restore scroll/scale properly. > > So this needs to be done conditionally that the *last* navigation was a special > RDS reload. Does it mean that I must add a parameter which let the system know the reload is actually from RDS and set it as TRUE at the moment when NavigationControllerAndroid::SetUseDesktopUserAgent() is called (i.e., RDS button is clicked)? In the case, the parameter definitely will sent to settings (e.g., we can get it by calling frame_->GetSettings()->GetRDSEnabled() here).
It should be a one-time thing tied to the transition itself, whereas settings are persistent. And there should already be enough information available in Blink. For instance, you could tie it to the viewportEnabled transition point. But I think it would be better to observe if there are any unusual properties on the reload (which there should be, since it's an unusual content-changing reload).
I tested the normal reload, mobile -> RDS, and RDS -> mobile. It seems this CL works as expected. One question here: when doing the normal reload after zooming in, must it be scaled but not scrolled?
On 2017/06/02 at 08:07:42, jaebaek wrote: > One question here: when doing the normal reload after zooming in, must it be scaled but not scrolled? I think we should treat scroll and scale the same way. I'd consider the current behavior somewhat buggy. What effect does your patch have on that?
https://codereview.chromium.org/2898423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2898423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:3319: document->Loader()->GetHistoryItem()->SetDidSaveScrollOrScaleState(false); This seems reasonable to me. Could you add a unit test? I suggest adding one in WebViewTest.cpp, see for example related test WebViewTest.HistoryResetScrollAndScaleState.
Oh my god .. I did not realize I must create a unit test :( I added an instrumentation test. I will add a unit test too, but do you think it would be better that I just drop the instrumentation test?
On 2017/06/07 at 09:44:13, jaebaek wrote: > Oh my god .. I did not realize I must create a unit test :( I added an instrumentation test. I will add a unit test too, but do you think it would be better that I just drop the instrumentation test? Yeah, it might be better to drop instrumentation test, because instrumentation tests are more likely to be flaky and they only run on Android. So if we get similar coverage from a unit test, it's probably less maintenance burden to not bother with an instrumentation test. Sorry if you spent some hours on it... Also, does this instrumentation test fail if the production-code change is reverted? I'm guessing it might not because it doesn't zoom in with a pinch event or anything, which was part of bug repro. If a test is green regardless of whether it has the fix or not, then it's not testing the right thing.
The CQ bit was checked by jaebaek@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: 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...)
The CQ bit was checked by jaebaek@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.
PTAL
lgtm https://codereview.chromium.org/2898423002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2898423002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4276: // Reload the page and end up at the same url. State should be propagated. Also fix comment: "not be"
Please also fix patch description to remove [WIP] from title and subject line, and describe the new approach. You need to click "Edit" in Rietveld to do that, amending your local git commit doesn't work after the initial upload.
Description was changed from ========== [WIP] Page is zoomed in while toggling "Request desktop site" If we zoom the mobile page a little, and change to "Request desktop site", the desktop version is zoomed very high. The expected result is that the page should always be fully zoomed out after a reload. When WebKit handles the post-behavior of reload, it determines if restoring scale and scroll is required (See FrameLoader::RestoreScrollPositionAndViewStateForLoadType). This CL checks if the reload is caused by changing the user agent. If yes, it just set those restoring scale and scroll as falses so that it does not call VisualViewport::SetScaleAndLocation(). I tried to check side effects caused by this CL and I cautiously think it would be fine. BUG=693877 ========== to ========== This CL resets scroll and scale when a reload occurs because of toggling "Request desktop site". BUG=693877 ==========
The CQ bit was checked by jaebaek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org Link to the patchset: https://codereview.chromium.org/2898423002/#ps120001 (title: "WebView bug")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497404061261200,
"parent_rev": "b70e6e172fe3cbfe860678ca27cc5ef5c0c0003e", "commit_rev":
"4c2b2cf9b350fd155fd0c1b9159b5f5c9477ff94"}
Message was sent while issue was closed.
Description was changed from ========== This CL resets scroll and scale when a reload occurs because of toggling "Request desktop site". BUG=693877 ========== to ========== This CL resets scroll and scale when a reload occurs because of toggling "Request desktop site". BUG=693877 Review-Url: https://codereview.chromium.org/2898423002 Cr-Commit-Position: refs/heads/master@{#479276} Committed: https://chromium.googlesource.com/chromium/src/+/4c2b2cf9b350fd155fd0c1b9159b... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/4c2b2cf9b350fd155fd0c1b9159b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
