Bug 75864 – [chromium] Push CCLayerIteratorPosition struct into CCLayerIterator class.

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Push CCLayerIteratorPosition struct into CCLayerIterator class.

[Dana Jansens]

Created attachment 121679 [details]
Patch

This removes the CCLayerIteratorPosition struct, moving the data fields into the CCLayerIterator class.
I think it's a little less complicated this way and drops ~50 lines of code.

[Dana Jansens]

Created attachment 121752 [details]
Patch

This appears to compile on the mac and windows bots. The windows one keeps dying/timing out during compile (>_<) but it doesn't error on CCLayerIterator.cpp or webcore_platform at all.

[David Reveman]

Comment on attachment 121752 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:159
>  // Orderings for iterating over the RenderSurface-Layer tree.

Rather than instantiating CCLayerIteratorActions, could we implement this by subclassing CCLayerIteratorActions? Giving us CCBackToFrontLayerIterator and CCFrontToBackLayerIterator. That way we can also add subclasses that hide the use of templates (e.g. CCBackToFrontLayerChromiumIterator) which I prefer over having to declare all template configurations that we use in the .cpp file.

[Shawn Singh]

(In reply to comment #3)
> (From update of attachment 121752 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121752&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:159
> >  // Orderings for iterating over the RenderSurface-Layer tree.
> 
> Rather than instantiating CCLayerIteratorActions, could we implement this by subclassing CCLayerIteratorActions? Giving us CCBackToFrontLayerIterator and CCFrontToBackLayerIterator. That way we can also add subclasses that hide the use of templates (e.g. CCBackToFrontLayerChromiumIterator) which I prefer over having to declare all template configurations that we use in the .cpp file.

Personally I disagree... I see two disadvantages with this, which may be important:
 1. The explicit and clear distinction between back-to-front and front-to-back becomes visually hidden by being prefixed/suffixed.  This can make it more error-prone...
 2. There would be an explosion of different data types that the user of this code needs to be aware of - as it is now, the user's perspective is simpler, even if the internals are slightly more awkward, but personally I think its readable enough that the simplicity is worth it.

What do you think?  Or maybe I misunderstood what you're suggesting?

[Dana Jansens]

My first version used subclassing, but with templates we got the same general code without virtual method overhead. It also didn't require a super class and I think is fewer lines of code. At least that was my intuition on it.

[David Reveman]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 121752 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=121752&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerIterator.h:159
> > >  // Orderings for iterating over the RenderSurface-Layer tree.
> > 
> > Rather than instantiating CCLayerIteratorActions, could we implement this by subclassing CCLayerIteratorActions? Giving us CCBackToFrontLayerIterator and CCFrontToBackLayerIterator. That way we can also add subclasses that hide the use of templates (e.g. CCBackToFrontLayerChromiumIterator) which I prefer over having to declare all template configurations that we use in the .cpp file.
> 
> Personally I disagree... I see two disadvantages with this, which may be important:
>  1. The explicit and clear distinction between back-to-front and front-to-back becomes visually hidden by being prefixed/suffixed.  This can make it more error-prone...

I'm not sure I understand what you mean by this. It would be CCBackToFrontLayerIterator instead of CCLayerIterator<CCLayerIteratorActions::BackToFront> and I'm not sure I understand how that can be more error prone. Do you have an example?

>  2. There would be an explosion of different data types that the user of this code needs to be aware of - as it is now, the user's perspective is simpler, even if the internals are slightly more awkward, but personally I think its readable enough that the simplicity is worth it.

Isn't the alternative just that the user needs to be aware of template types instead data types? This is the current usage:

typedef CCLayerIterator<LayerChromium, RenderSurfaceChromium, CCLayerIteratorActions::FrontToBack> CCLayerIteratorType;                                 
                                                                              
CCLayerIteratorType end = CCLayerIteratorType::end(&renderSurfaceLayerList);                                                                            
for (CCLayerIteratorType it = CCLayerIteratorType::begin(&renderSurfaceLayerList); it != end; ++it) { }

I think the need to use a typedef alone is a good indication that this isn't ideal. This is my suggested usage:

CCBackToFrontLayerChromiumIterator end = CCBackToFrontLayerChromiumIterator::end(&renderSurfaceLayerList);                                                                            
for (CCBackToFrontLayerChromiumIterator it = CCBackToFrontLayerChromiumIterator::begin(&renderSurfaceLayerList); it != end; ++it) { }

> 
> What do you think?  Or maybe I misunderstood what you're suggesting?

I generally dislike templates in an API unless necessary. Whenever there's limited set of template configurations used and the implementation is in a .cpp file, I prefer hiding the template usage in helper functions/classes instead of declaring each configuration used in the .cpp file.

In this specific case, I also dislike that ever iterator requires a heap allocation.

[Dana Jansens]

Things I like about the template version:
- no branching (if type == fronttoback)
- no virtual functions (class named directly in template)
- single CCLayerIterator class
- compile error if you try compare a FrontToBack == BackToFront iterator
- easy to read

I tried making subclasses for the various Layer types, or for the various IteratorActions, or both.

It feels like it adds a lot of code to CCLayerIterator.h, making the interface harder to read, but the line length of 

CCLayerIteratorLayerChromium<CCLayerIteratorActions::FrontToBack> end = CCLayerIteratorLayerChromium<CCLayerIteratorActions::FrontToBack>::end(&renderSurfaceLayerList); 

and

CCLayerIteratorFrontToBack<LayerType, RenderSurfaceType> end = CCLayerIteratorFrontToBack<LayerType, RenderSurfaceType>::end(&renderSurfaceLayerList); 

are all still long enough that I'd probably want to typedef it anyway for clarity. So I opt for just keeping it as templates as is.

[Dana Jansens]

Created attachment 121923 [details]
Patch

Got rid of the heap allocation.

[James Robinson]

Comment on attachment 121923 [details]
Patch

I think I'm missing something obvious but I can't figure out the lifecycle for m_renderSurfaceLayerList. Who's supposed to allocate memory for this and who's supposed to delete it?

[Shawn Singh]

(In reply to comment #9)
> (From update of attachment 121923 [details])
> I think I'm missing something obvious but I can't figure out the lifecycle for m_renderSurfaceLayerList. Who's supposed to allocate memory for this and who's supposed to delete it?

Unless something has changed, here's what I understand:

(1) in CCLayerTreeHostImpl::calculateRenderPasses()

(2) in CCLayerTreeHost::updateLayers() --> but here, it appears that those layerlists are owned by RenderSurfaceChromiums, so the renderSurfaceLayerList is actually owned by the root RenderSurfacechromium?

[Shawn Singh]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 121923 [details] [details])
> > I think I'm missing something obvious but I can't figure out the lifecycle for m_renderSurfaceLayerList. Who's supposed to allocate memory for this and who's supposed to delete it?
> 
> Unless something has changed, here's what I understand:
> 
> (1) in CCLayerTreeHostImpl::calculateRenderPasses()
> 
> (2) in CCLayerTreeHost::updateLayers() --> but here, it appears that those layerlists are owned by RenderSurfaceChromiums, so the renderSurfaceLayerList is actually owned by the root RenderSurfacechromium?

Sorry, for (1) I meant that its a local variable within that function.

[Dana Jansens]

(In reply to comment #9)
> I think I'm missing something obvious but I can't figure out the lifecycle for m_renderSurfaceLayerList. Who's supposed to allocate memory for this and who's supposed to delete it?

From the perspective of the iterators, it is a pointer passed to begin()/end() and something that needs to live at least as long as the iterators do.

It's assumed iterators are short-lived and this won't be a problem. But if it was the Vector* could be a RefPtr<Vector> instead (making it a RefPtr<Vector<RefPtr<LayerType> > >).

[James Robinson]

Comment on attachment 121923 [details]
Patch

R=me. Looks correct as far as I can tell and if not hopefully we have enough test coverage now to let us know.

(I actually thought for some reason this had landed some time back, but I guess not).

[Dana Jansens]

Comment on attachment 121923 [details]
Patch

Yay, thanks! I kept thinking about bumping this. :)

[WebKit Review Bot]

Comment on attachment 121923 [details]
Patch

Rejecting attachment 121923 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ium', '--update-chromium']" exit_code: 2
enderSurfaceLayer() const [with LayerType = WebCore::LayerChromium, RenderSurfaceType = WebCore::RenderSurfaceChromium, IteratorActionType = WebCore::CCLayerIteratorActions::FrontToBack]' is private
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:563: error: within this context
make: *** [out/Debug/obj.target/webcore_platform/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/11558373

[W. James MacLean]

Created attachment 128234 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 128234 [details]
Patch for landing

Clearing flags on attachment: 128234

Committed r108518: <http://trac.webkit.org/changeset/108518>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adrienne Walker]

Reverted r108518 for reason:

Breaks

Committed r108532: <http://trac.webkit.org/changeset/108532>

[Adrienne Walker]

(In reply to comment #19)
> Reverted r108518 for reason:
> 
> Breaks
> 
> Committed r108532: <http://trac.webkit.org/changeset/108532>

Er, that should have been breaks surfaceOcclusionWithOverlappingSiblingSurfaces in webkit_unit_tests.

[WebKit Review Bot]

Comment on attachment 128234 [details]
Patch for landing

Clearing flags on attachment: 128234

Committed r108678: <http://trac.webkit.org/changeset/108678>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.