SSIA
Created attachment 403547 [details] Patch
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>