Restore scale even if scroll is not restored

Restore scale even if scroll restoration is disabled

If scroll is being restored we will still restore
both scroll and scale together otherwise we restore
scale on its own.

Here is the rationale of decoupling scale and scroll restoration:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/p1sy9aQDmtU/jndSickDCAAJ


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

[majidvp, 2015-08-06 19:57:19]

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

[Nate Chapin, 2015-08-06 21:01:59]

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

[Nate Chapin, 2015-08-06 21:02:00]

Couple of code health and style nitpicks...

https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1137: if (shouldRestoreScale &&
shouldRestoreScroll) {
Style nit: this if/else block is all single line, so {} aren't necessary.

https://codereview.chromium.org/1277583003/diff/20001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/1277583003/diff/20001/Source/core/page/Page.c...
Source/core/page/Page.cpp:309: if (!mainFrame()->isLocalFrame())
This early exit looks like it's no longer necessary?

https://codereview.chromium.org/1277583003/diff/20001/Source/core/page/Page.c...
Source/core/page/Page.cpp:313: viewport.setScale(scale);
I think this function is now only called by Internals.cpp. Can we remove it and
call setScale() directly there?

[majidvp, 2015-08-06 21:25:36]

https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1137: if (shouldRestoreScale &&
shouldRestoreScroll) {
On 2015/08/06 21:02:00, Nate Chapin wrote:
> Style nit: this if/else block is all single line, so {} aren't necessary.

Acknowledged.

https://codereview.chromium.org/1277583003/diff/20001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/1277583003/diff/20001/Source/core/page/Page.c...
Source/core/page/Page.cpp:309: if (!mainFrame()->isLocalFrame())
On 2015/08/06 21:02:00, Nate Chapin wrote:
> This early exit looks like it's no longer necessary?

Acknowledged.

https://codereview.chromium.org/1277583003/diff/20001/Source/core/page/Page.c...
Source/core/page/Page.cpp:313: viewport.setScale(scale);
On 2015/08/06 21:02:00, Nate Chapin wrote:
> I think this function is now only called by Internals.cpp. Can we remove it
and
> call setScale() directly there?

That is true. The reasons I like to keep it are:
1- symmetry with getter pageScaleFactor()
2- hide visual viewport which is a page impl details from Internal.cpp

[Nate Chapin, 2015-08-06 21:28:54]

On 2015/08/06 21:25:36, majidvp wrote:
>
https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
> File Source/core/loader/FrameLoader.cpp (right):
> 
>
https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
> Source/core/loader/FrameLoader.cpp:1137: if (shouldRestoreScale &&
> shouldRestoreScroll) {
> On 2015/08/06 21:02:00, Nate Chapin wrote:
> > Style nit: this if/else block is all single line, so {} aren't necessary.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1277583003/diff/20001/Source/core/page/Page.cpp
> File Source/core/page/Page.cpp (right):
> 
>
https://codereview.chromium.org/1277583003/diff/20001/Source/core/page/Page.c...
> Source/core/page/Page.cpp:309: if (!mainFrame()->isLocalFrame())
> On 2015/08/06 21:02:00, Nate Chapin wrote:
> > This early exit looks like it's no longer necessary?
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1277583003/diff/20001/Source/core/page/Page.c...
> Source/core/page/Page.cpp:313: viewport.setScale(scale);
> On 2015/08/06 21:02:00, Nate Chapin wrote:
> > I think this function is now only called by Internals.cpp. Can we remove it
> and
> > call setScale() directly there?
> 
> That is true. The reasons I like to keep it are:
> 1- symmetry with getter pageScaleFactor()
> 2- hide visual viewport which is a page impl details from Internal.cpp

(1) is certainly a fair point, though I'm less convinced about (2). Moving both
setPageScaleFactor and pageScaleFactor off of Page is probably desirable
eventually, but doesn't need to happen now.

[bokan, 2015-08-07 15:08:02]

https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1146:
scrollingCoordinator->frameViewRootLayerDidChange(view);
Why is this no longer needed?

https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1120: if (shouldRestoreScroll &&
!canRestoreWithoutAnnoyingUser)
Are you sure we want to restore scale if it would "annoy the user"? If the user
started scrolling I think we shouldn't do anything.

https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1124:
view->setScrollPosition(m_currentItem->scrollPoint(), ProgrammaticScroll);
This should be view->layoutViewportScrollableArea()->setScrollPosition(...)

https://codereview.chromium.org/1277583003/diff/20001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/1277583003/diff/20001/Source/core/page/Page.c...
Source/core/page/Page.cpp:313: viewport.setScale(scale);
On 2015/08/06 21:25:36, majidvp wrote:
> On 2015/08/06 21:02:00, Nate Chapin wrote:
> > I think this function is now only called by Internals.cpp. Can we remove it
> and
> > call setScale() directly there?
> 
> That is true. The reasons I like to keep it are:
> 1- symmetry with getter pageScaleFactor()
> 2- hide visual viewport which is a page impl details from Internal.cpp

I agree with Nate, these methods should be removed. Though not necessary for
this patch.

https://codereview.chromium.org/1277583003/diff/20001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/1277583003/diff/20001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:214:
page->deprecatedLocalMainFrame()->view()->setScrollPosition(IntPoint(0, 0),
ProgrammaticScroll);
view()->layoutViewportScrollableArea()->setScrollPosition(...)

[majidvp, 2015-08-07 18:24:07]

PTAL

https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1146:
scrollingCoordinator->frameViewRootLayerDidChange(view);
On 2015/08/07 15:08:01, bokan wrote:
> Why is this no longer needed?

Added back. Actually this was a mistake as I thought VisualViewport will notify
scrollingcoordinator.
The call does more work that is needed and it may indeed be redundant 
(no test failed!) but I will look into that at a later point.

https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1120: if (shouldRestoreScroll &&
!canRestoreWithoutAnnoyingUser)
On 2015/08/07 15:08:01, bokan wrote:
> Are you sure we want to restore scale if it would "annoy the user"? If the
user
> started scrolling I think we shouldn't do anything.

That is reasonable. Updated the logic notto restore anything if user has 
scrolled. Perhaps we should do the same if user has scaled. 
I added a TODO for this.

https://codereview.chromium.org/1277583003/diff/20001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1124:
view->setScrollPosition(m_currentItem->scrollPoint(), ProgrammaticScroll);
On 2015/08/07 15:08:01, bokan wrote:
> This should be view->layoutViewportScrollableArea()->setScrollPosition(...)

Done.

https://codereview.chromium.org/1277583003/diff/20001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/1277583003/diff/20001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:214:
page->deprecatedLocalMainFrame()->view()->setScrollPosition(IntPoint(0, 0),
ProgrammaticScroll);
On 2015/08/07 15:08:01, bokan wrote:
> view()->layoutViewportScrollableArea()->setScrollPosition(...)

Done.

[bokan, 2015-08-07 19:31:00]

Code looks good, how about a test?

[majidvp, 2015-08-11 23:41:06]

On 2015/08/07 19:31:00, bokan wrote:
> Code looks good, how about a test?

PTAL. Added a test that verifies scale restoration is not impacted:
'fast/history/scroll-restoration/scroll-restoration-scale-not-impacted.html'

[bokan, 2015-08-13 00:37:11]

lgtm!

[majidvp, 2015-08-14 14:37:45]

On 2015/08/13 00:37:11, bokan wrote:
> lgtm!

japhet: friendly ping :)

[Nate Chapin, 2015-08-14 20:24:35]

lgtm

[majidvp, 2015-08-14 20:30:04]

The CQ bit was checked by majidvp@chromium.org

[commit-bot: I haz the power, 2015-08-14 20:30:28]

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

[commit-bot: I haz the power, 2015-08-14 22:53:13]

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

[commit-bot: I haz the power, 2015-08-14 22:53:14]

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

[majidvp, 2015-08-15 16:18:50]

The CQ bit was checked by majidvp@chromium.org

[commit-bot: I haz the power, 2015-08-15 16:18:58]

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

[commit-bot: I haz the power, 2015-08-15 18:04:27]

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

[commit-bot: I haz the power, 2015-08-15 18:04:29]

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

[majidvp, 2015-08-17 15:42:14]

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

[commit-bot: I haz the power, 2015-08-17 15:42:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1277583003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1277583003/120001

[majidvp, 2015-08-17 17:09:29]

The CQ bit was unchecked by majidvp@chromium.org

[majidvp, 2015-08-17 17:09:33]

The CQ bit was checked by majidvp@chromium.org

[majidvp, 2015-08-17 17:09:33]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1277583003/#ps120001
(title: "Use window.scroll{X,y}")

[commit-bot: I haz the power, 2015-08-17 17:09:47]

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

[commit-bot: I haz the power, 2015-08-17 17:35:46]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200652