Bug 73247 – [chromium] CCLayerQuad does not return FloatQuad in correct order

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alok Priyadarshi]

CCLayerQuad::floatQuad() does not return correct FloatQuad. The orientation is reversed.

[Alok Priyadarshi]

Created attachment 116823 [details]
proposed patch

I am expecting that a few compositing tests will need to rebaselined.

[WebKit Review Bot]

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.

[Alok Priyadarshi]

ping!

[James Robinson]

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'

[WebKit Review Bot]

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

[Alok Priyadarshi]

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.

[James Robinson]

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.

[Alok Priyadarshi]

(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

[Alok Priyadarshi]

Created attachment 117242 [details]
proposed patch

[WebKit Review Bot]

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.

[Alok Priyadarshi]

James: Any other concerns?
Is it OK to commit after resolving test_expectations.txt?

[Alok Priyadarshi]

Created attachment 117490 [details]
proposed patch

Updated test_expectations.

[WebKit Review Bot]

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.

[James Robinson]

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.

[Alok Priyadarshi]

cr-linux bot is happy.

[James Robinson]

Comment on attachment 117490 [details]
proposed patch

Here goes!

[WebKit Review Bot]

Comment on attachment 117490 [details]
proposed patch

Clearing flags on attachment: 117490

Committed r101833: <http://trac.webkit.org/changeset/101833>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alok Priyadarshi]

The patch was rolled out in r101841 due to invalid test_expectations.txt.
https://bugs.webkit.org/show_bug.cgi?id=73678

[Alok Priyadarshi]

Created attachment 117672 [details]
proposed patch

Now with test_expectations.txt certified by run_webkit_test --lint-test-files.

[WebKit Review Bot]

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.

[Alok Priyadarshi]

Committed r101844: <http://trac.webkit.org/changeset/101844>