cc: LayerTreeHostImpl uses element id to tick animations

cc: LayerTreeHostImpl uses element id to tick animations

Remove converting to layer id when animation is ticked with element id
on LayerTreeHostImpl. Also avoids passing in LayerTreeImpl pointer to
property trees.

Opacity animation on scrollbar still uses layer id, this is due to
scrollbars aren't guaranteed with element id. Work is needed in
crbug.com/702832 to address this.

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2762123004
Cr-Commit-Position: refs/heads/master@{#460423}
Committed: https://chromium.googlesource.com/chromium/src/+/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e

[weiliangc, 2017-03-21 21:16:37]

Description was changed from

==========
cc: LayerTreeHostImpl uses element id to tick animations

Remove converting to layer id when animation is ticked with element id
on LayerTreeHostImpl. Also avoids passing in LayerTreeImpl pointer to
property trees

BUG=702774
R=ajuma
==========

to

==========
cc: LayerTreeHostImpl uses element id to tick animations

Remove converting to layer id when animation is ticked with element id
on LayerTreeHostImpl. Also avoids passing in LayerTreeImpl pointer to
property trees

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[weiliangc, 2017-03-21 21:16:48]

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

[commit-bot: I haz the power, 2017-03-21 21:17:27]

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

[ajuma, 2017-03-21 22:00:30]

https://codereview.chromium.org/2762123004/diff/20001/cc/layers/scrollbar_lay...
File cc/layers/scrollbar_layer_impl_base.cc (right):

https://codereview.chromium.org/2762123004/diff/20001/cc/layers/scrollbar_lay...
cc/layers/scrollbar_layer_impl_base.cc:223:
property_trees->effect_tree.Node(effect_tree_index())) {
Since we're already checking for an invalid id above do we still need this null
check?

https://codereview.chromium.org/2762123004/diff/20001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/2762123004/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:155: void SetTreeLayerFilterMutated(ElementId
element_id,
Wdyt of removing 'TreeLayer' from these names since they just involve property
trees rather than layers now?

[weiliangc, 2017-03-22 18:44:07]

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

[weiliangc, 2017-03-22 18:44:15]

https://codereview.chromium.org/2762123004/diff/20001/cc/layers/scrollbar_lay...
File cc/layers/scrollbar_layer_impl_base.cc (right):

https://codereview.chromium.org/2762123004/diff/20001/cc/layers/scrollbar_lay...
cc/layers/scrollbar_layer_impl_base.cc:223:
property_trees->effect_tree.Node(effect_tree_index())) {
On 2017/03/21 at 22:00:30, ajuma wrote:
> Since we're already checking for an invalid id above do we still need this
null check?

I early out for the DCHECK now because when the layer list and the property tree
are not synced up we could be accessing a non existence node here.

https://codereview.chromium.org/2762123004/diff/20001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/2762123004/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:155: void SetTreeLayerFilterMutated(ElementId
element_id,
On 2017/03/21 at 22:00:30, ajuma wrote:
> Wdyt of removing 'TreeLayer' from these names since they just involve property
trees rather than layers now?

Done.

[commit-bot: I haz the power, 2017-03-22 18:45:14]

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, 2017-03-22 18:49:01]

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

[commit-bot: I haz the power, 2017-03-22 18:49:03]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[weiliangc, 2017-03-22 18:52:25]

Err need to rebase. Ignore all the red try bots it's fine.

[ajuma, 2017-03-22 19:00:18]

Thanks, lgtm

[weiliangc, 2017-03-27 15:34:13]

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

[commit-bot: I haz the power, 2017-03-27 15:34:53]

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

[weiliangc, 2017-03-27 15:38:07]

weiliangc@chromium.org changed reviewers:
+ pdr@google.com

[weiliangc, 2017-03-27 15:38:08]

+pdr for Blink review.

There are also ScrollOffset functions that looks could be refactored similarly.
I am leaving those alone since I heard pdr@ you are working on element ID +
scroll tree?

[commit-bot: I haz the power, 2017-03-27 18:15:48]

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

[commit-bot: I haz the power, 2017-03-27 18:15:49]

Dry run: This issue passed the CQ dry run.

[pdr., 2017-03-27 21:21:26]

pdr@chromium.org changed reviewers:
+ pdr@chromium.org, wkorman@chromium.org

[pdr., 2017-03-27 21:21:28]

LGTM here, nice cleanup.

I've added wkorman too, who is working all over this space. Could you please
wait for his review?

https://codereview.chromium.org/2762123004/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2762123004/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:1329:
transform_tree.FindNodeFromOwningLayerId(root_layer->id());
Do we need to add an owning layer id dependency here? In other words, can this
be refactored to not use FindNodeFromOwningLayerId?

[wkorman, 2017-03-27 22:21:52]

lgtm

https://codereview.chromium.org/2762123004/diff/80001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (left):

https://codereview.chromium.org/2762123004/diff/80001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:642:
element_id_to_opacity_animations_[layer->element_id()] = opacity;
Presuming opacity can't be addressed here due to pending further work on overlay
scrollbars? If so worth updating CL description to note the opacity animation
exception.

https://codereview.chromium.org/2762123004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp
(right):

