Replace history.options with history.scrollRestoration attribute

Replace history.options with history.scrollRestoration attribute

Remove both history.options and optional fourth arguments from
{replace,push}State and instead allow a single attribute
(history.scrollRestoration) to control the page scroll restoration.

Also fix a existing issue where scrolling to fragment was incorrectly getting
precedent over history restoration scrolls when 'manual' scroll restoration
was used and add additional web-platform-tests covering this issue and changes
in the  API.

These changes are based on latest discussion on whatwg list and changes in the
unofficial spec: http://majido.github.io/scroll-restoration-proposal/history-
based-api.html

BUG=444094
TEST=fast/history/scroll-restoration

Committed: https://crrev.com/e59dcbff357afc2cd52d67267e3ee32e9f005bf1
git-svn-id: svn://svn.chromium.org/blink/trunk@199597 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[majidvp, 2015-07-16 16:04:25]

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

[Nate Chapin, 2015-07-16 21:34:01]

https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
File Source/core/frame/History.cpp (right):

https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
Source/core/frame/History.cpp:85: HistoryScrollRestorationType scrollRestoration
= value == "manual" ? ScrollRestorationManual : ScrollRestorationAuto;
My knowledge of the v8 bindings is outdated. Are we guaranteed to get either
"manual" or "auto" here? Should we ASSERT that?

https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
Source/core/frame/History.cpp:89:
m_frame->loader().updateForSameDocumentNavigation(m_frame->loader().currentItem()->url(),
SameDocumentNavigationHistoryApi, stateInternal(), scrollRestoration,
FrameLoadTypeReplaceCurrentItem);
Why is this modeled as a navigation?

https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
File Source/core/frame/History.h (right):

https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
Source/core/frame/History.h:71: HistoryScrollRestorationType
scrollRestorationInternal() const;
Should this be private?

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

https://codereview.chromium.org/1239463005/diff/140001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:921: processFragment(url,
NavigationWithinSameDocument);
Why is this moving?

[majidvp, 2015-07-16 22:11:50]

https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
File Source/core/frame/History.cpp (right):

https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
Source/core/frame/History.cpp:85: HistoryScrollRestorationType scrollRestoration
= value == "manual" ? ScrollRestorationManual : ScrollRestorationAuto;
On 2015/07/16 21:34:01, Nate Chapin wrote:
> My knowledge of the v8 bindings is outdated. Are we guaranteed to get either
> "manual" or "auto" here? Should we ASSERT that?

Yes V8 bindings validates that only valid values are allowed here. I will add an
assert as well.

See for example:
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...

https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
Source/core/frame/History.cpp:89:
m_frame->loader().updateForSameDocumentNavigation(m_frame->loader().currentItem()->url(),
SameDocumentNavigationHistoryApi, stateInternal(), scrollRestoration,
FrameLoadTypeReplaceCurrentItem);
On 2015/07/16 21:34:01, Nate Chapin wrote:
> Why is this modeled as a navigation?

Hmm, perhaps it can be made simpler. I think all it needs to do is to update the
current state and notify |FrameLoaderClient| that it has done do. wdyt?

https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
File Source/core/frame/History.h (right):

https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
Source/core/frame/History.h:71: HistoryScrollRestorationType
scrollRestorationInternal() const;
On 2015/07/16 21:34:01, Nate Chapin wrote:
> Should this be private?

It should.

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

https://codereview.chromium.org/1239463005/diff/140001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:921: processFragment(url,
NavigationWithinSameDocument);
On 2015/07/16 21:34:01, Nate Chapin wrote:
> Why is this moving?

The past logic was:
 1- scroll to fragment (checks didRestoreFromHistory flag)
 2- attempts to scroll to recorded scroll position overriding fragment scroll.

This does not work in "scrollRestoration=Manual" case because restoring scroll
position actually does not restore any scroll position but we still need to
prevent fragment scrolling. So I reversed the order and use an already existing
mechanism:

 1. attempt scroll to recorded scroll (set the didRestoreFromHistory if
successful)
 2. scroll to fragment (checks didRestoreFromHistory flag)

This can be made even cleaner but I think it needs to be done in a separate
patch. Here is my proposed refactoring:
https://docs.google.com/document/d/12RZGD2IsyA7Co9yAGb6f4r1YDIkPe7Nte0P82zWIz...

[Nate Chapin, 2015-07-16 22:19:32]

https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
File Source/core/frame/History.cpp (right):

https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
Source/core/frame/History.cpp:89:
m_frame->loader().updateForSameDocumentNavigation(m_frame->loader().currentItem()->url(),
SameDocumentNavigationHistoryApi, stateInternal(), scrollRestoration,
FrameLoadTypeReplaceCurrentItem);
On 2015/07/16 22:11:49, majidvp wrote:
> On 2015/07/16 21:34:01, Nate Chapin wrote:
> > Why is this modeled as a navigation?
> 
> Hmm, perhaps it can be made simpler. I think all it needs to do is to update
the
> current state and notify |FrameLoaderClient| that it has done do. wdyt?

