Bug 234812

Summary:

Refactor code creating css values and lists for animation and transition properties

Product:

WebKit

Reporter:

Antoine Quint <graouts>

Component:

Animations

Assignee:

Antoine Quint <graouts>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, dino, esprehn+autocc, ews-watchlist, glenn, graouts, gyuyoung.kim, koivisto, macpherson, menard, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=234792

Attachments:

Description	Flags
Patch	koivisto: review+

Description Antoine Quint 2022-01-03 04:13:29 PST

Refactor code creating css values and lists for animation and transition properties

Comment 1 Antoine Quint 2022-01-03 04:14:29 PST

This is work that started from a comment made by Darin in bug 234792.

Comment 2 Antoine Quint 2022-01-03 04:21:18 PST

Created attachment 448225 [details]
Patch

Comment 3 Antti Koivisto 2022-01-03 04:29:21 PST

Comment on attachment 448225 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1450
> +void ComputedStyleExtractor::addCSSValueForAnimationPropertyToList(CSSPropertyID property, const Animation* animation, CSSValueList& list)

I'd make the target list the first argument.

Comment 4 Antti Koivisto 2022-01-03 04:31:31 PST

Comment on attachment 448225 [details]
Patch

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

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1450
>> +void ComputedStyleExtractor::addCSSValueForAnimationPropertyToList(CSSPropertyID property, const Animation* animation, CSSValueList& list)
> 
> I'd make the target list the first argument.

Also "CSS" in the name is bit unnecessary.

Comment 5 Antoine Quint 2022-01-03 05:51:20 PST

Committed r287537 (245672@trunk): <https://commits.webkit.org/245672@trunk>

Comment 6 Radar WebKit Bug Importer 2022-01-03 05:52:18 PST

<rdar://problem/87065662>

Comment 7 Darin Adler 2022-01-03 14:08:04 PST

Comment on attachment 448225 [details]
Patch

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

>>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1450
>>> +void ComputedStyleExtractor::addCSSValueForAnimationPropertyToList(CSSPropertyID property, const Animation* animation, CSSValueList& list)
>> 
>> I'd make the target list the first argument.
> 
> Also "CSS" in the name is bit unnecessary.

I’d also leave out the “to list” part of the name, and say “append” instead of “add”

    appendValueForAnimationProperty

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1493
> +            auto shorthand = shorthandForProperty(CSSPropertyAnimation);

No need for a local variable here. Reads nicely inside the for statement.

Comment 8 Antoine Quint 2022-01-05 01:26:30 PST

(In reply to Darin Adler from comment #7)
> Comment on attachment 448225 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448225&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1493
> > +            auto shorthand = shorthandForProperty(CSSPropertyAnimation);
> 
> No need for a local variable here. Reads nicely inside the for statement.

I snuck that into some more refactoring in bug 234872.