Prevent restoreScrollPositionAndViewState called in setContentsSize when loading (issue 2541513004 by chaopeng@chromium.org)

[chao...@chromium.org]

Reviewers: bokan
CL: https://codereview.chromium.org/2541513004/

Message:
PTAL

Description:
Prevent restoreScrollPositionAndViewState called in setContentsSize when loading

This issue is cause by FrameView::setContentsSize called
FrameLoader::restoreScrollPositionAndViewState when reloading the page and
restoreScrollPositionAndViewState called ScrollableArea::setScrollOffset that
clear FrameView::m_fragmentAnchor. That casuse FrameView::scrollToFragmentAnchor
had no m_fragmentAnchor to scroll to.

In this patch we call scrollToFragmentAnchor before
restoreScrollPositionAndViewState in setContentsSize.

BUG=656658
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Affected files (+321, -217 lines):
M content/browser/frame_host/navigation_controller_impl_browsertest.cc
A content/test/data/navigation_controller/reload-with-url-anchor.html
M third_party/WebKit/Source/core/frame/FrameView.cpp

[bo...@chromium.org]

https://codereview.chromium.org/2541513004/diff/120001/third_party/WebKit/Source/core/frame/FrameView.cpp
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2541513004/diff/120001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode649
third_party/WebKit/Source/core/frame/FrameView.cpp:649:
scrollToFragmentAnchor();
I think you can remove it from performPostLayoutTasks.

https://codereview.chromium.org/2541513004/

[bo...@chromium.org]

Also, the title is wrong, you're not preventing
restoreScrollPositionAndViewState. You're scrolling to the fragment before
restoring history scroll position.

https://codereview.chromium.org/2541513004/

[chao...@chromium.org]

On 2016/12/02 17:12:21, bokan wrote:
> Also, the title is wrong, you're not preventing
> restoreScrollPositionAndViewState. You're scrolling to the fragment before
> restoring history scroll position.

Updated, PTAL.

https://codereview.chromium.org/2541513004/

[bo...@chromium.org]

I tweaked the commit message a bit but lgtm.

https://codereview.chromium.org/2541513004/

[chao...@chromium.org]

cr...@chromium.org: Please review changes in content



https://codereview.chromium.org/2541513004/

[cr...@chromium.org]

Thanks! LGTM with minor nits.


https://codereview.chromium.org/2541513004/diff/140001/content/browser/frame_host/navigation_controller_impl_browsertest.cc
File
content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2541513004/diff/140001/content/browser/frame_host/navigation_controller_impl_browsertest.cc#newcode1339
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1339:
// Verify that reload page with url anchor scroll to correct position.
nit: s/reload/reloading a/
nit: s/scroll/scrolls/

https://codereview.chromium.org/2541513004/diff/140001/content/browser/frame_host/navigation_controller_impl_browsertest.cc#newcode1351
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1351:
// reload
Minor nit: Capitalize and end with period.

https://codereview.chromium.org/2541513004/diff/140001/third_party/WebKit/Source/core/frame/FrameView.cpp
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2541513004/diff/140001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode648
third_party/WebKit/Source/core/frame/FrameView.cpp:648: //
restoreScrollPositionAndViewState when reload
nit: when reloading.
nit: End with period.

https://codereview.chromium.org/2541513004/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2541513004/180001


https://codereview.chromium.org/2541513004/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:
win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/344007)

https://codereview.chromium.org/2541513004/

[bo...@chromium.org]

https://codereview.chromium.org/2541513004/diff/180001/third_party/WebKit/Source/core/frame/FrameView.cpp
File third_party/WebKit/Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/2541513004/diff/180001/third_party/WebKit/Source/core/frame/FrameView.cpp#oldcode2382
third_party/WebKit/Source/core/frame/FrameView.cpp:2382:
scrollToFragmentAnchor();
Ok, leaving this in sgtm.

https://codereview.chromium.org/2541513004/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2541513004/200001


https://codereview.chromium.org/2541513004/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #7 (id:200001)

https://codereview.chromium.org/2541513004/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 7 (id:??) landed as
https://crrev.com/6d072f7c3b55dbe96358cb9623dfce5e756bad54
Cr-Commit-Position: refs/heads/master@{#436682}

https://codereview.chromium.org/2541513004/