Keeping track of descendants that draw content instead of recalcualting

Keeping track of descendants that draw content instead of recalcualting

This is a required for removing Render Surface creation from CalcDrawProps.

BUG=386788
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288740

[awoloszyn, 2014-07-14 14:50:03]

Instead of using RAII objects to track dirtiness of DrawsContent, instead make
DrawsContent non-virtual and update it when it can change.

[danakj, 2014-07-14 14:56:31]

https://codereview.chromium.org/373113003/diff/80001/cc/layers/image_layer.cc
File cc/layers/image_layer.cc (right):

https://codereview.chromium.org/373113003/diff/80001/cc/layers/image_layer.cc...
cc/layers/image_layer.cc:37: void ImageLayer::UpdateDrawsContent(bool
drawsContent) {
draws_content thoughout

https://codereview.chromium.org/373113003/diff/80001/cc/layers/image_layer.cc...
cc/layers/image_layer.cc:38: TiledLayer::UpdateDrawsContent(drawsContent &&
!bitmap_.isNull());
so I think what you're trying to do here, you want DrawsContent to be true only
if someone did SetDrawsContent(true) externally on the layer? But to get that
effect you should be && with the current DrawsContent() value, not with |true|
as you would here. Do I have that right?

However, can be simplify this a bit? We have 2 things:
1. SetIsDrawable() which is an external flag saying whether the layer should
draw anything at all.
2. Layer-type-specific internal flag saying whether the layer has anything to
draw.

Can we just use 2 bools on Layer to represent these two things instead of doing
boolean logic to store that in one?

Then Layer::DrawsContent() would be is_drawable_ && has_drawable_content_ or
whatever. WDYT?

[danakj, 2014-07-14 14:56:57]

> BUG=crbug.com/386788

BTW you can just use

BUG=386788

[danakj, 2014-07-14 14:57:45]

https://codereview.chromium.org/373113003/diff/80001/cc/layers/delegated_rend...
File cc/layers/delegated_renderer_layer.h (right):

https://codereview.chromium.org/373113003/diff/80001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer.h:35: virtual int
NumDescendantsThatDrawContent() const OVERRIDE;
Oh, instead of this, can we put HasDelegatedContent() on this layer like we have
on DRLImpl instead? The lying about num descendants can come at the place we
compute that instead of here?

[awoloszyn, 2014-07-14 15:25:09]

https://codereview.chromium.org/373113003/diff/80001/cc/layers/image_layer.cc
File cc/layers/image_layer.cc (right):

https://codereview.chromium.org/373113003/diff/80001/cc/layers/image_layer.cc...
cc/layers/image_layer.cc:38: TiledLayer::UpdateDrawsContent(drawsContent &&
!bitmap_.isNull());
On 2014/07/14 14:56:31, danakj wrote:
> so I think what you're trying to do here, you want DrawsContent to be true
only
> if someone did SetDrawsContent(true) externally on the layer? But to get that
> effect you should be && with the current DrawsContent() value, not with |true|
> as you would here. Do I have that right?
So how I had set UpateDrawsContent up, currently, is that it is originally
called with true, and then every class in the hierarchy has the ability to veto
the fact that before delegating onto its super-class. Once we hit
Layer::UpdateDrawsContent we will have the actual value of draws_content (with
respect to every subclass on the way down), and set the value.

This could probably be made clearer with a "UpdateDrawsContent" function with no
parameters, and then a helper PropagateDrawsContent(bool) that does this work.


> 
> However, can be simplify this a bit? We have 2 things:
> 1. SetIsDrawable() which is an external flag saying whether the layer should
> draw anything at all.
> 2. Layer-type-specific internal flag saying whether the layer has anything to
> draw.
> 
> Can we just use 2 bools on Layer to represent these two things instead of
doing
> boolean logic to store that in one?
> 
> Then Layer::DrawsContent() would be is_drawable_ && has_drawable_content_ or
> whatever. WDYT?
My hope with how this was set up was that we can keep the "truth" if you will,
of whether or not we draw content, in a single variable. This was so that we
could do the required book-keeping to notify our ancestors of our drawing in a
single place, SetDrawsContent. Otherwise we will have to track changes to the
result of (is_drawable_ && has_drawable_content_), both whenever is_drawable_
changes and when has_drawable_content_ changes.

I am certainly amenable to change, I just felt it would be nice to keep the
book-keeping self-contained.

[danakj, 2014-07-14 15:49:24]

Ah, ok I guess I earlyoutted on codereview a bit too early, thanks for your
comments.

https://codereview.chromium.org/373113003/diff/80001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/373113003/diff/80001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:1033: void Layer::UpdateDrawsContent(bool draws_content) {
I like your idea about splitting this, how about this?

UpdateDrawsContent() {
  has_content = HasDrawableContent();  // This is virtual and returns true if
all the layer subtypes say true for this object. ie we pass by return value
instead of passing by parameter.
  draws_content = has_content && is_drawable_;
  ... inline the rest of SetDrawsContent here ...
}

https://codereview.chromium.org/373113003/diff/80001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:1221: void Layer::AddDrawableDescendants(int num) {
You only need one method here, the only difference in the two is += vs -=. Just
pass negative to remove a child and drop the Remove method?

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

https://codereview.chromium.org/373113003/diff/80001/cc/layers/layer.h#newcod...
cc/layers/layer.h:464: // Called when our number of Drawable Descendants changes
nit: s/D/d/g and add a .

https://codereview.chromium.org/373113003/diff/80001/cc/layers/layer.h#newcod...
cc/layers/layer.h:465: void AddDrawableDescendants(int num);
Can these all be protected?

[awoloszyn, 2014-07-14 19:38:33]

https://codereview.chromium.org/373113003/diff/80001/cc/layers/delegated_rend...
File cc/layers/delegated_renderer_layer.h (right):

https://codereview.chromium.org/373113003/diff/80001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer.h:35: virtual int
NumDescendantsThatDrawContent() const OVERRIDE;
On 2014/07/14 14:57:45, danakj wrote:
> Oh, instead of this, can we put HasDelegatedContent() on this layer like we
have
> on DRLImpl instead? The lying about num descendants can come at the place we
> compute that instead of here?

Done.

https://codereview.chromium.org/373113003/diff/80001/cc/layers/image_layer.cc
File cc/layers/image_layer.cc (right):

https://codereview.chromium.org/373113003/diff/80001/cc/layers/image_layer.cc...
cc/layers/image_layer.cc:37: void ImageLayer::UpdateDrawsContent(bool
drawsContent) {
On 2014/07/14 14:56:31, danakj wrote:
> draws_content thoughout

Done.

https://codereview.chromium.org/373113003/diff/80001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/373113003/diff/80001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:1033: void Layer::UpdateDrawsContent(bool draws_content) {
On 2014/07/14 15:49:23, danakj wrote:
> I like your idea about splitting this, how about this?
> 
> UpdateDrawsContent() {
>   has_content = HasDrawableContent();  // This is virtual and returns true if
> all the layer subtypes say true for this object. ie we pass by return value
> instead of passing by parameter.
>   draws_content = has_content && is_drawable_;
>   ... inline the rest of SetDrawsContent here ...
> }

Done.

https://codereview.chromium.org/373113003/diff/80001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:1221: void Layer::AddDrawableDescendants(int num) {
On 2014/07/14 15:49:23, danakj wrote:
> You only need one method here, the only difference in the two is += vs -=.
Just
> pass negative to remove a child and drop the Remove method?

Done.

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

https://codereview.chromium.org/373113003/diff/80001/cc/layers/layer.h#newcod...
cc/layers/layer.h:464: // Called when our number of Drawable Descendants changes
On 2014/07/14 15:49:23, danakj wrote:
> nit: s/D/d/g and add a .

Done.

https://codereview.chromium.org/373113003/diff/80001/cc/layers/layer.h#newcod...
cc/layers/layer.h:465: void AddDrawableDescendants(int num);
On 2014/07/14 15:49:23, danakj wrote:
> Can these all be protected?

Done.

[danakj, 2014-07-14 20:22:03]

https://codereview.chromium.org/373113003/diff/100001/cc/layers/image_layer.cc
File cc/layers/image_layer.cc (right):

https://codereview.chromium.org/373113003/diff/100001/cc/layers/image_layer.c...
cc/layers/image_layer.cc:38: return !bitmap_.isNull() &&
TiledLayer::HasDrawableContent();
Ok this is more clear to me what's going on, but I guess I'm not excited about
this being separate from the places that call UpdateDrawsContent without it
being clear why we called UpdateDrawsContent where we did.

Before, I disliked the UpdateDrawsContent(true) call, but it had similar
properties of disconnecting this logic from the UpdateDrawsContent call.

I wish we didn't have to deal with multiple levels up inheritance here. But ok
one more suggestion that I think will help make this relationship more clear.
How about we pass the return value of HasDrawableContent() to
UpdateDrawsContent() instead of having it call back to that method.

Then the relation between these two methods is more obvious down in these
subclasses.

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:290: (child->HasDelegatedContent() ? 1000 : 0));
i don't think you need to do this here after you address comments below, it
should be in the NumDescendants* stuff.

Can you make sure we have tests to verify this is working right for delegated
layers, as we currently do for the delegated impl layers.

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1032: // Layers with delegated content need to be treated as
if they have as
This isn't quite what we're doing yet. Right now you're saying if a layer has
delegated content, that its parent has 1000 children that draw content.

What we want is for the layer *with* delegated content to have 1000 children
that draw content. So we'll have to change this->num_descenda... in this method
here to add/subtract 1000.

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1044: draws_content_ = draws_content;
SetNeedsCommit in here, please add this to the tests we have for setting
properties causing a commit and needs_push_props to be true.

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1249: }  // namespace cc
blank line above

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

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer.h#newco...
cc/layers/layer.h:355: int NumDescendantsThatDrawContent() const;
move this up above the // These methods need to be overwritten

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:180: num_descendants_that_draw_content_ =
num_descendants;
You need to call SetNeedsPushProperties() to ensure this gets to the active
tree, please add this to the tests we have for this.

