Bug 69765 – [chromium] Update comments about transform hierarchy in CCLayerTreeHostCommon

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

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.

[Shawn Singh]

Created attachment 110396 [details]
Patch

[James Robinson]

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"

[Shawn Singh]

Created attachment 110417 [details]
Patch

[Adrienne Walker]

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]?

[Shawn Singh]

OK thanks for the comments!  Will upload a new patch tonight.

[Shawn Singh]

Created attachment 110465 [details]
addressed review comments

[Adrienne Walker]

Looks unofficially good to me.  Thanks for doing this.  :)

[Vangelis Kokkevis]

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.

[Shawn Singh]

Created attachment 110702 [details]
added point 5 about render surface

[Vangelis Kokkevis]

(In reply to comment #9)
> Created an attachment (id=110702) [details]
> added point 5 about render surface

Looks great.  Thanks.

[WebKit Review Bot]

Comment on attachment 110702 [details]
added point 5 about render surface

Clearing flags on attachment: 110702

Committed r97685: <http://trac.webkit.org/changeset/97685>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.