Move cc::ClipNode::target_is_clipped to cc::EffectNode

Move cc::ClipNode::target_is_clipped to cc::EffectNode

The flag means whether the clip node's target surface will be subject to
some ancestor clip. Move it to the effect node and access it through
ClipNode::target_effect_id instead.

This lifts the restriction that every effect node needs to associate with
a clip node created by the same owner. Thus will unblock
https://codereview.chromium.org/2495973002/

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[trchen, 2016-11-10 23:58:27]

Description was changed from

==========
Move cc::ClipNode::target_is_clipped to cc::EffectNode

The flag means whether the clip node's target surface will be subject to
some ancestor clip. Move it to the effect node and access it through
ClipNode::target_effect_id instead.

This gets us one step closer to decoupling clip nodes from target effect,
and not requiring a dummy clip for each effect.
==========

to

==========
Move cc::ClipNode::target_is_clipped to cc::EffectNode

The flag means whether the clip node's target surface will be subject to
some ancestor clip. Move it to the effect node and access it through
ClipNode::target_effect_id instead.

This gets us one step closer to decoupling clip nodes from target effect,
and not requiring a dummy clip for each effect.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[trchen, 2016-11-10 23:58:31]

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

[commit-bot: I haz the power, 2016-11-10 23:59:06]

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

[trchen, 2016-11-10 23:59:15]

trchen@chromium.org changed reviewers:
+ ajuma@chromium.org, sunxd@chromium.org

[commit-bot: I haz the power, 2016-11-11 00:11:46]

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

[commit-bot: I haz the power, 2016-11-11 00:11:47]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[trchen, 2016-11-11 23:06:49]

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

[commit-bot: I haz the power, 2016-11-11 23:07: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-11-12 00:01:12]

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

[commit-bot: I haz the power, 2016-11-12 00:01:12]

Dry run: This issue passed the CQ dry run.

[trchen, 2016-11-15 21:59:51]

trchen@chromium.org changed reviewers:
+ weiliangc@chromium.org

[trchen, 2016-11-15 21:59:52]

+weiliangc

[weiliangc, 2016-11-15 23:07:49]

Ideally this field needs to be removed.

The best approach I see is to generate this with target_id from effect node and
not use clip node's target_clip_id, compare with current value for
SurfaceIsClipped, and remove this when we are confident with the result.

Is there any particular reason you would want to move this field to effect tree?

[trchen, 2016-11-15 23:51:45]

It will lift the restriction that effect node must associate with a clip
node owned by the same layer. Actually it blocks one of my other CL.

Yea, I too feel this should be a render surface / render pass thing. I
don't know how easy is it to remove it though. Just did the easiest thing
to unblock myself. :)

On Tuesday, November 15, 2016, weiliangc@chromium.org
<javascript:_e(%7B%7D,'cvml','weiliangc@chromium.org');> via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com
<javascript:_e(%7B%7D,'cvml','reply@chromiumcodereview-hr.appspotmail.com');>>
wrote:

> Ideally this field needs to be removed.
>
> The best approach I see is to generate this with target_id from effect
> node and
> not use clip node's target_clip_id, compare with current value for
> SurfaceIsClipped, and remove this when we are confident with the result.
>
> Is there any particular reason you would want to move this field to effect
> tree?
>
> https://codereview.chromium.org/2490273004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[weiliangc, 2016-11-16 17:04:44]

On 2016/11/15 at 23:51:45, trchen wrote:
> It will lift the restriction that effect node must associate with a clip
> node owned by the same layer. Actually it blocks one of my other CL.
> 
> Yea, I too feel this should be a render surface / render pass thing. I
> don't know how easy is it to remove it though. Just did the easiest thing
> to unblock myself. :)
> 
> On Tuesday, November 15, 2016, weiliangc@chromium.org
> <javascript:_e(%7B%7D,'cvml','weiliangc@chromium.org');> via
> codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com
> <javascript:_e(%7B%7D,'cvml','reply@chromiumcodereview-hr.appspotmail.com');>>
> wrote:
> 
> > Ideally this field needs to be removed.
> >
> > The best approach I see is to generate this with target_id from effect
> > node and
> > not use clip node's target_clip_id, compare with current value for
> > SurfaceIsClipped, and remove this when we are confident with the result.
> >
> > Is there any particular reason you would want to move this field to effect
> > tree?
> >
> > https://codereview.chromium.org/2490273004/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 