https://codereview.chromium.org/373113003/diff/100001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host_impl.cc (right):

https://codereview.chromium.org/373113003/diff/100001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_impl.cc:70: active_tree()->UpdateDrawProperties();
FakeSyncDrawProps already did this UDP() call, seems like we can merge these
methods maybe?

https://codereview.chromium.org/373113003/diff/100001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_impl.cc:73: void
FakeLayerTreeHostImpl::FakeSyncDrawProperties(LayerTreeImpl* layerTree) {
UpdateNumChildrenAndDrawProperties?

[awoloszyn, 2014-07-16 20:44:20]

https://codereview.chromium.org/373113003/diff/100001/cc/layers/image_layer.cc
File cc/layers/image_layer.cc (right):

https://codereview.chromium.org/373113003/diff/100001/cc/layers/image_layer.c...
cc/layers/image_layer.cc:38: return !bitmap_.isNull() &&
TiledLayer::HasDrawableContent();
On 2014/07/14 20:22:02, danakj wrote:
> Ok this is more clear to me what's going on, but I guess I'm not excited about
> this being separate from the places that call UpdateDrawsContent without it
> being clear why we called UpdateDrawsContent where we did.
> 
> Before, I disliked the UpdateDrawsContent(true) call, but it had similar
> properties of disconnecting this logic from the UpdateDrawsContent call.
> 
> I wish we didn't have to deal with multiple levels up inheritance here. But ok
> one more suggestion that I think will help make this relationship more clear.
> How about we pass the return value of HasDrawableContent() to
> UpdateDrawsContent() instead of having it call back to that method.
> 
> Then the relation between these two methods is more obvious down in these
> subclasses.

Done.

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:290: (child->HasDelegatedContent() ? 1000 : 0));
On 2014/07/14 20:22:02, danakj wrote:
> i don't think you need to do this here after you address comments below, it
> should be in the NumDescendants* stuff.
> 
> Can you make sure we have tests to verify this is working right for delegated
> layers, as we currently do for the delegated impl layers.

