Bug 82114 – [chromium] Simplify and fix CCLayerSorter

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Vangelis Kokkevis]

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.

[Vangelis Kokkevis]

Created attachment 133660 [details]
Patch

[Kenneth Russell]

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.

[Vangelis Kokkevis]

Committed r112182: <http://trac.webkit.org/changeset/112182>

[Vangelis Kokkevis]

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