Bug 223148

Summary:

Improve coding style of CSSPropertyAnimation.cpp

Product:

WebKit

Reporter:

Antoine Quint <graouts>

Component:

Animations

Assignee:

Antoine Quint <graouts>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aakash_jain, darin, dino, graouts, sam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Safari Technology Preview

Hardware:

Unspecified

OS:

Unspecified

See Also:

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

Attachments:

Description	Flags
Patch	none
Patch	darin: review+, ews-feeder: commit-queue-

Description Antoine Quint 2021-03-13 02:45:54 PST

Darin provided review feedback in bug 223124 which would really apply to most of CSSPropertyAnimation.cpp. This bug is to ensure that we:

- use final instead of override where applicable
- put override methods in private sections
- don't abbreviate parameters

There may be more.

Comment 1 Antoine Quint 2021-03-13 07:43:25 PST

Created attachment 423102 [details]
Patch

Comment 2 Antoine Quint 2021-03-13 07:47:43 PST

The patch is failing to apply due to the GitHub mirror being way behind :(

Comment 3 Aakash Jain 2021-03-13 08:09:14 PST

(In reply to Antoine Quint from comment #2)
> The patch is failing to apply due to the GitHub mirror being way behind :(
That has been fixed now. Your patch is being processed by EWS now.

Comment 4 Antoine Quint 2021-03-13 13:40:17 PST

Created attachment 423108 [details]
Patch

Comment 5 Darin Adler 2021-03-14 13:24:05 PDT

Comment on attachment 423108 [details]
Patch

A lot of this code also uses pointers that are required to be non-null rather than references. Like the RenderStyle and the clients.

Comment 6 Sam Weinig 2021-03-14 14:19:13 PDT

Comment on attachment 423108 [details]
Patch

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

> Source/WebCore/animation/CSSPropertyAnimation.cpp:2249
>  // Returns true if we need to start animation timers
> -void CSSPropertyAnimation::blendProperties(const CSSPropertyBlendingClient* anim, CSSPropertyID prop, RenderStyle* dst, const RenderStyle* a, const RenderStyle* b, double progress)
> +void CSSPropertyAnimation::blendProperties(const CSSPropertyBlendingClient* client, CSSPropertyID property, RenderStyle* destination, const RenderStyle* from, const RenderStyle* to, double progress)

Can this comment be removed? The function returns void.

Comment 7 Antoine Quint 2021-03-15 00:38:06 PDT

Committed r274409 (235276@main): <https://commits.webkit.org/235276@main>

Comment 8 Radar WebKit Bug Importer 2021-03-15 00:39:15 PDT

<rdar://problem/75420721>

Comment 9 Antoine Quint 2021-03-15 00:40:25 PDT

(In reply to Sam Weinig from comment #6)
> Comment on attachment 423108 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423108&action=review
> 
> > Source/WebCore/animation/CSSPropertyAnimation.cpp:2249
> >  // Returns true if we need to start animation timers
> > -void CSSPropertyAnimation::blendProperties(const CSSPropertyBlendingClient* anim, CSSPropertyID prop, RenderStyle* dst, const RenderStyle* a, const RenderStyle* b, double progress)
> > +void CSSPropertyAnimation::blendProperties(const CSSPropertyBlendingClient* client, CSSPropertyID property, RenderStyle* destination, const RenderStyle* from, const RenderStyle* to, double progress)
> 
> Can this comment be removed? The function returns void.

Fixed in commit.

Comment 10 Antoine Quint 2021-03-15 00:43:12 PDT

(In reply to Darin Adler from comment #5)
> Comment on attachment 423108 [details]
> Patch
> 
> A lot of this code also uses pointers that are required to be non-null
> rather than references. Like the RenderStyle and the clients.

I'll look into that in bug 223173.