Compute if a layer is clipped outside CalcDrawProps

Compute if a layer is clipped outside CalcDrawProps

We need to know if a layer is clipped to compute its drawable
visible rect. So, we need to compute it outside CalcDrawProps to move
drawable visible rect computation outside CalcDrawProps.
BUG=497817
Committed: https://crrev.com/2af7226e29be65a8ea4a279b9358a6fcfada5cd3
Cr-Commit-Position: refs/heads/master@{#338515}
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[jaydasika, 2015-07-07 15:38:41]

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

[weiliangc, 2015-07-07 15:54:02]

https://codereview.chromium.org/1231453002/diff/1/cc/trees/property_tree_buil...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1231453002/diff/1/cc/trees/property_tree_buil...
cc/trees/property_tree_builder.cc:148: // TODO(jaydasika): This value depends on
whether a layer has a render
Thanks for this. :) Could you also add crbug.com/504467 to the comments?

[ajuma, 2015-07-07 17:58:48]

https://codereview.chromium.org/1231453002/diff/20001/cc/layers/draw_properti...
File cc/layers/draw_properties.h (right):

https://codereview.chromium.org/1231453002/diff/20001/cc/layers/draw_properti...
cc/layers/draw_properties.h:77: bool is_clipped_from_property_trees;
So far I think we've been adding 'from_property_trees' values directly on
Layer/LayerImpl rather than adding them to DrawProperties. Also, the setter for
this on Layer will need to SetNeedsPushProperties when this value changes.

[enne (OOO), 2015-07-07 18:25:47]

enne@chromium.org changed reviewers:
+ enne@chromium.org

[enne (OOO), 2015-07-07 18:25:48]

https://codereview.chromium.org/1231453002/diff/20001/cc/layers/draw_properti...
File cc/layers/draw_properties.h (right):

https://codereview.chromium.org/1231453002/diff/20001/cc/layers/draw_properti...
cc/layers/draw_properties.h:77: bool is_clipped_from_property_trees;
This is a bit odd to have both at this point in time where we are only doing CDP
on the main thread in unit tests.  Does is_clipped ever change on the compositor
thread?  It looks like the CHECK_EQ is expecting it not to.

Also, draw properties aren't pushed in general.  Can this be a layer property? 

I think what I'd like to see is have Layer::is_clipped set by the property tree
builder and pushed to LayerImpl::is_clipped.  If there are somehow unit tests
that are still calling main thread CDP only and are failing, then maybe just
make sure they call the property tree builder too.

How's that sound?

[jaydasika, 2015-07-07 18:35:28]

https://codereview.chromium.org/1231453002/diff/1/cc/trees/property_tree_buil...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1231453002/diff/1/cc/trees/property_tree_buil...
cc/trees/property_tree_builder.cc:148: // TODO(jaydasika): This value depends on
whether a layer has a render
On 2015/07/07 15:54:02, weiliangc wrote:
> Thanks for this. :) Could you also add crbug.com/504467 to the comments?

Done.

https://codereview.chromium.org/1231453002/diff/20001/cc/layers/draw_properti...
File cc/layers/draw_properties.h (right):

https://codereview.chromium.org/1231453002/diff/20001/cc/layers/draw_properti...
cc/layers/draw_properties.h:77: bool is_clipped_from_property_trees;
On 2015/07/07 18:25:48, enne wrote:
> This is a bit odd to have both at this point in time where we are only doing
CDP
> on the main thread in unit tests.  Does is_clipped ever change on the
compositor
> thread?  It looks like the CHECK_EQ is expecting it not to.
> 
> Also, draw properties aren't pushed in general.  Can this be a layer property?

> 
> I think what I'd like to see is have Layer::is_clipped set by the property
tree
> builder and pushed to LayerImpl::is_clipped.  If there are somehow unit tests
> that are still calling main thread CDP only and are failing, then maybe just
> make sure they call the property tree builder too.
> 
> How's that sound?

moved it from draw_properties to Layer/LayerImpl.

[enne (OOO), 2015-07-07 18:47:51]

On 2015/07/07 at 18:35:28, jaydasika wrote:

