cc : Delete LayerImpl::NoteLayerPropertyChangedForSubtree

cc : Delete LayerImpl::NoteLayerPropertyChangedForSubtree

This CL also :
  * tracks filter changes on main thread and filter
    animations on effect tree.
  * tracks scroll offset changes on transform tree.
  * tracks bounds delta changes on property trees.

BUG=584512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[jaydasika, 2016-03-01 23:09:25]

Description was changed from

==========
cc : Track changes to filters and scroll offset without layer tree hierarchy

We need to track these changes to compute damage rect.
BUG=584512
==========

to

==========
cc : Track changes to filters and scroll offset without layer tree hierarchy

We need to track these changes to compute damage rect.
BUG=584512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-03-02 01:02:22]

Description was changed from

==========
cc : Track changes to filters and scroll offset without layer tree hierarchy

We need to track these changes to compute damage rect.
BUG=584512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc : Delete LayerImpl::NoteLayerPropertyChangedForSubtree

This CL also :
  * tracks filter changes on main thread and filter
    animations on effect tree.
  * tracks scroll offset changes on transform tree.
  * tracks bounds delta changes on property trees.

BUG=584512

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

[jaydasika, 2016-03-02 01:08:30]

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

[jaydasika, 2016-03-02 01:08:31]

SetBoundsDelta is called only by viewport scroll and container layers. So,
instead of walking the entire clip tree and setting the damage status, I have
added a global bool on property trees to track full tree damage for this case.

[chromium-reviews, 2016-03-02 01:14:30]

+cc sunxd

On Tue, Mar 1, 2016 at 5:08 PM <jaydasika@chromium.org> wrote:

> Reviewers: weiliangc, ajuma, enne
> CL: https://codereview.chromium.org/1752993002/
>
> Message:
> SetBoundsDelta is called only by viewport scroll and container layers. So,
> instead of walking the entire clip tree and setting the damage status, I
> have
> added a global bool on property trees to track full tree damage for this
> case.
>
> Description:
> cc : Delete LayerImpl::NoteLayerPropertyChangedForSubtree
>
> This CL also :
> * tracks filter changes on main thread and filter
> animations on effect tree.
> * tracks scroll offset changes on transform tree.
> * tracks bounds delta changes on property trees.
>
> BUG=584512
>
> CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+109, -119 lines):
> M cc/layers/layer.cc
> M cc/layers/layer_impl.h
> M cc/layers/layer_impl.cc
> M cc/layers/layer_impl_unittest.cc
> M cc/layers/layer_unittest.cc
> M cc/proto/property_tree.proto
> M cc/trees/layer_tree_host.cc
> M cc/trees/layer_tree_host_impl.cc
> M cc/trees/layer_tree_host_unittest.cc
> M cc/trees/property_tree.h
> M cc/trees/property_tree.cc
>
>
>

-- 
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.

[jaydasika, 2016-03-02 01:16:50]

On 2016/03/02 01:14:30, chromium-reviews wrote:
> +cc sunxd
> 
> On Tue, Mar 1, 2016 at 5:08 PM <mailto:jaydasika@chromium.org> wrote:
> 
> > Reviewers: weiliangc, ajuma, enne
> > CL: https://codereview.chromium.org/1752993002/
> >
> > Message:
> > SetBoundsDelta is called only by viewport scroll and container layers. So,
> > instead of walking the entire clip tree and setting the damage status, I
> > have
> > added a global bool on property trees to track full tree damage for this
> > case.
> >
> > Description:
> > cc : Delete LayerImpl::NoteLayerPropertyChangedForSubtree
> >
> > This CL also :
> > * tracks filter changes on main thread and filter
> > animations on effect tree.
> > * tracks scroll offset changes on transform tree.
> > * tracks bounds delta changes on property trees.
> >
> > BUG=584512
> >
> > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> >
> > Base URL: https://chromium.googlesource.com/chromium/src.git@master
> >
> > Affected files (+109, -119 lines):
> > M cc/layers/layer.cc
> > M cc/layers/layer_impl.h
> > M cc/layers/layer_impl.cc
> > M cc/layers/layer_impl_unittest.cc
> > M cc/layers/layer_unittest.cc
> > M cc/proto/property_tree.proto
> > M cc/trees/layer_tree_host.cc
> > M cc/trees/layer_tree_host_impl.cc
> > M cc/trees/layer_tree_host_unittest.cc
> > M cc/trees/property_tree.h
> > M cc/trees/property_tree.cc
> >
> >
> >
> 
> -- 
> 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.

sunxd : This changes LayerImpl::DidUpdateScrollOffset. Let me know if this
conflicts with what you are working on.

[ajuma, 2016-03-02 14:34:10]

https://codereview.chromium.org/1752993002/diff/40001/cc/layers/layer_impl_un...
File cc/layers/layer_impl_unittest.cc (right):

