Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
336493004:
Removing a RenderLayer's CLM should trigger a repaint (Closed)
|
Created:
6 years, 6 months ago by ajuma Modified:
6 years, 6 months ago CC:
abarth-chromium, blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr., rune+blink, zoltan1 Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionRemoving a RenderLayer's CLM should trigger a repaint
This makes RLC::applyUpdateLayerCompositingStateChickenEggHacks
issue a repaint when a RenderLayer gains or loses a CLM.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176128
Patch Set 1 : #
Total comments: 7
Patch Set 2 : Address review comments #
Total comments: 2
Patch Set 3 : Also recompute repaint rects #Patch Set 4 : Fix test failure #
Total comments: 3
Patch Set 5 : Add comment #
Messages
Total messages: 22 (0 generated)
It looks like this line might have been accidentally removed with the line above it. Example where this causes contents to disappear: http://fiddle.jshell.net/H2Qq7/1/show/
https://codereview.chromium.org/336493004/diff/40001/LayoutTests/compositing/... File LayoutTests/compositing/layer-creation/compositing-reason-removed.html (right): https://codereview.chromium.org/336493004/diff/40001/LayoutTests/compositing/... LayoutTests/compositing/layer-creation/compositing-reason-removed.html:1: <style> DOCTYPE https://codereview.chromium.org/336493004/diff/40001/Source/core/rendering/co... File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/336493004/diff/40001/Source/core/rendering/co... Source/core/rendering/compositing/RenderLayerCompositor.cpp:437: repaintOnCompositingChange(layer); This is interesting. When compositedLayerMappingChanged is set to true, the CompositingLayerAssign adds it to a list of layers to repaint later on. Is it somehow invalid to be issuing deferred repaints?
https://codereview.chromium.org/336493004/diff/40001/Source/core/rendering/co... File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/336493004/diff/40001/Source/core/rendering/co... Source/core/rendering/compositing/RenderLayerCompositor.cpp:437: repaintOnCompositingChange(layer); On 2014/06/12 20:17:10, Ian Vollick wrote: > This is interesting. When compositedLayerMappingChanged is set to true, the > CompositingLayerAssign adds it to a list of layers to repaint later on. Is it > somehow invalid to be issuing deferred repaints? The call to allocateOrClearCompositedLayerMapping made in applyUpdateLayerCompositingStateChickenEggHacks ignores the returned value (that is, the value of compositedLayerMappingChanged). Issuing a repaint there when the returned value is true fixes the bug too.
https://codereview.chromium.org/336493004/diff/40001/Source/core/rendering/co... File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/336493004/diff/40001/Source/core/rendering/co... Source/core/rendering/compositing/RenderLayerCompositor.cpp:437: repaintOnCompositingChange(layer); On 2014/06/12 20:33:50, ajuma wrote: > On 2014/06/12 20:17:10, Ian Vollick wrote: > > This is interesting. When compositedLayerMappingChanged is set to true, the > > CompositingLayerAssign adds it to a list of layers to repaint later on. Is it > > somehow invalid to be issuing deferred repaints? > > The call to allocateOrClearCompositedLayerMapping made in > applyUpdateLayerCompositingStateChickenEggHacks ignores the returned value (that > is, the value of compositedLayerMappingChanged). Issuing a repaint there when > the returned value is true fixes the bug too. Ah, right! I think that's far preferable to issuing the repaint here. Not only will that let us defer repaints correctly on the awesome-RLC-codepath, it'll ensure that it's removed when the hacks go away.
https://codereview.chromium.org/336493004/diff/40001/Source/core/rendering/co... File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/336493004/diff/40001/Source/core/rendering/co... Source/core/rendering/compositing/RenderLayerCompositor.cpp:437: repaintOnCompositingChange(layer); On 2014/06/12 20:33:50, ajuma wrote: > On 2014/06/12 20:17:10, Ian Vollick wrote: > > This is interesting. When compositedLayerMappingChanged is set to true, the > > CompositingLayerAssign adds it to a list of layers to repaint later on. Is it > > somehow invalid to be issuing deferred repaints? > > The call to allocateOrClearCompositedLayerMapping made in > applyUpdateLayerCompositingStateChickenEggHacks ignores the returned value (that > is, the value of compositedLayerMappingChanged). Issuing a repaint there when > the returned value is true fixes the bug too. Please do it there instead. That way it's contained to the chicken-egg hack case.
PTAL. https://codereview.chromium.org/336493004/diff/40001/LayoutTests/compositing/... File LayoutTests/compositing/layer-creation/compositing-reason-removed.html (right): https://codereview.chromium.org/336493004/diff/40001/LayoutTests/compositing/... LayoutTests/compositing/layer-creation/compositing-reason-removed.html:1: <style> On 2014/06/12 20:17:09, Ian Vollick wrote: > DOCTYPE Done. https://codereview.chromium.org/336493004/diff/40001/Source/core/rendering/co... File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/336493004/diff/40001/Source/core/rendering/co... Source/core/rendering/compositing/RenderLayerCompositor.cpp:437: repaintOnCompositingChange(layer); On 2014/06/12 20:37:29, chrishtr wrote: > On 2014/06/12 20:33:50, ajuma wrote: > > On 2014/06/12 20:17:10, Ian Vollick wrote: > > > This is interesting. When compositedLayerMappingChanged is set to true, the > > > CompositingLayerAssign adds it to a list of layers to repaint later on. Is > it > > > somehow invalid to be issuing deferred repaints? > > > > The call to allocateOrClearCompositedLayerMapping made in > > applyUpdateLayerCompositingStateChickenEggHacks ignores the returned value > (that > > is, the value of compositedLayerMappingChanged). Issuing a repaint there when > > the returned value is true fixes the bug too. > > Please do it there instead. That way it's contained to the chicken-egg hack > case. Done.
On 2014/06/12 21:08:00, ajuma wrote: > PTAL. > > https://codereview.chromium.org/336493004/diff/40001/LayoutTests/compositing/... > File LayoutTests/compositing/layer-creation/compositing-reason-removed.html > (right): > > https://codereview.chromium.org/336493004/diff/40001/LayoutTests/compositing/... > LayoutTests/compositing/layer-creation/compositing-reason-removed.html:1: > <style> > On 2014/06/12 20:17:09, Ian Vollick wrote: > > DOCTYPE > > Done. > > https://codereview.chromium.org/336493004/diff/40001/Source/core/rendering/co... > File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right): > > https://codereview.chromium.org/336493004/diff/40001/Source/core/rendering/co... > Source/core/rendering/compositing/RenderLayerCompositor.cpp:437: > repaintOnCompositingChange(layer); > On 2014/06/12 20:37:29, chrishtr wrote: > > On 2014/06/12 20:33:50, ajuma wrote: > > > On 2014/06/12 20:17:10, Ian Vollick wrote: > > > > This is interesting. When compositedLayerMappingChanged is set to true, > the > > > > CompositingLayerAssign adds it to a list of layers to repaint later on. Is > > it > > > > somehow invalid to be issuing deferred repaints? > > > > > > The call to allocateOrClearCompositedLayerMapping made in > > > applyUpdateLayerCompositingStateChickenEggHacks ignores the returned value > > (that > > > is, the value of compositedLayerMappingChanged). Issuing a repaint there > when > > > the returned value is true fixes the bug too. > > > > Please do it there instead. That way it's contained to the chicken-egg hack > > case. > > Done. lgtm
https://codereview.chromium.org/336493004/diff/60001/Source/core/rendering/co... File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/336493004/diff/60001/Source/core/rendering/co... Source/core/rendering/compositing/RenderLayerCompositor.cpp:486: repaintOnCompositingChange(layer); Also recompute repaint rects via computeRepaintRectsIncludingNonCompositingDescendants().
https://codereview.chromium.org/336493004/diff/60001/Source/core/rendering/co... File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/336493004/diff/60001/Source/core/rendering/co... Source/core/rendering/compositing/RenderLayerCompositor.cpp:486: repaintOnCompositingChange(layer); On 2014/06/12 21:09:54, chrishtr wrote: > Also recompute repaint rects via > computeRepaintRectsIncludingNonCompositingDescendants(). Done.
lgtm
https://codereview.chromium.org/336493004/diff/100001/Source/core/rendering/c... File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/336493004/diff/100001/Source/core/rendering/c... Source/core/rendering/compositing/RenderLayerCompositor.cpp:487: layer->repainter().computeRepaintRectsIncludingNonCompositingDescendants(); I also needed to make this call conditional on having a parent (matching the logic that used to exist in allocateOrClearCompositedLayerMapping for the case of allocating a new mapping) -- the chicken-egg compositing update can happen before a RenderObject is added to the render tree (see the logic in RenderTreeBuilder::createRendererForElementIfNeeded, where the compositing update happens during the call to setStyle, which happens before the RenderObject is added to its parent), but trying to compute repaint rects without a parent leads to a crash in RenderTableCell::clippedOverflowRectForRepaint, since this assumes a non-null parent.
On 2014/06/13 14:54:52, ajuma wrote: > https://codereview.chromium.org/336493004/diff/100001/Source/core/rendering/c... > File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right): > > https://codereview.chromium.org/336493004/diff/100001/Source/core/rendering/c... > Source/core/rendering/compositing/RenderLayerCompositor.cpp:487: > layer->repainter().computeRepaintRectsIncludingNonCompositingDescendants(); > I also needed to make this call conditional on having a parent (matching the > logic that used to exist in allocateOrClearCompositedLayerMapping for the case > of allocating a new mapping) -- the chicken-egg compositing update can happen > before a RenderObject is added to the render tree (see the logic in > RenderTreeBuilder::createRendererForElementIfNeeded, where the compositing > update happens during the call to setStyle, which happens before the > RenderObject is added to its parent), but trying to compute repaint rects > without a parent leads to a crash in > RenderTableCell::clippedOverflowRectForRepaint, since this assumes a non-null > parent. Makes sense. slgtm.
The CQ bit was checked by ajuma@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/336493004/100001
https://codereview.chromium.org/336493004/diff/100001/Source/core/rendering/c... File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/336493004/diff/100001/Source/core/rendering/c... Source/core/rendering/compositing/RenderLayerCompositor.cpp:486: if (layer->parent()) Please add a comment here about why the layer->parent() part is here, as well as why parent() will always be non-null in cases when the layer is attached. We had no idea why the old conditional was there, which is why I removed it.
The CQ bit was unchecked by ajuma@chromium.org
https://codereview.chromium.org/336493004/diff/100001/Source/core/rendering/c... File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/336493004/diff/100001/Source/core/rendering/c... Source/core/rendering/compositing/RenderLayerCompositor.cpp:486: if (layer->parent()) On 2014/06/13 15:45:38, chrishtr wrote: > Please add a comment here about why the layer->parent() part is here, as well as > why parent() will > always be non-null in cases when the layer is attached. We had no idea why the > old conditional was there, > which is why I removed it. Done.
lgtm
The CQ bit was checked by ajuma@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/336493004/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11947)
Message was sent while issue was closed.
Change committed as 176128 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