>
https://codereview.chromium.org/1231453002/diff/20001/cc/layers/draw_properti...
> cc/layers/draw_properties.h:77: bool is_clipped_from_property_trees;
> On 2015/07/07 18:25:48, enne wrote:
> > This is a bit odd to have both at this point in time where we are only doing
CDP
> > on the main thread in unit tests.  Does is_clipped ever change on the
compositor
> > thread?  It looks like the CHECK_EQ is expecting it not to.
> >
> > I think what I'd like to see is have Layer::is_clipped set by the property
tree
> > builder and pushed to LayerImpl::is_clipped.  If there are somehow unit
tests
> > that are still calling main thread CDP only and are failing, then maybe just
> > make sure they call the property tree builder too.
> > 
> > How's that sound?
> 
> moved it from draw_properties to Layer/LayerImpl.

Is it possible to remove is_clipped from draw_properties?

[jaydasika, 2015-07-07 18:53:27]

On 2015/07/07 18:47:51, enne wrote:
> On 2015/07/07 at 18:35:28, jaydasika wrote:
> 
> >
>
https://codereview.chromium.org/1231453002/diff/20001/cc/layers/draw_properti...
> > cc/layers/draw_properties.h:77: bool is_clipped_from_property_trees;
> > On 2015/07/07 18:25:48, enne wrote:
> > > This is a bit odd to have both at this point in time where we are only
doing
> CDP
> > > on the main thread in unit tests.  Does is_clipped ever change on the
> compositor
> > > thread?  It looks like the CHECK_EQ is expecting it not to.
> > >
> > > I think what I'd like to see is have Layer::is_clipped set by the property
> tree
> > > builder and pushed to LayerImpl::is_clipped.  If there are somehow unit
> tests
> > > that are still calling main thread CDP only and are failing, then maybe
just
> > > make sure they call the property tree builder too.
> > > 
> > > How's that sound?
> > 
> > moved it from draw_properties to Layer/LayerImpl.
> 
> Is it possible to remove is_clipped from draw_properties?

It is used in CDP

[enne (OOO), 2015-07-07 18:56:13]

On 2015/07/07 at 18:53:27, jaydasika wrote:

> > Is it possible to remove is_clipped from draw_properties?
> 
> It is used in CDP

On the main thread we always do property trees, except for a few unit tests that
still call CDP directly.  So it should be safe to assume that is_clipped is set
properly on the main thread and then pushed in all cases.  Is it possible to
remove the draw_properties version of is_clipped and have everything refer to
the Layer(Impl) version?

[jaydasika, 2015-07-07 19:01:32]

On 2015/07/07 18:56:13, enne wrote:
> On 2015/07/07 at 18:53:27, jaydasika wrote:
> 
> > > Is it possible to remove is_clipped from draw_properties?
> > 
> > It is used in CDP
> 
> On the main thread we always do property trees, except for a few unit tests
that
> still call CDP directly.  So it should be safe to assume that is_clipped is
set
> properly on the main thread and then pushed in all cases.  Is it possible to
> remove the draw_properties version of is_clipped and have everything refer to
> the Layer(Impl) version?

draw_properties version of is_clipped is referred to only inside CDP and
unittests.
So, we can remove it.

[Ian Vollick, 2015-07-07 19:09:39]

vollick@chromium.org changed reviewers:
+ vollick@chromium.org

[Ian Vollick, 2015-07-07 19:09:39]

https://codereview.chromium.org/1231453002/diff/40001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1231453002/diff/40001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:150: // render surfaces(see
http://crbug.com/504467), this should move.
Hey, I wanted to mention here what we'd discussed offline -- that constructing
clip trees (unlike transform trees) will likely come after layerization and
render surface determination. I've made a comment on the liked bug. This second
sentence should probably be ditched?

[jaydasika, 2015-07-07 19:15:56]

https://codereview.chromium.org/1231453002/diff/40001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1231453002/diff/40001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:150: // render surfaces(see
http://crbug.com/504467), this should move.
On 2015/07/07 19:09:39, vollick wrote:
> Hey, I wanted to mention here what we'd discussed offline -- that constructing
> clip trees (unlike transform trees) will likely come after layerization and
> render surface determination. I've made a comment on the liked bug. This
second
> sentence should probably be ditched?

If render surfaces are determined before building clip tree, the whole comment
can be ditched as we don't need to move is_clipped computation

[jaydasika, 2015-07-07 20:07:40]

