Bug 74203 – [chromium] Create iterators for the RenderSurface-Layer tree

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Create iterators for the RenderSurface-Layer tree

[Dana Jansens]

Created attachment 118614 [details]
Patch

This patch makes a CCLayerIterator class with 3 possible iterator orderings.

The iterator class is used in 2 places to demonstrate its use (CCLayerTreeHost and CCLayerTreeHostCommon) but could be used it more places as well.

[James Robinson]

Have you thought about how this interacts with https://bugs.webkit.org/show_bug.cgi?id=73059?

[Dana Jansens]

I have run a previous iteration by enne, but I will read through eir patch thoroughly.

[Dana Jansens]

Spoke with enne about bug #73059. The tl;dr is it doesn't conflict, and could later be used in calculateRenderPasses().


73059 removes all the layer tree walks it touches, and adds one new one in calculateRenderPasses(). This would equate to the same sort of walk done in CCLayerTreeHost in the current patch, and could make use of the iterator there. 

The patch does not seem to intersect with #73059 at all at the moment, other than 
intellectually where it is doing its own tree walk.  That can be switched to use the iterator afterward easily enough. 

Draw culling may be able to use the iterators also if it needs a top-to-bottom layer walk.

There are other tree walks in the code that could also be switched later. I thought it best to make it minimal, but wanted to actually demonstrate its use also and prove that it works by passing unit/layout tests.

[Dana Jansens]

Created attachment 119025 [details]
Patch

[Dana Jansens]

Changed front-to-back/back-to-front to include renderSurfaceLayers so we can use it for the paint loop.

[Shawn Singh]

Comment on attachment 119025 [details]
Patch

This iterator is a really cool idea.
First round of comments... they are mostly nits, but I think all of these suggestions combined can make the iterator "safer" in the face of abuse and more readable =)


View in context: https://bugs.webkit.org/attachment.cgi?id=119025&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:33
> +// These classes provide means to iterate over the RenderSurface-Layer tree.

As per our offline discussion - would it be OK if we used the following terminology?  Shouldnt require too many changes in code:

(1) "target surface" --> the surface that layers/surface contribute to
(2) "contributing surface" --> a surface that is hosted by a layer, which contributes to a target surface.
(3) "renderSurfaceLayer" seems too ambiguous, and we need a distinction between #2 and #3
(4) Instead of "hosting", could we say "representing"?  This feels more correct to me because a layer technically can have all 3 roles at different times (layer represents a contributing surface, layer represents a target surface, layer represents itself as a layer).  we can't say "layer hosts itself", so we lose some clarity in explanation if we use that word.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:35
> +// The iterator's current position is always a child layer of a RenderSurface, or an owning layer of a RenderSurface. While a layer can be both, some iterators will return it once
> +// in each of these states that it fulfills. A layer that owns a RenderSurface is referred to as a renderSurfaceLayer. This layer may also be a child of a RenderSurface,

(1) Maybe we should be even more explicit to say, "If a layer owns a surface, it will usually appear multiple times in the iteration (<on which iterators exactly?>) -- once representing the target surface, once representing the contributing surface, and once representing the layer itself contributing to its own surface." -- or something like that

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:41
> +// - The currentRenderSurface owns a RenderSurface, which in turn has a list of child layers. The m_currentChildLayer is a position in this child list. But can also be -1 to

can we make (unsigned)-1 a constant?  I was thinking "kLayerRepresentsTargetSurface"...  If that's OK, I guess the comment here should also be updated.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:52
> +    inline LayerType* currentLayer() const { return m_currentChildLayer == (unsigned)-1 ? currentRenderSurfaceLayer() : currentRenderSurfaceChildren()[m_currentChildLayer].get(); }

Can we use your helper function instead of the explicit == conditional?  -->  if currentLayerIsRenderSurfaceLayer() or whatever the new name becomes

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:53
> +    inline LayerType* currentRenderSurfaceLayer() const { return (*m_renderSurfaceLayerList)[m_currentRenderSurfaceLayer].get(); }

If possible, maybe we should make this protected/private, and "friend" the iterators, or some other way.  In my imagination, the public interface for this struct should only be "currentLayer(),  representsTargetSurface(), representsContributingSurface()".  everything else should be protected, i think.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:91
> +    bool isRenderSurfaceLayer() { return m_position.currentLayerIsRenderSurfaceLayer(); }

