WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
69765
[chromium] Update comments about transform hierarchy in CCLayerTreeHostCommon
https://bugs.webkit.org/show_bug.cgi?id=69765
Summary
[chromium] Update comments about transform hierarchy in CCLayerTreeHostCommon
Shawn Singh
Reported
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.
Attachments
Patch
(9.73 KB, patch)
2011-10-10 13:44 PDT
,
Shawn Singh
no flags
Details
Formatted Diff
Diff
Patch
(9.73 KB, patch)
2011-10-10 15:21 PDT
,
Shawn Singh
no flags
Details
Formatted Diff
Diff
addressed review comments
(10.20 KB, patch)
2011-10-10 20:04 PDT
,
Shawn Singh
no flags
Details
Formatted Diff
Diff
added point 5 about render surface
(10.80 KB, patch)
2011-10-12 10:43 PDT
,
Shawn Singh
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Shawn Singh
Comment 1
2011-10-10 13:44:20 PDT
Created
attachment 110396
[details]
Patch
James Robinson
Comment 2
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"
Shawn Singh
Comment 3
2011-10-10 15:21:16 PDT
Created
attachment 110417
[details]
Patch
Adrienne Walker
Comment 4
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]?
Shawn Singh
Comment 5
2011-10-10 18:26:12 PDT
OK thanks for the comments! Will upload a new patch tonight.
Shawn Singh
Comment 6
2011-10-10 20:04:23 PDT
Created
attachment 110465
[details]
addressed review comments
Adrienne Walker
Comment 7
2011-10-11 09:42:01 PDT
Looks unofficially good to me. Thanks for doing this. :)
Vangelis Kokkevis
Comment 8
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.
Shawn Singh
Comment 9
2011-10-12 10:43:07 PDT
Created
attachment 110702
[details]
added point 5 about render surface
Vangelis Kokkevis
Comment 10
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.
WebKit Review Bot
Comment 11
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
>
WebKit Review Bot
Comment 12
2011-10-17 17:53:54 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug