Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2425873005:
Don't apply clips to children of composited-scrolling elements for overlap testing. (Closed)
|
Created:
4 years, 2 months ago by chrishtr Modified:
4 years, 2 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, 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. |
DescriptionDon't apply clips to children of composited-scrolling elements for overlap testing.
However, only compare these unclipped rects with other unclipped rects. This
prevents excessive layer explosion outside of the composited-scrolling layer.
This requires storing unclipped absolute rects for every PaintLayer.
BUG=652448
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/0a78cbe13de10c5a1c951d753fd13d01c8b3488e
Cr-Commit-Position: refs/heads/master@{#426664}
Patch Set 1 #Patch Set 2 : none #Patch Set 3 : none #
Total comments: 4
Patch Set 4 : none #Patch Set 5 : none #Patch Set 6 : none #Patch Set 7 : none #Messages
Total messages: 35 (24 generated)
Description was changed from ========== none none none none none none BUG= ========== to ========== none none none none none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== none none none none none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Don't apply clips to children of composited-scrolling elements for overlap testing. BUG=600785 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Don't apply clips to children of composited-scrolling elements for overlap testing. BUG=600785 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Don't apply clips to children of composited-scrolling elements for overlap testing. However, only compare these unclipped rects with other unclipped rects. This prevents excessive layer explosion outside of the composited-scrolling layer. BUG=600785 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Don't apply clips to children of composited-scrolling elements for overlap testing. However, only compare these unclipped rects with other unclipped rects. This prevents excessive layer explosion outside of the composited-scrolling layer. BUG=600785 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Don't apply clips to children of composited-scrolling elements for overlap testing. However, only compare these unclipped rects with other unclipped rects. This prevents excessive layer explosion outside of the composited-scrolling layer. This requires storing unclipped absolute rects for every PaintLayer. BUG=600785 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by chrishtr@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: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
chrishtr@chromium.org changed reviewers: + trchen@chromium.org, vollick@chromium.org
The CQ bit was checked by chrishtr@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...
https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/overflow/composited-scroll-overlap-test.html (right): https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/composited-scroll-overlap-test.html:29: output.innerHTML = internals.layerTreeAsText(document); nit: to avoid text diff churn, can you check the order in the JSON? Something like this? https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/paint/inv... https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:355: overlaps ? CompositingReasonOverlap : CompositingReasonNone; I think I follow. If we're below the composited scroller, we want to overlap using our unclipped bounds. This sounds reasonable, but only for discounting clips due to the scroller. Eg, in this example (https://jsfiddle.net/z0q9m85y/), will the orange cause the blue div to promote?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/overflow/composited-scroll-overlap-test.html (right): https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/composited-scroll-overlap-test.html:29: output.innerHTML = internals.layerTreeAsText(document); On 2016/10/19 at 20:16:31, Ian Vollick wrote: > nit: to avoid text diff churn, can you check the order in the JSON? Something like this? > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/paint/inv... Done. https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:355: overlaps ? CompositingReasonOverlap : CompositingReasonNone; On 2016/10/19 at 20:16:31, Ian Vollick wrote: > I think I follow. If we're below the composited scroller, we want to overlap using our unclipped bounds. This sounds reasonable, but only for discounting clips due to the scroller. > > Eg, in this example (https://jsfiddle.net/z0q9m85y/), will the orange cause the blue div to promote? Exactly, it will promote. (though you have to add position: relative to your example to get a PaintLayer). I double-checked that this occurs with this patch.
The CQ bit was checked by chrishtr@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...
On 2016/10/19 21:44:51, chrishtr wrote: > https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/compositing/overflow/composited-scroll-overlap-test.html > (right): > > https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/compositing/overflow/composited-scroll-overlap-test.html:29: > output.innerHTML = internals.layerTreeAsText(document); > On 2016/10/19 at 20:16:31, Ian Vollick wrote: > > nit: to avoid text diff churn, can you check the order in the JSON? Something > like this? > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/paint/inv... > > Done. > > https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp > (right): > > https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:355: > overlaps ? CompositingReasonOverlap : CompositingReasonNone; > On 2016/10/19 at 20:16:31, Ian Vollick wrote: > > I think I follow. If we're below the composited scroller, we want to overlap > using our unclipped bounds. This sounds reasonable, but only for discounting > clips due to the scroller. > > > > Eg, in this example (https://jsfiddle.net/z0q9m85y/), will the orange cause > the blue div to promote? > > Exactly, it will promote. (though you have to add position: relative to your > example to get a PaintLayer). Ah, right! > > I double-checked that this occurs with this patch. That's a problem, though, isn't it? (In this case, I thought the blue div didn't actually need to be separately composited since it doesn't overlap the clipped orange div). Say we have a scroller which contains clipped content. Don't we now stand a chance of exploding the content within the scroller since those contents may now incorrectly start thinking that they overlap with one another?
On 2016/10/19 at 22:15:04, vollick wrote: > On 2016/10/19 21:44:51, chrishtr wrote: > > https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Layo... > > File > > third_party/WebKit/LayoutTests/compositing/overflow/composited-scroll-overlap-test.html > > (right): > > > > https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/compositing/overflow/composited-scroll-overlap-test.html:29: > > output.innerHTML = internals.layerTreeAsText(document); > > On 2016/10/19 at 20:16:31, Ian Vollick wrote: > > > nit: to avoid text diff churn, can you check the order in the JSON? Something > > like this? > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/paint/inv... > > > > Done. > > > > https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Sour... > > File > > third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp > > (right): > > > > https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:355: > > overlaps ? CompositingReasonOverlap : CompositingReasonNone; > > On 2016/10/19 at 20:16:31, Ian Vollick wrote: > > > I think I follow. If we're below the composited scroller, we want to overlap > > using our unclipped bounds. This sounds reasonable, but only for discounting > > clips due to the scroller. > > > > > > Eg, in this example (https://jsfiddle.net/z0q9m85y/), will the orange cause > > the blue div to promote? > > > > Exactly, it will promote. (though you have to add position: relative to your > > example to get a PaintLayer). > > Ah, right! > > > > I double-checked that this occurs with this patch. > > That's a problem, though, isn't it? (In this case, I thought the blue div didn't actually need to be separately composited since it doesn't overlap the clipped orange div). Say we have a scroller which contains clipped content. Don't we now stand a chance of exploding the content within the scroller since those contents may now incorrectly start thinking that they overlap with one another? Yes it's a potential problem. Clipping would not reduce overlap testing false-positives inside of a composited scroller. I special-cased the situation of preventing this from leaking outside of the composited scroller, but nothing else. I couldn't come up with a simple enough and performant enough solution for the more general setting. That will have to wait for SPv2, unless there is an additional idea I'm missing.
On 2016/10/19 22:21:39, chrishtr wrote: > On 2016/10/19 at 22:15:04, vollick wrote: > > On 2016/10/19 21:44:51, chrishtr wrote: > > > > https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Layo... > > > File > > > > third_party/WebKit/LayoutTests/compositing/overflow/composited-scroll-overlap-test.html > > > (right): > > > > > > > https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Layo... > > > > third_party/WebKit/LayoutTests/compositing/overflow/composited-scroll-overlap-test.html:29: > > > output.innerHTML = internals.layerTreeAsText(document); > > > On 2016/10/19 at 20:16:31, Ian Vollick wrote: > > > > nit: to avoid text diff churn, can you check the order in the JSON? > Something > > > like this? > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/paint/inv... > > > > > > Done. > > > > > > > https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Sour... > > > File > > > > third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp > > > (right): > > > > > > > https://codereview.chromium.org/2425873005/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:355: > > > overlaps ? CompositingReasonOverlap : CompositingReasonNone; > > > On 2016/10/19 at 20:16:31, Ian Vollick wrote: > > > > I think I follow. If we're below the composited scroller, we want to > overlap > > > using our unclipped bounds. This sounds reasonable, but only for discounting > > > clips due to the scroller. > > > > > > > > Eg, in this example (https://jsfiddle.net/z0q9m85y/), will the orange > cause > > > the blue div to promote? > > > > > > Exactly, it will promote. (though you have to add position: relative to your > > > example to get a PaintLayer). > > > > Ah, right! > > > > > > I double-checked that this occurs with this patch. > > > > That's a problem, though, isn't it? (In this case, I thought the blue div > didn't actually need to be separately composited since it doesn't overlap the > clipped orange div). Say we have a scroller which contains clipped content. > Don't we now stand a chance of exploding the content within the scroller since > those contents may now incorrectly start thinking that they overlap with one > another? > > Yes it's a potential problem. Clipping would not reduce overlap testing > false-positives inside of a composited scroller. > I special-cased the situation of preventing this from leaking outside of the > composited scroller, but nothing else. > > I couldn't come up with a simple enough and performant enough solution for the > more general setting. That will have > to wait for SPv2, unless there is an additional idea I'm missing. kk. lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vollick@chromium.org Link to the patchset: https://codereview.chromium.org/2425873005/#ps100001 (title: "none")
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
Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
Description was changed from ========== Don't apply clips to children of composited-scrolling elements for overlap testing. However, only compare these unclipped rects with other unclipped rects. This prevents excessive layer explosion outside of the composited-scrolling layer. This requires storing unclipped absolute rects for every PaintLayer. BUG=600785 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Don't apply clips to children of composited-scrolling elements for overlap testing. However, only compare these unclipped rects with other unclipped rects. This prevents excessive layer explosion outside of the composited-scrolling layer. This requires storing unclipped absolute rects for every PaintLayer. BUG=652448 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vollick@chromium.org Link to the patchset: https://codereview.chromium.org/2425873005/#ps120001 (title: "none")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Don't apply clips to children of composited-scrolling elements for overlap testing. However, only compare these unclipped rects with other unclipped rects. This prevents excessive layer explosion outside of the composited-scrolling layer. This requires storing unclipped absolute rects for every PaintLayer. BUG=652448 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Don't apply clips to children of composited-scrolling elements for overlap testing. However, only compare these unclipped rects with other unclipped rects. This prevents excessive layer explosion outside of the composited-scrolling layer. This requires storing unclipped absolute rects for every PaintLayer. BUG=652448 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/0a78cbe13de10c5a1c951d753fd13d01c8b3488e Cr-Commit-Position: refs/heads/master@{#426664} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0a78cbe13de10c5a1c951d753fd13d01c8b3488e Cr-Commit-Position: refs/heads/master@{#426664} |
