Check scrollRestorationType explicitly in FrameLoader::processFragment()

Check scrollRestorationType explicitly in FrameLoader::processFragment()

This is preparation for [1], fixing test failure of
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
with [1].

Before [1], the test is passing because, in FrameLoader::finishedParsing(),
restoreScrollPositionAndViewState() is called (in checkCompleted()) before
processFragment(), and thus processFragment() doesn't cause scroll
because |shouldScrollToFragment| is false because
|didRestoreFromHistory| is set to true by restoreScrollPositionAndViewState().

However, after [1], restoreScrollPositionAndViewState() is NOT called in
checkCompleted() because [1] causes the document onload to be called after
finishedParsing(), and thus processFragment() causes scrolling unexpectedly,
not reflecting history.scrollRestoration = 'manual'.

This CL makes processFragment() to check scrollRestorationType explicitly,
not depending on previous restoreScrollPositionAndViewState() call.

[1] https://codereview.chromium.org/2269043002/.

BUG=624697, 417782
TEST=virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html

Committed: https://crrev.com/41ca771082edfcdbe304abbce42fce6de58c0657
Cr-Commit-Position: refs/heads/master@{#430237}

[hiroshige, 2016-10-31 09:04:06]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 09:04:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2016-10-31 09:08:50]

Description was changed from

==========
Call restoreScrollPositionAndViewState() before processFragment()

BUG=
==========

to

==========
Call restoreScrollPositionAndViewState() before processFragment()

This is preparation for [1], fixing test failure of
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
with [1].

Before [1], in FrameLoader::finishedParsing(),
restoreScrollPositionAndViewState() is called (in checkCompleted()) before
processFragment(), and thus processFragment() doesn't cause scroll
because |shouldScrollToFragment| is false because
|didRestoreFromHistory| is set to true by restoreScrollPositionAndViewState().

However, after [1], restoreScrollPositionAndViewState() is NOT called in
checkCompleted() because [1] causes the document onload to be called after
finishedParsing(), and thus processFragment() causes scrolling unexpectedly.

This CL calls restoreScrollPositionAndViewState() before the
processFragment() call in finishedParsing() to fix the test.

[1] https://codereview.chromium.org/2269043002/.

BUG=624697
==========

[hiroshige, 2016-10-31 09:17:57]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 09:18:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2016-10-31 09:18:51]

Description was changed from

==========
Call restoreScrollPositionAndViewState() before processFragment()

This is preparation for [1], fixing test failure of
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
with [1].

Before [1], in FrameLoader::finishedParsing(),
restoreScrollPositionAndViewState() is called (in checkCompleted()) before
processFragment(), and thus processFragment() doesn't cause scroll
because |shouldScrollToFragment| is false because
|didRestoreFromHistory| is set to true by restoreScrollPositionAndViewState().

However, after [1], restoreScrollPositionAndViewState() is NOT called in
checkCompleted() because [1] causes the document onload to be called after
finishedParsing(), and thus processFragment() causes scrolling unexpectedly.

This CL calls restoreScrollPositionAndViewState() before the
processFragment() call in finishedParsing() to fix the test.

[1] https://codereview.chromium.org/2269043002/.

BUG=624697
==========

to

==========
Call restoreScrollPositionAndViewState() before processFragment()

This is preparation for [1], fixing test failure of
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
with [1].

Before [1], in FrameLoader::finishedParsing(),
restoreScrollPositionAndViewState() is called (in checkCompleted()) before
processFragment(), and thus processFragment() doesn't cause scroll
because |shouldScrollToFragment| is false because
|didRestoreFromHistory| is set to true by restoreScrollPositionAndViewState().

However, after [1], restoreScrollPositionAndViewState() is NOT called in
checkCompleted() because [1] causes the document onload to be called after
finishedParsing(), and thus processFragment() causes scrolling unexpectedly.

This CL calls restoreScrollPositionAndViewState() before the
processFragment() call in finishedParsing() to fix the test.

[1] https://codereview.chromium.org/2269043002/.

BUG=624697, 417782
==========

[hiroshige, 2016-10-31 09:18:51]

hiroshige@chromium.org changed reviewers:
+ skobes@chromium.org

[hiroshige, 2016-10-31 09:28:43]

Description was changed from

==========
Call restoreScrollPositionAndViewState() before processFragment()

This is preparation for [1], fixing test failure of
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
with [1].

