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.
Created attachment 152709 [details]
(In reply to comment #1)
> Created an attachment (id=152709) [details]
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.
Created attachment 152711 [details]
Same patch, rebased to tip of tree
Comment on attachment 152711 [details]
Attachment 152711 [details] did not pass chromium-ews (chromium-xvfb):
New failing tests:
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
Created attachment 152796 [details]
Updated to tip of tree and fixed previous errors
Comment on 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.
Committed r122980: <http://trac.webkit.org/changeset/122980>
(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...
(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. :)