Could you add the patch being unblocked by this to the description and maybe add
a TODO in the code?

Thanks.

[trchen, 2016-11-16 20:25:20]

Description was changed from

==========
Move cc::ClipNode::target_is_clipped to cc::EffectNode

The flag means whether the clip node's target surface will be subject to
some ancestor clip. Move it to the effect node and access it through
ClipNode::target_effect_id instead.

This gets us one step closer to decoupling clip nodes from target effect,
and not requiring a dummy clip for each effect.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Move cc::ClipNode::target_is_clipped to cc::EffectNode

The flag means whether the clip node's target surface will be subject to
some ancestor clip. Move it to the effect node and access it through
ClipNode::target_effect_id instead.

This lifts the restriction that every effect node needs to associate with
a clip node created by the same owner. Thus will unblock
https://codereview.chromium.org/2495973002/

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

[trchen, 2016-11-16 20:39:42]

On 2016/11/16 17:04:44, weiliangc wrote:
> On 2016/11/15 at 23:51:45, trchen wrote:
> > It will lift the restriction that effect node must associate with a clip
> > node owned by the same layer. Actually it blocks one of my other CL.
> > 
> > Yea, I too feel this should be a render surface / render pass thing. I
> > don't know how easy is it to remove it though. Just did the easiest thing
> > to unblock myself. :)
> > 
> > On Tuesday, November 15, 2016, mailto:weiliangc@chromium.org
> > <javascript:_e(%7B%7D,'cvml','weiliangc@chromium.org');> via
> > http://codereview.chromium.org
<mailto:reply@chromiumcodereview-hr.appspotmail.com
> >
<javascript:_e(%7B%7D,'cvml','reply@chromiumcodereview-hr.appspotmail.com');>>
> > wrote:
> > 
> > > Ideally this field needs to be removed.
> > >
> > > The best approach I see is to generate this with target_id from effect
> > > node and
> > > not use clip node's target_clip_id, compare with current value for
> > > SurfaceIsClipped, and remove this when we are confident with the result.
> > >
> > > Is there any particular reason you would want to move this field to effect
> > > tree?
> > >
> > > https://codereview.chromium.org/2490273004/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email to mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> 
> Could you add the patch being unblocked by this to the description and maybe
add
> a TODO in the code?
> 
> Thanks.

Sure & done! Let me know if the new comment makes sense to you. Thanks!

[jaydasika, 2016-11-17 23:31:29]

On 2016/11/16 20:39:42, trchen wrote:
> On 2016/11/16 17:04:44, weiliangc wrote:
> > On 2016/11/15 at 23:51:45, trchen wrote:
> > > It will lift the restriction that effect node must associate with a clip
> > > node owned by the same layer. Actually it blocks one of my other CL.
> > > 
> > > Yea, I too feel this should be a render surface / render pass thing. I
> > > don't know how easy is it to remove it though. Just did the easiest thing
> > > to unblock myself. :)
> > > 
> > > On Tuesday, November 15, 2016, mailto:weiliangc@chromium.org
> > > <javascript:_e(%7B%7D,'cvml','weiliangc@chromium.org');> via
> > > http://codereview.chromium.org
> <mailto:reply@chromiumcodereview-hr.appspotmail.com
> > >
> <javascript:_e(%7B%7D,'cvml','reply@chromiumcodereview-hr.appspotmail.com');>>
> > > wrote:
> > > 
> > > > Ideally this field needs to be removed.
> > > >
> > > > The best approach I see is to generate this with target_id from effect
> > > > node and
> > > > not use clip node's target_clip_id, compare with current value for
> > > > SurfaceIsClipped, and remove this when we are confident with the result.
> > > >
> > > > Is there any particular reason you would want to move this field to
effect
> > > > tree?
> > > >
> > > > https://codereview.chromium.org/2490273004/
> > > >
> > > 
> > > -- 
> > > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email to mailto:chromium-reviews+unsubscribe@chromium.org.
> > > 
> > 
> > Could you add the patch being unblocked by this to the description and maybe
> add
> > a TODO in the code?
> > 
> > Thanks.
> 
> Sure & done! Let me know if the new comment makes sense to you. Thanks!

