cc: Move filters to the effect tree

cc: Move filters to the effect tree

This removes filters from LayerImpl, and adds them to the
effect tree and to LayerImplTestProperties.

BUG=622410
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/cb2b7443459c111f28e8c44050d1137d3ee44719
Cr-Commit-Position: refs/heads/master@{#406927}

[ajuma, 2016-06-29 18:07:13]

Description was changed from

==========
WIP: cc: Move filters to the effect tree

BUG=
==========

to

==========
WIP: cc: Move filters to the effect tree

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ajuma, 2016-07-19 20:19:26]

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

[commit-bot: I haz the power, 2016-07-19 20:20:02]

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, 2016-07-19 20:20:04]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-19 21:55:31]

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

[commit-bot: I haz the power, 2016-07-19 21:55:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

[ajuma, 2016-07-20 18:06:16]

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

[commit-bot: I haz the power, 2016-07-20 18:06:50]

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, 2016-07-20 18:06:52]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[ajuma, 2016-07-20 18:34:05]

Description was changed from

==========
WIP: cc: Move filters to the effect tree

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Move filters to the effect tree

This removes filters from LayerImpl, and adds them to the
effect tree and to LayerImplTestProperties.

BUG=622410
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-07-20 19:04:32]

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

[commit-bot: I haz the power, 2016-07-20 19:04:33]

Dry run: This issue passed the CQ dry run.

[ajuma, 2016-07-20 19:23:19]

ajuma@chromium.org changed reviewers:
+ jaydasika@chromium.org, weiliangc@chromium.org

[jaydasika, 2016-07-20 20:53:06]

https://codereview.chromium.org/2110683004/diff/140001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2110683004/diff/140001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:669: node->filters == layer_id_to_filters.second)
Only elements that satisfy this if condition should be removed from the map
(https://codereview.chromium.org/2105173006/). Can you also add a corresponding
case in the test ?

https://codereview.chromium.org/2110683004/diff/140001/cc/trees/property_tree...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2110683004/diff/140001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:949: has_potential_filter_animation ||
should_create_render_surface;
Is this condition required ? Doesn't has_potential_filter_animation =>
should_create_render_surface ?

[jaydasika, 2016-07-20 21:00:36]

https://codereview.chromium.org/2110683004/diff/140001/cc/proto/property_tree...
File cc/proto/property_tree.proto (right):

https://codereview.chromium.org/2110683004/diff/140001/cc/proto/property_tree...
cc/proto/property_tree.proto:101: optional bool is_currently_animating_filter =
22;
Since filters and background filters are no longer in LayerImpl, I think the
TODO in layer.proto to add support to filters can move to this file ?

[weiliangc, 2016-07-21 01:06:38]

https://codereview.chromium.org/2110683004/diff/140001/cc/trees/property_tree...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2110683004/diff/140001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:971: node.filters = Filters(layer);
This would make a copy of the Filter owned by Layer/LayerImpl's test properties.
Would explicitly changing ownership be better? To avoid the case of changing
filter on layers when only using filters on effect node?

[ajuma, 2016-07-21 16:38:27]

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

[commit-bot: I haz the power, 2016-07-21 16:38:37]

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, 2016-07-21 16:38:39]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[ajuma, 2016-07-21 16:42:19]

https://codereview.chromium.org/2110683004/diff/140001/cc/proto/property_tree...
File cc/proto/property_tree.proto (right):

https://codereview.chromium.org/2110683004/diff/140001/cc/proto/property_tree...
cc/proto/property_tree.proto:101: optional bool is_currently_animating_filter =
22;
On 2016/07/20 21:00:36, jaydasika wrote:
> Since filters and background filters are no longer in LayerImpl, I think the
> TODO in layer.proto to add support to filters can move to this file ?

Copied the TODO. I think the one in layer.proto is still needed, given that
Blimp is serializing Layers but not property trees (so filters will still need
to be serialized with Layers).

