Bug 82114

Summary: [chromium] Simplify and fix CCLayerSorter
Product: WebKit Reporter: Vangelis Kokkevis <vangelis>
Component: WebCore Misc.Assignee: Vangelis Kokkevis <vangelis>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, enne, jamesr, kbr, shawnsingh, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch kbr: review+

Description Vangelis Kokkevis 2012-03-23 19:06:49 PDT 
CCLayerSorter is unnecessarily complex and doesn't work quite correctly.  The code can be greatly simplified by taking advantage of some of the existing datastructures in WebCore.
Comment 1 Vangelis Kokkevis 2012-03-24 16:09:19 PDT 
Created attachment 133660 [details]
Patch
Comment 2 Kenneth Russell 2012-03-26 16:02:48 PDT 
Comment on attachment 133660 [details]
Patch

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

Looks good overall. A couple of minor comments. r=me

> Source/WebCore/platform/graphics/chromium/cc/CCLayerSorter.cpp:86
> +CCLayerSorter::ABCompareResult CCLayerSorter::checkOverlap(LayerShape* a, LayerShape* b, float zThreshold, float* weight)

I'm pretty sure that the preferred style in WebKit would be to use a reference rather than a pointer for the outgoing argument "weight".

> Source/WebCore/platform/graphics/chromium/cc/CCLayerSorter.cpp:242
> +    float zThreshold = m_zRange * 0.01;

Magic constant? Should this be defined somewhere above and named?

> Source/WebKit/chromium/tests/CCLayerSorterTest.cpp:37
> +TEST(CCLayerSorterTest, CheckOverlap)

These tests should be split up for finer grained reporting of results.
Comment 3 Vangelis Kokkevis 2012-03-26 17:16:43 PDT 
Committed r112182: <http://trac.webkit.org/changeset/112182>
Comment 4 Vangelis Kokkevis 2012-03-26 17:17:57 PDT 
(In reply to comment #2)
> (From update of attachment 133660 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133660&action=review
> 
> Looks good overall. A couple of minor comments. r=me
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerSorter.cpp:86
> > +CCLayerSorter::ABCompareResult CCLayerSorter::checkOverlap(LayerShape* a, LayerShape* b, float zThreshold, float* weight)
> 
> I'm pretty sure that the preferred style in WebKit would be to use a reference rather than a pointer for the outgoing argument "weight".

Done.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerSorter.cpp:242
> > +    float zThreshold = m_zRange * 0.01;
> 
> Magic constant? Should this be defined somewhere above and named?
>

Done.
 
> > Source/WebKit/chromium/tests/CCLayerSorterTest.cpp:37
> > +TEST(CCLayerSorterTest, CheckOverlap)
> 
> These tests should be split up for finer grained reporting of results.

Done.

Modified and landed.