cc: Add a flag to layers that returns true if the layer is in RSLL.

cc: Add a flag to layers that returns true if the layer is in RSLL.

This patch adds a flag that says whether the layer is currently in RSLL.
Used to remove layers that previously were in RSLL and are no longer
there.

R=enne, danakj

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

[vmpstr, 2014-05-02 23:06:27]

Please take a look. Honestly, I'm not too sure about the place where I update
all of this, so suggestions welcome.

https://codereview.chromium.org/265883013/diff/20001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/265883013/diff/20001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:101: calculate_draw_properties_count_(0u) {
There is a SourceAnimationFrameNumber, which happens to be used in unittests
only... I'm somewhat hesitant on relying on that, so I opted in using a separate
count. 

If that's not acceptable, then I will have to do a deeper investigation into
whether the SourceAnimationFrameNumber is really what I want here.

https://codereview.chromium.org/265883013/diff/20001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:498:
layer->draw_properties().last_render_surface_list_membership_index =
I suspect this should happen in LayerTreeHostCommon::CalculateDrawProperties? Is
there a place where we iterate all of the render surface layer list in there? I
found one push back to the list, but the iterate iterates more than just the
list.

[danakj, 2014-05-02 23:09:24]

https://codereview.chromium.org/265883013/diff/20001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/265883013/diff/20001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:498:
layer->draw_properties().last_render_surface_list_membership_index =
On 2014/05/02 23:06:27, vmpstr wrote:
> I suspect this should happen in LayerTreeHostCommon::CalculateDrawProperties?
Is
> there a place where we iterate all of the render surface layer list in there?
I
> found one push back to the list, but the iterate iterates more than just the
> list.

Right, the output of CalcDrawProps is the RSLL. If you set this on every layer
that you push into the list, then you'll have what you want.

[vmpstr, 2014-05-02 23:41:22]

https://codereview.chromium.org/265883013/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/265883013/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:2268: // If the layer passed all of the early
outs, that means it's finally in the
Kind of like so?

[danakj, 2014-05-02 23:45:26]

https://codereview.chromium.org/265883013/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/265883013/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:2015: descendants.push_back(layer);
Here we might not have added the layer even tho we recursed over it.

IOW, a parent might not be in the RSLL cuz it draws nothing but the child is. I
think you wanna break it out into a helper method, and call it on each case of
push_back().

And in RemoveSurfaceForEarlyExit() reset the value to 0.

https://codereview.chromium.org/265883013/diff/40001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/265883013/diff/40001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:462: ++calculate_draw_properties_count_;
Make this skip 0 on wraparound please.

[enne (OOO), 2014-05-02 23:47:53]

https://codereview.chromium.org/265883013/diff/40001/cc/layers/draw_properties.h
File cc/layers/draw_properties.h (right):

https://codereview.chromium.org/265883013/diff/40001/cc/layers/draw_propertie...
cc/layers/draw_properties.h:122: size_t
last_render_surface_list_membership_index;
Bikeshed hammer: this isn't really an index, also masks and replicas aren't in
the render surface layer list but they are drawn.  Maybe this should just say
"last_frame_number_valid"?

https://codereview.chromium.org/265883013/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/265883013/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.h:71: size_t
current_calculate_draw_properties_count;
I think I would prefer using SourceFrameNumber here.

(Also, size_t => int)

[vmpstr, 2014-05-05 18:46:42]

PTAL

https://codereview.chromium.org/265883013/diff/40001/cc/layers/draw_properties.h
File cc/layers/draw_properties.h (right):

https://codereview.chromium.org/265883013/diff/40001/cc/layers/draw_propertie...
cc/layers/draw_properties.h:122: size_t
last_render_surface_list_membership_index;
On 2014/05/02 23:47:53, enne wrote:
> Bikeshed hammer: this isn't really an index, also masks and replicas aren't in
> the render surface layer list but they are drawn.  Maybe this should just say
> "last_frame_number_valid"?

Done.

