Bug 107090 – [TexMap] Match initializing members in GraphicsLayerTransform to initializing members in GraphicsLayer.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dongseong Hwang]

GraphicsLayer initializes m_anchorPoint to FloatPoint3D(0.5, 0.5, 0) and
m_preserves3D to false, while GraphicsLayerTransform initializes m_anchorPoint
to FloatPoint3D(0, 0, 0) and m_flattening (= !m_preserves3D) to false. It is a
potential bug. This patch corrects initialization of GraphicsLayerTransform.

[Dongseong Hwang]

Created attachment 183112 [details]
Patch

[Dongseong Hwang]

Created attachment 183114 [details]
Patch

[Noam Rosenthal]

Comment on attachment 183114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183114&action=review

This is pretty trivial so I would let it go in, but please be more explicit about how it's tested.

> Source/WebCore/ChangeLog:13
> +        Covered by existing variables tests.

Does this currently get reproduced?

> Source/WebCore/platform/graphics/GraphicsLayerTransform.cpp:26
> +    : m_anchorPoint(0.5f, 0.5f, 0) // see m_anchorPoint initialization in GraphicsLayer::GraphicsLayer().

You don't need f.
Also please remove the comment, if GraphicsLayer constructor is changed, it will get stale.

> Source/WebCore/platform/graphics/GraphicsLayerTransform.cpp:27
> +    , m_flattening(true) // see m_preserves3D initialization in GraphicsLayer::GraphicsLayer().

Same here re. the comment.

[Dongseong Hwang]

(In reply to comment #3)
> (From update of attachment 183114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183114&action=review
> 
> This is pretty trivial so I would let it go in, but please be more explicit about how it's tested.
> 
> > Source/WebCore/ChangeLog:13
> > +        Covered by existing variables tests.
> 
> Does this currently get reproduced?

After Bug 107073, many pixel tests will be failed.

But currently fine, and we can not reproduce. It is because TextureMapperLayer::flushCompositingStateForThisLayerOnly() always make m_currentTransform dirty via redundant calling all setters of GraphicsLayerTransform.

[Dongseong Hwang]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 183114 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=183114&action=review
> > 
> > This is pretty trivial so I would let it go in, but please be more explicit about how it's tested.
> > 
> > > Source/WebCore/ChangeLog:13
> > > +        Covered by existing variables tests.
> > 
> > Does this currently get reproduced?
> 
> After Bug 107073, many pixel tests will be failed.
> 
> But currently fine, and we can not reproduce. It is because TextureMapperLayer::flushCompositingStateForThisLayerOnly() always make m_currentTransform dirty via redundant calling all setters of GraphicsLayerTransform.

I will put specific test cases in chagelog that modify anchor point and perserve3D.

[Dongseong Hwang]

Created attachment 183141 [details]
Patch

[Dongseong Hwang]

Created attachment 183146 [details]
Patch

[WebKit Review Bot]

Attachment 183146 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/comp..." exit_code: 1
LayoutTests/compositing/geometry/composited-2d-rotate-transform-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dongseong Hwang]

(In reply to comment #7)
> Created an attachment (id=183146) [details]
> Patch

I made a test case!
This test success regardless of this patch.
But if we land Bug 107073, test pass only after this patch.


The third patch's test compare the result with -expected.html to avoid pixel test, but sadly the test always fail by 0.08%, because software qt and texmap render differently by 0.08%. So the fourth patch uses pixel test.

[Dongseong Hwang]

(In reply to comment #8)
> Attachment 183146 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/comp..." exit_code: 1
> LayoutTests/compositing/geometry/composited-2d-rotate-transform-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
> Total errors found: 1 in 6 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

in my machine, check-webkit-style passed.

[Dongseong Hwang]

Created attachment 183147 [details]
Patch

[WebKit Review Bot]

Attachment 183147 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/comp..." exit_code: 1
LayoutTests/compositing/geometry/composited-2d-rotate-transform-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dongseong Hwang]

Comment on attachment 183147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183147&action=review

> LayoutTests/compositing/geometry/composited-2d-rotate-transform.html:25
> +<div id="testCase1" style="background-color: green; -webkit-transform: translateZ(0);">

I think this test is a bit unnatural (nobody can not understand why I put -webkit-transform: translateZ(0)) and it requires pixel test for trivial initialization bug.
Do you think this test is necessary?

[Dongseong Hwang]

(In reply to comment #13)
> (From update of attachment 183147 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183147&action=review
> 
> > LayoutTests/compositing/geometry/composited-2d-rotate-transform.html:25
> > +<div id="testCase1" style="background-color: green; -webkit-transform: translateZ(0);">
> 
> I think this test is a bit unnatural (nobody can not understand why I put -webkit-transform: translateZ(0)) and it requires pixel test for trivial initialization bug.
> Do you think this test is necessary?

FYI, this test is subset of backface-visibility/backface-visibility-non3d.html.
And as I mentioned, many pixel tests are affected by Bug 107073 without this patch.
However, unfortunately, the tests that are affected are already failed without Bug 107073 because of other texmap problems. so It is hard to list exact tests for this bug.

After running pixel tests in compositing folder, I now know we have a lot of jobs we have to :)

[Dongseong Hwang]

Created attachment 183156 [details]
Patch

[Dongseong Hwang]

(In reply to comment #15)
> Created an attachment (id=183156) [details]
> Patch

I give up an unnatural and duplicated new test. This patch is covered by existing compositing pixel tests.

[WebKit Review Bot]

Comment on attachment 183156 [details]
Patch

Attachment 183156 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15906838

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Dongseong Hwang]

Created attachment 183287 [details]
Patch

[WebKit Review Bot]

Comment on attachment 183287 [details]
Patch

Clearing flags on attachment: 183287

Committed r140063: <http://trac.webkit.org/changeset/140063>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.