Summary: | [Repaint] RenderLayerModelObject::styleWillChange may issue redundant repaint | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | zalan <zalan> | ||||||||
Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bfulgham, changseok, esprehn+autocc, ews-watchlist, fred.wang, glenn, kondapallykalyan, pdr, simon.fraser, webkit-bug-importer, zalan | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
zalan
2020-09-10 12:19:36 PDT
Created attachment 408467 [details]
Patch
EWS testing first. Comment on attachment 408467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408467&action=review > Source/WebCore/rendering/RenderElement.cpp:389 > + enum RepaintCoverage { None, RendererOnly, RendererAndDescendantsRenderersWithLayers }; RepaintCoverage is a bit ambiguous; it's not clear if it's "repaints done" or "repaints that need to be done". Maybe call it RequiredRepaint? > Source/WebCore/rendering/RenderElement.cpp:400 > + // When a layout hint happens, we do a repaint of the layer, since the layer could end up being destroyed. "layout hint" seems like old terminology. > Source/WebCore/rendering/RenderElement.cpp:401 > + auto layerMayGetDestroyed = oldStyle.position() != newStyle.position() || oldStyle.usedZIndex() != newStyle.usedZIndex() || oldStyle.clip() != newStyle.clip() || oldStyle.hasClip() != newStyle.hasClip() We can clearly do a better job of testing which style changes will result in the layer actually being destroyed. (In reply to Simon Fraser (smfr) from comment #4) > Comment on attachment 408467 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=408467&action=review > > > Source/WebCore/rendering/RenderElement.cpp:389 > > + enum RepaintCoverage { None, RendererOnly, RendererAndDescendantsRenderersWithLayers }; > > RepaintCoverage is a bit ambiguous; it's not clear if it's "repaints done" > or "repaints that need to be done". Maybe call it RequiredRepaint? Yeah, that sounds better. > > > Source/WebCore/rendering/RenderElement.cpp:400 > > + // When a layout hint happens, we do a repaint of the layer, since the layer could end up being destroyed. > > "layout hint" seems like old terminology. Will remove. > > > Source/WebCore/rendering/RenderElement.cpp:401 > > + auto layerMayGetDestroyed = oldStyle.position() != newStyle.position() || oldStyle.usedZIndex() != newStyle.usedZIndex() || oldStyle.clip() != newStyle.clip() || oldStyle.hasClip() != newStyle.hasClip() > > We can clearly do a better job of testing which style changes will result in > the layer actually being destroyed. I am sure we can. I don't think it should be part of this patch though. Created attachment 408479 [details]
Patch
Comment on attachment 408479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408479&action=review > Source/WebCore/rendering/RenderElement.cpp:401 > + auto layerMayGetDestroyed = oldStyle.position() != newStyle.position() || oldStyle.usedZIndex() != newStyle.usedZIndex() || oldStyle.clip() != newStyle.clip() || oldStyle.hasClip() != newStyle.hasClip() > + || oldStyle.hasAutoUsedZIndex() != newStyle.hasAutoUsedZIndex() || oldStyle.opacity() != newStyle.opacity() || oldStyle.transform() != newStyle.transform() || oldStyle.filter() != newStyle.filter(); I prefer the more wrapped version but ¯\_(ツ)_/¯ Created attachment 408482 [details]
Patch
Committed r266901: <https://trac.webkit.org/changeset/266901> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408482 [details]. |