https://codereview.chromium.org/265883013/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/265883013/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:2015: descendants.push_back(layer);
On 2014/05/02 23:45:26, danakj wrote:
> Here we might not have added the layer even tho we recursed over it.
> 
> IOW, a parent might not be in the RSLL cuz it draws nothing but the child is.
I
> think you wanna break it out into a helper method, and call it on each case of
> push_back().
> 
> And in RemoveSurfaceForEarlyExit() reset the value to 0.

I've changed this around a bit. From my understanding of RSLL, I think this is
correct now?

https://codereview.chromium.org/265883013/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/265883013/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.h:71: size_t
current_calculate_draw_properties_count;
On 2014/05/02 23:47:53, enne wrote:
> I think I would prefer using SourceFrameNumber here.
> 
> (Also, size_t => int)

I would still prefer to have an independent count here... The reason is that I
want to ensure that this doesn't change anywhere except when
CalculateDrawProperties is called (or right before that), and remains unchanged
otherwise (so that ManageTiles has correct information). Using the
Source{,Animation}FrameNumber seem like a risk to me. That is, it changes when
we draw, and it only changes sometimes (if we don't early out from drawing).
It's very much dependent on the order of operations.. So, if we ever calc draw
props, then update the frame number, and then call manage tiles, then wrong
things will happen.

I _think_ this is OK now, but it really feels like a source of future bugs (or
maybe current bugs). Anyway, if you still insist that we use the frame number,
I'll dig in to see if there are any cases where it's not appropriate (which is
hopefully none). Otherwise, please consider the current implementation.

https://codereview.chromium.org/265883013/diff/40001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/265883013/diff/40001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:462: ++calculate_draw_properties_count_;
On 2014/05/02 23:45:26, danakj wrote:
> Make this skip 0 on wraparound please.

Done.

[danakj, 2014-05-05 19:25:40]

I'd like we didn't call the RSLL id number a "count" as I think that is
misleading a little. Can we call it an "iteration" maybe? or an "id" or
something?

Needs some tests in layer_tree_host_common_unittest.cc, please have tests with a
layer that has a render surface (can use SetForceRenderSurface(true)), that hit
the early outs and that don't.

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

https://codereview.chromium.org/265883013/diff/80001/cc/layers/draw_propertie...
cc/layers/draw_properties.h:122: int last_valid_render_surface_list_count;
Separate this out from he others here and put a comment on it. 

bikeshed: member_of_render_surface_layer_list_iteration or something?

|count| seems to imply something this isn't, like how many times it was in RSLL

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

https://codereview.chromium.org/265883013/diff/80001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:537: bool is_render_surface_list_member() const;
IsRenderSurfaceLayerListMember() since this is not an inline function

https://codereview.chromium.org/265883013/diff/80001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/265883013/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:689: for (int i = 0; i <
static_cast<int>(layers_.size()); ++i) {
use "size_t i" here for index into an array

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:790: 0);
can you put this in a temp var to give it a name?

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

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1133: static inline void
MarkLayerWithFrameNumber(
"FrameNumber" seems like a misnomer here

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1144: if (layer->replica_layer() &&
layer->replica_layer()->mask_layer()) {
can you mark the replica layer too (it doesn't have tiles but to be complete)?

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1155: scoped_refptr<LayerType> layer,
Does the templated method above not handle this case too where
LayerType=scoped_refptr<Layer> ?

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1190: 
nit: drop extra whitespace?

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1984:
render_surface_layer_list->push_back(layer);
Mark the layer with the number here also, if you want to include layers in the
RSLL top list, that aren't in some RS's layer_list

Currently I think this would miss the root layer if it doesn't draw

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:2134: MarkLayerWithFrameNumber(child,
current_calculate_draw_properties_count);
then this one won't be needed any more

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:466: if (calculate_draw_properties_count_ ==
std::numeric_limits<int>::max())
nit:
if (==max())
  = 1
else
  ++

to also make use of the max value

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:503: 
nit: remove

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.h:246: int CurrentDrawPropertiesCount() const {
calculate_draw_properties_count() since it's an inline accessor

[enne (OOO), 2014-05-05 20:13:33]

A separate count seems reasonable.

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.h:246: int CurrentDrawPropertiesCount() const {
On 2014/05/05 19:25:41, danakj wrote:
> calculate_draw_properties_count() since it's an inline accessor

(This is optional style and is totally ok as-is.)

[vmpstr, 2014-05-05 20:50:33]

I've updated based on comments (PTAL). I'll work on adding tests now, but I have
a few questions about when to mark or not mark (see below). Thanks.

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

https://codereview.chromium.org/265883013/diff/80001/cc/layers/draw_propertie...
cc/layers/draw_properties.h:122: int last_valid_render_surface_list_count;
On 2014/05/05 19:25:41, danakj wrote:
> Separate this out from he others here and put a comment on it. 
> 
> bikeshed: member_of_render_surface_layer_list_iteration or something?
> 
> |count| seems to imply something this isn't, like how many times it was in
RSLL

Done.

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

https://codereview.chromium.org/265883013/diff/80001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:537: bool is_render_surface_list_member() const;
On 2014/05/05 19:25:41, danakj wrote:
> IsRenderSurfaceLayerListMember() since this is not an inline function

Done.

https://codereview.chromium.org/265883013/diff/80001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/265883013/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:689: for (int i = 0; i <
static_cast<int>(layers_.size()); ++i) {
On 2014/05/05 19:25:41, danakj wrote:
> use "size_t i" here for index into an array

Ok, I just don't like i=0; --i; (i == std::numeric_limits<size_it>::max()), then
++i back to 0 :P I figured negative numbers are more fun, but the other way
works.

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:790: 0);
On 2014/05/05 19:25:41, danakj wrote:
> can you put this in a temp var to give it a name?

Done.

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

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1133: static inline void
MarkLayerWithFrameNumber(
On 2014/05/05 19:25:41, danakj wrote:
> "FrameNumber" seems like a misnomer here

Done.

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1144: if (layer->replica_layer() &&
layer->replica_layer()->mask_layer()) {
On 2014/05/05 19:25:41, danakj wrote:
> can you mark the replica layer too (it doesn't have tiles but to be complete)?

Done.

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1155: scoped_refptr<LayerType> layer,
On 2014/05/05 19:25:41, danakj wrote:
> Does the templated method above not handle this case too where
> LayerType=scoped_refptr<Layer> ?

No, the signature is LayerType*, and scoped_refptr<A> doesn't bind to A*. I
think it would be fine if I accept a LayerType&, but that seems less in line
with "pass things by pointer if it's meant to be modified"

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1190: 
On 2014/05/05 19:25:41, danakj wrote:
> nit: drop extra whitespace?

Done.

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1984:
render_surface_layer_list->push_back(layer);
On 2014/05/05 19:25:41, danakj wrote:
> Mark the layer with the number here also, if you want to include layers in the
> RSLL top list, that aren't in some RS's layer_list
> 
> Currently I think this would miss the root layer if it doesn't draw

In this case, would the layer still be visited by the iterator? If not, then I'm
not sure it should be marked... If so, then it definitely should be :)

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:2134: MarkLayerWithFrameNumber(child,
current_calculate_draw_properties_count);
On 2014/05/05 19:25:41, danakj wrote:
> then this one won't be needed any more

I don't follow... The above would only mark the parent layer, this pushes a
child: shouldn't this be marked independently?

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:466: if (calculate_draw_properties_count_ ==
std::numeric_limits<int>::max())
On 2014/05/05 19:25:41, danakj wrote:
> nit:
> if (==max())
>   = 1
> else
>   ++
> 
> to also make use of the max value

Done.

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:503: 
On 2014/05/05 19:25:41, danakj wrote:
> nit: remove

Done.

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.h:246: int CurrentDrawPropertiesCount() const {
On 2014/05/05 20:13:34, enne wrote:
> On 2014/05/05 19:25:41, danakj wrote:
> > calculate_draw_properties_count() since it's an inline accessor
> 
> (This is optional style and is totally ok as-is.)

Done.

[danakj, 2014-05-05 21:25:21]

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

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1984:
render_surface_layer_list->push_back(layer);
On 2014/05/05 20:50:34, vmpstr wrote:
> On 2014/05/05 19:25:41, danakj wrote:
> > Mark the layer with the number here also, if you want to include layers in
the
> > RSLL top list, that aren't in some RS's layer_list
> > 
> > Currently I think this would miss the root layer if it doesn't draw
> 
> In this case, would the layer still be visited by the iterator? If not, then
I'm
> not sure it should be marked... If so, then it definitely should be :)

It would be visited by the iterator with the state
represents_target_render_surface()

https://codereview.chromium.org/265883013/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:2134: MarkLayerWithFrameNumber(child,
current_calculate_draw_properties_count);
On 2014/05/05 20:50:34, vmpstr wrote:
> On 2014/05/05 19:25:41, danakj wrote:
> > then this one won't be needed any more
> 
> I don't follow... The above would only mark the parent layer, this pushes a
> child: shouldn't this be marked independently?

This pushes a child that has a render surface. But if it has a render surface,
it was pushed onto the RSLL top level list already by the above one.

IOW you could DCHECK that the rsll.back() is this child here and it should pass
(maybe do that with a comment saying you don't need to mark it?)

[vmpstr, 2014-05-06 18:17:36]

PTAL. I've added some unittests, but please let me know if you'd like more cases
handled (let me know which ones)

[danakj, 2014-05-06 19:21:30]

A couple more naming quibbles but LGTM

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

https://codereview.chromium.org/265883013/diff/180001/cc/layers/draw_properti...
cc/layers/draw_properties.h:123: // Each time we generate a new render surface
layer list, and ID is used to
and->an

https://codereview.chromium.org/265883013/diff/180001/cc/layers/draw_properti...
cc/layers/draw_properties.h:128: int last_valid_render_surface_list_id;
s/surface_list/surface_layer_list/

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

https://codereview.chromium.org/265883013/diff/180001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:537: bool is_render_surface_list_member() const;
IsRenderSurfaceLayerListMember

camel-case is required for non-inline methods (the inverse is optional for
inline methods), and use the full RSLL instead of just RSL in the name.

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

https://codereview.chromium.org/265883013/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1133: static inline void
MarkLayerWithDrawPropertiesId(
We have 3 names for this ID in this cl now :) How about
MarkLayerWithRenderSurfaceLayerListId?

https://codereview.chromium.org/265883013/diff/180001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/265883013/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:10268: std::set<LayerImpl*>
expected;
If it's easier you could just compare the pointers in the rsll to the layer
pointers (they have a deterministic order)

https://codereview.chromium.org/265883013/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:10345: }
Cool! I think that this test covers us calling RemoveSurfaceForEarlyExit(), can
you confirm?

[vmpstr, 2014-05-06 20:52:14]

enne, did you want to give this another look?

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

https://codereview.chromium.org/265883013/diff/180001/cc/layers/draw_properti...
cc/layers/draw_properties.h:123: // Each time we generate a new render surface
layer list, and ID is used to
On 2014/05/06 19:21:31, danakj wrote:
> and->an

Done.

https://codereview.chromium.org/265883013/diff/180001/cc/layers/draw_properti...
cc/layers/draw_properties.h:128: int last_valid_render_surface_list_id;
On 2014/05/06 19:21:31, danakj wrote:
> s/surface_list/surface_layer_list/

Done.

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

https://codereview.chromium.org/265883013/diff/180001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:537: bool is_render_surface_list_member() const;
On 2014/05/06 19:21:31, danakj wrote:
> IsRenderSurfaceLayerListMember
> 
> camel-case is required for non-inline methods (the inverse is optional for
> inline methods), and use the full RSLL instead of just RSL in the name.

Done.

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

https://codereview.chromium.org/265883013/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1133: static inline void
MarkLayerWithDrawPropertiesId(
On 2014/05/06 19:21:31, danakj wrote:
> We have 3 names for this ID in this cl now :) How about
> MarkLayerWithRenderSurfaceLayerListId?

Done.

https://codereview.chromium.org/265883013/diff/180001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/265883013/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:10268: std::set<LayerImpl*>
expected;
On 2014/05/06 19:21:31, danakj wrote:
> If it's easier you could just compare the pointers in the rsll to the layer
> pointers (they have a deterministic order)

I'd rather leave it like this, tbh. This test isn't really testing the order of
elements, so if those happen to change for whatever reason, this test should
still pass.

https://codereview.chromium.org/265883013/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:10345: }
On 2014/05/06 19:21:31, danakj wrote:
> Cool! I think that this test covers us calling RemoveSurfaceForEarlyExit(),
can
> you confirm?

Yes, there's a case where that's called (I know because I had a bug where I
didn't mark the layer itself as not a member, instead of just marking the render
surface's layer list)

[danakj, 2014-05-06 21:02:36]

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

https://codereview.chromium.org/265883013/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1983:
MarkLayerWithRenderSurfaceLayerListId(layer,
We were discussing this and enne@ pointed out that you only want to mark layers
that contribute their content to their render target (ie the layer itself draws
something), as opposed to all layers that are in the RSLL at all?

Meaning, you don't want to include a picture layer that doesn't draw anything
but owns a RenderSurface?

If that's corrent then apologies, this should go away, along with the DCHECK
below that you added, and your test expectations would have to be adjusted to
match.

[enne (OOO), 2014-05-06 21:10:27]

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

https://codereview.chromium.org/265883013/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1184:
MarkLayerListWithRenderSurfaceLayerListId(
This doesn't do anything.  You're calling this after
ClearRenderSurfaceLayerList.

https://codereview.chromium.org/265883013/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1186:
MarkLayerWithRenderSurfaceLayerListId(render_surface_layer_list->back(), 0);
Likewise, this.

https://codereview.chromium.org/265883013/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1190:
MarkLayerListWithRenderSurfaceLayerListId(
We will only get into this function if
layer_to_remove->render_surface()->layer_list() is empty.  Can you just assert
instead of calling this function?

https://codereview.chromium.org/265883013/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1192:
MarkLayerWithRenderSurfaceLayerListId(layer_to_remove, 0);
This should already be marked properly.  The layer was not added to its own RSLL
by definition because of the check before calling this function.  Therefore, its
id is already correctly stale.  Can you just assert instead of calling this
function?

https://codereview.chromium.org/265883013/diff/200001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/265883013/diff/200001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:465: if (render_surface_layer_list_id_ ==
std::numeric_limits<int>::max())
There's really no need for this.  2^31 frames / 60fps = 1.13 years.

[vmpstr, 2014-05-06 22:38:52]

PTAL o_o;

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

https://codereview.chromium.org/265883013/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1184:
MarkLayerListWithRenderSurfaceLayerListId(
On 2014/05/06 21:10:27, enne wrote:
> This doesn't do anything.  You're calling this after
> ClearRenderSurfaceLayerList.

Fixed.

https://codereview.chromium.org/265883013/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1186:
MarkLayerWithRenderSurfaceLayerListId(render_surface_layer_list->back(), 0);
On 2014/05/06 21:10:27, enne wrote:
> Likewise, this.

This marks the layer itself?

https://codereview.chromium.org/265883013/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1190:
MarkLayerListWithRenderSurfaceLayerListId(
On 2014/05/06 21:10:27, enne wrote:
> We will only get into this function if
> layer_to_remove->render_surface()->layer_list() is empty.  Can you just assert
> instead of calling this function?

It doesn't have to be empty here, we also call this function when the whole
thing is clipped away. At least, I think that's the reason we have to clear the
render surface layer list?

https://codereview.chromium.org/265883013/diff/200001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/265883013/diff/200001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:465: if (render_surface_layer_list_id_ ==
std::numeric_limits<int>::max())
On 2014/05/06 21:10:27, enne wrote:
> There's really no need for this.  2^31 frames / 60fps = 1.13 years.

Done.

[danakj, 2014-05-06 22:48:01]

LG % names

Do we need to add mask/replicamask to the test or is this going to get coverage
from glen's change?

https://codereview.chromium.org/265883013/diff/240001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (left):

https://codereview.chromium.org/265883013/diff/240001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1937: 
nit: keep this whitespace i guess, separates it from the comment.

[danakj, 2014-05-06 22:49:04]

On Tue, May 6, 2014 at 6:48 PM, <danakj@chromium.org> wrote:

> LG % names
>
> Do we need to add mask/replicamask to the test or is this going to get
> coverage
> from glen's change?
>

nvm i glossed over that.

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

[vmpstr, 2014-05-06 23:24:46]

https://codereview.chromium.org/265883013/diff/240001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (left):

https://codereview.chromium.org/265883013/diff/240001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1937: 
On 2014/05/06 22:48:02, danakj wrote:
> nit: keep this whitespace i guess, separates it from the comment.

Done.

[enne (OOO), 2014-05-07 00:12:11]

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

https://codereview.chromium.org/265883013/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1186:
MarkLayerWithRenderSurfaceLayerListId(render_surface_layer_list->back(), 0);
On 2014/05/06 22:38:53, vmpstr wrote:
> On 2014/05/06 21:10:27, enne wrote:
> > Likewise, this.
> 
> This marks the layer itself?

Ok, I guess it's getting the mask and replica from the layer.  The layer itself
got cleared in the list above.

https://codereview.chromium.org/265883013/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1190:
MarkLayerListWithRenderSurfaceLayerListId(
On 2014/05/06 22:38:53, vmpstr wrote:
> On 2014/05/06 21:10:27, enne wrote:
> > We will only get into this function if
> > layer_to_remove->render_surface()->layer_list() is empty.  Can you just
assert
> > instead of calling this function?
> 
> It doesn't have to be empty here, we also call this function when the whole
> thing is clipped away. At least, I think that's the reason we have to clear
the
> render surface layer list?

Oh right, I forgot the clip case.

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

https://codereview.chromium.org/265883013/diff/280001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2055:
MarkLayerWithRenderSurfaceLayerListId(layer,
This is a little weird because it also adds the mask and the replica, but this
conditional is only about the layer itself.  You then also go handle the mask
and replica down below again.  Can this be less redundant?

[vmpstr, 2014-05-07 17:35:57]

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

https://codereview.chromium.org/265883013/diff/280001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2055:
MarkLayerWithRenderSurfaceLayerListId(layer,
On 2014/05/07 00:12:11, enne wrote:
> This is a little weird because it also adds the mask and the replica, but this
> conditional is only about the layer itself.  You then also go handle the mask
> and replica down below again.  Can this be less redundant?

You mean this could just be marking the layer and on the way up as a child, the
code below would mark the masks? Would that also apply to the root layer (that
is not a child of anyone)? No! Because root layer doesn't have masks, if I read
a DCHECK somewhere correctly.

Do you think it's worth passing an enum to the function like "MARK_LAYER_ONLY,
MARK_MASKS_ONLY, MARK_EVERYTHING"

[enne (OOO), 2014-05-07 17:41:16]

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

https://codereview.chromium.org/265883013/diff/280001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2055:
MarkLayerWithRenderSurfaceLayerListId(layer,
On 2014/05/07 17:35:58, vmpstr wrote:
> On 2014/05/07 00:12:11, enne wrote:
> > This is a little weird because it also adds the mask and the replica, but
this
> > conditional is only about the layer itself.  You then also go handle the
mask
> > and replica down below again.  Can this be less redundant?
> 
> You mean this could just be marking the layer and on the way up as a child,
the
> code below would mark the masks? Would that also apply to the root layer (that
> is not a child of anyone)? No! Because root layer doesn't have masks, if I
read
> a DCHECK somewhere correctly.
> 
> Do you think it's worth passing an enum to the function like "MARK_LAYER_ONLY,
> MARK_MASKS_ONLY, MARK_EVERYTHING"

Yeah, the masks get marked twice.  It's not a huge deal, but it means that the
code below could get removed and things would probably still work, which just
seems like code smell to me.

Please no enums.  Just mark it explicitly without the helper like you do with
the masks?

[danakj, 2014-05-07 17:43:05]

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

https://codereview.chromium.org/265883013/diff/280001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2055:
MarkLayerWithRenderSurfaceLayerListId(layer,
On 2014/05/07 17:41:17, enne wrote:
> On 2014/05/07 17:35:58, vmpstr wrote:
> > On 2014/05/07 00:12:11, enne wrote:
> > > This is a little weird because it also adds the mask and the replica, but
> this
> > > conditional is only about the layer itself.  You then also go handle the
> mask
> > > and replica down below again.  Can this be less redundant?
> > 
> > You mean this could just be marking the layer and on the way up as a child,
> the
> > code below would mark the masks? Would that also apply to the root layer
(that
> > is not a child of anyone)? No! Because root layer doesn't have masks, if I
> read
> > a DCHECK somewhere correctly.
> > 
> > Do you think it's worth passing an enum to the function like
"MARK_LAYER_ONLY,
> > MARK_MASKS_ONLY, MARK_EVERYTHING"
> 
> Yeah, the masks get marked twice.  It's not a huge deal, but it means that the
> code below could get removed and things would probably still work, which just
> seems like code smell to me.

Ya there's really no reason to mark the masks here. Below is the correct place,
I agree.

> Please no enums.  Just mark it explicitly without the helper like you do with
> the masks?

This would make the clearing code a bit more verbose to clear the masks as well,
or make two methods, one that does the masks too?

[vmpstr, 2014-05-07 18:26:04]

PTAL, I've separated layer and mask marking into separate helpers.

[Ian Vollick, 2014-05-08 00:37:35]

On 2014/05/07 18:26:04, vmpstr wrote:
> PTAL, I've separated layer and mask marking into separate helpers.

This patch not only adds the RSLL id, it also makes use of it. Could you please
comment on how it's used in the patch description?

[enne (OOO), 2014-05-08 00:40:57]

lgtm

https://codereview.chromium.org/265883013/diff/350001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/265883013/diff/350001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:688: void TileManager::CleanUpLayers() {
Could you maybe split this out of this change, as vollick wants to use this
patch too, so landing just the rsll-id (which is less likely to be reverted)
might be better?

https://codereview.chromium.org/265883013/diff/350001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/265883013/diff/350001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:10151: static void
gather_drawn_layers(LayerImplList* rsll,
style nit: GatherDrawnLayers ;)

https://codereview.chromium.org/265883013/diff/350001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:10171:
TEST_F(LayerTreeHostCommonTest, RenderSurfaceLayerListMembership) {
Please add replica mask layers here too.

[Ian Vollick, 2014-05-08 00:41:53]

Drive-by nitting!

https://codereview.chromium.org/265883013/diff/350001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/265883013/diff/350001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:7: #include <limits>
nit: this looks unused.

[vmpstr, 2014-05-08 17:02:57]

https://codereview.chromium.org/265883013/diff/350001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/265883013/diff/350001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:688: void TileManager::CleanUpLayers() {
On 2014/05/08 00:40:57, enne wrote:
> Could you maybe split this out of this change, as vollick wants to use this
> patch too, so landing just the rsll-id (which is less likely to be reverted)
> might be better?

Done.

https://codereview.chromium.org/265883013/diff/350001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/265883013/diff/350001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:10151: static void
gather_drawn_layers(LayerImplList* rsll,
On 2014/05/08 00:40:57, enne wrote:
> style nit: GatherDrawnLayers ;)

Done.

https://codereview.chromium.org/265883013/diff/350001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:10171:
TEST_F(LayerTreeHostCommonTest, RenderSurfaceLayerListMembership) {
On 2014/05/08 00:40:57, enne wrote:
> Please add replica mask layers here too.

Done.

https://codereview.chromium.org/265883013/diff/350001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/265883013/diff/350001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:7: #include <limits>
On 2014/05/08 00:41:54, Ian Vollick wrote:
> nit: this looks unused.

Done.

[vmpstr, 2014-05-08 17:03:04]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-05-08 17:06:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vmpstr@chromium.org/265883013/370001

[vmpstr, 2014-05-08 17:47:08]

The CQ bit was unchecked by vmpstr@chromium.org

[vmpstr, 2014-05-08 17:47:53]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-05-08 17:52:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vmpstr@chromium.org/265883013/390001

[commit-bot: I haz the power, 2014-05-09 17:14:42]

Change committed as 269353