On 2015/07/07 19:01:32, jaydasika wrote:
> On 2015/07/07 18:56:13, enne wrote:
> > On 2015/07/07 at 18:53:27, jaydasika wrote:
> > 
> > > > Is it possible to remove is_clipped from draw_properties?
> > > 
> > > It is used in CDP
> > 
> > On the main thread we always do property trees, except for a few unit tests
> that
> > still call CDP directly.  So it should be safe to assume that is_clipped is
> set
> > properly on the main thread and then pushed in all cases.  Is it possible to
> > remove the draw_properties version of is_clipped and have everything refer
to
> > the Layer(Impl) version?
> 
> draw_properties version of is_clipped is referred to only inside CDP and
> unittests.
> So, we can remove it.

But what about draw_properties version of is_clipped referred to inside CDP?
We can't use the Layer(Impl) version there as property trees get build after
CDP.

[enne (OOO), 2015-07-07 20:29:26]

On 2015/07/07 at 20:07:40, jaydasika wrote:

> > So, we can remove it.
> 
> But what about draw_properties version of is_clipped referred to inside CDP?
> We can't use the Layer(Impl) version there as property trees get build after
CDP.

Hmm.  On the main thread, there is no CDP (barring unit tests), so
DrawProperties::is_clipped is not needed.  On the compositor thread, push
properties happens before property trees or CDP, and so if the LayerImpl version
gets used, then it will be valid.  Am I misunderstanding something?

[jaydasika, 2015-07-07 20:37:05]

On 2015/07/07 20:29:26, enne wrote:
> On 2015/07/07 at 20:07:40, jaydasika wrote:
> 
> > > So, we can remove it.
> > 
> > But what about draw_properties version of is_clipped referred to inside CDP?
> > We can't use the Layer(Impl) version there as property trees get build after
> CDP.
> 
> Hmm.  On the main thread, there is no CDP (barring unit tests), so
> DrawProperties::is_clipped is not needed.  On the compositor thread, push
> properties happens before property trees or CDP, and so if the LayerImpl
version
> gets used, then it will be valid.  Am I misunderstanding something?

I was referring to unit tests. I guess we could just BuildPropertyTrees first
and then call CDP in ExecuteCDP(called by most unit tests) ?

[enne (OOO), 2015-07-07 20:44:08]

On 2015/07/07 at 20:37:05, jaydasika wrote:
> On 2015/07/07 20:29:26, enne wrote:
> > On 2015/07/07 at 20:07:40, jaydasika wrote:
> > 
> > > > So, we can remove it.
> > > 
> > > But what about draw_properties version of is_clipped referred to inside
CDP?
> > > We can't use the Layer(Impl) version there as property trees get build
after
> > CDP.
> > 
> > Hmm.  On the main thread, there is no CDP (barring unit tests), so
> > DrawProperties::is_clipped is not needed.  On the compositor thread, push
> > properties happens before property trees or CDP, and so if the LayerImpl
version
> > gets used, then it will be valid.  Am I misunderstanding something?
> 
> I was referring to unit tests. I guess we could just BuildPropertyTrees first
> and then call CDP in ExecuteCDP(called by most unit tests) ?

It depends on the unit tests, but yeah I'd say do whatever makes sense to keep
the unit tests passing.  Really, we should just get rid of main thread cdp.

[jaydasika, 2015-07-09 14:18:44]

On 2015/07/09 00:00:00, jaydasika wrote:

The DrawProperties version of is_clipped is removed in PatchSet 4, all other
changes in this patch are to make the unit tests pass.

