cc: Make visible rect computation aware of pixel-moving filters

cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_CLIP) and uses such nodes to
account for the fact that certain kinds of effects have output that
depends on pixels that later get clipped out.

BUG=653631
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/5e48a2e0284b930c68e73f44ea15807a9ffab541
Cr-Original-Commit-Position: refs/heads/master@{#429050}
Review-Url: https://codereview.chromium.org/2423483003
Cr-Commit-Position: refs/heads/master@{#442652}
Committed: https://chromium.googlesource.com/chromium/src/+/e947a35bcd6dce91dc4651680c48f8b996f7ea5e

[ajuma, 2016-10-14 23:47:05]

Description was changed from

==========
WIP: Clip nodes that expand

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

to

==========
WIP: Clip nodes that expand

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

[ajuma, 2016-10-20 00:49:06]

Description was changed from

==========
WIP: Clip nodes that expand

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

to

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_VISIBLE_RECT) and uses such
nodes to account for the fact that certain kinds of effects have
output that depends on pixels that later get clipped out.

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

[ajuma, 2016-10-20 04:06:53]

Description was changed from

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_VISIBLE_RECT) and uses such
nodes to account for the fact that certain kinds of effects have
output that depends on pixels that later get clipped out.

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

to

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_VISIBLE_RECT) and uses such
nodes to account for the fact that certain kinds of effects have
output that depends on pixels that later get clipped out.

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

[ajuma, 2016-10-20 04:07:51]

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

[commit-bot: I haz the power, 2016-10-20 04:08:22]

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-10-20 04:35:33]

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

[commit-bot: I haz the power, 2016-10-20 04:35:34]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ajuma, 2016-10-20 17:33:53]

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

[commit-bot: I haz the power, 2016-10-20 17:34:27]

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-10-20 19:20:12]

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

[commit-bot: I haz the power, 2016-10-20 19:20:13]

Dry run: This issue passed the CQ dry run.

[ajuma, 2016-10-21 18:10:36]

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

[commit-bot: I haz the power, 2016-10-21 18:11:28]

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-10-21 19:52:24]

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

[commit-bot: I haz the power, 2016-10-21 19:52:25]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ajuma, 2016-10-21 23:11:00]

Description was changed from

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_VISIBLE_RECT) and uses such
nodes to account for the fact that certain kinds of effects have
output that depends on pixels that later get clipped out.

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

to

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_CLIP) and uses such nodes to
account for the fact that certain kinds of effects have output that
depends on pixels that later get clipped out.

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

[ajuma, 2016-10-25 15:32:26]

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

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

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-25 17:11:52]

The CQ bit was unchecked by ajuma@chromium.org

[ajuma, 2016-10-25 22:02:07]

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

[commit-bot: I haz the power, 2016-10-25 22:02:54]

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-10-25 23:40:03]

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

[commit-bot: I haz the power, 2016-10-25 23:40:04]

Dry run: This issue passed the CQ dry run.

[ajuma, 2016-10-26 12:47:29]

Description was changed from

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_CLIP) and uses such nodes to
account for the fact that certain kinds of effects have output that
depends on pixels that later get clipped out.

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

to

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_CLIP) and uses such nodes to
account for the fact that certain kinds of effects have output that
depends on pixels that later get clipped out.

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

[ajuma, 2016-10-26 12:47:30]

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

[weiliangc, 2016-10-26 16:57:04]

https://codereview.chromium.org/2423483003/diff/140001/cc/trees/clip_node.h
File cc/trees/clip_node.h (right):

https://codereview.chromium.org/2423483003/diff/140001/cc/trees/clip_node.h#n...
cc/trees/clip_node.h:56: ClipExpander clip_expander;
Why not a nullable unique_ptr? We could avoid default constructor and NONE type
for the clip expander. And to transfer through protobuf we could only transfer
the effect tree id and create it on the other side?