Done.

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1032: // Layers with delegated content need to be treated as
if they have as
On 2014/07/14 20:22:02, danakj wrote:
> This isn't quite what we're doing yet. Right now you're saying if a layer has
> delegated content, that its parent has 1000 children that draw content.
> 
> What we want is for the layer *with* delegated content to have 1000 children
> that draw content. So we'll have to change this->num_descenda... in this
method
> here to add/subtract 1000.
I moved this into NumDescendantsThatDrawContent(). This is consistent with the
old behavior where we counted the +1000 regardless of whether or not we drew
content.

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1044: draws_content_ = draws_content;
On 2014/07/14 20:22:02, danakj wrote:
> SetNeedsCommit in here, please add this to the tests we have for setting
> properties causing a commit and needs_push_props to be true.

Done.

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1249: }  // namespace cc
On 2014/07/14 20:22:02, danakj wrote:
> blank line above

Done.

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

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer.h#newco...
cc/layers/layer.h:355: int NumDescendantsThatDrawContent() const;
On 2014/07/14 20:22:02, danakj wrote:
> move this up above the // These methods need to be overwritten

Done.

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/373113003/diff/100001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:180: num_descendants_that_draw_content_ =
num_descendants;
On 2014/07/14 20:22:02, danakj wrote:
> You need to call SetNeedsPushProperties() to ensure this gets to the active
> tree, please add this to the tests we have for this.

Done.

https://codereview.chromium.org/373113003/diff/100001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host_impl.cc (right):

https://codereview.chromium.org/373113003/diff/100001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_impl.cc:70: active_tree()->UpdateDrawProperties();
On 2014/07/14 20:22:03, danakj wrote:
> FakeSyncDrawProps already did this UDP() call, seems like we can merge these
> methods maybe?

