Bug 213967

Comment on attachment 403547 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeMac.mm:128
> +#if HAVE(ALTERNATE_ICONS)
> +static const double progressAnimationNumFrames = 120;
> +#else
>  static const double progressAnimationNumFrames = 256;
> +#endif

The CoreUI folks don't really want anyone using kCUIAnimationFrameKey, which is what requires us to know the number of frames. Can we instead switch to kCUIAnimationStartTimeKey/kCUIAnimationTimeKey and avoid the problem of needing to know the number of frames entirely?

(In reply to Sam Weinig from comment #2)
> Comment on attachment 403547 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403547&action=review
> 
> > Source/WebCore/rendering/RenderThemeMac.mm:128
> > +#if HAVE(ALTERNATE_ICONS)
> > +static const double progressAnimationNumFrames = 120;
> > +#else
> >  static const double progressAnimationNumFrames = 256;
> > +#endif
> 
> The CoreUI folks don't really want anyone using kCUIAnimationFrameKey, which
> is what requires us to know the number of frames. Can we instead switch to
> kCUIAnimationStartTimeKey/kCUIAnimationTimeKey and avoid the problem of
> needing to know the number of frames entirely?

An added benefit of this would be that if we use a shared heartbeat, all the indeterminate progress indicators in a page will animate in sync, just like in a normal app.

Comment on attachment 403547 [details]
Patch

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

>>> Source/WebCore/rendering/RenderThemeMac.mm:128
>>> +#endif
>> 
>> The CoreUI folks don't really want anyone using kCUIAnimationFrameKey, which is what requires us to know the number of frames. Can we instead switch to kCUIAnimationStartTimeKey/kCUIAnimationTimeKey and avoid the problem of needing to know the number of frames entirely?
> 
> An added benefit of this would be that if we use a shared heartbeat, all the indeterminate progress indicators in a page will animate in sync, just like in a normal app.

Yep, this works great!

I wish this meant we could just remove animationDurationForProgressBar and animationRepeatIntervalForProgressBar, but other ports (Adwaita?) seem to override this and return a non-zero value. That said, I can at least remove the hard-coded constants and stop overriding it in RenderThemeMac (and RenderThemeIOS as well, which for some reason overrides it but returns zero which is the default anyways 🤷🏻‍♂️).

Created attachment 403558 [details]
Patch

Comment on attachment 403558 [details]
Patch

Very nice. We could probably optimize things a bit in the future (and make it match macOS a bit more) if we wanted by having a shared heartbeat timer for all indeterminate progress indicators, instead of using a timer per-RenderProgress.

Comment on attachment 403558 [details]
Patch

(In reply to Sam Weinig from comment #6)
> Comment on attachment 403558 [details]
> Patch
> 
> Very nice. We could probably optimize things a bit in the future (and make
> it match macOS a bit more) if we wanted by having a shared heartbeat timer
> for all indeterminate progress indicators, instead of using a timer
> per-RenderProgress.

Indeed!

Committed r263952: <https://trac.webkit.org/changeset/263952>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403558 [details].

<rdar://problem/65109650>