Bug 107090

Created attachment 183112 [details] Patch

Created attachment 183114 [details] Patch

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.

(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.

(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.

Created attachment 183141 [details] Patch

Created attachment 183146 [details] Patch

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 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.

(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.

Created attachment 183147 [details] Patch

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.

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?

(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 :)

Created attachment 183156 [details] Patch

(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.

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

Created attachment 183287 [details] Patch

Comment on attachment 183287 [details] Patch Clearing flags on attachment: 183287 Committed r140063: <http://trac.webkit.org/changeset/140063>

All reviewed patches have been landed. Closing bug.