CCLayerQuad::floatQuad() does not return correct FloatQuad. The orientation is reversed.
Created attachment 116823 [details] proposed patch I am expecting that a few compositing tests will need to rebaselined.
Attachment 116823 [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/CCLayerQuadTest.cpp:38: Extra space after ( in function call [whitespace/parens] [4] Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:39: Extra space after ( in function call [whitespace/parens] [4] Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:54: Extra space after ( in function call [whitespace/parens] [4] Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:55: Extra space after ( in function call [whitespace/parens] [4] Total errors found: 4 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
ping!
Comment on attachment 116823 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=116823&action=review I don't really understand this change. What is correct/incorrect about a certain orientation? What problem does this cause? > Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:37 > + FloatPoint p1(-0.5f, -0.5f); you shouldn't have the trailing 'f' here. see http://www.webkit.org/coding/coding-style.html 'Floating point literals'
Comment on attachment 116823 [details] proposed patch Attachment 116823 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10678858 New failing tests: platform/chromium/compositing/huge-layer-rotated.html compositing/geometry/vertical-scroll-composited.html compositing/geometry/layer-due-to-layer-children.html fast/replaced/width100percent-textarea.html compositing/geometry/ancestor-overflow-change.html http/tests/inspector/network-preflight-options.html compositing/reflections/nested-reflection-transition.html compositing/direct-image-compositing.html compositing/shadows/shadow-drawing.html compositing/geometry/layer-due-to-layer-children-deep.html compositing/color-matching/image-color-matching.html compositing/reflections/nested-reflection-transformed.html compositing/reflections/nested-reflection-transformed2.html compositing/reflections/reflection-in-composited.html compositing/geometry/fixed-position-transform-composited-page-scale.html
Comment on attachment 116823 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=116823&action=review Sorry it does not change the orientation. It just returns a FloatQuad with vertices in the wrong sequence. Suppose you construct a CCLayerQuad with a FloatQuad(p1, p2, p3, p4). CCLayerQuad::floatQuad() will return a FloatQuad(p4, p1, p2, p3). I think CCLayerQuad::floatQuad() should return the same FloatQuad it was constructed with in the first place. Without this patch the new unit test FloatQuadConversion will fail. It does not make any difference with the current implementation but will be important for one of the fixes for https://bugs.webkit.org/show_bug.cgi?id=72760. >> Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:37 >> + FloatPoint p1(-0.5f, -0.5f); > > you shouldn't have the trailing 'f' here. see http://www.webkit.org/coding/coding-style.html 'Floating point literals' ok. >> Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:38 >> + FloatPoint p2( 0.5f, -0.5f); > > Extra space after ( in function call [whitespace/parens] [4] should I fix this style recommendation? It is much more readable this way.
I think that space is probably fine for alignment. Are the layout test failures really due to this patch? That would be alarming if so.
(In reply to comment #7) > I think that space is probably fine for alignment. > > Are the layout test failures really due to this patch? That would be alarming if so. Yes all the compositing failures are due to this patch. They are 1-2 pixel differences due to rotating the quad underlying a layer texture by 90 degrees. There are more tests to be rebaselined under webkit_gpu_tests. I have just sent a try run to find all those. I will update the patch with test-expectations as soon as I have the final list of tests. These are flaky: fast/replaced/width100percent-textarea.html http/tests/inspector/network-preflight-options.html
Created attachment 117242 [details] proposed patch
Attachment 117242 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:38: Extra space after ( in function call [whitespace/parens] [4] Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:39: Extra space after ( in function call [whitespace/parens] [4] Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:54: Extra space after ( in function call [whitespace/parens] [4] Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:55: Extra space after ( in function call [whitespace/parens] [4] Total errors found: 4 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
James: Any other concerns? Is it OK to commit after resolving test_expectations.txt?
Created attachment 117490 [details] proposed patch Updated test_expectations.
Attachment 117490 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:38: Extra space after ( in function call [whitespace/parens] [4] Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:39: Extra space after ( in function call [whitespace/parens] [4] Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:54: Extra space after ( in function call [whitespace/parens] [4] Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:55: Extra space after ( in function call [whitespace/parens] [4] Total errors found: 4 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
I think this fine. If you don't mind I'm going to let the cr-linux bot eat the most recent patch to make sure that the test_expectations covers everything - life's been pretty hard for the WebKit gardener lately and I don't want to make it worse.
cr-linux bot is happy.
Comment on attachment 117490 [details] proposed patch Here goes!
Comment on attachment 117490 [details] proposed patch Clearing flags on attachment: 117490 Committed r101833: <http://trac.webkit.org/changeset/101833>
All reviewed patches have been landed. Closing bug.
The patch was rolled out in r101841 due to invalid test_expectations.txt. https://bugs.webkit.org/show_bug.cgi?id=73678
Created attachment 117672 [details] proposed patch Now with test_expectations.txt certified by run_webkit_test --lint-test-files.
Attachment 117672 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:38: Extra space after ( in function call [whitespace/parens] [4] Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:39: Extra space after ( in function call [whitespace/parens] [4] Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:54: Extra space after ( in function call [whitespace/parens] [4] Source/WebKit/chromium/tests/CCLayerQuadTest.cpp:55: Extra space after ( in function call [whitespace/parens] [4] Total errors found: 4 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r101844: <http://trac.webkit.org/changeset/101844>