[chromium] Unify compositor quad transforms into content space
Created attachment 152466 [details] Patch
shawnsingh, danakj: Could one or both of y'all unofficially review this patch?
Comment on attachment 152466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152466&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:160 > + if (contentBounds().isEmpty()) > + quadTransformation.scale(1 / contentsScale()); > + else { > + quadTransformation.scaleNonUniform(bounds().width() / static_cast<double>(contentBounds().width()), > + bounds().height() / static_cast<double>(contentBounds().height())); > + quadTransformation.translate(-contentBounds().width() / 2.0, -contentBounds().height() / 2.0); > + } In retrospect, I don't think this is always right, but I'm not sure how to reconcile contentsScale which is a single value and the contentBounds/bounds ratio that handles different integer rounding in width and height.
Comment on attachment 152466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152466&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:154 > + if (contentBounds().isEmpty()) Do we even need to return anything other than nullptr in this case?
Comment on attachment 152466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152466&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:160 >> + } > > In retrospect, I don't think this is always right, but I'm not sure how to reconcile contentsScale which is a single value and the contentBounds/bounds ratio that handles different integer rounding in width and height. What's an example of a case where this isn't right? The few times I've encountered this pattern, contentsScale wasn't good enough because of ImageLayers in particular, which have different horizontal and vertical scaling. But it seemed to me like this code would handle all the cases I was aware of?
Comment on attachment 152466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152466&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:154 >> + if (contentBounds().isEmpty()) > > Do we even need to return anything other than nullptr in this case? Yeah. The gutter quads use the root layer (the real root, not the NCCH) to generate a shared quad state. Maybe we should just create that manually. >>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:160 >>> + } >> >> In retrospect, I don't think this is always right, but I'm not sure how to reconcile contentsScale which is a single value and the contentBounds/bounds ratio that handles different integer rounding in width and height. > > What's an example of a case where this isn't right? The few times I've encountered this pattern, contentsScale wasn't good enough because of ImageLayers in particular, which have different horizontal and vertical scaling. But it seemed to me like this code would handle all the cases I was aware of? It may just be an issue in tests, where contentBounds is not always up to date with contentsScale. I'll take another look and see if there's something I can clean up.
Created attachment 152586 [details] Patch
Created attachment 152593 [details] Handle empty bounds in create quad state
Comment on attachment 152593 [details] Handle empty bounds in create quad state lgtm!
kbr, senorblanco: Would one of you mind taking a look at this compositor patch that has an unofficial lgtm from danakj? It's just a refactoring that shouldn't cause any change in behavior. All layers other than tiled layers have bounds == contentBounds, so the extra code in createSharedQuadState generates the same quadTransform that it used to for both tiled layers and non-tiled layers.
Comment on attachment 152593 [details] Handle empty bounds in create quad state View in context: https://bugs.webkit.org/attachment.cgi?id=152593&action=review Mostly a rubber stamp, though I did look through the code. One question. Go ahead and cq+ if this isn't an issue. > Source/WebKit/chromium/tests/CCQuadCullerTest.cpp:202 > + ASSERT_EQ(quadList.size(), 12u); Will this get run in non-debug builds?
(In reply to comment #11) > (From update of attachment 152593 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152593&action=review > > Mostly a rubber stamp, though I did look through the code. One question. Go ahead and cq+ if this isn't an issue. Thanks. :) > > Source/WebKit/chromium/tests/CCQuadCullerTest.cpp:202 > > + ASSERT_EQ(quadList.size(), 12u); > > Will this get run in non-debug builds? Yeah. ASSERT_EQ aborts the test if it doesn't pass. Since later checks reference quadList[i], if that's wrong, then those will assert. See http://code.google.com/p/googletest/wiki/Primer#Basic_Assertions
Comment on attachment 152593 [details] Handle empty bounds in create quad state Clearing flags on attachment: 152593 Committed r122776: <http://trac.webkit.org/changeset/122776>
All reviewed patches have been landed. Closing bug.