trchen : Thanks for doing this. I am currently looking at removing creation of
clip nodes for render surfaces and this is required for that too!

weiliangc : I did not understand what you meant by "generate this with target_id
from effect node and
not use clip node's target_clip_id" (from #14). Can you elaborate ?

[weiliangc, 2016-11-18 16:27:10]

On 2016/11/17 at 23:31:29, jaydasika wrote:
> On 2016/11/16 20:39:42, trchen wrote:
> > On 2016/11/16 17:04:44, weiliangc wrote:
> > > On 2016/11/15 at 23:51:45, trchen wrote:
> > > > It will lift the restriction that effect node must associate with a clip
> > > > node owned by the same layer. Actually it blocks one of my other CL.
> > > > 
> > > > Yea, I too feel this should be a render surface / render pass thing. I
> > > > don't know how easy is it to remove it though. Just did the easiest
thing
> > > > to unblock myself. :)
> > > > 
> > > > On Tuesday, November 15, 2016, mailto:weiliangc@chromium.org
> > > > <javascript:_e(%7B%7D,'cvml','weiliangc@chromium.org');> via
> > > > http://codereview.chromium.org
> > <mailto:reply@chromiumcodereview-hr.appspotmail.com
> > > >
> >
<javascript:_e(%7B%7D,'cvml','reply@chromiumcodereview-hr.appspotmail.com');>>
> > > > wrote:
> > > > 
> > > > > Ideally this field needs to be removed.
> > > > >
> > > > > The best approach I see is to generate this with target_id from effect
> > > > > node and
> > > > > not use clip node's target_clip_id, compare with current value for
> > > > > SurfaceIsClipped, and remove this when we are confident with the
result.
> > > > >
> > > > > Is there any particular reason you would want to move this field to
effect
> > > > > tree?
> > > > >
> > > > > https://codereview.chromium.org/2490273004/
> > > > >
> > > > 
> > > > -- 
> > > > You received this message because you are subscribed to the Google
Groups
> > > "Chromium-reviews" group.
> > > > To unsubscribe from this group and stop receiving emails from it, send
an
> > > email to mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > 
> > > 
> > > Could you add the patch being unblocked by this to the description and
maybe
> > add
> > > a TODO in the code?
> > > 
> > > Thanks.
> > 
> > Sure & done! Let me know if the new comment makes sense to you. Thanks!
> 
> trchen : Thanks for doing this. I am currently looking at removing creation of
clip nodes for render surfaces and this is required for that too!
> 
> weiliangc : I did not understand what you meant by "generate this with
target_id from effect node and
> not use clip node's target_clip_id" (from #14). Can you elaborate ?

Sorry I meant clip node's target_effect_id. Target info should be on effect
node, and updated after Render Surface determination, and clip nodes probably
should not be aware of what target effect is.

[weiliangc, 2016-11-18 16:28:10]

LGTM

Thanks for doing this.

[trchen, 2016-11-18 22:19:58]