[Ian Vollick, 2015-07-09 14:26:27]

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_impl_unittest.cc (left):

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl_unittest.cc:797: 
nit: no need for spurious whitespace changes.

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/layer.h#newco...
cc/layers/layer.h:508: bool is_clipped_from_property_tree() const {
the "from_property_tree" suffix is only used to distinguish a method from its
non-property-tree analog. It's a reminder that we have to reconcile the two.
There is no such analog here, so this should just be called is_clipped.

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

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:186: void set_is_clipped_from_property_tree(bool
is_clipped) {
nit: I see that you've followed the example of
set_should_flatten_transform_from_property_tree, but that function isn't named
correctly either. You should only use hacker case if it's a trivial setter that
can be defined in the header. Since these functions call SetNeedsPushProperties
and are defined in the cc file, they should be camel case.

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

https://codereview.chromium.org/1231453002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2505: 
nit: spurious ws.

https://codereview.chromium.org/1231453002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2558: }
This TRACE_EVENT_BEGIN0 and TRACE_EVENT_END0 here are used for measuring the
performance of CDP. If you're going to move CDP (and it's awesome that you are),
you'll need to move these, too.

[ajuma, 2015-07-09 14:50:16]

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1329:
host_impl_.pending_tree()->BuildPropertyTreesForTesting();
Do we need to rebuild property trees again? (Has anything changed since the
previous rebuild?)

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1460:
host_impl_.pending_tree()->BuildPropertyTreesForTesting();
Here too, do we need to rebuild again?

https://codereview.chromium.org/1231453002/diff/60001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/1231453002/diff/60001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl_unittest.cc:975: std::set<LayerImpl*>* scroll_children
= new std::set<LayerImpl*>;
Nit: use a scoped_ptr here to make the ownership transfer more explicit. See
LayerTreeHostCommonTest.ClippedByOutOfOrderScrollParent for an example.

https://codereview.chromium.org/1231453002/diff/60001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1231453002/diff/60001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:105:
layer->clip_parent()->is_clipped_from_property_tree());
This looks like a logic change (since we're now checking is_clipped in addition
to checking whether we have a clip parent). Is there a test that fails without
this?

[jaydasika, 2015-07-09 15:11:49]

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_impl_unittest.cc (left):

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl_unittest.cc:797: 
On 2015/07/09 14:26:27, vollick wrote:
> nit: no need for spurious whitespace changes.

Done.

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/layer.h#newco...
cc/layers/layer.h:508: bool is_clipped_from_property_tree() const {
On 2015/07/09 14:26:27, vollick wrote:
> the "from_property_tree" suffix is only used to distinguish a method from its
> non-property-tree analog. It's a reminder that we have to reconcile the two.
> There is no such analog here, so this should just be called is_clipped.

Done.

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

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:186: void set_is_clipped_from_property_tree(bool
is_clipped) {
On 2015/07/09 14:26:27, vollick wrote:
> nit: I see that you've followed the example of
> set_should_flatten_transform_from_property_tree, but that function isn't named
> correctly either. You should only use hacker case if it's a trivial setter
that
> can be defined in the header. Since these functions call
SetNeedsPushProperties
> and are defined in the cc file, they should be camel case.

It is defined in the header file. I have followed the example of
set_should_flatten_transform_from_property_tree and
set_offset_to_transform_parent, both set and call SetNeedsPushProperties and are
defined inside the header itself. Should I move it to cc?

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1329:
host_impl_.pending_tree()->BuildPropertyTreesForTesting();
On 2015/07/09 14:50:15, ajuma wrote:
> Do we need to rebuild property trees again? (Has anything changed since the
> previous rebuild?)

Done. Its not required here.

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1460:
host_impl_.pending_tree()->BuildPropertyTreesForTesting();
On 2015/07/09 14:50:15, ajuma wrote:
> Here too, do we need to rebuild again?

Done. Not required here too

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

https://codereview.chromium.org/1231453002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2505: 
On 2015/07/09 14:26:27, vollick wrote:
> nit: spurious ws.

Done.

https://codereview.chromium.org/1231453002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2558: }
On 2015/07/09 14:26:27, vollick wrote:
> This TRACE_EVENT_BEGIN0 and TRACE_EVENT_END0 here are used for measuring the
> performance of CDP. If you're going to move CDP (and it's awesome that you
are),
> you'll need to move these, too.

Done.

https://codereview.chromium.org/1231453002/diff/60001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/1231453002/diff/60001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl_unittest.cc:975: std::set<LayerImpl*>* scroll_children
= new std::set<LayerImpl*>;
On 2015/07/09 14:50:15, ajuma wrote:
> Nit: use a scoped_ptr here to make the ownership transfer more explicit. See
> LayerTreeHostCommonTest.ClippedByOutOfOrderScrollParent for an example.

Done.

https://codereview.chromium.org/1231453002/diff/60001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1231453002/diff/60001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:105:
layer->clip_parent()->is_clipped_from_property_tree());
On 2015/07/09 14:50:15, ajuma wrote:
> This looks like a logic change (since we're now checking is_clipped in
addition
> to checking whether we have a clip parent). Is there a test that fails without
> this?

