| Summary: | Add unit test for CCLayerSorter | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Iain Merrick <husky> | ||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | enne, husky, jamesr, shawnsingh, vangelis, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Iain Merrick
2011-09-22 08:46:47 PDT
Created attachment 108338 [details]
Patch
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.
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. Re style errors: I added spaces to make the constants readable. Happy to remove them if that error can be suppressed. Created attachment 108657 [details]
Patch
Vangelis, since you wrote the sorter originally would you mind reviewing this patch? 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. 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). Created attachment 108862 [details]
Patch
Comment on attachment 108862 [details]
Patch
Looks good. (unofficial) R+ from me.
Comment on attachment 108862 [details]
Patch
R=me
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. Oops, guess I need commit-queue? instead of commit-queue+. Fixed. James, could you please commit-queue+ this for me? Thanks! 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 on attachment 108862 [details] Patch Clearing flags on attachment: 108862 Committed r96337: <http://trac.webkit.org/changeset/96337> All reviewed patches have been landed. Closing bug. |