New issue
Advanced search Search tips

Issue 743241 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Remove LayerImpl dependency from LayerTreeImpl::DidUpdateScrollOffset

Project Member Reported by pdr@chromium.org, Jul 14 2017

Issue description

This was attempted in https://chromium-review.googlesource.com/c/560584/ but got rolled out because the dcheck was failing on Android ( https://crbug.com/742039 ). We would still like to do this refactoring because DidUpdateScrollOffset should not depend on LayerImpl.
 
Labels: PaintTeamTriaged-20170717 BugSource-Team
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/80d5a3ec9a3f7a2a94bfaa36aacc62979322125a

commit 80d5a3ec9a3f7a2a94bfaa36aacc62979322125a
Author: pdr <pdr@chromium.org>
Date: Mon Jul 17 19:04:07 2017

Remove LayerImpl dependency from LayerTreeImpl::DidUpdateScrollOffset

This patch removes the LayerImpl references from DidUpdateScrollOffset.
Instead of looking up scroll and transform property nodes with Layers,
ElementId is used.

Tests have been modified to ensure property trees exist before updating
scroll offset. This should be true in all real cases but is not for
synthetic testing scenarios.

A bug has been fixed in UpdateScrollChildPosition where the scroll layer
was not actually scrollable.

This is a reland of [1] which was rolled out because the active tree can
have a scroll offset for an element id that is not present in the pending
tree. The comment in DidUpdateScrollOffset has been updated to reflect
this. This is tested in PushPropertiesToMirrorsCurrentScrollOffset.
[1] https://chromium-review.googlesource.com/c/560584/

Bug:  743241 ,  742039 
Change-Id: Ia13380d749843064f30de76ccfeeb5e72502e6b2
Reviewed-on: https://chromium-review.googlesource.com/572426
Reviewed-by: enne <enne@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487183}
[modify] https://crrev.com/80d5a3ec9a3f7a2a94bfaa36aacc62979322125a/cc/test/layer_test_common.h
[modify] https://crrev.com/80d5a3ec9a3f7a2a94bfaa36aacc62979322125a/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/80d5a3ec9a3f7a2a94bfaa36aacc62979322125a/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/80d5a3ec9a3f7a2a94bfaa36aacc62979322125a/cc/trees/layer_tree_impl.cc

Comment 3 by pdr@chromium.org, Jul 17 2017

Status: Fixed (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4131a1c1ef8bdde8e752353ad6e24771d3b866da

commit 4131a1c1ef8bdde8e752353ad6e24771d3b866da
Author: Philip Rogers <pdr@chromium.org>
Date: Fri Jul 21 19:36:50 2017

Revert "Remove LayerImpl dependency from LayerTreeImpl::DidUpdateScrollOffset"

This reverts commit 80d5a3ec9a3f7a2a94bfaa36aacc62979322125a.

Reason for revert: May be the cause of scroll offset crashes ( crbug.com/746718 )

Original change's description:
> Remove LayerImpl dependency from LayerTreeImpl::DidUpdateScrollOffset
> 
> This patch removes the LayerImpl references from DidUpdateScrollOffset.
> Instead of looking up scroll and transform property nodes with Layers,
> ElementId is used.
> 
> Tests have been modified to ensure property trees exist before updating
> scroll offset. This should be true in all real cases but is not for
> synthetic testing scenarios.
> 
> A bug has been fixed in UpdateScrollChildPosition where the scroll layer
> was not actually scrollable.
> 
> This is a reland of [1] which was rolled out because the active tree can
> have a scroll offset for an element id that is not present in the pending
> tree. The comment in DidUpdateScrollOffset has been updated to reflect
> this. This is tested in PushPropertiesToMirrorsCurrentScrollOffset.
> [1] https://chromium-review.googlesource.com/c/560584/
> 
> Bug:  743241 ,  742039 
> Change-Id: Ia13380d749843064f30de76ccfeeb5e72502e6b2
> Reviewed-on: https://chromium-review.googlesource.com/572426
> Reviewed-by: enne <enne@chromium.org>
> Commit-Queue: Philip Rogers <pdr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#487183}

TBR=pdr@chromium.org,enne@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  743241 ,  746718 
Change-Id: I3a02c7a22c6db2d2e3eafcb83eace9e37db8c1d6
Reviewed-on: https://chromium-review.googlesource.com/581629
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488737}
[modify] https://crrev.com/4131a1c1ef8bdde8e752353ad6e24771d3b866da/cc/test/layer_test_common.h
[modify] https://crrev.com/4131a1c1ef8bdde8e752353ad6e24771d3b866da/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/4131a1c1ef8bdde8e752353ad6e24771d3b866da/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/4131a1c1ef8bdde8e752353ad6e24771d3b866da/cc/trees/layer_tree_impl.cc

Comment 5 by pdr@chromium.org, Jul 27 2017

Status: Started (was: Fixed)

Comment 6 by pdr@chromium.org, Jul 27 2017

Cc: pdr@chromium.org petermayo@chromium.org
 Issue 749818  has been merged into this issue.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d09da8ef0ef6295d8a7e56147f22c0c70d4369ff

commit d09da8ef0ef6295d8a7e56147f22c0c70d4369ff
Author: pdr <pdr@chromium.org>
Date: Tue Aug 01 17:54:52 2017

Remove LayerImpl dependency from DidUpdateScrollOffset

This patch removes the LayerImpl references from
DidUpdateScrollOffset. Instead of looking up scroll and
transform property nodes with Layers, ElementId is used.

Tests have been modified to ensure property trees exist before updating
scroll offset. This should be true in all real cases but is not for
synthetic testing scenarios.

A bug has been fixed in UpdateScrollChildPosition where the scroll layer
was not actually scrollable.

This is a reland of [1] which was rolled out due to flakiness on
telemetry_perf_unittests. This flakiness has been fixed by [2].

[1] itself is a reland of [3] which was rolled out because the active tree
can have a scroll offset for an element id that is not present in the
pending tree. The comment in DidUpdateScrollOffset has been updated to
reflect this. This is tested in PushPropertiesToMirrorsCurrentScrollOffset.

[1] https://chromium.googlesource.com/chromium/src/+/80d5a3ec9a3f7a2a94bfaa36aacc62979322125a
[2] https://chromium.googlesource.com/chromium/src/+/79fe848764520df822d23d9a234a7710a7ebfe83
[3] https://chromium.googlesource.com/chromium/src/+/5657b4033dc807c8e34a76ce44f3d1e1c3afe174

TBR=enne@chromium.org

Bug:  746718 ,  743241 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iff2431887585b624264e5699ce0373f68f458929
Reviewed-on: https://chromium-review.googlesource.com/594707
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491048}
[modify] https://crrev.com/d09da8ef0ef6295d8a7e56147f22c0c70d4369ff/cc/test/layer_test_common.h
[modify] https://crrev.com/d09da8ef0ef6295d8a7e56147f22c0c70d4369ff/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/d09da8ef0ef6295d8a7e56147f22c0c70d4369ff/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/d09da8ef0ef6295d8a7e56147f22c0c70d4369ff/cc/trees/layer_tree_impl.cc

Comment 8 by pdr@chromium.org, Aug 2 2017

Status: Fixed (was: Started)

Sign in to add a comment