Bug 69765

Summary: [chromium] Update comments about transform hierarchy in CCLayerTreeHostCommon
Product: WebKit Reporter: Shawn Singh <shawnsingh>
Component: Layout and RenderingAssignee: Shawn Singh <shawnsingh>
Status: RESOLVED FIXED    
Severity: Normal CC: enne, jamesr, nduca, vangelis, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
addressed review comments
none
added point 5 about render surface none

Description Shawn Singh 2011-10-10 10:24:33 PDT
This patch (coming shortly) extends the comments in calculateDrawTransformsAndVisibility, so that this complex function is hopefully a bit more readable.

This change was originally part of https://bugs.webkit.org/show_bug.cgi?id=69197 but it made sense to separate them since feedback may still continue on this one.
Comment 1 Shawn Singh 2011-10-10 13:44:20 PDT
Created attachment 110396 [details]
Patch
Comment 2 James Robinson 2011-10-10 14:42:12 PDT
Comment on attachment 110396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110396&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:117
> +    // 4. Definition of various transformsed used:

typo "transformsed"
Comment 3 Shawn Singh 2011-10-10 15:21:16 PDT
Created attachment 110417 [details]
Patch
Comment 4 Adrienne Walker 2011-10-10 17:44:26 PDT
Comment on attachment 110417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110417&action=review

Awesome.  This is much more understandable.  :)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:102
> +    //    projection applied at render time flips the Y axis appropriately.

s/render/draw/, for consistency

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:122
> +    //        Tr[o2a] is the translation from the layer's origin to its anchor point
> +    //        Tr[o2c] is the translation from the layer's origin to its center

I realize it's wordy, but originToAnchor, originToCenter, anchorToCenter would be better than abbreviating.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:129
> +    //        Tr[a2c] = T[o2a].inverse * T[o2c]

T => Tr?, inverse => inverse()?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:140
> +    // The draw transform for the layer is:

It is a little inconsistent to me that draw transforms go to the center of the layer, but screen space transforms go to the origin.  Can you maybe call this out explicitly in these comments? I know it's obvious from the math, but...

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:146
> +    //                       = M[root] * Tr[layer->position()] * M[l] * Tr[o2a].inverse()

M[l] => M[layer]?
Comment 5 Shawn Singh 2011-10-10 18:26:12 PDT
OK thanks for the comments!  Will upload a new patch tonight.
Comment 6 Shawn Singh 2011-10-10 20:04:23 PDT
Created attachment 110465 [details]
addressed review comments
Comment 7 Adrienne Walker 2011-10-11 09:42:01 PDT
Looks unofficially good to me.  Thanks for doing this.  :)
Comment 8 Vangelis Kokkevis 2011-10-11 11:01:00 PDT
Comment on attachment 110465 [details]
addressed review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=110465&action=review

This is looking great.  Just one more possible addition to consider.  Otherwise, an (unofficial) r+ from me.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:144
> +    //        Interpreting the math left-to-right, this transforms from the layer's render surface to the center of the layer.

I think it would be nice to put another point in the list above about render surfaces, since you're mentioning them here.  Something to help folks understand that the draw transform is relative to the layer's target render surface, not to the viewport.
Comment 9 Shawn Singh 2011-10-12 10:43:07 PDT
Created attachment 110702 [details]
added point 5 about render surface
Comment 10 Vangelis Kokkevis 2011-10-12 21:54:21 PDT
(In reply to comment #9)
> Created an attachment (id=110702) [details]
> added point 5 about render surface

Looks great.  Thanks.
Comment 11 WebKit Review Bot 2011-10-17 17:53:50 PDT
Comment on attachment 110702 [details]
added point 5 about render surface

Clearing flags on attachment: 110702

Committed r97685: <http://trac.webkit.org/changeset/97685>
Comment 12 WebKit Review Bot 2011-10-17 17:53:54 PDT
All reviewed patches have been landed.  Closing bug.