Ignore redundant calls to ScrollAnchor::save.

Ignore redundant calls to ScrollAnchor::save.

Flex scrollers go through LayoutBlock::layout multiple times; we call save()
every time, but make only one call to restore() when we exit the outermost
DelayScrollPositionClampScope (see http://crrev.com/412450).

The second save() cleared ScrollAnchor::m_scrollAnchorDisablingStyleChanged,
because the first layout had cleared the corresponding LayoutObject bit.

BUG=641815
Committed: https://crrev.com/4217fd09fbd83f49bfa33288311192a1ef067361
Cr-Commit-Position: refs/heads/master@{#416143}

[skobes, 2016-09-01 03:42:02]

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

[commit-bot: I haz the power, 2016-09-01 03:42:13]

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

[skobes, 2016-09-01 03:46:15]

Description was changed from

==========
Ignore redundant calls to ScrollAnchor::save.

Flex scrollers go through LayoutBlock::layout multiple times in a single
(FrameView) layout pass.  We call save() every time, but make only one call to
restore() when we exit the outermost DelayScrollPositionClampScope (see
http://crrev.com/412450).

The second call to save() was resetting m_scrollAnchorDisablingStyleChanged on
the ScrollAnchor, because the corresponding bit on the LayoutObject had already
been cleared at the end of the first layout.

BUG=641815
==========

to

==========
Ignore redundant calls to ScrollAnchor::save.

Flex scrollers go through LayoutBlock::layout multiple times; we call save()
every time, but make only one call to restore() when we exit the outermost
DelayScrollPositionClampScope (see http://crrev.com/412450).

The second save() cleared ScrollAnchor::m_scrollAnchorDisablingStyleChanged,
because the first layout cleared the corresponding LayoutObject bit.

BUG=641815
==========

[skobes, 2016-09-01 03:46:33]

Description was changed from

==========
Ignore redundant calls to ScrollAnchor::save.

Flex scrollers go through LayoutBlock::layout multiple times; we call save()
every time, but make only one call to restore() when we exit the outermost
DelayScrollPositionClampScope (see http://crrev.com/412450).

The second save() cleared ScrollAnchor::m_scrollAnchorDisablingStyleChanged,
because the first layout cleared the corresponding LayoutObject bit.

BUG=641815
==========

to

==========
Ignore redundant calls to ScrollAnchor::save.

Flex scrollers go through LayoutBlock::layout multiple times; we call save()
every time, but make only one call to restore() when we exit the outermost
DelayScrollPositionClampScope (see http://crrev.com/412450).

The second save() cleared ScrollAnchor::m_scrollAnchorDisablingStyleChanged,
because the first layout had cleared the corresponding LayoutObject bit.

BUG=641815
==========

[skobes, 2016-09-01 05:34:31]

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

[ojan, 2016-09-01 05:39:48]

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

[ojan, 2016-09-01 05:39:49]

lgtm

https://codereview.chromium.org/2304493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ScrollAnchor.h (right):

https://codereview.chromium.org/2304493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ScrollAnchor.h:116: bool m_saved;
Might be nice to explain that this is to deal with multi-pass layout and to have
a TODO to explore changing scroll anchoring to be at frame boundaries and
removing this bool.

[commit-bot: I haz the power, 2016-09-01 05:58:45]

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

[commit-bot: I haz the power, 2016-09-01 05:58:46]

Dry run: This issue passed the CQ dry run.

[ymalik, 2016-09-01 14:13:04]

lgtm

https://codereview.chromium.org/2304493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right):

https://codereview.chromium.org/2304493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:249: m_saved = false;
Add a DCHECK that m_saved is true? We should never be calling restore without
save right?

[skobes, 2016-09-01 20:41:09]

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

[commit-bot: I haz the power, 2016-09-01 20:41:48]

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

[skobes, 2016-09-01 20:41:55]

Patchset #2 (id:20001) has been deleted

[skobes, 2016-09-01 20:42:19]

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

[commit-bot: I haz the power, 2016-09-01 20:42:56]

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

[skobes, 2016-09-01 20:47:21]

https://codereview.chromium.org/2304493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right):

https://codereview.chromium.org/2304493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:249: m_saved = false;
On 2016/09/01 14:13:04, ymalik wrote:
> Add a DCHECK that m_saved is true? We should never be calling restore without
> save right? 

I tried this, but it breaks tests due to nested FrameView layouts, and due to
the use of restore from PLSA::clampScrollPositionsAfterLayout (for scenarios
that don't involve DelayScrollPositionClampScope).

Instead I've added an early return in restore() to parallel the early return in
save().

https://codereview.chromium.org/2304493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ScrollAnchor.h (right):

https://codereview.chromium.org/2304493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ScrollAnchor.h:116: bool m_saved;
On 2016/09/01 05:39:48, ojan wrote:
> Might be nice to explain that this is to deal with multi-pass layout and to
have
> a TODO to explore changing scroll anchoring to be at frame boundaries and
> removing this bool.

Done.

[commit-bot: I haz the power, 2016-09-01 23:53:12]

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

[commit-bot: I haz the power, 2016-09-01 23:53:13]

Dry run: This issue passed the CQ dry run.

[skobes, 2016-09-02 00:23:51]

The CQ bit was checked by skobes@chromium.org

[skobes, 2016-09-02 00:23:51]

The patchset sent to the CQ was uploaded after l-g-t-m from ymalik@chromium.org,
ojan@chromium.org
Link to the patchset: https://codereview.chromium.org/2304493002/#ps40001
(title: "add TODO; skip restore if !m_saved")

[commit-bot: I haz the power, 2016-09-02 00:24:11]

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-09-02 00:28:18]

Description was changed from

==========
Ignore redundant calls to ScrollAnchor::save.

Flex scrollers go through LayoutBlock::layout multiple times; we call save()
every time, but make only one call to restore() when we exit the outermost
DelayScrollPositionClampScope (see http://crrev.com/412450).

The second save() cleared ScrollAnchor::m_scrollAnchorDisablingStyleChanged,
because the first layout had cleared the corresponding LayoutObject bit.

BUG=641815
==========

to

==========
Ignore redundant calls to ScrollAnchor::save.

Flex scrollers go through LayoutBlock::layout multiple times; we call save()
every time, but make only one call to restore() when we exit the outermost
DelayScrollPositionClampScope (see http://crrev.com/412450).

The second save() cleared ScrollAnchor::m_scrollAnchorDisablingStyleChanged,
because the first layout had cleared the corresponding LayoutObject bit.

BUG=641815
==========

[commit-bot: I haz the power, 2016-09-02 00:28:19]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-09-02 00:31:54]

Description was changed from

==========
Ignore redundant calls to ScrollAnchor::save.

Flex scrollers go through LayoutBlock::layout multiple times; we call save()
every time, but make only one call to restore() when we exit the outermost
DelayScrollPositionClampScope (see http://crrev.com/412450).

The second save() cleared ScrollAnchor::m_scrollAnchorDisablingStyleChanged,
because the first layout had cleared the corresponding LayoutObject bit.

BUG=641815
==========

to

==========
Ignore redundant calls to ScrollAnchor::save.

Flex scrollers go through LayoutBlock::layout multiple times; we call save()
every time, but make only one call to restore() when we exit the outermost
DelayScrollPositionClampScope (see http://crrev.com/412450).

The second save() cleared ScrollAnchor::m_scrollAnchorDisablingStyleChanged,
because the first layout had cleared the corresponding LayoutObject bit.

BUG=641815

Committed: https://crrev.com/4217fd09fbd83f49bfa33288311192a1ef067361
Cr-Commit-Position: refs/heads/master@{#416143}
==========

[commit-bot: I haz the power, 2016-09-02 00:31:55]

Patchset 2 (id:??) landed as
https://crrev.com/4217fd09fbd83f49bfa33288311192a1ef067361
Cr-Commit-Position: refs/heads/master@{#416143}