https://codereview.chromium.org/1752993002/diff/40001/cc/layers/layer_impl_un...
cc/layers/layer_impl_unittest.cc:182: // SetBounsDelta changes subtree only when
masksToBounds is true and it
typos: SetBoundsDelta, masks_to_bounds

https://codereview.chromium.org/1752993002/diff/40001/cc/layers/layer_impl_un...
cc/layers/layer_impl_unittest.cc:183: // doesn't set needsPushProperties as it
is always called on active tree.
needs_push_properties

https://codereview.chromium.org/1752993002/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1752993002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1714: active_tree_->property_trees()->changed =
false;
Would it make sense to add a method to PropertyTrees that does all this
reseting, so we don't need to duplicate this sequence of calls whenever we need
to reset?

[sunxd, 2016-03-02 15:36:44]

On 2016/03/02 01:16:50, jaydasika wrote:
> On 2016/03/02 01:14:30, chromium-reviews wrote:
> > +cc sunxd
> > 
> > On Tue, Mar 1, 2016 at 5:08 PM <mailto:jaydasika@chromium.org> wrote:
> > 
> > > Reviewers: weiliangc, ajuma, enne
> > > CL: https://codereview.chromium.org/1752993002/
> > >
> > > Message:
> > > SetBoundsDelta is called only by viewport scroll and container layers. So,
> > > instead of walking the entire clip tree and setting the damage status, I
> > > have
> > > added a global bool on property trees to track full tree damage for this
> > > case.
> > >
> > > Description:
> > > cc : Delete LayerImpl::NoteLayerPropertyChangedForSubtree
> > >
> > > This CL also :
> > > * tracks filter changes on main thread and filter
> > > animations on effect tree.
> > > * tracks scroll offset changes on transform tree.
> > > * tracks bounds delta changes on property trees.
> > >
> > > BUG=584512
> > >
> > > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> > >
> > > Base URL: https://chromium.googlesource.com/chromium/src.git@master
> > >
> > > Affected files (+109, -119 lines):
> > > M cc/layers/layer.cc
> > > M cc/layers/layer_impl.h
> > > M cc/layers/layer_impl.cc
> > > M cc/layers/layer_impl_unittest.cc
> > > M cc/layers/layer_unittest.cc
> > > M cc/proto/property_tree.proto
> > > M cc/trees/layer_tree_host.cc
> > > M cc/trees/layer_tree_host_impl.cc
> > > M cc/trees/layer_tree_host_unittest.cc
> > > M cc/trees/property_tree.h
> > > M cc/trees/property_tree.cc
> > >
> > >
> > >
> > 
> > -- 
> > 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.
> 
> sunxd : This changes LayerImpl::DidUpdateScrollOffset. Let me know if this
> conflicts with what you are working on.

I'm fine with that change, my change can still work if the interface is not
changed.
interface.

[jaydasika, 2016-03-02 16:27:09]

https://codereview.chromium.org/1752993002/diff/40001/cc/layers/layer_impl_un...
File cc/layers/layer_impl_unittest.cc (right):

https://codereview.chromium.org/1752993002/diff/40001/cc/layers/layer_impl_un...
cc/layers/layer_impl_unittest.cc:182: // SetBounsDelta changes subtree only when
masksToBounds is true and it
On 2016/03/02 14:34:09, ajuma wrote:
> typos: SetBoundsDelta, masks_to_bounds

Done.

https://codereview.chromium.org/1752993002/diff/40001/cc/layers/layer_impl_un...
cc/layers/layer_impl_unittest.cc:183: // doesn't set needsPushProperties as it
is always called on active tree.
On 2016/03/02 14:34:09, ajuma wrote:
> needs_push_properties

Done.

https://codereview.chromium.org/1752993002/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1752993002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1714: active_tree_->property_trees()->changed =
false;
On 2016/03/02 14:34:10, ajuma wrote:
> Would it make sense to add a method to PropertyTrees that does all this
> reseting, so we don't need to duplicate this sequence of calls whenever we
need
> to reset?

Done.

https://codereview.chromium.org/1752993002/diff/60001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1752993002/diff/60001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1001: NoteLayerPropertyChanged();
Change from a previous CL.

https://codereview.chromium.org/1752993002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1752993002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:500: property_trees_.full_tree_damaged = false;
I am not calling the property trees reset function here as we don't have to
reset effect tree every time. We can make  ResetAllChageTracking in property
trees take an argument should_reset_effect_tree_also and then call it here also.
Do you think that's better ?

[ajuma, 2016-03-02 16:31:31]

https://codereview.chromium.org/1752993002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1752993002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:500: property_trees_.full_tree_damaged = false;
On 2016/03/02 16:27:08, jaydasika wrote:
> I am not calling the property trees reset function here as we don't have to
> reset effect tree every time. We can make  ResetAllChageTracking in property
> trees take an argument should_reset_effect_tree_also and then call it here
also.
> Do you think that's better ?

Could you remind me why we don't need to reset the effect tree here as well?

