Bug 73247 - [chromium] CCLayerQuad does not return FloatQuad in correct order
Summary: [chromium] CCLayerQuad does not return FloatQuad in correct order
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alok Priyadarshi
URL:
Keywords:
Depends on: 73678
Blocks: 72760
  Show dependency treegraph
 
Reported: 2011-11-28 14:00 PST by Alok Priyadarshi
Modified: 2011-12-02 14:34 PST (History)
7 users (show)

See Also:


Attachments
proposed patch (5.37 KB, patch)
2011-11-28 14:46 PST, Alok Priyadarshi
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
proposed patch (7.73 KB, patch)
2011-11-30 11:50 PST, Alok Priyadarshi
no flags Details | Formatted Diff | Diff
proposed patch (7.89 KB, patch)
2011-12-01 14:25 PST, Alok Priyadarshi
no flags Details | Formatted Diff | Diff
proposed patch (7.91 KB, patch)
2011-12-02 12:56 PST, Alok Priyadarshi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alok Priyadarshi 2011-11-28 14:00:03 PST
CCLayerQuad::floatQuad() does not return correct FloatQuad. The orientation is reversed.
Comment 1 Alok Priyadarshi 2011-11-28 14:46:22 PST
Created attachment 116823 [details]
proposed patch

I am expecting that a few compositing tests will need to rebaselined.
Comment 2 WebKit Review Bot 2011-11-28 14:48:10 PST
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.
Comment 3 Alok Priyadarshi 2011-11-29 15:26:00 PST
ping!
Comment 4 James Robinson 2011-11-29 16:02:26 PST
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 5 WebKit Review Bot 2011-11-29 18:02:11 PST
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 6 Alok Priyadarshi 2011-11-29 20:19:39 PST
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.
Comment 7 James Robinson 2011-11-29 22:01:49 PST
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.
Comment 8 Alok Priyadarshi 2011-11-30 10:43:19 PST
(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
Comment 9 Alok Priyadarshi 2011-11-30 11:50:09 PST
Created attachment 117242 [details]
proposed patch
Comment 10 WebKit Review Bot 2011-11-30 11:53:15 PST
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.
Comment 11 Alok Priyadarshi 2011-12-01 13:10:11 PST
James: Any other concerns?
Is it OK to commit after resolving test_expectations.txt?
Comment 12 Alok Priyadarshi 2011-12-01 14:25:55 PST
Created attachment 117490 [details]
proposed patch

Updated test_expectations.
Comment 13 WebKit Review Bot 2011-12-01 14:39:40 PST
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.
Comment 14 James Robinson 2011-12-01 14:47:47 PST
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.
Comment 15 Alok Priyadarshi 2011-12-02 08:25:42 PST
cr-linux bot is happy.
Comment 16 James Robinson 2011-12-02 10:35:21 PST
Comment on attachment 117490 [details]
proposed patch

Here goes!
Comment 17 WebKit Review Bot 2011-12-02 11:08:39 PST
Comment on attachment 117490 [details]
proposed patch

Clearing flags on attachment: 117490

Committed r101833: <http://trac.webkit.org/changeset/101833>
Comment 18 WebKit Review Bot 2011-12-02 11:08:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Alok Priyadarshi 2011-12-02 11:47:43 PST
The patch was rolled out in r101841 due to invalid test_expectations.txt.
https://bugs.webkit.org/show_bug.cgi?id=73678
Comment 20 Alok Priyadarshi 2011-12-02 12:56:22 PST
Created attachment 117672 [details]
proposed patch

Now with test_expectations.txt certified by run_webkit_test --lint-test-files.
Comment 21 WebKit Review Bot 2011-12-02 12:58:01 PST
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.
Comment 22 Alok Priyadarshi 2011-12-02 13:09:09 PST
Committed r101844: <http://trac.webkit.org/changeset/101844>