Bug 142687 – Parsing and Style Resolution of Container-based Animation Triggers

• Component: Animations
• Status: RESOLVED FIXED

[Dean Jackson]

Prototype an implementation of animation-trigger, as described by
https://lists.w3.org/Archives/Public/www-style/2014Sep/0135.html

[Radar WebKit Bug Importer]

<rdar://problem/20162335>

[Dean Jackson]

Created attachment 248632 [details]
WIP

[WebKit Commit Bot]

Attachment 248632 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSToStyleMap.cpp:529:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/animation/Animation.cpp:138:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/animation/Animation.cpp:151:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1229:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/animation/Animation.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/animation/Animation.h:189:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 7 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Created attachment 248714 [details]
Patch

[WebKit Commit Bot]

Attachment 248714 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/animation/Animation.h:202:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1236:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1237:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

(In reply to comment #5)
> Attachment 248714 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/platform/animation/Animation.h:202:  Please declare
> enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
> ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1236:  Weird
> number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1237:  Weird
> number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> Total errors found: 3 in 21 files
>

I don't like either of those warnings. I'm going to file a bug for the first one at least.

[Dean Jackson]

Created attachment 248730 [details]
Patch

[WebKit Commit Bot]

Attachment 248730 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/animation/Animation.h:202:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1236:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1237:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 248730 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248730&action=review

> Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:37
> +    if (m_hasEndValue)
> +        return "container-scroll(" + m_startValue->cssText() + ", " + m_endValue->cssText() + ')';
> +    return "container-scroll(" + m_startValue->cssText() + ')';

This should probably use StringBuilder.

> Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:42
> +    return m_startValue->equals(*other.m_startValue.get()) && m_endValue->equals(*other.m_endValue.get()) && m_hasEndValue == other.m_hasEndValue;

Check m_hasEndValue == other.m_hasEndValue before comparing m_endValues?

> Source/WebCore/css/CSSAnimationTriggerScrollValue.h:44
> +    CSSValue* startValue() const { return m_startValue.get(); }
> +    CSSValue* endValue() const { return m_endValue.get(); }

Return const CSSValue*

> Source/WebCore/css/CSSAnimationTriggerScrollValue.h:62
> +    RefPtr<CSSValue> m_endValue;
> +    bool m_hasEndValue;

Do you need to store m_hasEndValue rather than just null-check m_endValue?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1230
> +static Ref<CSSValue> createAnimationTriggerValue(const AnimationTrigger* trigger, const RenderStyle* style)

Pass AnimationTrigger by reference.

> Source/WebCore/css/CSSParser.cpp:4768
> +        if (!validateUnit(firstArgumentWithCalculation, FLength))

Has to be a length? No percentages?

> Source/WebCore/css/CSSToStyleMap.h:71
> +    void mapAnimationTrigger(Animation&, CSSValue&);

These CSSValue& should all be const :|

> Source/WebCore/platform/animation/Animation.h:141
> +    const PassRefPtr<AnimationTrigger> trigger() const { return m_trigger; }

This should return a raw const*. You're not passing off ownership.

> Source/WebCore/platform/animation/Animation.h:155
> +    void setTrigger(PassRefPtr<AnimationTrigger> f) { m_trigger = f; m_triggerSet = true; }

f? Do you want to do m_triggerSet = true if the trigger is null?

> Source/WebCore/platform/animation/AnimationTrigger.h:42
> +    enum AnimationTriggerType {

enum class.

> Source/WebCore/platform/animation/AnimationTrigger.h:101
> +        if (other.isScrollAnimationTrigger()) {

Early return would be better.

> Source/WebCore/platform/animation/AnimationTrigger.h:105
> +            const ScrollAnimationTrigger* otherTrigger = static_cast<const ScrollAnimationTrigger*>(&other);
> +            return m_startValue == otherTrigger->m_startValue
> +                && m_endValue == otherTrigger->m_endValue
> +                && m_hasEndValue == otherTrigger->m_hasEndValue;

Doesn't ScrollAnimationTrigger have an operator== you can use?

[Dean Jackson]

Comment on attachment 248730 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248730&action=review

>> Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:37
>> +    return "container-scroll(" + m_startValue->cssText() + ')';
> 
> This should probably use StringBuilder.

Done. Now:

    StringBuilder result;
    result.append("container-scroll(");
    result.append(m_startValue->cssText());
    if (m_hasEndValue) {
        result.append(", ");
        result.append(m_endValue->cssText());
    }
    result.append(')');
    return result.toString();

>> Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:42
>> +    return m_startValue->equals(*other.m_startValue.get()) && m_endValue->equals(*other.m_endValue.get()) && m_hasEndValue == other.m_hasEndValue;
> 
> Check m_hasEndValue == other.m_hasEndValue before comparing m_endValues?

Done.

>> Source/WebCore/css/CSSAnimationTriggerScrollValue.h:44
>> +    CSSValue* endValue() const { return m_endValue.get(); }
> 
> Return const CSSValue*

Done

>> Source/WebCore/css/CSSAnimationTriggerScrollValue.h:62
>> +    bool m_hasEndValue;
> 
> Do you need to store m_hasEndValue rather than just null-check m_endValue?

Removed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1230
>> +static Ref<CSSValue> createAnimationTriggerValue(const AnimationTrigger* trigger, const RenderStyle* style)
> 
> Pass AnimationTrigger by reference.

I don't think there is any need to do this, and the rest of CSSComputedStyleDeclaration passes around the const pointers here.

>> Source/WebCore/css/CSSParser.cpp:4768
>> +        if (!validateUnit(firstArgumentWithCalculation, FLength))
> 
> Has to be a length? No percentages?

For now it is a length. I'll add percentages soon (and you'll definitely want them).

>> Source/WebCore/css/CSSToStyleMap.h:71
>> +    void mapAnimationTrigger(Animation&, CSSValue&);
> 
> These CSSValue& should all be const :|

Separate patch.

>> Source/WebCore/platform/animation/Animation.h:141
>> +    const PassRefPtr<AnimationTrigger> trigger() const { return m_trigger; }
> 
> This should return a raw const*. You're not passing off ownership.

We can't do this due to the way StyleBuilder works. It expects to get and set RefPtrs.

>> Source/WebCore/platform/animation/Animation.h:155
>> +    void setTrigger(PassRefPtr<AnimationTrigger> f) { m_trigger = f; m_triggerSet = true; }
> 
> f? Do you want to do m_triggerSet = true if the trigger is null?

Yeah - we want to know if it is ever been changed from the initial value.

>> Source/WebCore/platform/animation/AnimationTrigger.h:42
>> +    enum AnimationTriggerType {
> 
> enum class.

Done

>> Source/WebCore/platform/animation/AnimationTrigger.h:105
>> +                && m_hasEndValue == otherTrigger->m_hasEndValue;
> 
> Doesn't ScrollAnimationTrigger have an operator== you can use?

This is ScrollAnimationTrigger::operator== :)

[Dean Jackson]

Committed r181572: <http://trac.webkit.org/changeset/181572>

[Alexey Proskuryakov]

Looks like this broke a test:

https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=transitions%2Ftransitions-parsing.html

[Alexey Proskuryakov]

Also this one: https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=animations%2Fanimation-shorthand.html

[Chris Dumez]

And now this one:
https://build.webkit.org/results/Apple%20Yosemite%20Release%20WK2%20(Tests)/r181581%20(3643)/animations/fill-unset-properties-pretty-diff.html

Every time, the parsed list values are in wrong order and on different tests. It looks like this patch caused weird side effects.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 142755

[Dean Jackson]

Landed again in r181602