Before [1], in FrameLoader::finishedParsing(),
restoreScrollPositionAndViewState() is called (in checkCompleted()) before
processFragment(), and thus processFragment() doesn't cause scroll
because |shouldScrollToFragment| is false because
|didRestoreFromHistory| is set to true by restoreScrollPositionAndViewState().

However, after [1], restoreScrollPositionAndViewState() is NOT called in
checkCompleted() because [1] causes the document onload to be called after
finishedParsing(), and thus processFragment() causes scrolling unexpectedly.

This CL calls restoreScrollPositionAndViewState() before the
processFragment() call in finishedParsing() to fix the test.

[1] https://codereview.chromium.org/2269043002/.

BUG=624697, 417782
==========

to

==========
Call restoreScrollPositionAndViewState() before processFragment()

This is preparation for [1], fixing test failure of
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
with [1].

Before [1], in FrameLoader::finishedParsing(),
restoreScrollPositionAndViewState() is called (in checkCompleted()) before
processFragment(), and thus processFragment() doesn't cause scroll
because |shouldScrollToFragment| is false because
|didRestoreFromHistory| is set to true by restoreScrollPositionAndViewState().

However, after [1], restoreScrollPositionAndViewState() is NOT called in
checkCompleted() because [1] causes the document onload to be called after
finishedParsing(), and thus processFragment() causes scrolling unexpectedly,
not reflecting history.scrollRestoration = 'manual'.

This CL calls restoreScrollPositionAndViewState() before the
processFragment() call in finishedParsing() to fix the test.

[1] https://codereview.chromium.org/2269043002/.

BUG=624697, 417782
==========

[hiroshige, 2016-10-31 09:29:04]

Could you take a look?

[commit-bot: I haz the power, 2016-10-31 10:20:35]

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

[commit-bot: I haz the power, 2016-10-31 10:20:36]

Dry run: This issue passed the CQ dry run.

[skobes, 2016-11-02 04:05:40]

You wrote:

"after [1], restoreScrollPositionAndViewState() is NOT called in
checkCompleted() because [1] causes the document onload to be called after
finishedParsing()"

I'm afraid I don't understand this.  It looks like checkCompleted still calls
restoreScrollPositionAndViewState.  Can you clarify why we need it in two
places?

[hiroshige, 2016-11-02 04:20:04]

On 2016/11/02 04:05:40, skobes wrote:
> You wrote:
> 
> "after [1], restoreScrollPositionAndViewState() is NOT called in
> checkCompleted() because [1] causes the document onload to be called after
> finishedParsing()"
> 
> I'm afraid I don't understand this.  It looks like checkCompleted still calls
> restoreScrollPositionAndViewState.  Can you clarify why we need it in two
> places?

checkCompleted() calls restoreScrollPositionAndViewState(), only if
shouldComplete() returns true.
My CL [1] causes shouldComplete() to return false in the case of
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
and thus restoreScrollPositionAndViewState() would not be called.

[skobes, 2016-11-02 04:38:44]

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

[skobes, 2016-11-02 04:39:45]

On 2016/11/02 04:20:04, hiroshige wrote:
> checkCompleted() calls restoreScrollPositionAndViewState(), only if
> shouldComplete() returns true.
> My CL [1] causes shouldComplete() to return false in the case of
>
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
> and thus restoreScrollPositionAndViewState() would not be called.

I see, thanks for explaining.

Couldn't FrameLoader::processFragment just check
m_currentItem->scrollRestorationType() and set shouldScrollToFragment to false
if it's 'manual'?  That seems safer than relying on didRestoreFromHistory to be
set first.

[hiroshige, 2016-11-04 08:01:56]

Description was changed from

==========
Call restoreScrollPositionAndViewState() before processFragment()

This is preparation for [1], fixing test failure of
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
with [1].

Before [1], in FrameLoader::finishedParsing(),
restoreScrollPositionAndViewState() is called (in checkCompleted()) before
processFragment(), and thus processFragment() doesn't cause scroll
because |shouldScrollToFragment| is false because
|didRestoreFromHistory| is set to true by restoreScrollPositionAndViewState().

However, after [1], restoreScrollPositionAndViewState() is NOT called in
checkCompleted() because [1] causes the document onload to be called after
finishedParsing(), and thus processFragment() causes scrolling unexpectedly,
not reflecting history.scrollRestoration = 'manual'.

