cc: Update scroll offset before property tree scrolling and animation.

cc: Update scroll offset before property tree scrolling and animation.

In LayerTreeHostInProcess::FinishCommitOnImplThread, scroll offset in
scroll tree was updated after updating scrolling and animation.

Previously we actively update scroll offset in transform tree when we
later pushing scroll offset map from main to impl. So transform node's
scroll offset is always up-to-date.

However in LayoutTests, we sometimes do not create pending tree. When
pushing directly from main to active, we fail to detect change if the
following criteria are met:

1) current_scroll_offset != new_scroll_offset;
2) new_scroll_offset.pending_value == new_scroll_offset.active_value.

This results in not updating the inner viewport scroll layer when we use
the same renderer and reset page scale factor.

BUG=648785
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/41781b5c4d26bc86779212a9dea21e36d0566622
Cr-Commit-Position: refs/heads/master@{#425353}

[sunxd, 2016-10-05 15:50:16]

Description was changed from

==========
cc: Update transform tree when pushing scroll offset from main to active.

The previous scroll tree scroll offset update logic only considers the
situations where we push from main to pending and pending to active.

However in LayoutTests, we sometimes do not create pending tree. When
pushing directly from main to active, we fail to detect change if the
following criteria are met:

1) current_scroll_offset != new_scroll_offset;
2) new_scroll_offset.pending_value == new_scroll_offset.active_value.

This results in not updating the inner viewport scroll layer when we use
the same renderer and reset page scale factor.

BUG=648785
==========

to

==========
cc: Update transform tree when pushing scroll offset from main to active.

The previous scroll tree scroll offset update logic only considers the
situations where we push from main to pending and pending to active.

However in LayoutTests, we sometimes do not create pending tree. When
pushing directly from main to active, we fail to detect change if the
following criteria are met:

1) current_scroll_offset != new_scroll_offset;
2) new_scroll_offset.pending_value == new_scroll_offset.active_value.

This results in not updating the inner viewport scroll layer when we use
the same renderer and reset page scale factor.

BUG=648785
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[sunxd, 2016-10-05 15:50:36]

Description was changed from

==========
cc: Update transform tree when pushing scroll offset from main to active.

The previous scroll tree scroll offset update logic only considers the
situations where we push from main to pending and pending to active.

However in LayoutTests, we sometimes do not create pending tree. When
pushing directly from main to active, we fail to detect change if the
following criteria are met:

1) current_scroll_offset != new_scroll_offset;
2) new_scroll_offset.pending_value == new_scroll_offset.active_value.

This results in not updating the inner viewport scroll layer when we use
the same renderer and reset page scale factor.

BUG=648785
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Update transform tree when pushing scroll offset from main to active.

The previous scroll tree scroll offset update logic only considers the
situations where we push from main to pending and pending to active.

However in LayoutTests, we sometimes do not create pending tree. When
pushing directly from main to active, we fail to detect change if the
following criteria are met:

1) current_scroll_offset != new_scroll_offset;
2) new_scroll_offset.pending_value == new_scroll_offset.active_value.

This results in not updating the inner viewport scroll layer when we use
the same renderer and reset page scale factor.

BUG=648785
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[sunxd, 2016-10-05 15:50:36]

sunxd@chromium.org changed reviewers:
+ ajuma@chromium.org, bokan@chromium.org, wangxianzhu@chromium.org

[sunxd, 2016-10-05 15:51:19]

PTAL.

Do we want a test case for this?

[ajuma, 2016-10-05 17:19:21]

On 2016/10/05 15:51:19, sunxd wrote:
> PTAL.
> 
> Do we want a test case for this?

Yes, please add a test.

https://codereview.chromium.org/2393213002/diff/1/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2393213002/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:1596:
new_scroll_offset_map->at(key)->Current(property_trees()->is_active);
I wonder if we're just missing a case here. In the old code (before
https://codereview.chromium.org/1736073002), we effectively had 3 cases
(main-to-pending, main-to-active, pending-to-active) but here we only have
to-pending and to-active. In the main-to-active case, what if we first call
PushFromMainThread and then call PushPendingToActive (and look at the return
values from both calls to compute |changed|)? Does that seem equivalent to what
the old LayerImpl::PushScrollOffset did in this case?

[sunxd, 2016-10-05 18:48:27]