Yes. LayerTreeHostCommonTest.LayerTreeRebuildTest crashes without it.
It crashes at the CHECK_EQ inside cdp that is added as a part of this patch

[Ian Vollick, 2015-07-09 16:42:18]

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

https://codereview.chromium.org/1231453002/diff/60001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:186: void set_is_clipped_from_property_tree(bool
is_clipped) {
On 2015/07/09 15:11:49, jaydasika wrote:
> On 2015/07/09 14:26:27, vollick wrote:
> > nit: I see that you've followed the example of
> > set_should_flatten_transform_from_property_tree, but that function isn't
named
> > correctly either. You should only use hacker case if it's a trivial setter
> that
> > can be defined in the header. Since these functions call
> SetNeedsPushProperties
> > and are defined in the cc file, they should be camel case.
> 
> It is defined in the header file.
Hah. I was clearly skimming your patch too quickly. :)

> I have followed the example of
> set_should_flatten_transform_from_property_tree and
> set_offset_to_transform_parent, both set and call SetNeedsPushProperties and
are
> defined inside the header itself. Should I move it to cc?
Yes, I'd prefer that these move to the cc and change to camel case, but since
there are many offenders, I'd just leave it as you have it in your current patch
and if we make the change, we can do it for all of these functions at once.

[enne (OOO), 2015-07-09 18:06:28]

I think the major things I was concerned with look fixed.  Thanks for taking the
time to remove this from draw properties.  

We have a lot of reviewers at this point, so lgtm % vollick && ajuma

[ajuma, 2015-07-09 18:13:50]

I have one more nit and then this lgtm.

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

https://codereview.chromium.org/1231453002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2064:
CHECK_EQ(layer_or_ancestor_clips_descendants, layer->is_clipped());
I think this should be a DCHECK rather than a CHECK.

[Ian Vollick, 2015-07-09 18:17:04]

On 2015/07/09 18:13:50, ajuma wrote:
> I have one more nit and then this lgtm.
> 
>
https://codereview.chromium.org/1231453002/diff/80001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_common.cc (right):
> 
>
https://codereview.chromium.org/1231453002/diff/80001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_common.cc:2064:
> CHECK_EQ(layer_or_ancestor_clips_descendants, layer->is_clipped());
> I think this should be a DCHECK rather than a CHECK.

All my concerns have been addressed. lgtm, too.

[jaydasika, 2015-07-09 18:25:47]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2015-07-09 18:25:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
vollick@chromium.org, ajuma@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1231453002/#ps100001
(title: "CHECK -> DCHECK")

[commit-bot: I haz the power, 2015-07-09 18:27:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231453002/100001

[commit-bot: I haz the power, 2015-07-09 19:54:49]

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

[commit-bot: I haz the power, 2015-07-09 19:54:50]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jaydasika, 2015-07-10 16:25:10]

Some browser tests fail with this patch. The reason is render surfaces are
updated on the compositor thread(in one case because the opacity is animating)
and is_clipped obtained from main thread during commit is no longer valid.
Any thoughts on what can be done?

[jaydasika, 2015-07-10 18:56:09]

On 2015/07/10 16:25:10, jaydasika wrote:
> Some browser tests fail with this patch. The reason is render surfaces are
> updated on the compositor thread(in one case because the opacity is animating)
> and is_clipped obtained from main thread during commit is no longer valid.
> Any thoughts on what can be done?

ajuma@ pointed out that render surfaces are not updated on the compositor
thread.
He is right. The test crashes because we do not rebuild property trees when
render surface is updated and not because the update happens on the
compositor thread. Sorry for the confusion. A potential solution is to
rebuild property trees when render surfaces are updated?

[ajuma, 2015-07-10 22:16:18]

https://codereview.chromium.org/1231453002/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/1231453002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common.cc:2410:
layer->layer_tree_host()->property_trees()->needs_rebuild = true;
How about doing this inside Layer::SetHasRenderSurface (whenever the new value
is different from the old one)?

[jaydasika, 2015-07-13 14:38:44]