Done.

https://codereview.chromium.org/373113003/diff/100001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_impl.cc:73: void
FakeLayerTreeHostImpl::FakeSyncDrawProperties(LayerTreeImpl* layerTree) {
On 2014/07/14 20:22:02, danakj wrote:
> UpdateNumChildrenAndDrawProperties?

Done.

[danakj, 2014-07-17 17:21:26]

https://codereview.chromium.org/373113003/diff/120001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_unittest.cc (right):

https://codereview.chromium.org/373113003/diff/120001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_unittest.cc:33:
always_impl_thread_and_main_thread_blocked_;
do you need this?

https://codereview.chromium.org/373113003/diff/120001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_unittest.cc:59: scoped_refptr<Layer>
layer_after_;
don't see you using this

https://codereview.chromium.org/373113003/diff/120001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_unittest.cc:75: EXPECT_EQ(1000,
delegated_renderer_layer_->NumDescendantsThatDrawContent());
if the DRL isn't going to draw, this should be 0

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

https://codereview.chromium.org/373113003/diff/120001/cc/layers/layer.h#newco...
cc/layers/layer.h:355: // This is only virtual for tests.
is this something we should change? maybe in a followup? why is this needed to
be virtual?

https://codereview.chromium.org/373113003/diff/120001/cc/layers/layer.h#newco...
cc/layers/layer.h:493: // Will relcalculate whether or not we draw content and
set draws_content_
recalculate

s/or not//

https://codereview.chromium.org/373113003/diff/120001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/373113003/diff/120001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:31: bool PictureLayer::HasDrawableContent() const {
can you move this by ClearClient? I liked them close together in your other file
changes

https://codereview.chromium.org/373113003/diff/120001/cc/layers/ui_resource_l...
File cc/layers/ui_resource_layer.cc (right):