https://codereview.chromium.org/2110683004/diff/140001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2110683004/diff/140001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:669: node->filters == layer_id_to_filters.second)
On 2016/07/20 20:53:06, jaydasika wrote:
> Only elements that satisfy this if condition should be removed from the map
> (https://codereview.chromium.org/2105173006/). Can you also add a
corresponding
> case in the test ?

Done.

https://codereview.chromium.org/2110683004/diff/140001/cc/trees/property_tree...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2110683004/diff/140001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:949: has_potential_filter_animation ||
should_create_render_surface;
On 2016/07/20 20:53:06, jaydasika wrote:
> Is this condition required ? Doesn't has_potential_filter_animation =>
> should_create_render_surface ?

Done. (It's surprising that has_potential_opacity_animation doesn't also imply
should_create_render_surface.)

https://codereview.chromium.org/2110683004/diff/140001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:971: node.filters = Filters(layer);
On 2016/07/21 01:06:38, weiliangc wrote:
> This would make a copy of the Filter owned by Layer/LayerImpl's test
properties.
> Would explicitly changing ownership be better? To avoid the case of changing
> filter on layers when only using filters on effect node?

If we transfer ownership rather than copying, we'll get stuck the next time we
need to rebuild property trees, since the old property trees (owning the filter)
will be gone, and the layer will no longer have a copy either.

[jaydasika, 2016-07-21 17:22:35]

lgtm % weiliangc

https://codereview.chromium.org/2110683004/diff/140001/cc/trees/property_tree...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2110683004/diff/140001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:949: has_potential_filter_animation ||
should_create_render_surface;
On 2016/07/21 16:42:18, ajuma wrote:
> On 2016/07/20 20:53:06, jaydasika wrote:
> > Is this condition required ? Doesn't has_potential_filter_animation =>
> > should_create_render_surface ?
> 
> Done. (It's surprising that has_potential_opacity_animation doesn't also imply
> should_create_render_surface.)

It sounds like a bug to me. For the case, current opacity is 1 and opacity is
animating we are not creating a render surface. I think we should. WDYT ?

[weiliangc, 2016-07-21 18:02:55]

On 2016/07/21 at 16:42:19, ajuma wrote:
>
https://codereview.chromium.org/2110683004/diff/140001/cc/proto/property_tree...
> File cc/proto/property_tree.proto (right):
> 
>
https://codereview.chromium.org/2110683004/diff/140001/cc/proto/property_tree...
> cc/proto/property_tree.proto:101: optional bool is_currently_animating_filter
= 22;
> On 2016/07/20 21:00:36, jaydasika wrote:
> > Since filters and background filters are no longer in LayerImpl, I think the
> > TODO in layer.proto to add support to filters can move to this file ?
> 
> Copied the TODO. I think the one in layer.proto is still needed, given that
Blimp is serializing Layers but not property trees (so filters will still need
to be serialized with Layers).
> 
>
https://codereview.chromium.org/2110683004/diff/140001/cc/trees/layer_tree_im...
> File cc/trees/layer_tree_impl.cc (right):
> 
>
https://codereview.chromium.org/2110683004/diff/140001/cc/trees/layer_tree_im...
> cc/trees/layer_tree_impl.cc:669: node->filters == layer_id_to_filters.second)
> On 2016/07/20 20:53:06, jaydasika wrote:
> > Only elements that satisfy this if condition should be removed from the map
> > (https://codereview.chromium.org/2105173006/). Can you also add a
corresponding
> > case in the test ?
> 
> Done.
> 
>
https://codereview.chromium.org/2110683004/diff/140001/cc/trees/property_tree...
> File cc/trees/property_tree_builder.cc (right):
> 
>
https://codereview.chromium.org/2110683004/diff/140001/cc/trees/property_tree...
> cc/trees/property_tree_builder.cc:949: has_potential_filter_animation ||
should_create_render_surface;
> On 2016/07/20 20:53:06, jaydasika wrote:
> > Is this condition required ? Doesn't has_potential_filter_animation =>
> > should_create_render_surface ?
> 
> Done. (It's surprising that has_potential_opacity_animation doesn't also imply
should_create_render_surface.)
> 
>
https://codereview.chromium.org/2110683004/diff/140001/cc/trees/property_tree...
> cc/trees/property_tree_builder.cc:971: node.filters = Filters(layer);
> On 2016/07/21 01:06:38, weiliangc wrote:
> > This would make a copy of the Filter owned by Layer/LayerImpl's test
properties.
> > Would explicitly changing ownership be better? To avoid the case of changing
> > filter on layers when only using filters on effect node?
> 
> If we transfer ownership rather than copying, we'll get stuck the next time we
need to rebuild property trees, since the old property trees (owning the filter)
will be gone, and the layer will no longer have a copy either.

I was worried that might lose some middle state of filter animation but forgot
animation controller would have enough knowledge.

LGTM

[ajuma, 2016-07-21 18:49:41]

https://codereview.chromium.org/2110683004/diff/140001/cc/trees/property_tree...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2110683004/diff/140001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:949: has_potential_filter_animation ||
should_create_render_surface;
On 2016/07/21 17:22:35, jaydasika wrote:
> On 2016/07/21 16:42:18, ajuma wrote:
> > On 2016/07/20 20:53:06, jaydasika wrote:
> > > Is this condition required ? Doesn't has_potential_filter_animation =>
> > > should_create_render_surface ?
> > 
> > Done. (It's surprising that has_potential_opacity_animation doesn't also
imply
> > should_create_render_surface.)
> 
> It sounds like a bug to me. For the case, current opacity is 1 and opacity is
> animating we are not creating a render surface. I think we should. WDYT ?

Yeah, it's a bug. I'll fix it in a separate CL.

It turns out that Blink happens to avoid hitting this bug since during opacity
animations, it changes opacity 1 to 1 - \epsilon. See
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/animation...
and
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol...

[ajuma, 2016-07-21 18:49:58]

The CQ bit was unchecked by ajuma@chromium.org

[ajuma, 2016-07-21 18:50:05]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2016-07-21 18:50:47]

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-07-21 18:50:49]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-21 19:11:30]

Description was changed from

==========
cc: Move filters to the effect tree

This removes filters from LayerImpl, and adds them to the
effect tree and to LayerImplTestProperties.

BUG=622410
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Move filters to the effect tree

This removes filters from LayerImpl, and adds them to the
effect tree and to LayerImplTestProperties.

BUG=622410
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-07-21 19:11:33]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-21 19:14:18]

Description was changed from

==========
cc: Move filters to the effect tree

This removes filters from LayerImpl, and adds them to the
effect tree and to LayerImplTestProperties.

BUG=622410
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Move filters to the effect tree

This removes filters from LayerImpl, and adds them to the
effect tree and to LayerImplTestProperties.

BUG=622410
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/cb2b7443459c111f28e8c44050d1137d3ee44719
Cr-Commit-Position: refs/heads/master@{#406927}
==========

[commit-bot: I haz the power, 2016-07-21 19:14:19]

Patchset 9 (id:??) landed as
https://crrev.com/cb2b7443459c111f28e8c44050d1137d3ee44719
Cr-Commit-Position: refs/heads/master@{#406927}