This CL calls restoreScrollPositionAndViewState() before the
processFragment() call in finishedParsing() to fix the test.

[1] https://codereview.chromium.org/2269043002/.

BUG=624697, 417782
==========

to

==========
Call restoreScrollPositionAndViewState() before processFragment()

This is preparation for [1], fixing test failure of
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
with [1].

Before [1], in FrameLoader::finishedParsing(),
restoreScrollPositionAndViewState() is called (in checkCompleted()) before
processFragment(), and thus processFragment() doesn't cause scroll
because |shouldScrollToFragment| is false because
|didRestoreFromHistory| is set to true by restoreScrollPositionAndViewState().

However, after [1], restoreScrollPositionAndViewState() is NOT called in
checkCompleted() because [1] causes the document onload to be called after
finishedParsing(), and thus processFragment() causes scrolling unexpectedly,
not reflecting history.scrollRestoration = 'manual'.

This CL calls restoreScrollPositionAndViewState() before the
processFragment() call in finishedParsing() to fix the test.

[1] https://codereview.chromium.org/2269043002/.

BUG=624697, 417782
TEST=virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
==========

[hiroshige, 2016-11-04 08:11:04]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 08:11:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2016-11-04 08:15:56]

Description was changed from

==========
Call restoreScrollPositionAndViewState() before processFragment()

This is preparation for [1], fixing test failure of
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
with [1].

Before [1], in FrameLoader::finishedParsing(),
restoreScrollPositionAndViewState() is called (in checkCompleted()) before
processFragment(), and thus processFragment() doesn't cause scroll
because |shouldScrollToFragment| is false because
|didRestoreFromHistory| is set to true by restoreScrollPositionAndViewState().

However, after [1], restoreScrollPositionAndViewState() is NOT called in
checkCompleted() because [1] causes the document onload to be called after
finishedParsing(), and thus processFragment() causes scrolling unexpectedly,
not reflecting history.scrollRestoration = 'manual'.

This CL calls restoreScrollPositionAndViewState() before the
processFragment() call in finishedParsing() to fix the test.

[1] https://codereview.chromium.org/2269043002/.

BUG=624697, 417782
TEST=virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
==========

to

==========
Check scrollRestorationType explicitly in FrameLoader::processFragment()

This is preparation for [1], fixing test failure of
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
with [1].

Before [1], the test is passing because, in FrameLoader::finishedParsing(),
restoreScrollPositionAndViewState() is called (in checkCompleted()) before
processFragment(), and thus processFragment() doesn't cause scroll
because |shouldScrollToFragment| is false because
|didRestoreFromHistory| is set to true by restoreScrollPositionAndViewState().

However, after [1], restoreScrollPositionAndViewState() is NOT called in
checkCompleted() because [1] causes the document onload to be called after
finishedParsing(), and thus processFragment() causes scrolling unexpectedly,
not reflecting history.scrollRestoration = 'manual'.

This CL makes processFragment() to check scrollRestorationType explicitly,
not depending on previous restoreScrollPositionAndViewState() call.

[1] https://codereview.chromium.org/2269043002/.

BUG=624697, 417782
TEST=virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
==========

[commit-bot: I haz the power, 2016-11-04 09:22:09]

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

[commit-bot: I haz the power, 2016-11-04 09:22:10]

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_...)

[hiroshige, 2016-11-04 09:37:24]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 09:37:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 10:43:54]

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

[commit-bot: I haz the power, 2016-11-04 10:43:54]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-11-04 11:57:00]

On 2016/11/02 04:39:45, skobes wrote:
> On 2016/11/02 04:20:04, hiroshige wrote:
> > checkCompleted() calls restoreScrollPositionAndViewState(), only if
> > shouldComplete() returns true.
> > My CL [1] causes shouldComplete() to return false in the case of
> >
>
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
> > and thus restoreScrollPositionAndViewState() would not be called.
> 
> I see, thanks for explaining.
> 
> Couldn't FrameLoader::processFragment just check
> m_currentItem->scrollRestorationType() and set shouldScrollToFragment to false
> if it's 'manual'?  That seems safer than relying on didRestoreFromHistory to
be
> set first.

Thanks for suggestion!
How about Patch Set 4?

[skobes, 2016-11-04 16:38:52]

lgtm

[hiroshige, 2016-11-07 06:26:05]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2016-11-07 06:26:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 09:06:51]

Description was changed from