https://codereview.chromium.org/373113003/diff/120001/cc/layers/ui_resource_l...
cc/layers/ui_resource_layer.cc:116: if (!layer_tree_host() || bitmap_.empty()) {
can you invert this and drop teh early out so we only have one call to
UpdateDrawsContent?

https://codereview.chromium.org/373113003/diff/120001/cc/test/fake_delegated_...
File cc/test/fake_delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/373113003/diff/120001/cc/test/fake_delegated_...
cc/test/fake_delegated_renderer_layer_impl.cc:73: int
FakeDelegatedRendererLayerImpl::NumDescendantsThatDrawContent() const {
blank line above

https://codereview.chromium.org/373113003/diff/120001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host_impl.cc (right):

https://codereview.chromium.org/373113003/diff/120001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_impl.cc:65: (layer->HasDelegatedContent() ? 1000 :
0);
It's unfortunate to require adding a virtual on LayerImpl just for unit tests
like this.

Are there actually tests that require this HasDelegatedContent on the impl side?
Seems like you're testing this behaviour on the main thread already.

https://codereview.chromium.org/373113003/diff/120001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_impl.cc:68: void
FakeLayerTreeHostImpl::UpdateDrawProperties() {
Let's name this to be more clear what it does from callsites.
UpdateNumChildrenAndDrawPropertiesForActiveTree?

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2994: SetIsDrawable(true);
What if we don't set this to true. How many of these tests don't need to be
changed? Or can we not set it true only for some tests and leave them unchanged
by this CL then?

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3110: // No layers need commit.
Update comment explaining why this happens now

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3115: // Layers added to the tree get
committed.
update comment

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3121: ++expected_push_properties_root_;
move below the comment and update it to reflect why this happens

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3128: // Layers added to the tree get
committed.
update comment

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3154: // Layers added to the tree get
committed.
update comment

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3270:
++expected_push_properties_root_impl_;
explain this one in comment too

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3279:
++expected_push_properties_root_impl_;
explain with commetn

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3287:
++expected_push_properties_child2_impl_;
explain with comment

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3296:
++expected_push_properties_root_impl_;
explain with comment

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3710: // We will need push properties
because our drawable children changed
nit: .

https://codereview.chromium.org/373113003/diff/120001/cc/trees/occlusion_trac...
File cc/trees/occlusion_tracker_unittest.cc (right):

https://codereview.chromium.org/373113003/diff/120001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:168: static void UpdateData(LayerType*
layer) {
Let's give this a more descriptive name. How about RecursiveUpdateNumChildren?

[awoloszyn, 2014-07-17 20:45:01]

https://codereview.chromium.org/373113003/diff/120001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_unittest.cc (right):

https://codereview.chromium.org/373113003/diff/120001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_unittest.cc:33:
always_impl_thread_and_main_thread_blocked_;
On 2014/07/17 17:21:25, danakj wrote:
> do you need this?

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_unittest.cc:59: scoped_refptr<Layer>
layer_after_;
On 2014/07/17 17:21:25, danakj wrote:
> don't see you using this

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_unittest.cc:75: EXPECT_EQ(1000,
delegated_renderer_layer_->NumDescendantsThatDrawContent());
This behavior has been changed, and DRLs won't contribute to the number of
drawable descendants when they themselves do not draw.
On 2014/07/17 17:21:25, danakj wrote:
> if the DRL isn't going to draw, this should be 0

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

https://codereview.chromium.org/373113003/diff/120001/cc/layers/layer.h#newco...
cc/layers/layer.h:355: // This is only virtual for tests.
It might be good to change this in a followup. It looks like a bunch of
unit-tests are dependent on this in subtle ways. It is overridden in the
heads_up_display_unittests, layer_iterator_unittests, and the layer_tree_host_*
unittests.
On 2014/07/17 17:21:25, danakj wrote:
> is this something we should change? maybe in a followup? why is this needed to
> be virtual?

https://codereview.chromium.org/373113003/diff/120001/cc/layers/layer.h#newco...
cc/layers/layer.h:493: // Will relcalculate whether or not we draw content and
set draws_content_
On 2014/07/17 17:21:25, danakj wrote:
> recalculate
> 
> s/or not//

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/373113003/diff/120001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:31: bool PictureLayer::HasDrawableContent() const {
On 2014/07/17 17:21:25, danakj wrote:
> can you move this by ClearClient? I liked them close together in your other
file
> changes

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/layers/ui_resource_l...
File cc/layers/ui_resource_layer.cc (right):

https://codereview.chromium.org/373113003/diff/120001/cc/layers/ui_resource_l...
cc/layers/ui_resource_layer.cc:116: if (!layer_tree_host() || bitmap_.empty()) {
On 2014/07/17 17:21:25, danakj wrote:
> can you invert this and drop teh early out so we only have one call to
> UpdateDrawsContent?

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/test/fake_delegated_...
File cc/test/fake_delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/373113003/diff/120001/cc/test/fake_delegated_...
cc/test/fake_delegated_renderer_layer_impl.cc:73: int
FakeDelegatedRendererLayerImpl::NumDescendantsThatDrawContent() const {
On 2014/07/17 17:21:25, danakj wrote:
> blank line above

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host_impl.cc (right):

https://codereview.chromium.org/373113003/diff/120001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_impl.cc:65: (layer->HasDelegatedContent() ? 1000 :
0);
I reworked the RecursiveUpdateNumChildren to behave the same as the original so
we no longer need a virtual NumDescendantsThatDrawContent.

https://codereview.chromium.org/373113003/diff/120001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_impl.cc:68: void
FakeLayerTreeHostImpl::UpdateDrawProperties() {
On 2014/07/17 17:21:25, danakj wrote:
> Let's name this to be more clear what it does from callsites.
> UpdateNumChildrenAndDrawPropertiesForActiveTree?

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2994: SetIsDrawable(true);
Very good call. We actually don't have to change any of the tests if we don't
set that to true. This actually makes me wonder why that was set to true in the
first place, but that's fairly irrelevant. I have reverted this file, except for
the new tests that I added, and have removed the SetIsDrawable.
On 2014/07/17 17:21:26, danakj wrote:
> What if we don't set this to true. How many of these tests don't need to be
> changed? Or can we not set it true only for some tests and leave them
unchanged
> by this CL then?

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3110: // No layers need commit.
On 2014/07/17 17:21:25, danakj wrote:
> Update comment explaining why this happens now

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3115: // Layers added to the tree get
committed.
On 2014/07/17 17:21:26, danakj wrote:
> update comment

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3121: ++expected_push_properties_root_;
On 2014/07/17 17:21:26, danakj wrote:
> move below the comment and update it to reflect why this happens

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3128: // Layers added to the tree get
committed.
On 2014/07/17 17:21:26, danakj wrote:
> update comment

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3154: // Layers added to the tree get
committed.
On 2014/07/17 17:21:26, danakj wrote:
> update comment

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3270:
++expected_push_properties_root_impl_;
On 2014/07/17 17:21:25, danakj wrote:
> explain this one in comment too

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3279:
++expected_push_properties_root_impl_;
On 2014/07/17 17:21:26, danakj wrote:
> explain with commetn

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3287:
++expected_push_properties_child2_impl_;
On 2014/07/17 17:21:26, danakj wrote:
> explain with comment

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3296:
++expected_push_properties_root_impl_;
On 2014/07/17 17:21:26, danakj wrote:
> explain with comment

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3710: // We will need push properties
because our drawable children changed
On 2014/07/17 17:21:25, danakj wrote:
> nit: .

Done.

https://codereview.chromium.org/373113003/diff/120001/cc/trees/occlusion_trac...
File cc/trees/occlusion_tracker_unittest.cc (right):

https://codereview.chromium.org/373113003/diff/120001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:168: static void UpdateData(LayerType*
layer) {
On 2014/07/17 17:21:26, danakj wrote:
> Let's give this a more descriptive name. How about RecursiveUpdateNumChildren?

Done.

[danakj, 2014-07-28 17:32:37]

lgtm

lgtm

[danakj, 2014-07-28 17:32:38]

lgtm

[commit-bot: I haz the power, 2014-07-28 17:33:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/awoloszyn@chromium.org/373113003/140001

[danakj, 2014-07-28 17:33:13]

haha... ooops. i hit the button by mistake.. laptops..

i was just noticing that I should review this. sorry for the noise.

[danakj, 2014-07-28 17:33:26]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-08-06 14:57:08]

Sorry for the long delay! Here's a few more comments/questions, but I'm really
happy with this, just minor things left.

https://codereview.chromium.org/373113003/diff/140001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_impl_unittest.cc (right):

https://codereview.chromium.org/373113003/diff/140001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl_unittest.cc:314:
FakeLayerTreeHostImpl::UpdateNumChildrenAndDrawProperties(
PrepareToDraw does an UpdateDrawProperties, how come this test needs
UpdateNumChildrenAndDrawProps but the test below needs UpdateNumChildren only?

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1024: bool draws_content = has_drawable_content;
can you DCHECK(is_drawable_ || !has_drawable_content) // Input should true only
if HasDrawableContent() is true.

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1029: // As per the comment that was originally in
I think you can drop the mention of Precalculate, the comment just belongs here
now.

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1040: parent()->AddDrawableDescendants(1);
draws_content ? 1 : -1 like you did above for delegated? then you dont need this
long if/else

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

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.h#newco...
cc/layers/layer.h:355: // This is only virtual for tests.
IIRC you're going to look into removing this virtual afterward? Can you add a
TODO if so?

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.h#newco...
cc/layers/layer.h:493: // Will recalculate whether we draw content and set
draws_content_
s/we draw/the layer draws/

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.h#newco...
cc/layers/layer.h:498: // Called when our number of drawable descendants
changes.
s/our/the layer's/

https://codereview.chromium.org/373113003/diff/140001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/373113003/diff/140001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:201: UpdateDrawsContent(HasDrawableContent());
Do we need to call this in the Constructor as well? Or why not?

https://codereview.chromium.org/373113003/diff/140001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/373113003/diff/140001/cc/layers/texture_layer...
cc/layers/texture_layer.cc:48: UpdateDrawsContent(HasDrawableContent());
Do we need to call this in the constructor as well? Or why not?

https://codereview.chromium.org/373113003/diff/140001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host_impl.cc (right):

https://codereview.chromium.org/373113003/diff/140001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_impl.cc:63: if (layer->DrawsContent())
nit: if (layer->DrawsContent && layer->HasDelegated) num += 1000;

https://codereview.chromium.org/373113003/diff/140001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/373113003/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:1865:
FakeLayerTreeHostImpl::UpdateNumChildrenAndDrawProperties(
PrepareToDraw does an UpdateDrawProps, why do we need to do that as well here?

https://codereview.chromium.org/373113003/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:3700:
FakeLayerTreeHostImpl::UpdateNumChildrenAndDrawProperties(
Same question

[awoloszyn, 2014-08-08 15:53:21]

https://codereview.chromium.org/373113003/diff/140001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_impl_unittest.cc (right):

https://codereview.chromium.org/373113003/diff/140001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl_unittest.cc:314:
FakeLayerTreeHostImpl::UpdateNumChildrenAndDrawProperties(
On 2014/08/06 14:57:07, danakj wrote:
> PrepareToDraw does an UpdateDrawProperties, how come this test needs
> UpdateNumChildrenAndDrawProps but the test below needs UpdateNumChildren only?

I think the DrawProperties was overkill, I have changed to UpdadeNumChildren.

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1024: bool draws_content = has_drawable_content;
On 2014/08/06 14:57:07, danakj wrote:
> can you DCHECK(is_drawable_ || !has_drawable_content) // Input should true
only
> if HasDrawableContent() is true.

I will DCHECK(HasDrawableContent() || !has_drawable_content). I can't DCHECK
against is_drawable_ because HeadsUpDisplayLayer is always drawable regardless
of layer's HasDrawableContent().

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1029: // As per the comment that was originally in
On 2014/08/06 14:57:07, danakj wrote:
> I think you can drop the mention of Precalculate, the comment just belongs
here
> now.

Done.

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1040: parent()->AddDrawableDescendants(1);
On 2014/08/06 14:57:07, danakj wrote:
> draws_content ? 1 : -1 like you did above for delegated? then you dont need
this
> long if/else

Done.

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

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.h#newco...
cc/layers/layer.h:355: // This is only virtual for tests.
On 2014/08/06 14:57:07, danakj wrote:
> IIRC you're going to look into removing this virtual afterward? Can you add a
> TODO if so?

Done.

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.h#newco...
cc/layers/layer.h:493: // Will recalculate whether we draw content and set
draws_content_
On 2014/08/06 14:57:07, danakj wrote:
> s/we draw/the layer draws/

Done.

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.h#newco...
cc/layers/layer.h:498: // Called when our number of drawable descendants
changes.
On 2014/08/06 14:57:07, danakj wrote:
> s/our/the layer's/

Done.

https://codereview.chromium.org/373113003/diff/140001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/373113003/diff/140001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:201: UpdateDrawsContent(HasDrawableContent());
On 2014/08/06 14:57:08, danakj wrote:
> Do we need to call this in the Constructor as well? Or why not?

There is no need in this case, since it will still be false, as
Layer::HasDrawableContent() will return false, since it has not been made
drawable.

https://codereview.chromium.org/373113003/diff/140001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/373113003/diff/140001/cc/layers/texture_layer...
cc/layers/texture_layer.cc:48: UpdateDrawsContent(HasDrawableContent());
On 2014/08/06 14:57:08, danakj wrote:
> Do we need to call this in the constructor as well? Or why not?

Same as PictureLayer. Layer::HasDrawableContent will return false, since we have
not set is_drawable_ yet, in the constructor.

https://codereview.chromium.org/373113003/diff/140001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host_impl.cc (right):

https://codereview.chromium.org/373113003/diff/140001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_impl.cc:63: if (layer->DrawsContent())
On 2014/08/06 14:57:08, danakj wrote:
> nit: if (layer->DrawsContent && layer->HasDelegated) num += 1000;

Done.

https://codereview.chromium.org/373113003/diff/140001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/373113003/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:1865:
FakeLayerTreeHostImpl::UpdateNumChildrenAndDrawProperties(
On 2014/08/06 14:57:08, danakj wrote:
> PrepareToDraw does an UpdateDrawProps, why do we need to do that as well here?

We do not need it. Will change.

https://codereview.chromium.org/373113003/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:3700:
FakeLayerTreeHostImpl::UpdateNumChildrenAndDrawProperties(
On 2014/08/06 14:57:08, danakj wrote:
> Same question

Same.

[danakj, 2014-08-08 16:04:05]

LGTM

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1024: bool draws_content = has_drawable_content;
On 2014/08/08 15:53:20, awoloszyn wrote:
> On 2014/08/06 14:57:07, danakj wrote:
> > can you DCHECK(is_drawable_ || !has_drawable_content) // Input should true
> only
> > if HasDrawableContent() is true.
> 
> I will DCHECK(HasDrawableContent() || !has_drawable_content). I can't DCHECK
> against is_drawable_ because HeadsUpDisplayLayer is always drawable regardless
> of layer's HasDrawableContent().

Oh I see. Can we just make HUD layer's constructor call SetIsDrawable(true) and
then we can dcheck that? I was trying to ensure layers are && with the base
class with that dcheck.

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

https://codereview.chromium.org/373113003/diff/160001/cc/layers/layer.h#newco...
cc/layers/layer.h:499: // Called when the layers number of drawable descendants
changes.
nit: "layer's" apostrophe :)

[awoloszyn, 2014-08-08 16:08:03]

On 2014/08/08 16:04:05, danakj wrote:
> LGTM
> 
> https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.cc
> File cc/layers/layer.cc (right):
> 
>
https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.cc#newc...
> cc/layers/layer.cc:1024: bool draws_content = has_drawable_content;
> On 2014/08/08 15:53:20, awoloszyn wrote:
> > On 2014/08/06 14:57:07, danakj wrote:
> > > can you DCHECK(is_drawable_ || !has_drawable_content) // Input should true
> > only
> > > if HasDrawableContent() is true.
> > 
> > I will DCHECK(HasDrawableContent() || !has_drawable_content). I can't DCHECK
> > against is_drawable_ because HeadsUpDisplayLayer is always drawable
regardless
> > of layer's HasDrawableContent().
> 
> Oh I see. Can we just make HUD layer's constructor call SetIsDrawable(true)
and
> then we can dcheck that? I was trying to ensure layers are && with the base
> class with that dcheck.
> 
The only problem I see with that would be if someone called SetIsDrawable(false)
on the HUD layer at some point after that, which would cause the assert to get
fired. I am not sure if this is ever done, but we may break existing behavior.

> https://codereview.chromium.org/373113003/diff/160001/cc/layers/layer.h
> File cc/layers/layer.h (right):
> 
>
https://codereview.chromium.org/373113003/diff/160001/cc/layers/layer.h#newco...
> cc/layers/layer.h:499: // Called when the layers number of drawable
descendants
> changes.
> nit: "layer's" apostrophe :)
Done.

[danakj, 2014-08-08 16:47:04]

On Fri, Aug 8, 2014 at 12:08 PM, <awoloszyn@chromium.org> wrote:

> On 2014/08/08 16:04:05, danakj wrote:
>
>> LGTM
>>
>
>  https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.cc
>> File cc/layers/layer.cc (right):
>>
>
>
> https://codereview.chromium.org/373113003/diff/140001/cc/
> layers/layer.cc#newcode1024
>
>> cc/layers/layer.cc:1024: bool draws_content = has_drawable_content;
>> On 2014/08/08 15:53:20, awoloszyn wrote:
>> > On 2014/08/06 14:57:07, danakj wrote:
>> > > can you DCHECK(is_drawable_ || !has_drawable_content) // Input should
>> true
>> > only
>> > > if HasDrawableContent() is true.
>> >
>> > I will DCHECK(HasDrawableContent() || !has_drawable_content). I can't
>> DCHECK
>> > against is_drawable_ because HeadsUpDisplayLayer is always drawable
>>
> regardless
>
>> > of layer's HasDrawableContent().
>>
>
>  Oh I see. Can we just make HUD layer's constructor call
>> SetIsDrawable(true)
>>
> and
>
>> then we can dcheck that? I was trying to ensure layers are && with the
>> base
>> class with that dcheck.
>>
>
> The only problem I see with that would be if someone called
> SetIsDrawable(false)
> on the HUD layer at some point after that, which would cause the assert to
> get
> fired. I am not sure if this is ever done, but we may break existing
> behavior.


The HUD layer is an internal thing no one should be using it except LTH. So
DCHECK firing cuz we did something wrong is okay.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[awoloszyn, 2014-08-11 15:35:26]

On 2014/08/08 16:47:04, danakj wrote:
> On Fri, Aug 8, 2014 at 12:08 PM, <mailto:awoloszyn@chromium.org> wrote:
> 
> > On 2014/08/08 16:04:05, danakj wrote:
> >
> >> LGTM
> >>
> >
> >  https://codereview.chromium.org/373113003/diff/140001/cc/layers/layer.cc
> >> File cc/layers/layer.cc (right):
> >>
> >
> >
> > https://codereview.chromium.org/373113003/diff/140001/cc/
> > layers/layer.cc#newcode1024
> >
> >> cc/layers/layer.cc:1024: bool draws_content = has_drawable_content;
> >> On 2014/08/08 15:53:20, awoloszyn wrote:
> >> > On 2014/08/06 14:57:07, danakj wrote:
> >> > > can you DCHECK(is_drawable_ || !has_drawable_content) // Input should
> >> true
> >> > only
> >> > > if HasDrawableContent() is true.
> >> >
> >> > I will DCHECK(HasDrawableContent() || !has_drawable_content). I can't
> >> DCHECK
> >> > against is_drawable_ because HeadsUpDisplayLayer is always drawable
> >>
> > regardless
> >
> >> > of layer's HasDrawableContent().
> >>
> >
> >  Oh I see. Can we just make HUD layer's constructor call
> >> SetIsDrawable(true)
> >>
> > and
> >
> >> then we can dcheck that? I was trying to ensure layers are && with the
> >> base
> >> class with that dcheck.
> >>
> >
> > The only problem I see with that would be if someone called
> > SetIsDrawable(false)
> > on the HUD layer at some point after that, which would cause the assert to
> > get
> > fired. I am not sure if this is ever done, but we may break existing
> > behavior.
> 
> 
> The HUD layer is an internal thing no one should be using it except LTH. So
> DCHECK firing cuz we did something wrong is okay.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
Done.

[danakj, 2014-08-11 15:37:53]

Thanks! LGTM

[awoloszyn, 2014-08-11 15:38:34]

The CQ bit was checked by awoloszyn@chromium.org

[commit-bot: I haz the power, 2014-08-11 15:39:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/awoloszyn@chromium.org/373113003/200001

[commit-bot: I haz the power, 2014-08-11 17:39:59]

Change committed as 288740