https://codereview.chromium.org/2393213002/diff/1/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2393213002/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:1596:
new_scroll_offset_map->at(key)->Current(property_trees()->is_active);
On 2016/10/05 17:19:21, ajuma wrote:
> I wonder if we're just missing a case here. In the old code (before
> https://codereview.chromium.org/1736073002), we effectively had 3 cases
> (main-to-pending, main-to-active, pending-to-active) but here we only have
> to-pending and to-active. In the main-to-active case, what if we first call
> PushFromMainThread and then call PushPendingToActive (and look at the return
> values from both calls to compute |changed|)? Does that seem equivalent to
what
> the old LayerImpl::PushScrollOffset did in this case?

I think as long as we overwrite the scroll offset in the map with the one in the
new map, we end up with the following two situation:

1) main to active: the synced scroll offset is overwritten with clobbering
active value;
2) pending to active: the synced scroll offset is unchanged.

Then we push pending value to active. This should be the same behavior as before
the change except that we didn't check if the new value is the same as the
clobbered old value.

[ajuma, 2016-10-05 19:16:05]

https://codereview.chromium.org/2393213002/diff/1/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2393213002/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:1596:
new_scroll_offset_map->at(key)->Current(property_trees()->is_active);
On 2016/10/05 18:48:26, sunxd wrote:
> On 2016/10/05 17:19:21, ajuma wrote:
> > I wonder if we're just missing a case here. In the old code (before
> > https://codereview.chromium.org/1736073002), we effectively had 3 cases
> > (main-to-pending, main-to-active, pending-to-active) but here we only have
> > to-pending and to-active. In the main-to-active case, what if we first call
> > PushFromMainThread and then call PushPendingToActive (and look at the return
> > values from both calls to compute |changed|)? Does that seem equivalent to
> what
> > the old LayerImpl::PushScrollOffset did in this case?
> 
> I think as long as we overwrite the scroll offset in the map with the one in
the
> new map, we end up with the following two situation:
> 
> 1) main to active: the synced scroll offset is overwritten with clobbering
> active value;
> 2) pending to active: the synced scroll offset is unchanged.
> 
> Then we push pending value to active. This should be the same behavior as
before
> the change except that we didn't check if the new value is the same as the
> clobbered old value.

Ah, makes sense now, thanks for the explanation! Please put this into a comment,
since the logic is non-obvious.

[Xianzhu, 2016-10-05 19:18:26]

Thanks for the fix. Please also enable
paint/invalidation/fixed-right-bottom-in-page-scale.html.

[sunxd, 2016-10-07 16:34:24]

Description was changed from

==========
cc: Update transform tree when pushing scroll offset from main to active.

The previous scroll tree scroll offset update logic only considers the
situations where we push from main to pending and pending to active.

However in LayoutTests, we sometimes do not create pending tree. When
pushing directly from main to active, we fail to detect change if the
following criteria are met:

1) current_scroll_offset != new_scroll_offset;
2) new_scroll_offset.pending_value == new_scroll_offset.active_value.

This results in not updating the inner viewport scroll layer when we use
the same renderer and reset page scale factor.

BUG=648785
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Update scroll offset before property tree scrolling and animation.

In LayerTreeHostInProcess::FinishCommitOnImplThread, scroll offset in
scroll tree was updated after updating scrolling and animation.

Previously we actively update scroll offset in transform tree when we
later pushing scroll offset map from main to impl. So transform node's
scroll offset is always up-to-date.

However in LayoutTests, we sometimes do not create pending tree. When
pushing directly from main to active, we fail to detect change if the
following criteria are met:

1) current_scroll_offset != new_scroll_offset;
2) new_scroll_offset.pending_value == new_scroll_offset.active_value.

This results in not updating the inner viewport scroll layer when we use
the same renderer and reset page scale factor.

BUG=648785
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[sunxd, 2016-10-07 16:35:17]

I digged further and found that the root cause is the order of updating scroll
offset in property trees.

We used to update transform node's scroll offset first, which causes this bug.

[ajuma, 2016-10-07 18:38:30]

https://codereview.chromium.org/2393213002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_in_process.cc (right):

https://codereview.chromium.org/2393213002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_in_process.cc:430:
!task_runner_provider_->ImplThreadTaskRunner());
Instead of calling this directly from your test, can you use a LayerTreeTest
(e.g. LayerTreeHostUnitTest or LayerTreeHostScrollTest) where this gets called
in the usual way?

[sunxd, 2016-10-12 14:32:33]

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

[commit-bot: I haz the power, 2016-10-12 14:32:47]

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

[sunxd, 2016-10-12 14:33:02]

Rewrite cc unit test.

[sunxd, 2016-10-12 15:00:02]

PTAL

[sunxd, 2016-10-12 15:00:09]

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

[commit-bot: I haz the power, 2016-10-12 15:00:15]

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