==========
Check scrollRestorationType explicitly in FrameLoader::processFragment()

This is preparation for [1], fixing test failure of
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
with [1].

Before [1], the test is passing because, in FrameLoader::finishedParsing(),
restoreScrollPositionAndViewState() is called (in checkCompleted()) before
processFragment(), and thus processFragment() doesn't cause scroll
because |shouldScrollToFragment| is false because
|didRestoreFromHistory| is set to true by restoreScrollPositionAndViewState().

However, after [1], restoreScrollPositionAndViewState() is NOT called in
checkCompleted() because [1] causes the document onload to be called after
finishedParsing(), and thus processFragment() causes scrolling unexpectedly,
not reflecting history.scrollRestoration = 'manual'.

This CL makes processFragment() to check scrollRestorationType explicitly,
not depending on previous restoreScrollPositionAndViewState() call.

[1] https://codereview.chromium.org/2269043002/.

BUG=624697, 417782
TEST=virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
==========

to

==========
Check scrollRestorationType explicitly in FrameLoader::processFragment()

This is preparation for [1], fixing test failure of
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
with [1].

Before [1], the test is passing because, in FrameLoader::finishedParsing(),
restoreScrollPositionAndViewState() is called (in checkCompleted()) before
processFragment(), and thus processFragment() doesn't cause scroll
because |shouldScrollToFragment| is false because
|didRestoreFromHistory| is set to true by restoreScrollPositionAndViewState().

However, after [1], restoreScrollPositionAndViewState() is NOT called in
checkCompleted() because [1] causes the document onload to be called after
finishedParsing(), and thus processFragment() causes scrolling unexpectedly,
not reflecting history.scrollRestoration = 'manual'.

This CL makes processFragment() to check scrollRestorationType explicitly,
not depending on previous restoreScrollPositionAndViewState() call.

[1] https://codereview.chromium.org/2269043002/.

BUG=624697, 417782
TEST=virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
==========

[commit-bot: I haz the power, 2016-11-07 09:06:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-07 09:08:20]

Description was changed from

==========
Check scrollRestorationType explicitly in FrameLoader::processFragment()

This is preparation for [1], fixing test failure of
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
with [1].

Before [1], the test is passing because, in FrameLoader::finishedParsing(),
restoreScrollPositionAndViewState() is called (in checkCompleted()) before
processFragment(), and thus processFragment() doesn't cause scroll
because |shouldScrollToFragment| is false because
|didRestoreFromHistory| is set to true by restoreScrollPositionAndViewState().

However, after [1], restoreScrollPositionAndViewState() is NOT called in
checkCompleted() because [1] causes the document onload to be called after
finishedParsing(), and thus processFragment() causes scrolling unexpectedly,
not reflecting history.scrollRestoration = 'manual'.

This CL makes processFragment() to check scrollRestorationType explicitly,
not depending on previous restoreScrollPositionAndViewState() call.

[1] https://codereview.chromium.org/2269043002/.

BUG=624697, 417782
TEST=virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
==========

to

==========
Check scrollRestorationType explicitly in FrameLoader::processFragment()

This is preparation for [1], fixing test failure of
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html
with [1].

Before [1], the test is passing because, in FrameLoader::finishedParsing(),
restoreScrollPositionAndViewState() is called (in checkCompleted()) before
processFragment(), and thus processFragment() doesn't cause scroll
because |shouldScrollToFragment| is false because
|didRestoreFromHistory| is set to true by restoreScrollPositionAndViewState().

However, after [1], restoreScrollPositionAndViewState() is NOT called in
checkCompleted() because [1] causes the document onload to be called after
finishedParsing(), and thus processFragment() causes scrolling unexpectedly,
not reflecting history.scrollRestoration = 'manual'.

This CL makes processFragment() to check scrollRestorationType explicitly,
not depending on previous restoreScrollPositionAndViewState() call.

[1] https://codereview.chromium.org/2269043002/.

BUG=624697, 417782
TEST=virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html

Committed: https://crrev.com/41ca771082edfcdbe304abbce42fce6de58c0657
Cr-Commit-Position: refs/heads/master@{#430237}
==========

[commit-bot: I haz the power, 2016-11-07 09:08:22]

Patchset 4 (id:??) landed as
https://crrev.com/41ca771082edfcdbe304abbce42fce6de58c0657
Cr-Commit-Position: refs/heads/master@{#430237}