Bug 68622 - Add unit test for CCLayerSorter
Summary: Add unit test for CCLayerSorter
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-22 08:46 PDT by Iain Merrick
Modified: 2011-09-29 10:04 PDT (History)
6 users (show)

See Also:


Attachments
Patch (18.47 KB, patch)
2011-09-22 08:50 PDT, Iain Merrick
no flags Details | Formatted Diff | Diff
Patch (18.49 KB, patch)
2011-09-26 06:02 PDT, Iain Merrick
no flags Details | Formatted Diff | Diff
Patch (18.38 KB, patch)
2011-09-27 10:07 PDT, Iain Merrick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Iain Merrick 2011-09-22 08:46:47 PDT
Add unit test for CCLayerSorter
Comment 1 Iain Merrick 2011-09-22 08:50:17 PDT
Created attachment 108338 [details]
Patch
Comment 2 WebKit Review Bot 2011-09-22 08:54:30 PDT
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.
Comment 3 Iain Merrick 2011-09-22 08:56:16 PDT
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.
Comment 4 Iain Merrick 2011-09-22 09:02:15 PDT
Re style errors: I added spaces to make the constants readable. Happy to remove them if that error can be suppressed.
Comment 5 Iain Merrick 2011-09-26 06:02:50 PDT
Created attachment 108657 [details]
Patch
Comment 6 James Robinson 2011-09-26 19:59:46 PDT
Vangelis, since you wrote the sorter originally would you mind reviewing this patch?
Comment 7 Vangelis Kokkevis 2011-09-26 22:58:34 PDT
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.
Comment 8 Iain Merrick 2011-09-27 09:37:01 PDT
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).
Comment 9 Iain Merrick 2011-09-27 10:07:46 PDT
Created attachment 108862 [details]
Patch
Comment 10 Vangelis Kokkevis 2011-09-27 10:56:49 PDT
Comment on attachment 108862 [details]
Patch

Looks good. (unofficial) R+ from me.
Comment 11 James Robinson 2011-09-27 11:09:11 PDT
Comment on attachment 108862 [details]
Patch

R=me
Comment 12 WebKit Review Bot 2011-09-28 03:30:02 PDT
Comment on attachment 108862 [details]
Patch

Rejecting attachment 108862 [details] from commit-queue.

husky@google.com 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.
Comment 13 Iain Merrick 2011-09-28 03:32:16 PDT
Oops, guess I need commit-queue? instead of commit-queue+. Fixed.
Comment 14 Iain Merrick 2011-09-29 05:12:17 PDT
James, could you please commit-queue+ this for me? Thanks!
Comment 15 James Robinson 2011-09-29 09:51:05 PDT
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.
Comment 16 WebKit Review Bot 2011-09-29 10:04:47 PDT
Comment on attachment 108862 [details]
Patch

Clearing flags on attachment: 108862

Committed r96337: <http://trac.webkit.org/changeset/96337>
Comment 17 WebKit Review Bot 2011-09-29 10:04:51 PDT
All reviewed patches have been landed.  Closing bug.