Bug 91472 – [chromium] Remove awkward anchorPoint usage that implicitly affects layer position

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

[Shawn Singh]

I've lost several days of work on three separate occasions, thinking that something is deeply wrong with the transform code, only to find that anchorPoint was not being properly initialized to (0,0).  The problem is that anchorPoint defaults to (0.5, 0.5), but when anchorPoint is nonzero, it implicitly affects position.  Therefore, for unit tests that deal with positioning layers, without initializing anchorpoint, the layers are not actually positioned where expected.  This is purely our chromium compositor choice - in GraphicsLayerChromium, we chose to define "position" as the sum of position and anchorPoint.

As I understand, we originally did this just to avoid a few extra matrix multiplications in calcDrawTransforms.  At this point, I think that reasoning is obsolete - the savings is not very significant (position and anchor can be summed before doing an additional matrix multiply), and it is definitely not worth wasting engineer time anymore just because anchorPoint was not initialized.  It's not anyone's fault, either - it genuinely does seem reasonable that anchorPoint is not relevant and should not need to be initialized beyond the default value for most tests.  But in the current code, unfortunately it does... and it's overlooked a lot.

Are you guys OK with separating anchor point and position, so that it doesn't matter what value anchorPoint is when we try to position layers in our code?

Enne, I suspect you're may run into this similar problem while working on the half-width, half-height removal.  Perhaps by pointing this out, it might save you days of frustration =)

Patch coming in a moment;  passes unit tests already, still testing layout tests.

[Shawn Singh]

Created attachment 152709 [details]
Patch

[Shawn Singh]

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

Passes unit tests and layout tests on mac.

One test will need rebaselining after this patch, but it looks like that test didn't behave correctly before.  After this patch, it behaves same/similar to Safari, and I think its correct.

[Shawn Singh]

Created attachment 152711 [details]
Patch

Same patch, rebased to tip of tree

[WebKit Review Bot]

Comment on attachment 152711 [details]
Patch

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

New failing tests:
compositing/reflections/nested-reflection-anchor-point.html
CCDamageTrackerTest.verifyDamageForReplicaMaskWithAnchor
CCLayerTreeHostImplTest.pageScaleDeltaAppliedToRootScrollLayerOnly
CCLayerTreeHostCommonTest.verifyTransformsForRenderSurfaceHierarchy

[WebKit Review Bot]

Created attachment 152719 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Shawn Singh]

Created attachment 152796 [details]
Patch

Updated to tip of tree and fixed previous errors

[Adrienne Walker]

Comment on attachment 152796 [details]
Patch

R=me.  This really cleans things up a lot!  I feel pretty comfortable with this given the amount of layout tests that would break if the transform math were wrong.

However, CCDamageTrackerTest suffers a lot from http://www.webkit.org/coding/coding-style.html#float-suffixes.  Your patch adds a few more, but there's enough in the file that I don't know that it's worth calling out specifically.

[Shawn Singh]

Committed r122980: <http://trac.webkit.org/changeset/122980>

[Shawn Singh]

(In reply to comment #7)
> (From update of attachment 152796 [details])
> R=me.  This really cleans things up a lot!  I feel pretty comfortable with this given the amount of layout tests that would break if the transform math were wrong.
> 
> However, CCDamageTrackerTest suffers a lot from http://www.webkit.org/coding/coding-style.html#float-suffixes.  Your patch adds a few more, but there's enough in the file that I don't know that it's worth calling out specifically.

OK, sounds good.  I'm making a patch to clean this up for various unit test files right now.

I do strongly disagree with this style rule =)  but of course it's more important to stick to the agreed style conventions...

[Adrienne Walker]

(In reply to comment #9)
>
> I do strongly disagree with this style rule =)  but of course it's more important to stick to the agreed style conventions...

It's ok to disagree! The list of things I disagree with in the WebKit project is immense.  :)