Changeset 190879 in webkit


Ignore:
Timestamp:
Oct 12, 2015 12:15:28 PM (9 years ago)
Author:
Simon Fraser
Message:

Clip-path transitions sometimes trigger endless animation timers
​https://bugs.webkit.org/show_bug.cgi?id=150018

Reviewed by Tim Horton.

Source/WebCore:

Transitioning -webkit-clip-path could trigger endless animation
timers, because when CompositeAnimation::updateTransitions() calls
isTargetPropertyEqual(), a false negative answer triggers canceling the
current transition and starting a new one over and over.

This happened because StyleRareNonInheritedData simply tested pointer
equality for m_clipPath and m_shapeOutside. Both of these need to do deep
equality testing, requiring the implementation of operator== in BasicShapes
classes.

In addition, the PropertyWrappers in CSSPropertyAnimation need equals()
implementations that also do more than pointer equality testing.

Tests: transitions/clip-path-transitions.html

transitions/shape-outside-transitions.html

  • page/animation/CSSPropertyAnimation.cpp:

(WebCore::PropertyWrapperClipPath::equals):
(WebCore::PropertyWrapperShape::equals):

  • rendering/ClipPathOperation.h:
  • rendering/style/BasicShapes.cpp:

(WebCore::BasicShapeCircle::operator==):
(WebCore::BasicShapeEllipse::operator==):
(WebCore::BasicShapePolygon::operator==):
(WebCore::BasicShapeInset::operator==):

  • rendering/style/BasicShapes.h:

(WebCore::BasicShapeCenterCoordinate::operator==):
(WebCore::BasicShapeRadius::operator==):

  • rendering/style/ShapeValue.cpp:

(WebCore::pointersOrValuesEqual):
(WebCore::ShapeValue::operator==):

  • rendering/style/ShapeValue.h:

(WebCore::ShapeValue::operator!=):
(WebCore::ShapeValue::operator==): Deleted.
(WebCore::ShapeValue::ShapeValue): Deleted.

  • rendering/style/StyleRareNonInheritedData.cpp:

(WebCore::StyleRareNonInheritedData::operator==):
(WebCore::StyleRareNonInheritedData::clipPathOperationsEquivalent):
(WebCore::StyleRareNonInheritedData::shapeOutsideDataEquivalent):

  • rendering/style/StyleRareNonInheritedData.h:

LayoutTests:

New tests for transitions of clip-path and shape-outside.

  • transitions/clip-path-transitions-expected.txt: Added.
  • transitions/clip-path-transitions.html: Added.
  • transitions/resources/transition-test-helpers.js:

(parseClipPath):
(checkExpectedValue):

  • transitions/shape-outside-transitions-expected.txt: Added.
  • transitions/shape-outside-transitions.html: Added.
  • transitions/svg-transitions-expected.txt:
