Don't reuse HistoryItems for multiple navigations

Don't reuse HistoryItems for multiple navigations

Explicitly propagate shared state instead of implicitly doing so via HistoryItem reuse in FrameLoader::setHistoryItemStateForCommit().

If, for example, a page calls replaceState() while a back/forward navigation is in progress, it would co-op the provisional history item for the back/forward navigation and set its scroll state on it. This state would then get reused when the back/forward navigation actually committed.

BUG=481393
TEST=fast/history/resources/replaceState-during-beforeunload.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196532

[Nate Chapin, 2015-06-02 18:52:19]

japhet@chromium.org changed reviewers:
+ dcheng@chromium.org

[Avi (use Gerrit), 2015-06-02 18:55:27]

random drive by comment

https://codereview.chromium.org/1159723003/diff/100001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1159723003/diff/100001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:303: // or if this is a back/forward
naivgation, since we explicitly want to restore the state we just
typo: navigation

[Nate Chapin, 2015-06-02 18:56:28]

https://codereview.chromium.org/1159723003/diff/100001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1159723003/diff/100001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:303: // or if this is a back/forward
naivgation, since we explicitly want to restore the state we just
On 2015/06/02 18:55:27, Avi wrote:
> typo: navigation

Done.

[dcheng, 2015-06-02 19:11:36]

https://codereview.chromium.org/1159723003/diff/120001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1159723003/diff/120001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:195:
m_currentItem->setPageScaleFactor(scaleFactor == 1.0f ? 0 : scaleFactor);
Does 0 have some special meaning here? Why don't we want to pass 1.0 through?

https://codereview.chromium.org/1159723003/diff/120001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:294: RefPtr<HistoryItem> oldItem =
m_currentItem;
I think this needs to be a RefPtrWillBeRawPtr, since HistoryItems are in Oilpan.

https://codereview.chromium.org/1159723003/diff/120001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/1159723003/diff/120001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.h:194: void
setHistoryItemStateForCommit(HistoryCommitType, bool isSameDocument, bool
isPushOrReplaceState = false);
I'd prefer to see enum parameters here instead of bools.

[Nate Chapin, 2015-06-02 21:18:57]

https://codereview.chromium.org/1159723003/diff/120001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1159723003/diff/120001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:195:
m_currentItem->setPageScaleFactor(scaleFactor == 1.0f ? 0 : scaleFactor);
On 2015/06/02 19:11:35, dcheng wrote:
> Does 0 have some special meaning here? Why don't we want to pass 1.0 through?

0 is a magic value that means "we don't know, so don't change the scale factor".
The version I just uploaded changed that magic value to 1.0, which presumes that
the default behavior on a different document navigation is to reset the page
scale. I'm not sure that's true or not....but no tests broke.

https://codereview.chromium.org/1159723003/diff/120001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:294: RefPtr<HistoryItem> oldItem =
m_currentItem;
On 2015/06/02 19:11:35, dcheng wrote:
> I think this needs to be a RefPtrWillBeRawPtr, since HistoryItems are in
Oilpan.

Done.

https://codereview.chromium.org/1159723003/diff/120001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/1159723003/diff/120001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.h:194: void
setHistoryItemStateForCommit(HistoryCommitType, bool isSameDocument, bool
isPushOrReplaceState = false);
On 2015/06/02 19:11:36, dcheng wrote:
> I'd prefer to see enum parameters here instead of bools.

Done.

[dcheng, 2015-06-02 21:42:38]

dcheng@chromium.org changed reviewers:
+ bokan@chromium.org

[dcheng, 2015-06-02 21:42:39]

lgtm if bokan@ is OK with the page scale changes.

[bokan, 2015-06-03 15:07:29]

https://codereview.chromium.org/1159723003/diff/140001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1159723003/diff/140001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:1043: if (m_frame->isMainFrame() &&
m_currentItem->pageScaleFactor() != 1.0f) {
I don't think this is right. This check was added in
https://chromium.googlesource.com/chromium/blink/+/299030c000660b7e7e18980f99...
and relies on the fact that HistoryItem::m_pageScaleFactor is initialized to 0,
which you haven't changed. We definitely don't want to use 1 as the "magic
value" since 1 is a valid scale that we may want to restore. It's not a no-op
since page loading initializes to the minimum allowable scale, which on Android
is frequently < 1.

[Nate Chapin, 2015-06-03 18:29:24]

On 2015/06/03 15:07:29, bokan wrote:
>
https://codereview.chromium.org/1159723003/diff/140001/Source/core/loader/Fra...
> File Source/core/loader/FrameLoader.cpp (right):
> 
>
https://codereview.chromium.org/1159723003/diff/140001/Source/core/loader/Fra...
> Source/core/loader/FrameLoader.cpp:1043: if (m_frame->isMainFrame() &&
> m_currentItem->pageScaleFactor() != 1.0f) {
> I don't think this is right. This check was added in
>
https://chromium.googlesource.com/chromium/blink/+/299030c000660b7e7e18980f99...
> and relies on the fact that HistoryItem::m_pageScaleFactor is initialized to
0,
> which you haven't changed. We definitely don't want to use 1 as the "magic
> value" since 1 is a valid scale that we may want to restore. It's not a no-op
> since page loading initializes to the minimum allowable scale, which on
Android
> is frequently < 1.

Ok, updated. 0 is still the magic number. I believe there are now 2 scroll state
changes in this CL:
* We only restore scroll and scale state on reload if the ending url is the same
as the pre-reload url. If redirects change (e.g.,Request desktop site), we're
more selective about restoring. See the WebFrameTest.cpp change.
* When we call WebViewImpl::resetScrollAndScaleState (I think this is only in
the RDS case), HistoryItem::m_pageScaleFactor gets set to the reset scale value
(always 1?) instead of 0. This should be a no-op, since we won't propagate and
restore the scale if the url changes, and if the url doesn't change, the
existing scale and the "restored" scale should be the same. However, since the
pre-reload scale factor changes, WebViewTest.HistoryResetScrollAndScaleState
needs to change.

[bokan, 2015-06-03 19:35:04]

Page scale changes lgtm

[Nate Chapin, 2015-06-03 19:43:22]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-06-03 19:43:22]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1159723003/#ps160001
(title: "Don't change the magic number")

[commit-bot: I haz the power, 2015-06-03 19:44:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1159723003/160001

[commit-bot: I haz the power, 2015-06-03 19:48:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-03 19:48:57]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/57535)

[Nate Chapin, 2015-06-04 20:18:52]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-06-04 20:18:53]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1159723003/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-04 20:19:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1159723003/180001

[commit-bot: I haz the power, 2015-06-04 21:30:43]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196532