Changeset 266803 in webkit


Ignore:
Timestamp:
Sep 9, 2020 3:45:45 PM (4 years ago)
Author:
Alan Bujtas
Message:

[Repaint] styleWillChange may call repaint on the same renderer multiple times.
​https://bugs.webkit.org/show_bug.cgi?id=216295
<rdar://problem/68538666>

Reviewed by Simon Fraser.

Source/WebCore:

RenderElement::styleWillChange is a virtual function. This function is called whenever the associated RenderStyle changes.
The subclass implementation (e.g. RenderBox::styleWillChange) calls the parent class to make sure the style change is covered properly.
Now in certain cases,

  1. this may trigger multiple calls to repaint() (e.g one in each ::styleWillChange implementation)
  2. paint invalidation requires absolute coordinates
  3. geometry does not change during styleWillChange

it could end up being redundant/unnecessarily expensive.

This patch moves all the style-will-change-requires-repaint logic to one single function so that we can limit the number of repaints.

  • rendering/RenderBox.cpp:

(WebCore::RenderBox::styleWillChange):

  • rendering/RenderElement.cpp:

(WebCore::RenderElement::issueRepaintBeforeStyleChange):
(WebCore::RenderElement::initializeStyle):
(WebCore::RenderElement::setStyle):
(WebCore::RenderElement::styleWillChange):

  • rendering/RenderElement.h:
  • rendering/RenderLayerModelObject.cpp:

(WebCore::RenderLayerModelObject::styleWillChange):

LayoutTests:

  • compositing/masks/compositing-clip-path-change-no-repaint-expected.txt:
  • compositing/shared-backing/overflow-scroll/shared-layer-repaint-expected.txt:
  • fast/css-custom-paint/delay-repaint-expected.txt:
  • fast/images/async-image-multiple-clients-repaint-expected.txt:
  • fast/repaint/focus-ring-repaint-expected.txt:
  • fast/repaint/horizontal-bt-overflow-child-expected.txt:
  • fast/repaint/horizontal-bt-overflow-parent-expected.txt:
  • fast/repaint/horizontal-bt-overflow-same-expected.txt:
  • fast/repaint/mutate-non-visible-expected.txt:
  • fast/repaint/negative-text-indent-with-overflow-hidden-expected.txt:
  • fast/repaint/overflow-flipped-writing-mode-table-expected.txt:
  • fast/repaint/table-row-repaint-expected.txt:
  • fast/repaint/vertical-overflow-child-expected.txt:
  • fast/repaint/vertical-overflow-parent-expected.txt:
  • fast/repaint/vertical-overflow-same-expected.txt:
  • svg/transforms/svg-transform-foreign-object-repaint-expected.txt:
