Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2598923002:
Update transform paint property's transform-offset on element size changes (Closed)
|
Created:
3 years, 12 months ago by pdr. Modified:
3 years, 12 months ago Reviewers:
Xianzhu CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate transform paint property's transform-offset on element size changes
If a transform (or motion path) and transform-origin are present, an object's
transform paint property can depend on the object's size. This patch has two
changes to support changing transform-origin values:
1) transform-origin is only created if it will be used. For example, the origin
will be zero if just will-change has created a transform paint property.
2) When an object's size changes and it has a transform that could depend on
the paint offset, the paint properties are marked as needing an update.
BUG=645667
TEST=fast/events/touch/touch-rect-assert-first-layer-special.html with --enable-slimming-paint-invalidation
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/700fe29521a77a433f07b85fda61a35609c46cf2
Cr-Commit-Position: refs/heads/master@{#440439}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Only create transform-origin when needed #
Total comments: 1
Messages
Total messages: 28 (15 generated)
pdr@chromium.org changed reviewers: + wangxianzhu@chromium.org
Description was changed from ========== Update transform paint property on element size changes If transform-origin is present, an object's transform can depend on the object's size. With this change, we update paint properties on size changes for objects with transforms. BUG=645667 TEST=fast/events/touch/touch-rect-assert-first-layer-special.html with --enable-slimming-paint-invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Update transform paint property on element size changes If transform-origin is present, an object's transform can depend on the object's size. With this change, we update paint properties on size changes for objects with transforms. Similar spv1 layer transform updates are in LayoutBox::updateLayerTransformAfterLayout(). BUG=645667 TEST=fast/events/touch/touch-rect-assert-first-layer-special.html with --enable-slimming-paint-invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2598923002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2598923002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1712: } else if (hasTransformRelatedProperty()) { I think the CL is fine to fix the layout test failure, but might need some follow-ups: - The above condition seems not to cover all cases that we'll create a transform node, e.g. will-change: opacity. - We could have more accurate conditions to trigger setNeedsPaintPropertyUpdate() for only transform related properties with percent or calculated values. - We could separate handling of location change and size change (I have an ongoing CL for this).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/22 at 05:10:11, wangxianzhu wrote: > lgtm > > https://codereview.chromium.org/2598923002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > https://codereview.chromium.org/2598923002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutBox.cpp:1712: } else if (hasTransformRelatedProperty()) { > I think the CL is fine to fix the layout test failure, but might need some follow-ups: > - The above condition seems not to cover all cases that we'll create a transform node, e.g. will-change: opacity. > - We could have more accurate conditions to trigger setNeedsPaintPropertyUpdate() for only transform related properties with percent or calculated values. > - We could separate handling of location change and size change (I have an ongoing CL for this). These are good points. I think the first one should block landing this. Do you know why we need a transform node for will-change opacity? I don't think we do, except that compositing reasons are stored on transform nodes.
On 2016/12/22 at 05:18:20, pdr (OOO Dec 23 to Jan 2) wrote: > These are good points. I think the first one should block landing this. > > Do you know why we need a transform node for will-change opacity? I don't think we do, except that compositing reasons are stored on transform nodes. I think I understand now. Patch forthcoming...
On 2016/12/22 05:18:20, pdr (OOO Dec 23 to Jan 2) wrote: > On 2016/12/22 at 05:10:11, wangxianzhu wrote: > > lgtm > > > > > https://codereview.chromium.org/2598923002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > > > > https://codereview.chromium.org/2598923002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:1712: } else if > (hasTransformRelatedProperty()) { > > I think the CL is fine to fix the layout test failure, but might need some > follow-ups: > > - The above condition seems not to cover all cases that we'll create a > transform node, e.g. will-change: opacity. > > - We could have more accurate conditions to trigger > setNeedsPaintPropertyUpdate() for only transform related properties with percent > or calculated values. > > - We could separate handling of location change and size change (I have an > ongoing CL for this). > > These are good points. I think the first one should block landing this. > > Do you know why we need a transform node for will-change opacity? I don't think > we do, except that compositing reasons are stored on transform nodes. Yes, the transform node created for will-change:opacity is just for storing compositing reasons.
Description was changed from ========== Update transform paint property on element size changes If transform-origin is present, an object's transform can depend on the object's size. With this change, we update paint properties on size changes for objects with transforms. Similar spv1 layer transform updates are in LayoutBox::updateLayerTransformAfterLayout(). BUG=645667 TEST=fast/events/touch/touch-rect-assert-first-layer-special.html with --enable-slimming-paint-invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Update transform paint property's transform-offset on element size changes If a transform (or motion path) and transform-origin are present, an object's transform paint property can depend on the object's size. This patch has two changes to support changing transform-origin values: 1) transform-origin is only created if it will be used. For example, the origin will be zero if just will-change has created a transform paint property. 2) When an object's size changes and it has a transform that could depend on the paint offset, the paint properties are marked as needing an update. BUG=645667 TEST=fast/events/touch/touch-rect-assert-first-layer-special.html with --enable-slimming-paint-invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
PTAL The latest patch only creates transform offsets if a transform exists that will use them.
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wangxianzhu@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482425028209470,
"parent_rev": "508a443dfc10bc2542bfaa8cd84f20373b5f5d8b", "commit_rev":
"2552e4f227163eab9d9f1eb3ccd6a8aa769dda65"}
Message was sent while issue was closed.
Description was changed from ========== Update transform paint property's transform-offset on element size changes If a transform (or motion path) and transform-origin are present, an object's transform paint property can depend on the object's size. This patch has two changes to support changing transform-origin values: 1) transform-origin is only created if it will be used. For example, the origin will be zero if just will-change has created a transform paint property. 2) When an object's size changes and it has a transform that could depend on the paint offset, the paint properties are marked as needing an update. BUG=645667 TEST=fast/events/touch/touch-rect-assert-first-layer-special.html with --enable-slimming-paint-invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Update transform paint property's transform-offset on element size changes If a transform (or motion path) and transform-origin are present, an object's transform paint property can depend on the object's size. This patch has two changes to support changing transform-origin values: 1) transform-origin is only created if it will be used. For example, the origin will be zero if just will-change has created a transform paint property. 2) When an object's size changes and it has a transform that could depend on the paint offset, the paint properties are marked as needing an update. BUG=645667 TEST=fast/events/touch/touch-rect-assert-first-layer-special.html with --enable-slimming-paint-invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2598923002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Update transform paint property's transform-offset on element size changes If a transform (or motion path) and transform-origin are present, an object's transform paint property can depend on the object's size. This patch has two changes to support changing transform-origin values: 1) transform-origin is only created if it will be used. For example, the origin will be zero if just will-change has created a transform paint property. 2) When an object's size changes and it has a transform that could depend on the paint offset, the paint properties are marked as needing an update. BUG=645667 TEST=fast/events/touch/touch-rect-assert-first-layer-special.html with --enable-slimming-paint-invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2598923002 ========== to ========== Update transform paint property's transform-offset on element size changes If a transform (or motion path) and transform-origin are present, an object's transform paint property can depend on the object's size. This patch has two changes to support changing transform-origin values: 1) transform-origin is only created if it will be used. For example, the origin will be zero if just will-change has created a transform paint property. 2) When an object's size changes and it has a transform that could depend on the paint offset, the paint properties are marked as needing an update. BUG=645667 TEST=fast/events/touch/touch-rect-assert-first-layer-special.html with --enable-slimming-paint-invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/700fe29521a77a433f07b85fda61a35609c46cf2 Cr-Commit-Position: refs/heads/master@{#440439} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/700fe29521a77a433f07b85fda61a35609c46cf2 Cr-Commit-Position: refs/heads/master@{#440439}
Message was sent while issue was closed.
https://codereview.chromium.org/2598923002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2598923002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1715: // See: PaintPropertyTreeBuilder::updateTransform(...). Just thought of 1. perspective-origin has the same issue. 2. the above condition also covers other percent/calculated values of transforms, which should be mentioned in the comments. Can you address these, and perhaps add tests?
Message was sent while issue was closed.
On 2016/12/22 at 17:28:42, wangxianzhu wrote: > https://codereview.chromium.org/2598923002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > https://codereview.chromium.org/2598923002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutBox.cpp:1715: // See: PaintPropertyTreeBuilder::updateTransform(...). > Just thought of > 1. perspective-origin has the same issue. > 2. the above condition also covers other percent/calculated values of transforms, which should be mentioned in the comments. > > Can you address these, and perhaps add tests? Done in https://codereview.chromium.org/2584653002
Message was sent while issue was closed.
On 2016/12/22 at 22:08:42, pdr (OOO Dec 23 to Jan 2) wrote: > On 2016/12/22 at 17:28:42, wangxianzhu wrote: > > https://codereview.chromium.org/2598923002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > > > https://codereview.chromium.org/2598923002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:1715: // See: PaintPropertyTreeBuilder::updateTransform(...). > > Just thought of > > 1. perspective-origin has the same issue. > > 2. the above condition also covers other percent/calculated values of transforms, which should be mentioned in the comments. > > > > Can you address these, and perhaps add tests? > > Done in https://codereview.chromium.org/2584653002 err... done in https://codereview.chromium.org/2602453002 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