As per our discussion offline, if I understand the code correctly, we might want to rename this as "hasTargetRenderSurface" ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:93
> +    bool isPaintingLayer() { return !isRenderSurfaceLayer() && !m_position.currentLayerHasRenderSurface(); }

As per our discussion offline, we probably want to rename this to "hasContributingRenderSurface" ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:126
> +            it.m_currentRenderSurfaceLayer = -1;

Should this be (unsigned)-1 ?  I think we should create a const unsigned int kLayerRepresentsTargetSurface for all (unsigned)-1.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:165
> +                it.m_currentChildLayer = -1;

Should this be (unsigned)-1 ?  I think we should create a const unsigned int kLayerRepresentsTargetSurface for all (unsigned)-1.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:217
> +                while (it.m_currentChildLayer == (unsigned)-1) {

Can we use your helper function instead of the explicit == conditional?  -->  if currentLayerIsRenderSurfaceLayer() or whatever the new name becomes

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:236
> +            if (it.m_currentChildLayer == (unsigned)-1)

Can we use your helper function instead of the explicit == conditional?  -->  if currentLayerIsRenderSurfaceLayer() or whatever the new name becomes

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:385
> +    typedef CCLayerIterator<LayerChromium, RenderSurfaceChromium, CCLayerIteratorActions::FrontToBack> CCLayerIteratorType;

I think every time we choose a particular ordering for the iterator, maybe we should add a comment justifying why that ordering is necessary.  As I understand, here front-to-back is necessary for culling purposes?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:406
> +    typedef CCLayerIterator<LayerChromium, RenderSurfaceChromium, CCLayerIteratorActions::DependencyOrdered> CCLayerIteratorType;

And here, a comment explaining that any ordering will probably (?) work correctly here, and it could be changed if there is a strong reason to use a different ordering.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:475
> +    typedef CCLayerIterator<LayerType, RenderSurfaceType, CCLayerIteratorActions::DependencyOrdered> CCLayerIteratorType;

Same here, a comment about which ordering is appropriate... is there is any particular reason to walk this in a particular order?

[Shawn Singh]

By the way, I think we should probably add unit tests that creates particular small renderSurfaceLayerLists and layerLists for each surface, and test that the iterator works as expected.   I apologize in advance for the extra time it will take =)

Especially since we don't yet use back-to-front, it feels risky without testing to keep that code around.

[Dana Jansens]

Comment on attachment 119025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119025&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:33
>> +// These classes provide means to iterate over the RenderSurface-Layer tree.
> 
> As per our offline discussion - would it be OK if we used the following terminology?  Shouldnt require too many changes in code:
> 
> (1) "target surface" --> the surface that layers/surface contribute to
> (2) "contributing surface" --> a surface that is hosted by a layer, which contributes to a target surface.
> (3) "renderSurfaceLayer" seems too ambiguous, and we need a distinction between #2 and #3
> (4) Instead of "hosting", could we say "representing"?  This feels more correct to me because a layer technically can have all 3 roles at different times (layer represents a contributing surface, layer represents a target surface, layer represents itself as a layer).  we can't say "layer hosts itself", so we lose some clarity in explanation if we use that word.

comment updated a lot for greater clarity. Using terms "layer representing X" where X is 1) target surface 2) contributing surface or 3) itself.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:35

> 
> (1) Maybe we should be even more explicit to say, "If a layer owns a surface, it will usually appear multiple times in the iteration (<on which iterators exactly?>) -- once representing the target surface, once representing the contributing surface, and once representing the layer itself contributing to its own surface." -- or something like that

This was wrong actually, all the iterators currently do return all the same layers (in all their states), just in different orders.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:41
>> +// - The currentRenderSurface owns a RenderSurface, which in turn has a list of child layers. The m_currentChildLayer is a position in this child list. But can also be -1 to
> 
> can we make (unsigned)-1 a constant?  I was thinking "kLayerRepresentsTargetSurface"...  If that's OK, I guess the comment here should also be updated.

done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:52
>> +    inline LayerType* currentLayer() const { return m_currentChildLayer == (unsigned)-1 ? currentRenderSurfaceLayer() : currentRenderSurfaceChildren()[m_currentChildLayer].get(); }
> 
> Can we use your helper function instead of the explicit == conditional?  -->  if currentLayerIsRenderSurfaceLayer() or whatever the new name becomes

