Bug 107090

Summary: [TexMap] Match initializing members in GraphicsLayerTransform to initializing members in GraphicsLayer.
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: Layout and RenderingAssignee: Dongseong Hwang <dongseong.hwang>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 107073    
Attachments:
Description Flags
Patch
none
Patch
noam: review+, noam: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Dongseong Hwang 2013-01-16 21:06:14 PST
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.
Comment 1 Dongseong Hwang 2013-01-16 21:07:53 PST
Created attachment 183112 [details]
Patch
Comment 2 Dongseong Hwang 2013-01-16 21:11:08 PST
Created attachment 183114 [details]
Patch
Comment 3 Noam Rosenthal 2013-01-16 23:42:50 PST
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.
Comment 4 Dongseong Hwang 2013-01-16 23:52:59 PST
(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.
Comment 5 Dongseong Hwang 2013-01-16 23:54:42 PST
(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.
Comment 6 Dongseong Hwang 2013-01-17 01:18:34 PST
Created attachment 183141 [details]
Patch
Comment 7 Dongseong Hwang 2013-01-17 01:38:17 PST
Created attachment 183146 [details]
Patch
Comment 8 WebKit Review Bot 2013-01-17 01:41:00 PST
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.
Comment 9 Dongseong Hwang 2013-01-17 01:42:55 PST
(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.
Comment 10 Dongseong Hwang 2013-01-17 01:46:48 PST
(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.
Comment 11 Dongseong Hwang 2013-01-17 01:47:07 PST
Created attachment 183147 [details]
Patch
Comment 12 WebKit Review Bot 2013-01-17 01:49:22 PST
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 13 Dongseong Hwang 2013-01-17 02:14:31 PST
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?
Comment 14 Dongseong Hwang 2013-01-17 02:26:09 PST
(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 :)
Comment 15 Dongseong Hwang 2013-01-17 02:32:00 PST
Created attachment 183156 [details]
Patch
Comment 16 Dongseong Hwang 2013-01-17 02:33:31 PST
(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 17 WebKit Review Bot 2013-01-17 03:40:56 PST
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
Comment 18 Dongseong Hwang 2013-01-17 14:48:05 PST
Created attachment 183287 [details]
Patch
Comment 19 WebKit Review Bot 2013-01-17 16:06:04 PST
Comment on attachment 183287 [details]
Patch

Clearing flags on attachment: 183287

Committed r140063: <http://trac.webkit.org/changeset/140063>
Comment 20 WebKit Review Bot 2013-01-17 16:06:09 PST
All reviewed patches have been landed.  Closing bug.