Bug 75864

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.

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.

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.

(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?

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.

(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.

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.

Created attachment 121923 [details] Patch Got rid of the heap allocation.

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?

(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?

(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.

(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> > >).

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).

Comment on attachment 121923 [details] Patch Yay, thanks! I kept thinking about bumping this. :)

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

Created attachment 128234 [details] Patch for landing

Comment on attachment 128234 [details] Patch for landing Clearing flags on attachment: 128234 Committed r108518: <http://trac.webkit.org/changeset/108518>

All reviewed patches have been landed. Closing bug.

Reverted r108518 for reason: Breaks Committed r108532: <http://trac.webkit.org/changeset/108532>

(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.

Comment on attachment 128234 [details] Patch for landing Clearing flags on attachment: 128234 Committed r108678: <http://trac.webkit.org/changeset/108678>

All reviewed patches have been landed. Closing bug.