https://codereview.chromium.org/1231453002/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/1231453002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common.cc:2410:
layer->layer_tree_host()->property_trees()->needs_rebuild = true;
On 2015/07/10 22:16:18, ajuma wrote:
> How about doing this inside Layer::SetHasRenderSurface (whenever the new value
> is different from the old one)?

Done.

[ajuma, 2015-07-13 14:59:49]

On 2015/07/13 14:38:44, jaydasika wrote:
>
https://codereview.chromium.org/1231453002/diff/140001/cc/trees/layer_tree_ho...
> File cc/trees/layer_tree_host_common.cc (right):
> 
>
https://codereview.chromium.org/1231453002/diff/140001/cc/trees/layer_tree_ho...
> cc/trees/layer_tree_host_common.cc:2410:
> layer->layer_tree_host()->property_trees()->needs_rebuild = true;
> On 2015/07/10 22:16:18, ajuma wrote:
> > How about doing this inside Layer::SetHasRenderSurface (whenever the new
value
> > is different from the old one)?
> 
> Done.

lgtm

[jaydasika, 2015-07-13 15:02:52]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2015-07-13 15:02:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
vollick@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1231453002/#ps160001
(title: "Rebuild moved to SetHasRenderSurface")

[commit-bot: I haz the power, 2015-07-13 15:03:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231453002/160001

[commit-bot: I haz the power, 2015-07-13 16:07:50]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-07-13 16:08:38]

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

[Dirk Pranke, 2015-07-13 18:48:17]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1236943002/ by dpranke@chromium.org.

The reason for reverting is: It looks like this is causing MSAN failures from an
uninitialized variable in the webkit_tests:

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/bu...

10:00:36.214 29049   ==29058==WARNING: MemorySanitizer:
use-of-uninitialized-value
10:00:36.214 29049       #0 0x7f620bf62c3e in set_is_clipped
cc/layers/layer.h:503:9
10:00:36.214 29049       #1 0x7f620bf62c3e in AddClipNodeIfNeeded<cc::Layer>
cc/trees/property_tree_builder.cc:149:0
10:00:36.214 29049       #2 0x7f620bf62c3e in void cc::(anonymous
namespace)::BuildPropertyTreesInternal<cc::Layer>(cc::Layer*, cc::(anonymous
namespace)::DataForRecursion<cc::Layer> const&)
cc/trees/property_tree_builder.cc:386:0
10:00:36.214 29049       #3 0x7f620bf5d128 in void
cc::BuildPropertyTreesTopLevelInternal<cc::Layer>(cc::Layer*, cc::Layer const*,
cc::Layer const*, cc::Layer const*, float, float, gfx::Rect const&,
gfx::Transform const&, cc::PropertyTrees*)
cc/trees/property_tree_builder.cc:466:3
10:00:36.214 29049       #4 0x7f620c206ee6 in
cc::BuildPropertyTreesAndComputeVisibleRects(cc::Layer*, cc::Layer const*,
cc::Layer const*, cc::Layer const*, float, float, gfx::Rect const&,
gfx::Transform const&, cc::PropertyTrees*,
std::__1::vector<scoped_refptr<cc::Layer>,
std::__1::allocator<scoped_refptr<cc::Layer> > >*)
cc/trees/draw_property_utils.cc:473:3
10:00:36.214 29049       #5 0x7f620be94a2f in
cc::LayerTreeHost::DoUpdateLayers(cc::Layer*) cc/trees/layer_tree_host.cc:789:5

on pretty much every test.

So I'm reverting this. Sorry!.

[jaydasika, 2015-07-13 21:22:41]

Patchset #11 (id:200001) has been deleted

[jaydasika, 2015-07-14 14:44:43]

Added a patch to that initializes is_clipped.

[ajuma, 2015-07-14 14:46:45]

On 2015/07/14 14:44:43, jaydasika wrote:
> Added a patch to that initializes is_clipped.

lgtm

[jaydasika, 2015-07-14 14:48:41]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2015-07-14 14:48:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
vollick@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1231453002/#ps180001
(title: "Initialized is_clipped_")

[commit-bot: I haz the power, 2015-07-14 14:49:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231453002/180001

[commit-bot: I haz the power, 2015-07-14 15:15:17]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-07-14 15:16:40]

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