https://codereview.chromium.org/2762123004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp:26:
m_mainLayer->layer_tree_impl()->SetOpacityMutated(m_mainLayer->element_id(),
How feasible to add unit tests for this and below to CompositorMutableStateTest?

[weiliangc, 2017-03-28 20:18:23]

Description was changed from

==========
cc: LayerTreeHostImpl uses element id to tick animations

Remove converting to layer id when animation is ticked with element id
on LayerTreeHostImpl. Also avoids passing in LayerTreeImpl pointer to
property trees

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: LayerTreeHostImpl uses element id to tick animations

Remove converting to layer id when animation is ticked with element id
on LayerTreeHostImpl. Also avoids passing in LayerTreeImpl pointer to
property trees.

Opacity animation on scrollbar still uses layer id, this is due to
scrollbars aren't guaranteed with element id. Work is needed in
crbug.com/702832 to address this.

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[weiliangc, 2017-03-28 20:21:28]

Address review comments.

https://codereview.chromium.org/2762123004/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2762123004/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:1329:
transform_tree.FindNodeFromOwningLayerId(root_layer->id());
On 2017/03/27 21:21:27, pdr. wrote:
> Do we need to add an owning layer id dependency here? In other words, can this
> be refactored to not use FindNodeFromOwningLayerId?

Done.

https://codereview.chromium.org/2762123004/diff/80001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (left):

https://codereview.chromium.org/2762123004/diff/80001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:642:
element_id_to_opacity_animations_[layer->element_id()] = opacity;
On 2017/03/27 22:21:52, wkorman wrote:
> Presuming opacity can't be addressed here due to pending further work on
overlay
> scrollbars? If so worth updating CL description to note the opacity animation
> exception.

Done.

https://codereview.chromium.org/2762123004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp
(right):

https://codereview.chromium.org/2762123004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp:26:
m_mainLayer->layer_tree_impl()->SetOpacityMutated(m_mainLayer->element_id(),
On 2017/03/27 22:21:52, wkorman wrote:
> How feasible to add unit tests for this and below to
CompositorMutableStateTest?

I assume this is more along the line of the CompositorMutableStateTest would
continue to pass. This test looks like it's testing setOpacity and setTransform
functions:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

[weiliangc, 2017-03-28 20:21:35]

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

[commit-bot: I haz the power, 2017-03-28 20:22:10]

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, 2017-03-28 21:16:12]

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

[commit-bot: I haz the power, 2017-03-28 21:16:13]

Dry run: CQ has no permission to schedule in bucket
master.tryserver.chromium.linux

[wkorman, 2017-03-28 23:32:47]

lgtm

https://codereview.chromium.org/2762123004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp
(right):

https://codereview.chromium.org/2762123004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp:26:
m_mainLayer->layer_tree_impl()->SetOpacityMutated(m_mainLayer->element_id(),
On 2017/03/28 20:21:28, weiliangc wrote:
> On 2017/03/27 22:21:52, wkorman wrote:
> > How feasible to add unit tests for this and below to
> CompositorMutableStateTest?
> 
> I assume this is more along the line of the CompositorMutableStateTest would
> continue to pass. This test looks like it's testing setOpacity and
setTransform
> functions:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

Oh cool, I looked but missed that somehow, thanks.

[weiliangc, 2017-03-29 14:49:13]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2017-03-29 14:49:14]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2762123004/#ps100001
(title: "review comments")

[commit-bot: I haz the power, 2017-03-29 14:49:53]

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, 2017-03-29 16:43:30]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490798953747820,
"parent_rev": "ad86bd0055e9755857af84d5216b3c81ddd23e9e", "commit_rev":
"ea09f3374e3fc4f7669390b1203b1354c3a9cd0e"}

[commit-bot: I haz the power, 2017-03-29 16:44:15]

Description was changed from

==========
cc: LayerTreeHostImpl uses element id to tick animations

Remove converting to layer id when animation is ticked with element id
on LayerTreeHostImpl. Also avoids passing in LayerTreeImpl pointer to
property trees.

Opacity animation on scrollbar still uses layer id, this is due to
scrollbars aren't guaranteed with element id. Work is needed in
crbug.com/702832 to address this.

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: LayerTreeHostImpl uses element id to tick animations

Remove converting to layer id when animation is ticked with element id
on LayerTreeHostImpl. Also avoids passing in LayerTreeImpl pointer to
property trees.

Opacity animation on scrollbar still uses layer id, this is due to
scrollbars aren't guaranteed with element id. Work is needed in
crbug.com/702832 to address this.

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2762123004
Cr-Commit-Position: refs/heads/master@{#460423}
Committed:
https://chromium.googlesource.com/chromium/src/+/ea09f3374e3fc4f7669390b1203b...
==========

[commit-bot: I haz the power, 2017-03-29 16:44:16]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/ea09f3374e3fc4f7669390b1203b...

[weiliangc, 2017-03-31 21:04:23]

https://codereview.chromium.org/2762123004/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/2762123004/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:4047:
property_trees->element_id_to_effect_node_index[element_id];
I think this line from before this CL is how crbug.com/706766 was hidden before.
>___>