https://codereview.chromium.org/2423483003/diff/140001/cc/trees/draw_property...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2423483003/diff/140001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:178: static ConditionalClip
ComputeTargetRectInTargetSpace(
This functions seems more like "ConvertRectBetweenSurfaceSpaces" since inside
the function there isn't really a target, and the two target in the name is a
bit confusing.

https://codereview.chromium.org/2423483003/diff/140001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:255: bool include_expanding_clips,
This seem to be not used?

https://codereview.chromium.org/2423483003/diff/140001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:313: switch (clip_node->clip_type) {
Nit: maybe make a helper function that applies clip node to accumulated clip
here? Just so it looks shorter.

[ajuma, 2016-10-31 17:41:34]

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

[commit-bot: I haz the power, 2016-10-31 17:42:03]

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-31 17:43:07]

https://codereview.chromium.org/2423483003/diff/140001/cc/trees/clip_node.h
File cc/trees/clip_node.h (right):

https://codereview.chromium.org/2423483003/diff/140001/cc/trees/clip_node.h#n...
cc/trees/clip_node.h:56: ClipExpander clip_expander;
On 2016/10/26 16:57:03, weiliangc wrote:
> Why not a nullable unique_ptr? We could avoid default constructor and NONE
type
> for the clip expander. And to transfer through protobuf we could only transfer
> the effect tree id and create it on the other side?

Done. Also added the protobuf transfer logic and a test.

https://codereview.chromium.org/2423483003/diff/140001/cc/trees/draw_property...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2423483003/diff/140001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:178: static ConditionalClip
ComputeTargetRectInTargetSpace(
On 2016/10/26 16:57:03, weiliangc wrote:
> This functions seems more like "ConvertRectBetweenSurfaceSpaces" since inside
> the function there isn't really a target, and the two target in the name is a
> bit confusing.  

Done.

https://codereview.chromium.org/2423483003/diff/140001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:255: bool include_expanding_clips,
On 2016/10/26 16:57:03, weiliangc wrote:
> This seem to be not used?

Oops, I forgot to handle that case, thanks for catching that! Fixed now, with a
test.

https://codereview.chromium.org/2423483003/diff/140001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:295: while (clip_node->clip_type ==
ClipNode::ClipType::NONE &&
I wasn't handling the case where the first clip in the chain is an expander.
Fixed and added a test.

https://codereview.chromium.org/2423483003/diff/140001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:313: switch (clip_node->clip_type) {
On 2016/10/26 16:57:03, weiliangc wrote:
> Nit: maybe make a helper function that applies clip node to accumulated clip
> here? Just so it looks shorter.

Done.

[commit-bot: I haz the power, 2016-10-31 18:06:07]

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

[commit-bot: I haz the power, 2016-10-31 18:06:08]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[weiliangc, 2016-10-31 18:20:25]

https://codereview.chromium.org/2423483003/diff/200001/cc/trees/clip_expander.h
File cc/trees/clip_expander.h (right):

https://codereview.chromium.org/2423483003/diff/200001/cc/trees/clip_expander...
cc/trees/clip_expander.h:23: ClipExpander(ExpanderType type, int
filter_effect_id);
Is there an expander type that is not Filter that we want to add later?

[ajuma, 2016-10-31 18:28:05]

https://codereview.chromium.org/2423483003/diff/200001/cc/trees/clip_expander.h
File cc/trees/clip_expander.h (right):

https://codereview.chromium.org/2423483003/diff/200001/cc/trees/clip_expander...
cc/trees/clip_expander.h:23: ClipExpander(ExpanderType type, int
filter_effect_id);
On 2016/10/31 18:20:25, weiliangc wrote:
> Is there an expander type that is not Filter that we want to add later?

Yeah, I believe there are other kinds of expanding effects we'll need to handle
(like the pixel-snapping type effects that the paint folks were mentioning a
couple weeks ago). Leaving in the type for now makes it easier to add those (and
if it turns out that none get added we can just delete it later).

[weiliangc, 2016-10-31 18:41:56]

On 2016/10/31 18:28:05, ajuma wrote:
>
https://codereview.chromium.org/2423483003/diff/200001/cc/trees/clip_expander.h
> File cc/trees/clip_expander.h (right):
> 
>
https://codereview.chromium.org/2423483003/diff/200001/cc/trees/clip_expander...
> cc/trees/clip_expander.h:23: ClipExpander(ExpanderType type, int
> filter_effect_id);
> On 2016/10/31 18:20:25, weiliangc wrote:
> > Is there an expander type that is not Filter that we want to add later?
> 
> Yeah, I believe there are other kinds of expanding effects we'll need to
handle
> (like the pixel-snapping type effects that the paint folks were mentioning a
> couple weeks ago). Leaving in the type for now makes it easier to add those
(and
> if it turns out that none get added we can just delete it later).

Would pixel snapping expander have an effect node id as well?

I kinda prefer add the type later since we can discuss what adding pixel
snapping expander look like when we actually add it, WDYT?

[ajuma, 2016-10-31 18:53:34]

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

[commit-bot: I haz the power, 2016-10-31 18:54:05]

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-31 18:56:07]