Location:
trunk
Files:
27 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r266801 r266803  
     12020-09-09  Zalan Bujtas  <zalan@apple.com>
     2
     3        [Repaint] styleWillChange may call repaint on the same renderer multiple times.
     4        https://bugs.webkit.org/show_bug.cgi?id=216295
     5        <rdar://problem/68538666>
     6
     7        Reviewed by Simon Fraser.
     8
     9        * compositing/masks/compositing-clip-path-change-no-repaint-expected.txt:
     10        * compositing/shared-backing/overflow-scroll/shared-layer-repaint-expected.txt:
     11        * fast/css-custom-paint/delay-repaint-expected.txt:
     12        * fast/images/async-image-multiple-clients-repaint-expected.txt:
     13        * fast/repaint/focus-ring-repaint-expected.txt:
     14        * fast/repaint/horizontal-bt-overflow-child-expected.txt:
     15        * fast/repaint/horizontal-bt-overflow-parent-expected.txt:
     16        * fast/repaint/horizontal-bt-overflow-same-expected.txt:
     17        * fast/repaint/mutate-non-visible-expected.txt:
     18        * fast/repaint/negative-text-indent-with-overflow-hidden-expected.txt:
     19        * fast/repaint/overflow-flipped-writing-mode-table-expected.txt:
     20        * fast/repaint/table-row-repaint-expected.txt:
     21        * fast/repaint/vertical-overflow-child-expected.txt:
     22        * fast/repaint/vertical-overflow-parent-expected.txt:
     23        * fast/repaint/vertical-overflow-same-expected.txt:
     24        * svg/transforms/svg-transform-foreign-object-repaint-expected.txt:
     25
    1262020-09-09  Sam Weinig  <weinig@apple.com>
    227

  • trunk/LayoutTests/compositing/masks/compositing-clip-path-change-no-repaint-expected.txt

    r237942 r266803  
    3131                (rect 0.00 0.00 300.00 300.00)
    3232                (rect 0.00 0.00 300.00 300.00)
    33                 (rect 0.00 0.00 300.00 300.00)
    3433              )
    3534            )
    3635          (repaint rects
    37             (rect 0.00 0.00 300.00 300.00)
    3836            (rect 0.00 0.00 300.00 300.00)
    3937            (rect 0.00 0.00 300.00 300.00)

  • trunk/LayoutTests/compositing/shared-backing/overflow-scroll/shared-layer-repaint-expected.txt

    r245170 r266803  
    2020            (rect 151.00 81.00 71.00 110.00)
    2121            (rect 151.00 81.00 71.00 110.00)
    22             (rect 151.00 81.00 71.00 110.00)
    2322          )
    2423        )

  • trunk/LayoutTests/fast/css-custom-paint/animate-repaint-expected.txt

    r239067 r266803  
    33  (rect 8 8 150 150)
    44  (rect 8 8 150 150)
    5   (rect 8 8 150 150)
    65)
    76

  • trunk/LayoutTests/fast/css-custom-paint/delay-repaint-expected.txt

    r239067 r266803  
    33  (rect 8 8 150 150)
    44  (rect 8 8 150 150)
    5   (rect 8 8 150 150)
    65)
    76

  • trunk/LayoutTests/fast/images/async-image-multiple-clients-repaint-expected.txt

    r219045 r266803  
    66  (rect 8 344 200 100)
    77  (rect 8 344 200 100)
    8   (rect 8 344 200 100)
    98)
    109

  • trunk/LayoutTests/fast/repaint/focus-ring-repaint-expected.txt

    r260367 r266803  
    77
    88(repaint rects
    9   (rect 5 457 103 67)
    109  (rect 5 457 103 67)
    1110  (rect 5 457 106 70)

  • trunk/LayoutTests/fast/repaint/horizontal-bt-overflow-child-expected.txt

    r163021 r266803  
    11(repaint rects
    2   (rect 29 106 100 100)
    32  (rect 29 106 100 100)
    43  (rect 29 106 100 100)

  • trunk/LayoutTests/fast/repaint/horizontal-bt-overflow-parent-expected.txt

    r163021 r266803  
    11(repaint rects
    2   (rect 29 29 100 100)
    32  (rect 29 29 100 100)
    43  (rect 29 29 100 100)

  • trunk/LayoutTests/fast/repaint/horizontal-bt-overflow-same-expected.txt

    r163021 r266803  
    11(repaint rects
    2   (rect 29 21 100 100)
    32  (rect 29 21 100 100)
    43  (rect 29 21 100 100)

  • trunk/LayoutTests/fast/repaint/mutate-non-visible-expected.txt

    r215507 r266803  
    33  (rect 10 28 100 100)
    44  (rect 10 28 100 100)
    5   (rect 10 28 100 100)
    65)
    76

  • trunk/LayoutTests/fast/repaint/negative-text-indent-with-overflow-hidden-expected.txt

    r180441 r266803  
    33  (rect 550 8 200 50)
    44  (rect 550 8 200 50)
    5   (rect 550 8 200 50)
    65)
    76

  • trunk/LayoutTests/fast/repaint/overflow-flipped-writing-mode-table-expected.txt

    r163021 r266803  
    11(repaint rects
    2   (rect 8 8 100 50)
    32  (rect 8 8 100 50)
    43  (rect 8 8 100 50)

  • trunk/LayoutTests/fast/repaint/table-row-repaint-expected.txt

    r176124 r266803  
    11(repaint rects
    2   (rect 8 61 106 15)
    32  (rect 8 61 106 15)
    43  (rect 8 61 106 15)

  • trunk/LayoutTests/fast/repaint/vertical-overflow-child-expected.txt

    r163021 r266803  
    11(repaint rects
    2   (rect 214 21 100 100)
    32  (rect 214 21 100 100)
    43  (rect 214 21 100 100)

  • trunk/LayoutTests/fast/repaint/vertical-overflow-parent-expected.txt

    r163021 r266803  
    11(repaint rects
    2   (rect 29 29 100 100)
    32  (rect 29 29 100 100)
    43  (rect 29 29 100 100)

  • trunk/LayoutTests/fast/repaint/vertical-overflow-same-expected.txt

    r163021 r266803  
    11(repaint rects
    2   (rect 29 21 100 100)
    32  (rect 29 21 100 100)
    43  (rect 29 21 100 100)

  • trunk/LayoutTests/platform/ios-wk2/compositing/columns/composited-lr-paginated-repaint-expected.txt

    r237942 r266803  
    1515            (rect 27.00 25.00 52.00 77.00)
    1616            (rect 27.00 25.00 52.00 77.00)
    17             (rect 27.00 25.00 52.00 77.00)
    1817          )
    1918        )

  • trunk/LayoutTests/platform/ios-wk2/compositing/columns/composited-rl-paginated-repaint-expected.txt

    r237942 r266803  
    1717            (rect 27.00 25.00 52.00 77.00)
    1818            (rect 27.00 25.00 52.00 77.00)
    19             (rect 27.00 25.00 52.00 77.00)
    2019          )
    2120        )

  • trunk/LayoutTests/platform/ios-wk2/compositing/repaint/repaint-on-layer-grouping-change-expected.txt

    r237942 r266803  
    2727          (repaint rects
    2828            (rect 490.00 190.00 50.00 50.00)
    29             (rect 490.00 190.00 50.00 50.00)
    3029            (rect 0.00 0.00 50.00 50.00)
    3130          )

  • trunk/LayoutTests/platform/ios/fast/images/async-image-multiple-clients-repaint-expected.txt

    r258993 r266803  
    66  (rect 8 348 200 100)
    77  (rect 8 348 200 100)
    8   (rect 8 348 200 100)
    98)
    109

  • trunk/LayoutTests/svg/transforms/svg-transform-foreign-object-repaint-expected.txt

    r175847 r266803  
    11(repaint rects
    2   (rect 58 58 100 100)
    32  (rect 58 58 100 100)
    43  (rect 58 58 100 100)

  • trunk/Source/WebCore/ChangeLog

    r266801 r266803  
     12020-09-09  Zalan Bujtas  <zalan@apple.com>
     2
     3        [Repaint] styleWillChange may call repaint on the same renderer multiple times.
     4        https://bugs.webkit.org/show_bug.cgi?id=216295
     5        <rdar://problem/68538666>
     6
     7        Reviewed by Simon Fraser.
     8
     9        RenderElement::styleWillChange is a virtual function. This function is called whenever the associated RenderStyle changes.
     10        The subclass implementation (e.g. RenderBox::styleWillChange) calls the parent class to make sure the style change is covered properly.
     11        Now in certain cases,
     12        1. this may trigger multiple calls to repaint() (e.g one in each ::styleWillChange implementation)
     13        2. paint invalidation requires absolute coordinates
     14        3. geometry does not change during styleWillChange
     15        it could end up being redundant/unnecessarily expensive.
     16
     17        This patch moves all the style-will-change-requires-repaint logic to one single function so that we can limit the number of repaints.
     18
     19        * rendering/RenderBox.cpp:
     20        (WebCore::RenderBox::styleWillChange):
     21        * rendering/RenderElement.cpp:
     22        (WebCore::RenderElement::issueRepaintBeforeStyleChange):
     23        (WebCore::RenderElement::initializeStyle):
     24        (WebCore::RenderElement::setStyle):
     25        (WebCore::RenderElement::styleWillChange):
     26        * rendering/RenderElement.h:
     27        * rendering/RenderLayerModelObject.cpp:
     28        (WebCore::RenderLayerModelObject::styleWillChange):
     29
    1302020-09-09  Sam Weinig  <weinig@apple.com>
    231

  • trunk/Source/WebCore/rendering/RenderBox.cpp

    r266716 r266803  
    270270        if (diff == StyleDifference::Layout && parent() && oldStyle->position() != newStyle.position()) {
    271271            markContainingBlocksForLayout();
    272             if (oldStyle->position() == PositionType::Static)
    273                 repaint();
    274             else if (newStyle.hasOutOfFlowPosition())
     272            if (oldStyle->position() != PositionType::Static && newStyle.hasOutOfFlowPosition())
    275273                parent()->setChildNeedsLayout();
    276274            if (isFloating() && !isOutOfFlowPositioned() && newStyle.hasOutOfFlowPosition())

  • trunk/Source/WebCore/rendering/RenderElement.cpp

    r263762 r266803  
    381381}
    382382
     383void RenderElement::repaintBeforeStyleChange(StyleDifference diff, const RenderStyle& oldStyle, const RenderStyle& newStyle)
     384{
     385    auto repaintBeforeStyleChange = [&] {
     386        if (!parent()) {
     387            // Can't resolve absolute coordinates.
     388            return false;
     389        }
     390        if (shouldRepaintForStyleDifference(diff))
     391            return true;
     392        if (newStyle.outlineSize() < oldStyle.outlineSize())
     393            return true;
     394        if (is<RenderLayerModelObject>(*this)) {
     395            // If we don't have a layer yet, but we are going to get one because of transform or opacity, then we need to repaint the old position of the object.
     396            bool hasLayer = downcast<RenderLayerModelObject>(*this).hasLayer();
     397            bool willHaveLayer = newStyle.hasTransform() || newStyle.opacity() < 1 || newStyle.hasFilter() || newStyle.hasBackdropFilter();
     398            if (!hasLayer && willHaveLayer)
     399                return true;
     400        }
     401        if (is<RenderBox>(*this)) {
     402            if (diff == StyleDifference::Layout && oldStyle.position() != newStyle.position() && oldStyle.position() == PositionType::Static)
     403                return true;
     404        }
     405        if (diff > StyleDifference::RepaintLayer && oldStyle.visibility() != newStyle.visibility()) {
     406            if (auto* enclosingLayer = this->enclosingLayer()) {
     407                auto rendererWillBeHidden = newStyle.visibility() != Visibility::Visible;
     408                if (rendererWillBeHidden && enclosingLayer->hasVisibleContent() && (this == &enclosingLayer->renderer() || enclosingLayer->renderer().style().visibility() != Visibility::Visible))
     409                    return true;
     410            }
     411        }
     412        return false;
     413    }();
     414    if (repaintBeforeStyleChange)
     415        repaint();
     416}
     417
    383418void RenderElement::initializeStyle()
    384419{
    … …  
    414449    Style::loadPendingResources(style, document(), element());
    415450
     451    repaintBeforeStyleChange(diff, m_style, style);
    416452    styleWillChange(diff, style);
    417453    auto oldStyle = m_style.replace(WTFMove(style));
    … …  
    729765                if (newStyle.visibility() == Visibility::Visible)
    730766                    layer->setHasVisibleContent();
    731                 else if (layer->hasVisibleContent() && (this == &layer->renderer() || layer->renderer().style().visibility() != Visibility::Visible)) {
     767                else if (layer->hasVisibleContent() && (this == &layer->renderer() || layer->renderer().style().visibility() != Visibility::Visible))
    732768                    layer->dirtyVisibleContentStatus();
    733                     if (diff > StyleDifference::RepaintLayer)
    734                         repaint();
    735                 }
    736769            }
    737770        }
    … …  
    760793                layer->invalidateEventRegion(RenderLayer::EventRegionInvalidationReason::Style);
    761794        }
    762 
    763         if (m_parent && (newStyle.outlineSize() < m_style.outlineSize() || shouldRepaintForStyleDifference(diff)))
    764             repaint();
    765795
    766796        if (isFloating() && m_style.floating() != newStyle.floating()) {

  • trunk/Source/WebCore/rendering/RenderElement.h

    r261637 r266803  
    260260    void setLastChild(RenderObject* child) { m_lastChild = child; }
    261261
     262    void repaintBeforeStyleChange(StyleDifference, const RenderStyle& oldStyle, const RenderStyle& newStyle);
     263
    262264    virtual void styleWillChange(StyleDifference, const RenderStyle& newStyle);
    263265    virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle);

  • trunk/Source/WebCore/rendering/RenderLayerModelObject.cpp

    r261637 r266803  
    125125                if (!(oldStyle->clip() == newStyle.clip()))
    126126                    layer()->clearClipRectsIncludingDescendants();
    127             } else if (diff == StyleDifference::Repaint || newStyle.outlineSize() < oldStyle->outlineSize())
    128                 repaint();
     127            }
    129128        }
    130129
    … …  
    142141                    )
    143142                layer()->repaintIncludingDescendants();
    144             } else if (newStyle.hasTransform() || newStyle.opacity() < 1 || newStyle.hasFilter() || newStyle.hasBackdropFilter()) {
    145                 // If we don't have a layer yet, but we are going to get one because of transform or opacity,
    146                 //  then we need to repaint the old position of the object.
    147                 repaint();
    148143            }
    149144        }
Note: See TracChangeset for help on using the changeset viewer.