done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:53
>> +    inline LayerType* currentRenderSurfaceLayer() const { return (*m_renderSurfaceLayerList)[m_currentRenderSurfaceLayer].get(); }
> 
> If possible, maybe we should make this protected/private, and "friend" the iterators, or some other way.  In my imagination, the public interface for this struct should only be "currentLayer(),  representsTargetSurface(), representsContributingSurface()".  everything else should be protected, i think.

the struct is not public data, i've added comments and changed variable names to clarify this.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:91
>> +    bool isRenderSurfaceLayer() { return m_position.currentLayerIsRenderSurfaceLayer(); }
> 
> As per our discussion offline, if I understand the code correctly, we might want to rename this as "hasTargetRenderSurface" ?

renamed to representsTargetRenderSurface()

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:93
>> +    bool isPaintingLayer() { return !isRenderSurfaceLayer() && !m_position.currentLayerHasRenderSurface(); }
> 
> As per our discussion offline, we probably want to rename this to "hasContributingRenderSurface" ?

renamed to representsItself()

also added representsContributingRenderSurface()

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:126
>> +            it.m_currentRenderSurfaceLayer = -1;
> 
> Should this be (unsigned)-1 ?  I think we should create a const unsigned int kLayerRepresentsTargetSurface for all (unsigned)-1.

no, the unsigned var is m_currentLayer  (m_currentChildLayer in this patch version).

This -1 indicates a position outside the bounds of the layer tree. I've clarified this better in the opening comments also and added a second constant.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:165
>> +                it.m_currentChildLayer = -1;
> 
> Should this be (unsigned)-1 ?  I think we should create a const unsigned int kLayerRepresentsTargetSurface for all (unsigned)-1.