On 2016/10/31 18:41:56, weiliangc wrote:
> On 2016/10/31 18:28:05, ajuma wrote:
> >
>
https://codereview.chromium.org/2423483003/diff/200001/cc/trees/clip_expander.h
> > File cc/trees/clip_expander.h (right):
> > 
> >
>
https://codereview.chromium.org/2423483003/diff/200001/cc/trees/clip_expander...
> > cc/trees/clip_expander.h:23: ClipExpander(ExpanderType type, int
> > filter_effect_id);
> > On 2016/10/31 18:20:25, weiliangc wrote:
> > > Is there an expander type that is not Filter that we want to add later?
> > 
> > Yeah, I believe there are other kinds of expanding effects we'll need to
> handle
> > (like the pixel-snapping type effects that the paint folks were mentioning a
> > couple weeks ago). Leaving in the type for now makes it easier to add those
> (and
> > if it turns out that none get added we can just delete it later).
> 
> Would pixel snapping expander have an effect node id as well?

I think so, since we'd need to know the space in which to apply the expansion.

> I kinda prefer add the type later since we can discuss what adding pixel
> snapping expander look like when we actually add it, WDYT?

That's a good point. Removed the type.

[commit-bot: I haz the power, 2016-10-31 19:34:54]

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

[commit-bot: I haz the power, 2016-10-31 19:34:56]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[weiliangc, 2016-11-01 17:46:24]

LGTM

[ajuma, 2016-11-01 17:47:06]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2016-11-01 17:47:21]

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-11-01 18:06:15]

Description was changed from

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_CLIP) and uses such nodes to
account for the fact that certain kinds of effects have output that
depends on pixels that later get clipped out.

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

to

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_CLIP) and uses such nodes to
account for the fact that certain kinds of effects have output that
depends on pixels that later get clipped out.

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

[commit-bot: I haz the power, 2016-11-01 18:06:20]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-11-01 18:24:54]

Description was changed from

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_CLIP) and uses such nodes to
account for the fact that certain kinds of effects have output that
depends on pixels that later get clipped out.

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

to

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_CLIP) and uses such nodes to
account for the fact that certain kinds of effects have output that
depends on pixels that later get clipped out.

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

Committed: https://crrev.com/5e48a2e0284b930c68e73f44ea15807a9ffab541
Cr-Commit-Position: refs/heads/master@{#429050}
==========

[commit-bot: I haz the power, 2016-11-01 18:24:55]

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

[kjellander_chromium, 2016-11-02 07:17:53]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2468113004/ by kjellander@chromium.org.

The reason for reverting is: Speculative revert for webkit_tests failure on
WebKit Win7 (dbg):
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%...
.

[ajuma, 2016-11-03 20:07:18]

Description was changed from

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_CLIP) and uses such nodes to
account for the fact that certain kinds of effects have output that
depends on pixels that later get clipped out.

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