[jaydasika, 2016-03-02 17:19:30]

https://codereview.chromium.org/1752993002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1752993002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:500: property_trees_.full_tree_damaged = false;
On 2016/03/02 16:31:31, ajuma wrote:
> On 2016/03/02 16:27:08, jaydasika wrote:
> > I am not calling the property trees reset function here as we don't have to
> > reset effect tree every time. We can make  ResetAllChageTracking in property
> > trees take an argument should_reset_effect_tree_also and then call it here
> also.
> > Do you think that's better ?
> 
> Could you remind me why we don't need to reset the effect tree here as well?

I meant, we don't have to do it everytime as we don't do any change tracking on
the effect tree on the main thread. We do have to reset the effect tree when we
have back-synced the property trees from active tree. (The comment below in line
501).

[ajuma, 2016-03-02 17:54:06]

https://codereview.chromium.org/1752993002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1752993002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:500: property_trees_.full_tree_damaged = false;
On 2016/03/02 17:19:30, jaydasika wrote:
> On 2016/03/02 16:31:31, ajuma wrote:
> > On 2016/03/02 16:27:08, jaydasika wrote:
> > > I am not calling the property trees reset function here as we don't have
to
> > > reset effect tree every time. We can make  ResetAllChageTracking in
property
> > > trees take an argument should_reset_effect_tree_also and then call it here
> > also.
> > > Do you think that's better ?
> > 
> > Could you remind me why we don't need to reset the effect tree here as well?
> 
> I meant, we don't have to do it everytime as we don't do any change tracking
on
> the effect tree on the main thread. We do have to reset the effect tree when
we
> have back-synced the property trees from active tree. (The comment below in
line
> 501). 

Oh, we don't change-track on the effect tree because opacity and filter changes
on the main thread currently trigger property tree rebuilds even when a Layer
already has an effect node. In that case, how about making ResetChangeTracking
take an enum with allowed values EFFECT_TREE, TRANSFORM_TREE, and ALL_TREES?

[jaydasika, 2016-03-02 19:13:07]

https://codereview.chromium.org/1752993002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1752993002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:500: property_trees_.full_tree_damaged = false;
On 2016/03/02 17:54:06, ajuma wrote:
> On 2016/03/02 17:19:30, jaydasika wrote:
> > On 2016/03/02 16:31:31, ajuma wrote:
> > > On 2016/03/02 16:27:08, jaydasika wrote:
> > > > I am not calling the property trees reset function here as we don't have
> to
> > > > reset effect tree every time. We can make  ResetAllChageTracking in
> property
> > > > trees take an argument should_reset_effect_tree_also and then call it
here
> > > also.
> > > > Do you think that's better ?
> > > 
> > > Could you remind me why we don't need to reset the effect tree here as
well?
> > 
> > I meant, we don't have to do it everytime as we don't do any change tracking
> on
> > the effect tree on the main thread. We do have to reset the effect tree when
> we
> > have back-synced the property trees from active tree. (The comment below in
> line
> > 501). 
> 
> Oh, we don't change-track on the effect tree because opacity and filter
changes
> on the main thread currently trigger property tree rebuilds even when a Layer
> already has an effect node. In that case, how about making ResetChangeTracking
> take an enum with allowed values EFFECT_TREE, TRANSFORM_TREE, and ALL_TREES?

Done.

[ajuma, 2016-03-02 19:39:21]

Thanks, lgtm.

[jaydasika, 2016-03-02 20:03:13]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2016-03-02 20:03:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1752993002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1752993002/80001

[commit-bot: I haz the power, 2016-03-02 20:49:17]

Description was changed from

==========
cc : Delete LayerImpl::NoteLayerPropertyChangedForSubtree

This CL also :
  * tracks filter changes on main thread and filter
    animations on effect tree.
  * tracks scroll offset changes on transform tree.
  * tracks bounds delta changes on property trees.

BUG=584512

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

to

==========
cc : Delete LayerImpl::NoteLayerPropertyChangedForSubtree

This CL also :
  * tracks filter changes on main thread and filter
    animations on effect tree.
  * tracks scroll offset changes on transform tree.
  * tracks bounds delta changes on property trees.

BUG=584512

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

[commit-bot: I haz the power, 2016-03-02 20:49:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-02 20:50:31]

Description was changed from

==========
cc : Delete LayerImpl::NoteLayerPropertyChangedForSubtree

This CL also :
  * tracks filter changes on main thread and filter
    animations on effect tree.
  * tracks scroll offset changes on transform tree.
  * tracks bounds delta changes on property trees.

BUG=584512

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

to

==========
cc : Delete LayerImpl::NoteLayerPropertyChangedForSubtree

This CL also :
  * tracks filter changes on main thread and filter
    animations on effect tree.
  * tracks scroll offset changes on transform tree.
  * tracks bounds delta changes on property trees.

BUG=584512

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2016-03-02 20:50:32]

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