It may not even need to notify FrameLoaderClient. content/renderer holds a
reference to the current HistoryItem (via WebHistoryItem), so I think it should
be updated automatically.

[majidvp, 2015-07-20 23:57:00]

PTAL.

On 2015/07/16 22:19:32, Nate Chapin wrote:
>
https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
> File Source/core/frame/History.cpp (right):
> 
>
https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
> Source/core/frame/History.cpp:89:
>
m_frame->loader().updateForSameDocumentNavigation(m_frame->loader().currentItem()->url(),
> SameDocumentNavigationHistoryApi, stateInternal(), scrollRestoration,
> FrameLoadTypeReplaceCurrentItem);
> On 2015/07/16 22:11:49, majidvp wrote:
> > On 2015/07/16 21:34:01, Nate Chapin wrote:
> > > Why is this modeled as a navigation?
> > 
> > Hmm, perhaps it can be made simpler. I think all it needs to do is to update
> the
> > current state and notify |FrameLoaderClient| that it has done do. wdyt?
> 
> It may not even need to notify FrameLoaderClient. content/renderer holds a
> reference to the current HistoryItem (via WebHistoryItem), so I think it
should
> be updated automatically.
I think we still need to notify the client to ensure that the state is going to 
be synced with the browser process.

[majidvp, 2015-07-24 16:42:45]

On 2015/07/20 23:57:00, majidvp wrote:
> PTAL.
> 
> On 2015/07/16 22:19:32, Nate Chapin wrote:
> >
>
https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
> > File Source/core/frame/History.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1239463005/diff/140001/Source/core/frame/Hist...
> > Source/core/frame/History.cpp:89:
> >
>
m_frame->loader().updateForSameDocumentNavigation(m_frame->loader().currentItem()->url(),
> > SameDocumentNavigationHistoryApi, stateInternal(), scrollRestoration,
> > FrameLoadTypeReplaceCurrentItem);
> > On 2015/07/16 22:11:49, majidvp wrote:
> > > On 2015/07/16 21:34:01, Nate Chapin wrote:
> > > > Why is this modeled as a navigation?
> > > 
> > > Hmm, perhaps it can be made simpler. I think all it needs to do is to
update
> > the
> > > current state and notify |FrameLoaderClient| that it has done do. wdyt?
> > 
> > It may not even need to notify FrameLoaderClient. content/renderer holds a
> > reference to the current HistoryItem (via WebHistoryItem), so I think it
> should
> > be updated automatically.
> I think we still need to notify the client to ensure that the state is going
to 
> be synced with the browser process.


friendly ping

[Nate Chapin, 2015-07-27 23:46:55]

The new test don't have expectations?

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

https://codereview.chromium.org/1239463005/diff/160001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:253: void
FrameLoader::setScrollRestorationType(HistoryScrollRestorationType
scrollRestoration)
I don't think this method is sufficiently reusable to stand on its own. It
should be fine to just put it in History::setScrollRestoration inline.

https://codereview.chromium.org/1239463005/diff/160001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:259: client()->didUpdateCurrentHistoryItem();
Is this actually necessary?

[majidvp, 2015-07-28 12:19:06]

> The new test don't have expectations?
I wrote these as w3c web platform tests which unlike layout tests do not have
expectations. The motivation was to bundle these with the spec.

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

https://codereview.chromium.org/1239463005/diff/160001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:253: void
FrameLoader::setScrollRestorationType(HistoryScrollRestorationType
scrollRestoration)
On 2015/07/27 23:46:55, Nate Chapin wrote:
> I don't think this method is sufficiently reusable to stand on its own. It
> should be fine to just put it in History::setScrollRestoration inline.

Acknowledged.

https://codereview.chromium.org/1239463005/diff/160001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:259: client()->didUpdateCurrentHistoryItem();
On 2015/07/27 23:46:55, Nate Chapin wrote:
> Is this actually necessary?

It causes a navigation sync with browser to be 
scheduled for the entry. I think without it, the 
updated value does not get communicated to browser
an may not get persisted. This is also what we do 
when we update scroll position as well.

[majidvp, 2015-07-28 15:44:11]

PTAL.

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

https://codereview.chromium.org/1239463005/diff/160001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:253: void
FrameLoader::setScrollRestorationType(HistoryScrollRestorationType
scrollRestoration)
On 2015/07/28 12:19:06, majidvp wrote:
> On 2015/07/27 23:46:55, Nate Chapin wrote:
> > I don't think this method is sufficiently reusable to stand on its own. It
> > should be fine to just put it in History::setScrollRestoration inline.
> 
> Acknowledged.

Done.

[Nate Chapin, 2015-07-28 18:49:22]

lgtm

[majidvp, 2015-07-28 19:02:50]

The CQ bit was checked by majidvp@chromium.org

[commit-bot: I haz the power, 2015-07-28 19:03:12]

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

[commit-bot: I haz the power, 2015-07-28 19:06:16]

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

[commit-bot: I haz the power, 2015-09-23 11:51:22]

Patchset 10 (id:??) landed as
https://crrev.com/e59dcbff357afc2cd52d67267e3ee32e9f005bf1