On 2016/11/18 16:27:10, weiliangc wrote:
> On 2016/11/17 at 23:31:29, jaydasika wrote:
> > On 2016/11/16 20:39:42, trchen wrote:
> > > On 2016/11/16 17:04:44, weiliangc wrote:
> > > > On 2016/11/15 at 23:51:45, trchen wrote:
> > > > > It will lift the restriction that effect node must associate with a
clip
> > > > > node owned by the same layer. Actually it blocks one of my other CL.
> > > > > 
> > > > > Yea, I too feel this should be a render surface / render pass thing. I
> > > > > don't know how easy is it to remove it though. Just did the easiest
> thing
> > > > > to unblock myself. :)
> > > > > 
> > > > > On Tuesday, November 15, 2016, mailto:weiliangc@chromium.org
> > > > > <javascript:_e(%7B%7D,'cvml','weiliangc@chromium.org');> via
> > > > > http://codereview.chromium.org
> > > <mailto:reply@chromiumcodereview-hr.appspotmail.com
> > > > >
> > >
> <javascript:_e(%7B%7D,'cvml','reply@chromiumcodereview-hr.appspotmail.com');>>
> > > > > wrote:
> > > > > 
> > > > > > Ideally this field needs to be removed.
> > > > > >
> > > > > > The best approach I see is to generate this with target_id from
effect
> > > > > > node and
> > > > > > not use clip node's target_clip_id, compare with current value for
> > > > > > SurfaceIsClipped, and remove this when we are confident with the
> result.
> > > > > >
> > > > > > Is there any particular reason you would want to move this field to
> effect
> > > > > > tree?
> > > > > >
> > > > > > https://codereview.chromium.org/2490273004/
> > > > > >
> > > > > 
> > > > > -- 
> > > > > You received this message because you are subscribed to the Google
> Groups
> > > > "Chromium-reviews" group.
> > > > > To unsubscribe from this group and stop receiving emails from it, send
> an
> > > > email to mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > > 
> > > > 
> > > > Could you add the patch being unblocked by this to the description and
> maybe
> > > add
> > > > a TODO in the code?
> > > > 
> > > > Thanks.
> > > 
> > > Sure & done! Let me know if the new comment makes sense to you. Thanks!
> > 
> > trchen : Thanks for doing this. I am currently looking at removing creation
of
> clip nodes for render surfaces and this is required for that too!
> > 
> > weiliangc : I did not understand what you meant by "generate this with
> target_id from effect node and
> > not use clip node's target_clip_id" (from #14). Can you elaborate ?
> 
> Sorry I meant clip node's target_effect_id. Target info should be on effect
> node, and updated after Render Surface determination, and clip nodes probably
> should not be aware of what target effect is.

Ah, I felt the same too. More than happy to comment that prior to landing. :)

[trchen, 2016-11-18 22:27:47]

The CQ bit was checked by trchen@chromium.org

[trchen, 2016-11-18 22:27:47]

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/2490273004/#ps60001
(title: "one more todo comment")

[commit-bot: I haz the power, 2016-11-18 22:28:16]

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

Description was changed from

==========
Move cc::ClipNode::target_is_clipped to cc::EffectNode

The flag means whether the clip node's target surface will be subject to
some ancestor clip. Move it to the effect node and access it through
ClipNode::target_effect_id instead.

This lifts the restriction that every effect node needs to associate with
a clip node created by the same owner. Thus will unblock
https://codereview.chromium.org/2495973002/

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

to

==========
Move cc::ClipNode::target_is_clipped to cc::EffectNode

The flag means whether the clip node's target surface will be subject to
some ancestor clip. Move it to the effect node and access it through
ClipNode::target_effect_id instead.

This lifts the restriction that every effect node needs to associate with
a clip node created by the same owner. Thus will unblock
https://codereview.chromium.org/2495973002/

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

[commit-bot: I haz the power, 2016-11-18 23:49:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-18 23:51:24]

Description was changed from

==========
Move cc::ClipNode::target_is_clipped to cc::EffectNode

The flag means whether the clip node's target surface will be subject to
some ancestor clip. Move it to the effect node and access it through
ClipNode::target_effect_id instead.

This lifts the restriction that every effect node needs to associate with
a clip node created by the same owner. Thus will unblock
https://codereview.chromium.org/2495973002/

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

to

==========
Move cc::ClipNode::target_is_clipped to cc::EffectNode

The flag means whether the clip node's target surface will be subject to
some ancestor clip. Move it to the effect node and access it through
ClipNode::target_effect_id instead.

This lifts the restriction that every effect node needs to associate with
a clip node created by the same owner. Thus will unblock
https://codereview.chromium.org/2495973002/

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[commit-bot: I haz the power, 2016-11-18 23:51:25]

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