yes.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:217
>> +                while (it.m_currentChildLayer == (unsigned)-1) {
> 
> Can we use your helper function instead of the explicit == conditional?  -->  if currentLayerIsRenderSurfaceLayer() or whatever the new name becomes

done

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:236
>> +            if (it.m_currentChildLayer == (unsigned)-1)
> 
> Can we use your helper function instead of the explicit == conditional?  -->  if currentLayerIsRenderSurfaceLayer() or whatever the new name becomes

done

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:385
>> +    typedef CCLayerIterator<LayerChromium, RenderSurfaceChromium, CCLayerIteratorActions::FrontToBack> CCLayerIteratorType;
> 
> I think every time we choose a particular ordering for the iterator, maybe we should add a comment justifying why that ordering is necessary.  As I understand, here front-to-back is necessary for culling purposes?

yes, done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:406
>> +    typedef CCLayerIterator<LayerChromium, RenderSurfaceChromium, CCLayerIteratorActions::DependencyOrdered> CCLayerIteratorType;
> 
> And here, a comment explaining that any ordering will probably (?) work correctly here, and it could be changed if there is a strong reason to use a different ordering.

done

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:475
>> +    typedef CCLayerIterator<LayerType, RenderSurfaceType, CCLayerIteratorActions::DependencyOrdered> CCLayerIteratorType;
> 
> Same here, a comment about which ordering is appropriate... is there is any particular reason to walk this in a particular order?

done. this is the cheapest ordering.

[Dana Jansens]

Created attachment 120188 [details]
Patch

comments addressed in this patch. will make a unit test.

@jamesr if you'd like i can make some other places make use of the iterators, though it might be better suited to other CLs.

[Dana Jansens]

Created attachment 120207 [details]
Patch

unit tests included.

[James Robinson]

(In reply to comment #10)
> Created an attachment (id=120188) [details]
> Patch
> 
> comments addressed in this patch. will make a unit test.
> 
> @jamesr if you'd like i can make some other places make use of the iterators, though it might be better suited to other CLs.

I think sticking to only these users in the initial CL is a good approach.

[Shawn Singh]

Comment on attachment 120207 [details]
Patch


This is great. I like how much cleaner the loops are, using the iterator.

One major point, and 3 minor nits.

1 Major point: I think we probably need put this patch on hold before trying to land it, because it will be plainly incorrect to iterate over LayerChromiums Front-to-Back before we add support for sorting layers on the LayerChromium side.  (Currently preserves3d sorting is only done on the CCLayerImpl side).  However, this might open a can of worms, because we may need to templatize all the layerSorter code.  I think we should ask the rest of the compositor team about this before continuing this patch.


3 minor nits: inline below

View in context: https://bugs.webkit.org/attachment.cgi?id=120207&action=review

> Source/WebCore/ChangeLog:8
> +        Iterators come with three orderings. DependencyOrdered (this is a simple linear walk over the tree) and is not

I think this second sentence came out a little unclear.  Maybe just remove the "and" ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:37
> +// A RenderSurface $R$ may be referred to in one of two difference contexts. One RenderSurface is "current" at any time, for

"difference" --> "different"

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:53
> +// Most iterators will return a layer representing the target surface before returning layers representing themselves as children

"Most iterators" --> "Dependency ordered and BackToFront interators"... if that's correct?

[James Robinson]

I don't think the preserve3d sorting is relevant for the main-thread users of FrontToBack sorting, since within a preserve3d subtree you probably won't be doing any occlusion/culling.  It is definitely something to point out very clearly.

[Shawn Singh]

(In reply to comment #14)
> I don't think the preserve3d sorting is relevant for the main-thread users of FrontToBack sorting, since within a preserve3d subtree you probably won't be doing any occlusion/culling.  It is definitely something to point out very clearly.

OK good point.  So scratch what I said then =)

[James Robinson]

Comment on attachment 120207 [details]
Patch

Some questions:

Do you expect that all iterator users will be locally creating an iterator and then using it for one loop? If so, do we really need arbitrary iterators or can we get away with something a little lighter (like a visitor solution that uses the stack for state instead of heap-allocating indices into a vector)?

Can we move some of that code out of the header? The header is very large and hard to read since they intermingle interface and implementation. There are great comments about how the things are implemented but not much about how you actually use them.

paintLayerContents() and updateCompositorResources() both have to deal with mask and replica layers by hand. Could/should that be a function of the iterator to know how to go through those layers? Not all iterations will need to touch mask/replica layers, but many will. Maybe it could be an option on the iterator?

since we rely on other bits of code building the render surfaces and sorting the layers, how big is the performance delta between the dependency and front-to-back/back-to-front iterators? it looks like the main overhead is keeping the history lists, but otherwise they should do the same work since they all iterate through the same set of layers visiting each layer once. could we simplify down to just having the front-to-back and back-to-front iterators? is the efficiency gain big enough to justify having another codepath to maintain?

on top of that, can we just have one bidirectional iterator type, or perhaps an iterator where you specify the direction at construction time or something to avoid having the loop logic in there twice? perhaps the iterations are too disimilar to fold together, i haven't fully groked that bit yet. it would be a really nice simplification if we could cut it down a bit.

[Adrienne Walker]

(In reply to comment #16)

> paintLayerContents() and updateCompositorResources() both have to deal with mask and replica layers by hand. Could/should that be a function of the iterator to know how to go through those layers? Not all iterations will need to touch mask/replica layers, but many will. Maybe it could be an option on the iterator?

I'd just punt on this.

My hope is that if we refactor render surfaces into their own layer type, this new layer type can encapsulate mask and replica layers.  For example, the hypothetical RenderSurfaceLayerChromium::paintContentsIfDirty could handle painting these mask and replicas, and no iterator would need to know that they exist.

[Dana Jansens]

(In reply to comment #16)
> (From update of attachment 120207 [details])
> Some questions:
> 
> Do you expect that all iterator users will be locally creating an iterator and then using it for one loop?

Yes that was how I intended it.

> If so, do we really need arbitrary iterators or can we get away with something a little lighter (like a visitor solution that uses the stack for state instead of heap-allocating indices into a vector)?

I think I could stick 2 ints in the RenderSurface classes. One to remember position in their child lists (m_currentLayerHistory). And one to remember the previous target RS (m_targetRenderSurfaceLayerHistory).

1) This would prevent you from doing an iteration during another iteration.. I presume this would be okay?
2) This code is going to be a little less readable I think. I can prototype it and you can be the judge.

Another option would be to specify a stack size to those vectors (something like 20?), and at least it will use only the stack while it stays below that size.

> Can we move some of that code out of the header? The header is very large and hard to read since they intermingle interface and implementation. There are great comments about how the things are implemented but not much about how you actually use them.

Yes I'll split it out.

> paintLayerContents() and updateCompositorResources() both have to deal with mask and replica layers by hand. Could/should that be a function of the iterator to know how to go through those layers? Not all iterations will need to touch mask/replica layers, but many will. Maybe it could be an option on the iterator?

Punted to enne punting.

> since we rely on other bits of code building the render surfaces and sorting the layers, how big is the performance delta between the dependency and front-to-back/back-to-front iterators? it looks like the main overhead is keeping the history lists, but otherwise they should do the same work since they all iterate through the same set of layers visiting each layer once. could we simplify down to just having the front-to-back and back-to-front iterators? is the efficiency gain big enough to justify having another codepath to maintain?

I think that we could drop DepOrdered, yes. I don't think the perf diffence of keeping a vector stack of ints is a big deal.

BackToFront preserves the invariant that the layer representing the current target surface is visited before layers that contribute to that surface. The difference is that the target surface can change and return again, whereas in DepOrdered you deal with all things in a given target surface in sequence. But at least in the current loops I changed this is not a concern, and it's something that could be dealt with without needing the DepOrdered iterator.

> on top of that, can we just have one bidirectional iterator type, or perhaps an iterator where you specify the direction at construction time or something to avoid having the loop logic in there twice? perhaps the iterations are too disimilar to fold together, i haven't fully groked that bit yet. it would be a really nice simplification if we could cut it down a bit.

Lemme think about this one more after addressing the above stuff.. I think a bi-directional iterator would make the code much harder to read, as it would need to track more state (either in the iterator class or visitor-like on the surfaces/layers). Picking a direction at construction time would look more or less like it does now, I think? The loop logic is pretty different I don't see a lot of code in common between the two.

[Dana Jansens]

(In reply to comment #18)
> (In reply to comment #16)
> > could we simplify down to just having the front-to-back and back-to-front iterators? is the efficiency gain big enough to justify having another codepath to maintain?
> I think that we could drop DepOrdered, yes.

One nice thing about the DepOrdered is that it is very simple, which makes it valuable for the unit test to make sure that the other iterators are actually hitting everything. I think otherwise we end up duplicating the loop logic into the unit test which is less desirable maybe?

[Dana Jansens]

Created attachment 120478 [details]
Patch

(In reply to comment #18)
> (In reply to comment #16)
> > on top of that, can we just have one bidirectional iterator type, or perhaps an iterator where you specify the direction at construction time or something to avoid having the loop logic in there twice?

A bidirectional implementation would need to be stateless other than what is stored in CCLayerIteratorPosition. For times when history is used currently, this means walking from the start of the tree to your current position to build up that history (and then toss it away afterward). I think that this walk would look exactly like the code we have now (one forward one backward) with a while(){ } around it. Personally I think what we have now is nicer than that.. unless someone has another idea how to do it.

While we were on the topic of complexity. I should point out that FrontToBack does have an extra cost (vs BackToFront or DepOrdered), with a for-loop to find the next target RenderSurface's owning layer. BackToFront was doing this also, but I replaced the for-loop with an extra int state variable in the iterator, m_highestTargetRenderSurfaceLayer.

I've pulled the CCLayerIteratorActions over to a .cpp file, and gotten rid of the Vectors. It was actually very easy and I like the result a lot!

[Dana Jansens]

Created attachment 120480 [details]
Patch

Sorry, missed addressing one comment from Shawn.

[Adrienne Walker]

Comment on attachment 120480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120480&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:169
> +    // Walks layers sorted by z-order from back to front.

nit: comment is wrong.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:439
> +    // Use FrontToBack to allow for testing occlusion and performing culling during the tree walk.
> +    typedef CCLayerIterator<LayerChromium, RenderSurfaceChromium, CCLayerIteratorActions::FrontToBack> CCLayerIteratorType;

I realize that this behavior is unchanged, but some future patch is going to need to add in a different iteration order for texture reservation so that we don't drop the root layer or the scrollbars because we've run out of texture memory.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:457
> +    // Use BackToFront to hit target surfaces before their children.
> +    typedef CCLayerIterator<LayerChromium, RenderSurfaceChromium, CCLayerIteratorActions::BackToFront> CCLayerIteratorType;

Isn't updateCompositorResources order-independent?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:483
> +
> +    CCLayerIteratorType end = CCLayerIteratorType::end(&renderSurfaceLayerList);
> +    for (CCLayerIteratorType it = CCLayerIteratorType::begin(&renderSurfaceLayerList); it != end; ++it) {
> +        if (!it.representsTargetRenderSurface()) {
> +            IntRect visibleLayerRect = CCLayerTreeHostCommon::calculateVisibleLayerRect<LayerType>(*it);
> +            it->setVisibleLayerRect(visibleLayerRect);
>          }

When calculating visible layer rects, don't you need to process children before surfaces? Also, with that added conditional statement, where does the visible rect for layers that are surfaces get set?

Maybe I'm just misunderstanding something.

[Adrienne Walker]

Comment on attachment 120480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120480&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:483
>>          }
> 
> When calculating visible layer rects, don't you need to process children before surfaces? Also, with that added conditional statement, where does the visible rect for layers that are surfaces get set?
> 
> Maybe I'm just misunderstanding something.

Ah, my mistake.  I'm thinking of content rects, not visible layer rects.  Content rects require children to be processed before surfaces.  But, once everything has a valid content rect, then the visible layer rect can be done in any order.

[Dana Jansens]

Comment on attachment 120480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120480&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:169
>> +    // Walks layers sorted by z-order from back to front.
> 
> nit: comment is wrong.

thanks

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:439
>> +    typedef CCLayerIterator<LayerChromium, RenderSurfaceChromium, CCLayerIteratorActions::FrontToBack> CCLayerIteratorType;
> 
> I realize that this behavior is unchanged, but some future patch is going to need to add in a different iteration order for texture reservation so that we don't drop the root layer or the scrollbars because we've run out of texture memory.

Yeh. Punting this to later as discussed.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:457
>> +    typedef CCLayerIterator<LayerChromium, RenderSurfaceChromium, CCLayerIteratorActions::BackToFront> CCLayerIteratorType;
> 
> Isn't updateCompositorResources order-independent?

Yeh, comment updated.

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:483
>>>          }
>> 
>> When calculating visible layer rects, don't you need to process children before surfaces? Also, with that added conditional statement, where does the visible rect for layers that are surfaces get set?
>> 
>> Maybe I'm just misunderstanding something.
> 
> Ah, my mistake.  I'm thinking of content rects, not visible layer rects.  Content rects require children to be processed before surfaces.  But, once everything has a valid content rect, then the visible layer rect can be done in any order.

Fixed the comment to reflect this also.

[Dana Jansens]

Created attachment 121123 [details]
Patch

nits/comments addressed.

[James Robinson]

Comment on attachment 121123 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121123&action=review

Looking close! I'm worried about the IteratorActionType memory, there's a "new" here with no "delete" and no smart pointer type. Left some other comments as well inline.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:37
> +// A RenderSurface $R$ may be referred to in one of two different contexts. One RenderSurface is "current" at any time, for

i'm not sure what the $x$ naming convention means, but I find it makes this paragraph hard to read - the $'s dominate the letter on the inside. What about just "A RenderSurface R may be..." ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:62
> +// Create a stepping iterator and end iterator by calling CCLayerIterator::begin() and CCLayerIterator::end() and passing in the
> +// list of layers owning target RenderSurfaces. Step through the tree by incrementing the stepping iterator while it is != to
> +// the end iterator. At each step the iterator knows what the layer is representing, and you can query the iterator to decide
> +// what actions to perform with the layer given what it represents.

some short example code would be really nice here showing how to pick an iteration direction and how to actually get at the layers. maybe even put the example above the detailed explanation

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:80
> +struct CCLayerIteratorPosition {

this guy takes a lot of lines of code and puts the start of the actual CCLayerIterator interface down below line 100. can it get its own header?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:102
> +    static const int kInvalidTargetRenderSurfaceLayer = -1;
> +    // This must be -1 since the iterator action code assumes that this value can be
> +    // reached by subtracting one from the position of the first layer in the current
> +    // target surface's child layer list, which is 0.
> +    static const unsigned kLayerRepresentingTargetRenderSurface = (unsigned)-1;

we don't normally use the "k" prefix for constants in WebKit

what do you mean by (unsigned)-1 ? unsigned types + negative numbers normally means weird bugs. why the c-style cast?

do we really need a different set of these constants for every instantiation of this template, or can these be hoisted out?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:106
> +    const Vector<RefPtr<LayerType> >* m_renderSurfaceLayerList;
> +    int m_targetRenderSurfaceLayer;
> +    unsigned m_currentLayer;

we don't use the m_ prefix (or any prefix) for public members of a struct

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:137
> +        , m_actions(new IteratorActionType())

who owns the memory for m_actions? where's the delete?

> Source/WebKit/chromium/tests/CCLayerIteratorTest.cpp:235
> +    root2->setOpacity(0.5f); // Force the layer to own a new surface.

here and elsewhere: in webkit, we don't use the "f" suffix in cases like this where the compiler can figure out what we mean. just "0.5"

[Dana Jansens]

Created attachment 121367 [details]
Patch

Comments addressed. Split CCLayerIteratorPosition into its own header.

[Dana Jansens]

(In reply to comment #26)
> (From update of attachment 121123 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121123&action=review
> 
> Looking close! I'm worried about the IteratorActionType memory, there's a "new" here with no "delete" and no smart pointer type. Left some other comments as well inline.

> i'm not sure what the $x$ naming convention means, but I find it makes this paragraph hard to read - the $'s dominate the letter on the inside. What about just "A RenderSurface R may be..." ?

done. (it was latex notation.)

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:62
> some short example code would be really nice here showing how to pick an iteration direction and how to actually get at the layers. maybe even put the example above the detailed explanation

done.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:80
> > +struct CCLayerIteratorPosition {
> 
> this guy takes a lot of lines of code and puts the start of the actual CCLayerIterator interface down below line 100. can it get its own header?

done.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:102
> > +    static const int kInvalidTargetRenderSurfaceLayer = -1;
> > +    // This must be -1 since the iterator action code assumes that this value can be
> > +    // reached by subtracting one from the position of the first layer in the current
> > +    // target surface's child layer list, which is 0.
> > +    static const unsigned kLayerRepresentingTargetRenderSurface = (unsigned)-1;
> 
> we don't normally use the "k" prefix for constants in WebKit

done.

> what do you mean by (unsigned)-1 ? unsigned types + negative numbers normally means weird bugs. why the c-style cast?

changed it to a signed int like the other variable.

> do we really need a different set of these constants for every instantiation of this template, or can these be hoisted out?

yes! pulled them out to a CCLayerIteratorPositionValue struct.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:106
> we don't use the m_ prefix (or any prefix) for public members of a struct

done.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:137
> who owns the memory for m_actions? where's the delete?

Thanks, made it OwnPtr.

> > Source/WebKit/chromium/tests/CCLayerIteratorTest.cpp:235
> here and elsewhere: in webkit, we don't use the "f" suffix in cases like this where the compiler can figure out what we mean. just "0.5"

done, thanks.

[James Robinson]

Comment on attachment 121367 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121367&action=review

Awesome, R=me!

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.cpp:150
> +// Declare each of the above functions for LayerChromium and CCLayerImpl classes so that they are linked.
> +template void CCLayerIteratorActions::BackToFront::begin(CCLayerIteratorPosition<LayerChromium, RenderSurfaceChromium>&);

can you confirm that this compiles fine in visual studio before landing (it's not too advanced, but i've had odd problems with template instantiation before with msvs)? tryjobs should do the trick

> Source/WebKit/chromium/tests/CCLayerIteratorTest.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2012, please

[Dana Jansens]

Created attachment 121677 [details]
Patch

Some compile fixed for mac/windows. The templates were fine but the distinction between struct/class is more important on other platforms. This compiled on both mac and windows try bots.

[Dana Jansens]

Created attachment 121732 [details]
Patch

Dur, missed the 2012 change, sorry.

[James Robinson]

Comment on attachment 121732 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 121732 [details]
Patch

Clearing flags on attachment: 121732

Committed r104507: <http://trac.webkit.org/changeset/104507>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dana Jansens]

Created attachment 121897 [details]
Patch

We were adding a new assert which failed in the unit test. The assert used to be there but was removed at some point, and my rebasing missed it.

I was using component build which excludes the CCLayerTreeHostTest.cpp among others.. will have to stop using it.

[Dana Jansens]

Reopening

[James Robinson]

Comment on attachment 121897 [details]
Patch

Here we go again!

We really should make that test work in the component build, it's bitten me before as well.

[WebKit Review Bot]

Comment on attachment 121897 [details]
Patch

Clearing flags on attachment: 121897

Committed r104626: <http://trac.webkit.org/changeset/104626>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.