Committed: https://crrev.com/5e48a2e0284b930c68e73f44ea15807a9ffab541
Cr-Commit-Position: refs/heads/master@{#429050}
==========

to

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_CLIP) and uses such nodes to
account for the fact that certain kinds of effects have output that
depends on pixels that later get clipped out.

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

Committed: https://crrev.com/5e48a2e0284b930c68e73f44ea15807a9ffab541
Cr-Commit-Position: refs/heads/master@{#429050}
==========

[ajuma, 2017-01-10 15:23:38]

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

[commit-bot: I haz the power, 2017-01-10 15:24:03]

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-01-10 15:25:26]

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

[commit-bot: I haz the power, 2017-01-10 15:25:27]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[ajuma, 2017-01-10 15:27:25]

Description was changed from

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_CLIP) and uses such nodes to
account for the fact that certain kinds of effects have output that
depends on pixels that later get clipped out.

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

Committed: https://crrev.com/5e48a2e0284b930c68e73f44ea15807a9ffab541
Cr-Commit-Position: refs/heads/master@{#429050}
==========

to

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_CLIP) and uses such nodes to
account for the fact that certain kinds of effects have output that
depends on pixels that later get clipped out.

BUG=653631
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/5e48a2e0284b930c68e73f44ea15807a9ffab541
Cr-Commit-Position: refs/heads/master@{#429050}
==========

[ajuma, 2017-01-10 15:27:45]

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

[commit-bot: I haz the power, 2017-01-10 15:28:15]

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

[ajuma, 2017-01-10 17:16:02]

Going to re-land this now. The crashes that caused it to get reverted were fixed
by https://codereview.chromium.org/2543473004. 

The test cases from bug 653631 still have disappearing reflections when partial
swap is enabled, but that's caused by a damage tracking issue in
SurfaceAggregator, so I'll fix that in a separate CL.

[ajuma, 2017-01-10 17:16:12]

The CQ bit was checked by ajuma@chromium.org

[ajuma, 2017-01-10 17:16:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
weiliangc@chromium.org
Link to the patchset: https://codereview.chromium.org/2423483003/#ps280001
(title: "It's 2017 -- update copyright for new files")

[commit-bot: I haz the power, 2017-01-10 17:16:28]

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-01-10 19:17:53]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1484068572030670,
"parent_rev": "cf853f861018bf40bb283f50f12cade69db0091e", "commit_rev":
"e947a35bcd6dce91dc4651680c48f8b996f7ea5e"}

[commit-bot: I haz the power, 2017-01-10 19:18:21]

Description was changed from

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_CLIP) and uses such nodes to
account for the fact that certain kinds of effects have output that
depends on pixels that later get clipped out.

BUG=653631
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/5e48a2e0284b930c68e73f44ea15807a9ffab541
Cr-Commit-Position: refs/heads/master@{#429050}
==========

to

==========
cc: Make visible rect computation aware of pixel-moving filters

This adds a new ClipNode type (EXPANDS_CLIP) and uses such nodes to
account for the fact that certain kinds of effects have output that
depends on pixels that later get clipped out.

BUG=653631
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/5e48a2e0284b930c68e73f44ea15807a9ffab541
Cr-Original-Commit-Position: refs/heads/master@{#429050}
Review-Url: https://codereview.chromium.org/2423483003
Cr-Commit-Position: refs/heads/master@{#442652}
Committed:
https://chromium.googlesource.com/chromium/src/+/e947a35bcd6dce91dc4651680c48...
==========

[commit-bot: I haz the power, 2017-01-10 19:18:24]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/e947a35bcd6dce91dc4651680c48...