Bug 68622 – Add unit test for CCLayerSorter

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Iain Merrick]

Add unit test for CCLayerSorter

[Iain Merrick]

Created attachment 108338 [details]
Patch

[WebKit Review Bot]

Attachment 108338 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/tests/CCLayerSorterTest.cpp:74:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/chromium/tests/CCLayerSorterTest.cpp:75:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/chromium/tests/CCLayerSorterTest.cpp:80:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/chromium/tests/CCLayerSorterTest.cpp:81:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/chromium/tests/CCLayerSorterTest.cpp:96:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/chromium/tests/CCLayerSorterTest.cpp:97:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/chromium/tests/CCLayerSorterTest.cpp:131:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/chromium/tests/CCLayerSorterTest.cpp:132:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/chromium/tests/CCLayerSorterTest.cpp:136:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/chromium/tests/CCLayerSorterTest.cpp:139:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/chromium/tests/CCLayerSorterTest.cpp:143:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/chromium/tests/CCLayerSorterTest.cpp:144:  Extra space after ( in function call  [whitespace/parens] [4]
Total errors found: 12 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Iain Merrick]

No tests for the topological sort yet, that can come next.

Another thing that might be worth doing in a future patch is unifying the edgeEdgeTest function with findIntersection (in FloatPoint.h). They're almost identical except that findIntersection projects the lines to infinity, but we require an intersection that's inside both line segments. But the only extra code we need for that is pointInColinearEdge.

[Iain Merrick]

Re style errors: I added spaces to make the constants readable. Happy to remove them if that error can be suppressed.

[Iain Merrick]

Created attachment 108657 [details]
Patch

[James Robinson]

Vangelis, since you wrote the sorter originally would you mind reviewing this patch?

[Vangelis Kokkevis]

Comment on attachment 108657 [details]
Patch

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

Looks good (other than test comment)!  Thanks for taking care of it.

> Source/WebKit/chromium/tests/CCLayerSorterTest.cpp:117
> +    //  : ----A----/-

Intersecting layers don't currently get sorted consistently.  Ideally this test should use non-intersecting layers which could be achieved by spreading A and B a bit more on the X axis.

[Iain Merrick]

Sure, I'll make that change.

It looked to me as if intersecting layers actually *are* sorted consistently, at least in certain cases, which is why I tried it out in the test. I guess intersecting layers are basically cycles, so it depends whether we want to set any requirements on how cycles are broken (and it seems OK to leave it undefined for now).

[Iain Merrick]

Created attachment 108862 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 108862 [details]
Patch

Looks good. (unofficial) R+ from me.

[James Robinson]

Comment on attachment 108862 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 108862 [details]
Patch

Rejecting attachment 108862 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Iain Merrick]

Oops, guess I need commit-queue? instead of commit-queue+. Fixed.

[Iain Merrick]

James, could you please commit-queue+ this for me? Thanks!

[James Robinson]

Comment on attachment 108862 [details]
Patch

Any WebKit committer can set cq+ for a reviewed patch.  There are several WebKit committers in the LON office, so in the future it'll probably be faster for you to ask one of them rather than wait for me.

[WebKit Review Bot]

Comment on attachment 108862 [details]
Patch

Clearing flags on attachment: 108862

Committed r96337: <http://trac.webkit.org/changeset/96337>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.