Bug 91472 - [chromium] Remove awkward anchorPoint usage that implicitly affects layer position
Summary: [chromium] Remove awkward anchorPoint usage that implicitly affects layer pos...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
URL:
Keywords:
Depends on:
Blocks: 80622
  Show dependency treegraph
 
Reported: 2012-07-17 00:08 PDT by Shawn Singh
Modified: 2012-07-18 09:43 PDT (History)
8 users (show)

See Also:


Attachments
Patch (38.90 KB, patch)
2012-07-17 00:58 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch (38.89 KB, patch)
2012-07-17 01:03 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (1.52 MB, application/zip)
2012-07-17 01:42 PDT, WebKit Review Bot
no flags Details
Patch (42.66 KB, patch)
2012-07-17 11:44 PDT, Shawn Singh
enne: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Singh 2012-07-17 00:08:02 PDT
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.
Comment 1 Shawn Singh 2012-07-17 00:58:23 PDT
Created attachment 152709 [details]
Patch
Comment 2 Shawn Singh 2012-07-17 01:00:38 PDT
(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.
Comment 3 Shawn Singh 2012-07-17 01:03:13 PDT
Created attachment 152711 [details]
Patch

Same patch, rebased to tip of tree
Comment 4 WebKit Review Bot 2012-07-17 01:42:31 PDT
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
Comment 5 WebKit Review Bot 2012-07-17 01:42:34 PDT
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
Comment 6 Shawn Singh 2012-07-17 11:44:45 PDT
Created attachment 152796 [details]
Patch

Updated to tip of tree and fixed previous errors
Comment 7 Adrienne Walker 2012-07-17 16:37:15 PDT
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.
Comment 8 Shawn Singh 2012-07-18 09:35:08 PDT
Committed r122980: <http://trac.webkit.org/changeset/122980>
Comment 9 Shawn Singh 2012-07-18 09:37:05 PDT
(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...
Comment 10 Adrienne Walker 2012-07-18 09:43:54 PDT
(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.  :)