Location:
trunk
Files:
4 added
12 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r190877 r190879  
     12015-10-12  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Clip-path transitions sometimes trigger endless animation timers
     4        https://bugs.webkit.org/show_bug.cgi?id=150018
     5
     6        Reviewed by Tim Horton.
     7       
     8        New tests for transitions of clip-path and shape-outside.
     9
     10        * transitions/clip-path-transitions-expected.txt: Added.
     11        * transitions/clip-path-transitions.html: Added.
     12        * transitions/resources/transition-test-helpers.js:
     13        (parseClipPath):
     14        (checkExpectedValue):
     15        * transitions/shape-outside-transitions-expected.txt: Added.
     16        * transitions/shape-outside-transitions.html: Added.
     17        * transitions/svg-transitions-expected.txt:
     18
    1192015-10-12  Ryan Haddad  <ryanhaddad@apple.com>
    220

  • trunk/LayoutTests/transitions/resources/transition-test-helpers.js

    r187012 r190879  
    6868
    6969    return {"from": matches[1], "to": matches[2], "percent": parseFloat(matches[3])}
     70}
     71
     72function parseClipPath(s)
     73{
     74    // FIXME: This only matches a subset of the shape syntax, and the polygon expects 4 points.
     75    var patterns = [
     76        /inset\(([\d.]+)\w+ ([\d.]+)\w+\)/,
     77        /circle\(([\d.]+)\w+ at ([\d.]+)\w+ ([\d.]+)\w+\)/,
     78        /ellipse\(([\d.]+)\w+ ([\d.]+)\w+ at ([\d.]+)\w+ ([\d.]+)\w+\)/,
     79        /polygon\(([\d.]+)\w* ([\d.]+)\w*\, ([\d.]+)\w* ([\d.]+)\w*\, ([\d.]+)\w* ([\d.]+)\w*\, ([\d.]+)\w* ([\d.]+)\w*\)/
     80    ];
     81   
     82    for (pattern of patterns) {
     83        if (matchResult = s.match(pattern)) {
     84            var result = [];
     85            for (var i = 1; i < matchResult.length; ++i)
     86                result.push(parseFloat(matchResult[i]));
     87            return result;
     88        }
     89    }
     90
     91    window.console.log('failed to match ' + s);
     92    return null;
    7093}
    7194
    … …  
    137160            pass = isCloseEnough(computedCrossFade.percent, expectedValue, tolerance);
    138161        }
     162    } else if (property == "-webkit-clip-path" || property == "-webkit-shape-outside") {
     163        computedValue = window.getComputedStyle(document.getElementById(elementId)).getPropertyCSSValue(property).cssText;
     164
     165        var expectedValues = parseClipPath(expectedValue);
     166        var values = parseClipPath(computedValue);
     167       
     168        pass = false;
     169        if (values && values.length == expectedValues.length) {
     170            pass = true
     171            for (var i = 0; i < values.length; ++i)
     172                pass &= isCloseEnough(values[i], expectedValues[i], tolerance);
     173        }
    139174    } else {
    140175        var computedStyle = window.getComputedStyle(document.getElementById(elementId)).getPropertyCSSValue(property);

  • trunk/LayoutTests/transitions/svg-transitions-expected.txt

    r189646 r190879  
    1 CONSOLE MESSAGE: line 240: Failed to pause 'fill' transition on element 'rect7'
     1CONSOLE MESSAGE: line 275: Failed to pause 'fill' transition on element 'rect7'
    22Example
    33PASS - "fill-opacity" property for "rect1" element at 1s saw something close to: 0.6

  • trunk/Source/WebCore/ChangeLog

    r190876 r190879  
     12015-10-12  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Clip-path transitions sometimes trigger endless animation timers
     4        https://bugs.webkit.org/show_bug.cgi?id=150018
     5
     6        Reviewed by Tim Horton.
     7       
     8        Transitioning -webkit-clip-path could trigger endless animation
     9        timers, because when CompositeAnimation::updateTransitions() calls
     10        isTargetPropertyEqual(), a false negative answer triggers canceling the
     11        current transition and starting a new one over and over.
     12       
     13        This happened because StyleRareNonInheritedData simply tested pointer
     14        equality for m_clipPath and m_shapeOutside. Both of these need to do deep
     15        equality testing, requiring the implementation of operator== in BasicShapes
     16        classes.
     17       
     18        In addition, the PropertyWrappers in CSSPropertyAnimation need equals()
     19        implementations that also do more than pointer equality testing.
     20
     21        Tests: transitions/clip-path-transitions.html
     22               transitions/shape-outside-transitions.html
     23
     24        * page/animation/CSSPropertyAnimation.cpp:
     25        (WebCore::PropertyWrapperClipPath::equals):
     26        (WebCore::PropertyWrapperShape::equals):
     27        * rendering/ClipPathOperation.h:
     28        * rendering/style/BasicShapes.cpp:
     29        (WebCore::BasicShapeCircle::operator==):
     30        (WebCore::BasicShapeEllipse::operator==):
     31        (WebCore::BasicShapePolygon::operator==):
     32        (WebCore::BasicShapeInset::operator==):
     33        * rendering/style/BasicShapes.h:
     34        (WebCore::BasicShapeCenterCoordinate::operator==):
     35        (WebCore::BasicShapeRadius::operator==):
     36        * rendering/style/ShapeValue.cpp:
     37        (WebCore::pointersOrValuesEqual):
     38        (WebCore::ShapeValue::operator==):
     39        * rendering/style/ShapeValue.h:
     40        (WebCore::ShapeValue::operator!=):
     41        (WebCore::ShapeValue::operator==): Deleted.
     42        (WebCore::ShapeValue::ShapeValue): Deleted.
     43        * rendering/style/StyleRareNonInheritedData.cpp:
     44        (WebCore::StyleRareNonInheritedData::operator==):
     45        (WebCore::StyleRareNonInheritedData::clipPathOperationsEquivalent):
     46        (WebCore::StyleRareNonInheritedData::shapeOutsideDataEquivalent):
     47        * rendering/style/StyleRareNonInheritedData.h:
     48
    1492015-10-12  Myles C. Maxfield  <mmaxfield@apple.com>
    250

  • trunk/Source/WebCore/page/animation/CSSPropertyAnimation.cpp

    r188647 r190879  
    478478    {
    479479    }
     480
     481    virtual bool equals(const RenderStyle* a, const RenderStyle* b) const
     482    {
     483        // If the style pointers are the same, don't bother doing the test.
     484        // If either is null, return false. If both are null, return true.
     485        if (a == b)
     486            return true;
     487        if (!a || !b)
     488            return false;
     489
     490        ClipPathOperation* clipPathA = (a->*m_getter)();
     491        ClipPathOperation* clipPathB = (b->*m_getter)();
     492        if (clipPathA == clipPathB)
     493            return true;
     494        if (!clipPathA || !clipPathB)
     495            return false;
     496        return *clipPathA == *clipPathB;
     497    }
    480498};
    481499
    … …  
    487505        : RefCountedPropertyWrapper<ShapeValue>(prop, getter, setter)
    488506    {
     507    }
     508
     509    virtual bool equals(const RenderStyle* a, const RenderStyle* b) const
     510    {
     511        // If the style pointers are the same, don't bother doing the test.
     512        // If either is null, return false. If both are null, return true.
     513        if (a == b)
     514            return true;
     515        if (!a || !b)
     516            return false;
     517
     518        ShapeValue* shapeA = (a->*m_getter)();
     519        ShapeValue* shapeB = (b->*m_getter)();
     520        if (shapeA == shapeB)
     521            return true;
     522        if (!shapeA || !shapeB)
     523            return false;
     524        return *shapeA == *shapeB;
    489525    }
    490526};

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

    r185343 r190879  
    7676
    7777private:
    78     virtual bool operator==(const ClipPathOperation& o) const override
     78    virtual bool operator==(const ClipPathOperation& other) const override
    7979    {
    80         if (!isSameType(o))
     80        if (!isSameType(other))
    8181            return false;
    82         const ReferenceClipPathOperation* other = static_cast<const ReferenceClipPathOperation*>(&o);
    83         return m_url == other->m_url;
     82        auto& referenceClip = downcast<ReferenceClipPathOperation>(other);
     83        return m_url == referenceClip.m_url;
    8484    }
    8585
    … …  
    119119        if (!isSameType(other))
    120120            return false;
    121         const auto& shapeClip = downcast<ShapeClipPathOperation>(other);
    122         return m_shape.ptr() == shapeClip.m_shape.ptr();
     121        auto& shapeClip = downcast<ShapeClipPathOperation>(other);
     122        return m_referenceBox == shapeClip.referenceBox()
     123            && (m_shape.ptr() == shapeClip.m_shape.ptr() || m_shape.get() == shapeClip.m_shape.get());
    123124    }
    124125
    … …  
    150151
    151152private:
    152     virtual bool operator==(const ClipPathOperation& o) const override
     153    virtual bool operator==(const ClipPathOperation& other) const override
    153154    {
    154         if (!isSameType(o))
     155        if (!isSameType(other))
    155156            return false;
    156         const BoxClipPathOperation* other = static_cast<const BoxClipPathOperation*>(&o);
    157         return m_referenceBox == other->m_referenceBox;
     157        auto& boxClip = downcast<BoxClipPathOperation>(other);
     158        return m_referenceBox == boxClip.m_referenceBox;
    158159    }
    159160

  • trunk/Source/WebCore/rendering/style/BasicShapes.cpp

    r182560 r190879  
    8989}
    9090
     91
     92bool BasicShapeCircle::operator==(const BasicShape& other) const
     93{
     94    if (type() != other.type())
     95        return false;
     96
     97    const auto& otherCircle = downcast<BasicShapeCircle>(other);
     98    return m_centerX == otherCircle.m_centerX
     99        && m_centerY == otherCircle.m_centerY
     100        && m_radius == otherCircle.m_radius;
     101}
     102
    91103float BasicShapeCircle::floatValueForRadiusInBox(float boxWidth, float boxHeight) const
    92104{
    … …  
    133145}
    134146
     147bool BasicShapeEllipse::operator==(const BasicShape& other) const
     148{
     149    if (type() != other.type())
     150        return false;
     151
     152    const auto& otherEllipse = downcast<BasicShapeEllipse>(other);
     153    return m_centerX == otherEllipse.m_centerX
     154        && m_centerY == otherEllipse.m_centerY
     155        && m_radiusX == otherEllipse.m_radiusX
     156        && m_radiusY == otherEllipse.m_radiusY;
     157}
     158
    135159float BasicShapeEllipse::floatValueForRadiusInBox(const BasicShapeRadius& radius, float center, float boxWidthOrHeight) const
    136160{
    … …  
    183207}
    184208
     209bool BasicShapePolygon::operator==(const BasicShape& other) const
     210{
     211    if (type() != other.type())
     212        return false;
     213
     214    const auto& otherPolygon = downcast<BasicShapePolygon>(other);
     215    return m_windRule == otherPolygon.m_windRule
     216        && m_values == otherPolygon.m_values;
     217}
     218
    185219void BasicShapePolygon::path(Path& path, const FloatRect& boundingBox)
    186220{
    … …  
    222256
    223257    return result.releaseNonNull();
     258}
     259
     260bool BasicShapeInset::operator==(const BasicShape& other) const
     261{
     262    if (type() != other.type())
     263        return false;
     264
     265    const auto& otherInset = downcast<BasicShapeInset>(other);
     266    return m_right == otherInset.m_right
     267        && m_top == otherInset.m_top
     268        && m_bottom == otherInset.m_bottom
     269        && m_left == otherInset.m_left
     270        && m_topLeftRadius == otherInset.m_topLeftRadius
     271        && m_topRightRadius == otherInset.m_topRightRadius
     272        && m_bottomRightRadius == otherInset.m_bottomRightRadius
     273        && m_bottomLeftRadius == otherInset.m_bottomLeftRadius;
    224274}
    225275

  • trunk/Source/WebCore/rendering/style/BasicShapes.h

    r177733 r190879  
    6464
    6565    virtual Type type() const = 0;
     66    virtual bool operator==(const BasicShape&) const = 0;
    6667};
    6768
    … …  
    101102    {
    102103        return BasicShapeCenterCoordinate(TopLeft, m_computedLength.blend(other.m_computedLength, progress));
     104    }
     105   
     106    bool operator==(const BasicShapeCenterCoordinate& other) const
     107    {
     108        return m_direction == other.m_direction
     109            && m_length == other.m_length
     110            && m_computedLength == other.m_computedLength;
    103111    }
    104112
    … …  
    139147        return BasicShapeRadius(m_value.blend(other.value(), progress));
    140148    }
     149   
     150    bool operator==(const BasicShapeRadius& other) const
     151    {
     152        return m_value == other.m_value && m_type == other.m_type;
     153    }
    141154
    142155private:
    … …  
    163176
    164177    virtual Type type() const override { return BasicShapeCircleType; }
     178    virtual bool operator==(const BasicShape&) const override;
     179
    165180private:
    166181    BasicShapeCircle() { }
    … …  
    190205
    191206    virtual Type type() const override { return BasicShapeEllipseType; }
     207    virtual bool operator==(const BasicShape&) const override;
     208
    192209private:
    193210    BasicShapeEllipse() { }
    … …  
    216233
    217234    virtual Type type() const override { return BasicShapePolygonType; }
     235    virtual bool operator==(const BasicShape&) const override;
     236
    218237private:
    219238    BasicShapePolygon()
    … …  
    253272
    254273    virtual Type type() const override { return BasicShapeInsetType; }
     274    virtual bool operator==(const BasicShape&) const override;
     275
    255276private:
    256277    BasicShapeInset() { }

  • trunk/Source/WebCore/rendering/style/ShapeValue.cpp

    r169904 r190879  
    4040}
    4141
     42template <typename T>
     43bool pointersOrValuesEqual(T p1, T p2)
     44{
     45    if (p1 == p2)
     46        return true;
     47   
     48    if (!p1 || !p2)
     49        return false;
     50   
     51    return *p1 == *p2;
     52}
     53
     54bool ShapeValue::operator==(const ShapeValue& other) const
     55{
     56    if (m_type != other.m_type || m_cssBox != other.m_cssBox)
     57        return false;
     58
     59    return pointersOrValuesEqual(m_shape.get(), other.m_shape.get())
     60        && pointersOrValuesEqual(m_image.get(), other.m_image.get());
     61}
     62
     63
    4264} // namespace WebCore

  • trunk/Source/WebCore/rendering/style/ShapeValue.h

    r184147 r190879  
    7777    }
    7878
    79     bool operator==(const ShapeValue& other) const { return type() == other.type(); }
     79    bool operator==(const ShapeValue&) const;
     80    bool operator!=(const ShapeValue& other) const
     81    {
     82        return !(*this == other);
     83    }
    8084
    8185private:
    … …  
    8892    ShapeValue(Type type)
    8993        : m_type(type)
    90         , m_cssBox(BoxMissing)
    9194    {
    9295    }
    … …  
    9497        : m_type(Type::Image)
    9598        , m_image(image)
    96         , m_cssBox(BoxMissing)
    9799    {
    98100    }
    … …  
    107109    RefPtr<BasicShape> m_shape;
    108110    RefPtr<StyleImage> m_image;
    109     CSSBoxType m_cssBox;
     111    CSSBoxType m_cssBox { BoxMissing };
    110112};
    111113

  • trunk/Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp

    r188512 r190879  
    244244        && m_pageSize == o.m_pageSize
    245245#if ENABLE(CSS_SHAPES)
    246         && m_shapeOutside == o.m_shapeOutside
     246        && shapeOutsideDataEquivalent(o)
    247247        && m_shapeMargin == o.m_shapeMargin
    248248        && m_shapeImageThreshold == o.m_shapeImageThreshold
    249249#endif
    250         && m_clipPath == o.m_clipPath // FIXME: This needs to compare values.
     250        && clipPathOperationsEquivalent(o)
    251251        && m_textDecorationColor == o.m_textDecorationColor
    252252        && m_visitedLinkTextDecorationColor == o.m_visitedLinkTextDecorationColor
    … …  
    364364}
    365365
     366bool StyleRareNonInheritedData::clipPathOperationsEquivalent(const StyleRareNonInheritedData& o) const
     367{
     368    if ((!m_clipPath && o.m_clipPath) || (m_clipPath && !o.m_clipPath))
     369        return false;
     370    if (m_clipPath && o.m_clipPath && (*m_clipPath != *o.m_clipPath))
     371        return false;
     372    return true;
     373}
     374
     375#if ENABLE(CSS_SHAPES)
     376bool StyleRareNonInheritedData::shapeOutsideDataEquivalent(const StyleRareNonInheritedData& o) const
     377{
     378    if ((!m_shapeOutside && o.m_shapeOutside) || (m_shapeOutside && !o.m_shapeOutside))
     379        return false;
     380    if (m_shapeOutside && o.m_shapeOutside && (*m_shapeOutside != *o.m_shapeOutside))
     381        return false;
     382    return true;
     383}
     384#endif
     385
    366386bool StyleRareNonInheritedData::hasFilters() const
    367387{

  • trunk/Source/WebCore/rendering/style/StyleRareNonInheritedData.h

    r188512 r190879  
    9999    bool animationDataEquivalent(const StyleRareNonInheritedData&) const;
    100100    bool transitionDataEquivalent(const StyleRareNonInheritedData&) const;
     101    bool clipPathOperationsEquivalent(const StyleRareNonInheritedData&) const;
     102#if ENABLE(CSS_SHAPES)
     103    bool shapeOutsideDataEquivalent(const StyleRareNonInheritedData&) const;
     104#endif
    101105    bool hasFilters() const;
    102106#if ENABLE(FILTERS_LEVEL_2)
Note: See TracChangeset for help on using the changeset viewer.