Bug 75864 - [chromium] Push CCLayerIteratorPosition struct into CCLayerIterator class.
Summary: [chromium] Push CCLayerIteratorPosition struct into CCLayerIterator class.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on: 74203 79275
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-09 10:06 PST by Dana Jansens
Modified: 2012-02-23 15:03 PST (History)
9 users (show)

See Also:


Attachments
Patch (28.01 KB, patch)
2012-01-09 10:09 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (30.75 KB, patch)
2012-01-09 16:22 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (30.71 KB, patch)
2012-01-10 15:50 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch for landing (30.68 KB, patch)
2012-02-22 09:56 PST, W. James MacLean
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-01-09 10:06:08 PST
[chromium] Push CCLayerIteratorPosition struct into CCLayerIterator class.
Comment 1 Dana Jansens 2012-01-09 10:09:28 PST
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.
Comment 2 Dana Jansens 2012-01-09 16:22:16 PST
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 3 David Reveman 2012-01-10 10:22:18 PST
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.
Comment 4 Shawn Singh 2012-01-10 10:39:09 PST
(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?
Comment 5 Dana Jansens 2012-01-10 10:59:30 PST
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.
Comment 6 David Reveman 2012-01-10 11:32:38 PST
(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.
Comment 7 Dana Jansens 2012-01-10 15:45:32 PST
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.
Comment 8 Dana Jansens 2012-01-10 15:50:53 PST
Created attachment 121923 [details]
Patch

Got rid of the heap allocation.
Comment 9 James Robinson 2012-01-18 15:15:19 PST
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?
Comment 10 Shawn Singh 2012-01-18 15:20:46 PST
(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?
Comment 11 Shawn Singh 2012-01-18 15:21:20 PST
(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.
Comment 12 Dana Jansens 2012-01-18 15:54:37 PST
(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 13 James Robinson 2012-02-21 20:52:07 PST
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 14 Dana Jansens 2012-02-21 21:07:28 PST
Comment on attachment 121923 [details]
Patch

Yay, thanks! I kept thinking about bumping this. :)
Comment 15 WebKit Review Bot 2012-02-21 22:16:16 PST
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
Comment 16 W. James MacLean 2012-02-22 09:56:33 PST
Created attachment 128234 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-02-22 10:35:35 PST
Comment on attachment 128234 [details]
Patch for landing

Clearing flags on attachment: 128234

Committed r108518: <http://trac.webkit.org/changeset/108518>
Comment 18 WebKit Review Bot 2012-02-22 10:35:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Adrienne Walker 2012-02-22 12:02:37 PST
Reverted r108518 for reason:

Breaks

Committed r108532: <http://trac.webkit.org/changeset/108532>
Comment 20 Adrienne Walker 2012-02-22 12:03:07 PST
(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 21 WebKit Review Bot 2012-02-23 15:03:14 PST
Comment on attachment 128234 [details]
Patch for landing

Clearing flags on attachment: 128234

Committed r108678: <http://trac.webkit.org/changeset/108678>
Comment 22 WebKit Review Bot 2012-02-23 15:03:20 PST
All reviewed patches have been landed.  Closing bug.