Bug 234812 - Refactor code creating css values and lists for animation and transition properties
Summary: Refactor code creating css values and lists for animation and transition prop...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-03 04:13 PST by Antoine Quint
Modified: 2022-01-05 01:26 PST (History)
11 users (show)

See Also:

Attachments
Patch (19.95 KB, patch)
2022-01-03 04:21 PST, Antoine Quint 		koivisto: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.