Bug 82114 - [chromium] Simplify and fix CCLayerSorter
Summary: [chromium] Simplify and fix CCLayerSorter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vangelis Kokkevis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-23 19:06 PDT by Vangelis Kokkevis
Modified: 2012-03-26 17:17 PDT (History)
6 users (show)

See Also:


Attachments
Patch (35.65 KB, patch)
2012-03-24 16:09 PDT, Vangelis Kokkevis
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.