Bug 193195 – [Web Animations] Audit Web Animations classes for memory reduction

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

[Web Animations] Audit Web Animations classes for memory reduction

[Antoine Quint]

Created attachment 358494 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/animation/WebAnimation.h:136:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 20 files


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

[Simon Fraser (smfr)]

Comment on attachment 358494 [details]
Patch

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

> Source/WebCore/animation/ComputedEffectTiming.h:36
>      Optional<double> localTime;
>      Optional<double> progress;
>      Optional<double> currentIteration;

Can these be Markable<>?

> Source/WebCore/animation/OptionalEffectTiming.h:40
>      Optional<double> delay;
>      Optional<double> endDelay;
> -    Optional<FillMode> fill;
>      Optional<double> iterationStart;
>      Optional<double> iterations;

Can these be Markable<>?

> Source/WebCore/animation/OptionalEffectTiming.h:41
>      String easing;

What is this String for? Could it be an enum here?

> Source/WebCore/animation/OptionalEffectTiming.h:43
> +    Optional<FillMode> fill;
> +    Optional<PlaybackDirection> direction;

Can these be Markable<>?

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 358494 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=358494&action=review
> 
> > Source/WebCore/animation/ComputedEffectTiming.h:36
> >      Optional<double> localTime;
> >      Optional<double> progress;
> >      Optional<double> currentIteration;
> 
> Can these be Markable<>?
> 
> > Source/WebCore/animation/OptionalEffectTiming.h:40
> >      Optional<double> delay;
> >      Optional<double> endDelay;
> > -    Optional<FillMode> fill;
> >      Optional<double> iterationStart;
> >      Optional<double> iterations;
> 
> Can these be Markable<>?

I think we can safely convert all Optional<double> to Markable where we'd use NaN as an empty marker since NaN would never be a valid value for any of those. I'll look into this.

> > Source/WebCore/animation/OptionalEffectTiming.h:41
> >      String easing;
> 
> What is this String for? Could it be an enum here?
> 
> > Source/WebCore/animation/OptionalEffectTiming.h:43
> > +    Optional<FillMode> fill;
> > +    Optional<PlaybackDirection> direction;
> 
> Can these be Markable<>?

Yes, we can use EnumMarkableTraits but this just increases the padding there. I'll still make that change.

[Antoine Quint]

Created attachment 358696 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/animation/WebAnimation.h:137:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 23 files


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

[Yusuke Suzuki]

Comment on attachment 358696 [details]
Patch

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

r=me with some suggestions.

> Source/WebCore/animation/FillMode.h:34
> +typedef Markable<FillMode, EnumMarkableTraits<FillMode, 5>> OptionalFillMode;

Use `using` instead.
And you can omit this `5`.
EnumMarkableTraits automatically selects 255 for the empty mark since the underlying type is uint8_t and it picks the maximum value of the underlying type.
So,

using OptionalFillMode = Markable<FillMode, EnumMarkableTraits<FillMode>>;

works.

> Source/WebCore/animation/PlaybackDirection.h:34
> +typedef Markable<PlaybackDirection, EnumMarkableTraits<PlaybackDirection, 4>> OptionalPlaybackDirection;

Use `using` instead.
And you can omit `4` here.

> Source/WebCore/animation/WebAnimationUtilities.h:61
> +typedef Markable<double, WebAnimationsMarkableDoubleTraits> MarkableDouble;

Use `using` instead.

[Antoine Quint]

Committed r239820: <https://trac.webkit.org/changeset/239820>

[Radar WebKit Bug Importer]

<rdar://problem/47171502>