[ajuma, 2016-10-12 15:03:23]

Thanks, lgtm

https://codereview.chromium.org/2393213002/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/2393213002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_scroll.cc:1997: if
(!layer_tree_host()->SourceFrameNumber()) {
nit: I think it's clearer to compare to 0 here (or use a switch statement)

https://codereview.chromium.org/2393213002/diff/40001/cc/trees/tree_synchroni...
File cc/trees/tree_synchronizer_unittest.cc (right):

https://codereview.chromium.org/2393213002/diff/40001/cc/trees/tree_synchroni...
cc/trees/tree_synchronizer_unittest.cc:29: #include "cc/trees/transform_node.h"
This change doesn't seem needed anymore

[sunxd, 2016-10-12 15:15:22]

The CQ bit was checked by sunxd@chromium.org

[sunxd, 2016-10-12 15:15:23]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2393213002/#ps70001
(title: "Nit change.")

[commit-bot: I haz the power, 2016-10-12 15:15:32]

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-10-12 17:31:01]

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

[commit-bot: I haz the power, 2016-10-12 17:31:02]

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

[sunxd, 2016-10-14 15:10:30]

The CQ bit was checked by sunxd@chromium.org

[commit-bot: I haz the power, 2016-10-14 15:10:48]

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-10-14 16:41:48]

Description was changed from

==========
cc: Update scroll offset before property tree scrolling and animation.

In LayerTreeHostInProcess::FinishCommitOnImplThread, scroll offset in
scroll tree was updated after updating scrolling and animation.

Previously we actively update scroll offset in transform tree when we
later pushing scroll offset map from main to impl. So transform node's
scroll offset is always up-to-date.

However in LayoutTests, we sometimes do not create pending tree. When
pushing directly from main to active, we fail to detect change if the
following criteria are met:

1) current_scroll_offset != new_scroll_offset;
2) new_scroll_offset.pending_value == new_scroll_offset.active_value.

This results in not updating the inner viewport scroll layer when we use
the same renderer and reset page scale factor.

BUG=648785
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Update scroll offset before property tree scrolling and animation.

In LayerTreeHostInProcess::FinishCommitOnImplThread, scroll offset in
scroll tree was updated after updating scrolling and animation.

Previously we actively update scroll offset in transform tree when we
later pushing scroll offset map from main to impl. So transform node's
scroll offset is always up-to-date.

However in LayoutTests, we sometimes do not create pending tree. When
pushing directly from main to active, we fail to detect change if the
following criteria are met:

1) current_scroll_offset != new_scroll_offset;
2) new_scroll_offset.pending_value == new_scroll_offset.active_value.

This results in not updating the inner viewport scroll layer when we use
the same renderer and reset page scale factor.

BUG=648785
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-10-14 16:41:51]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-10-14 16:44:14]

Description was changed from

==========
cc: Update scroll offset before property tree scrolling and animation.

In LayerTreeHostInProcess::FinishCommitOnImplThread, scroll offset in
scroll tree was updated after updating scrolling and animation.

Previously we actively update scroll offset in transform tree when we
later pushing scroll offset map from main to impl. So transform node's
scroll offset is always up-to-date.

However in LayoutTests, we sometimes do not create pending tree. When
pushing directly from main to active, we fail to detect change if the
following criteria are met:

1) current_scroll_offset != new_scroll_offset;
2) new_scroll_offset.pending_value == new_scroll_offset.active_value.

This results in not updating the inner viewport scroll layer when we use
the same renderer and reset page scale factor.

BUG=648785
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Update scroll offset before property tree scrolling and animation.

In LayerTreeHostInProcess::FinishCommitOnImplThread, scroll offset in
scroll tree was updated after updating scrolling and animation.

Previously we actively update scroll offset in transform tree when we
later pushing scroll offset map from main to impl. So transform node's
scroll offset is always up-to-date.

However in LayoutTests, we sometimes do not create pending tree. When
pushing directly from main to active, we fail to detect change if the
following criteria are met:

1) current_scroll_offset != new_scroll_offset;
2) new_scroll_offset.pending_value == new_scroll_offset.active_value.

This results in not updating the inner viewport scroll layer when we use
the same renderer and reset page scale factor.

BUG=648785
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/41781b5c4d26bc86779212a9dea21e36d0566622
Cr-Commit-Position: refs/heads/master@{#425353}
==========

[commit-bot: I haz the power, 2016-10-14 16:44:16]

Patchset 5 (id:??) landed as
https://crrev.com/41781b5c4d26bc86779212a9dea21e36d0566622
Cr-Commit-Position: refs/heads/master@{#425353}