Bug 218864 – [iOS][FCR] Add new look for progress bars

• Component: Forms
• Status: RESOLVED FIXED

[Aditya Keerthi]

...

[Aditya Keerthi]

<rdar://problem/71334958>

[Aditya Keerthi]

Created attachment 413956 [details]
Patch

[Darin Adler]

Comment on attachment 413956 [details]
Patch

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

Can we find a way to regression-test this code?

> Source/WebCore/rendering/RenderThemeIOS.mm:954
> +static const Seconds progressAnimationFrameRate = 33_ms; // 30 fps

Please use constexpr for cases like this.

Please include comments explaining "why" in a case like this. How did we decide that this should be 30 fps?

This constant should not be named "frame rate" because it does not contain a frame rate or frame frequency value. Instead it contains a frame duration or frame interval. Let’s not use a "counterfactual" in the name.

> Source/WebCore/rendering/RenderThemeIOS.mm:969
> +    if (rect.width() < 10 || rect.height() < 4) {

Where did this constants come from? Ideally we use named constants for such things. I think the code below implies that these are "the size of a standard progress bar".

> Source/WebCore/rendering/RenderThemeIOS.mm:975
> +    int progressBarHeight = 4;

Please use constexpr for something like this. Maybe outside the function with the other constants? Or at the top? With comment explaining why.

> Source/WebCore/rendering/RenderThemeIOS.mm:977
> +    float verticalOffset = (rect.height() - progressBarHeight) / 2.0f;
> +    float verticalRenderingPosition = rect.y() + verticalOffset;

I don’t think we need a separate local variable for this. Also seems the name "vertical rendering position" is a little wordy. Might look for a shorter name, like top or barTop maybe?

> Source/WebCore/rendering/RenderThemeIOS.mm:978
> +    FloatSize roundedCornerRadius(2.5f, 1.5f);

I suggest using constexpr here and having a comment explaining where this comes from. Might also want a shorter name, like cornerRadius.

> Source/WebCore/rendering/RenderThemeIOS.mm:981
> +    FloatRoundedRect roundedTrackRect(trackRect, roundedCornerRadius, roundedCornerRadius, roundedCornerRadius, roundedCornerRadius);

Seems like we should add a constructor for FloatRoundedRect when the radius is the same for all four corners, to make this code easier to read.

> Source/WebCore/rendering/RenderThemeIOS.mm:982
> +    context.fillRoundedRect(roundedTrackRect, SRGBA<uint8_t> { 238, 238, 238 });

Seems like this color should also be named constant, so we can comment where it came from.

> Source/WebCore/rendering/RenderThemeIOS.mm:985
> +    float barLeft = trackRect.x();

Seems like this could be rect.x() instead that that would be a little clearer.

> Source/WebCore/rendering/RenderThemeIOS.mm:987
> +    auto& renderProgress = downcast<RenderProgress>(renderer);

Very strange that this cast is needed; can we fix theme API so this is done by the caller?

> Source/WebCore/rendering/RenderThemeIOS.mm:990
> +        double position = clampTo(renderProgress.position(), 0.0f, 1.0f);
> +        barWidth = position * trackRect.width();

It’s very strange to include the "f" and make these float constants when clamping a double. You could use clampTo<float> maybe? Or maybe this:

    barWidth = std::clamp(renderProgress.position(), 0, 1) * trackRect.width();

> Source/WebCore/rendering/RenderThemeIOS.mm:999
> +        auto startTime = renderProgress.animationStartTime();
> +        auto currentTime = MonotonicTime::now();
> +        Seconds elapsed = currentTime - startTime;

I don’t think we need these local variables.

    auto elapsed = renderProgress.animationStartTime() - MonotonicTime::now();

> Source/WebCore/rendering/RenderThemeIOS.mm:1002
> +        float position = fmodf(elapsed.value(), 1.0f);
> +        float offset = position * (trackRect.width() + barWidth);

Again, I think doing these in one line might be better.

> Source/WebCore/rendering/RenderThemeIOS.mm:1008
> +            barLeft = trackRect.x() - barWidth + offset;

Maybe this instead:

    barLeft -= barWidth - offset;

Trying to figure out what would be clearest.

> Source/WebCore/rendering/RenderThemeIOS.mm:1014
> +    context.fillRoundedRect(FloatRoundedRect(barRect, roundedCornerRadius, roundedCornerRadius, roundedCornerRadius, roundedCornerRadius), SRGBA<uint8_t> { 0, 122, 255 });

Named constant for this color?

[Aditya Keerthi]

(In reply to Darin Adler from comment #3)
> Comment on attachment 413956 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=413956&action=review
> 
> Can we find a way to regression-test this code?

I think (mismatch) ref tests are the only thing we can test for here. Such as a 50% progress bar having a different appearance from a 100% progress bar, as well as seeing how the progress bar responds to different CSS styles. I will add a runtime flag to enable testing of the redesigned elements and then come back and add tests to this patch. 
 
> > Source/WebCore/rendering/RenderThemeIOS.mm:954
> > +static const Seconds progressAnimationFrameRate = 33_ms; // 30 fps
> 
> Please use constexpr for cases like this.
> 
> Please include comments explaining "why" in a case like this. How did we
> decide that this should be 30 fps?
> 
> This constant should not be named "frame rate" because it does not contain a
> frame rate or frame frequency value. Instead it contains a frame duration or
> frame interval. Let’s not use a "counterfactual" in the name.

Will do. This constant was taken from RenderThemeMac, so I will clean up the name there too.

> > Source/WebCore/rendering/RenderThemeIOS.mm:987
> > +    auto& renderProgress = downcast<RenderProgress>(renderer);
> 
> Very strange that this cast is needed; can we fix theme API so this is done
> by the caller?

I can move the cast to the caller in this instance, since paintProgressBar has always early returned if !is<RenderProgress>(renderer).

But it cannot be a general change to the theme API, since any RenderBox could have "-webkit-appearance" in it's style to paint a native control. That is why RenderTheme::paint only deals with RenderBoxes, and calls the appropriate method based on their "-webkit-appearance" value.

It looks like the only two appearance values that enforce a particular "Render*" are progress-bar (RenderProgress) and attachment (RenderAttachment).

Should I still move the cast to the caller or keep things the way they are for more consistency in the RenderTheme::paint code?

[Darin Adler]

(In reply to Aditya Keerthi from comment #4)
> Should I still move the cast to the caller or keep things the way they are
> for more consistency in the RenderTheme::paint code?

The cast should go wherever the check is. I did’t completely follow the explanation. If there is somewhere that is doing a check, the cast should be right there.

The anti-pattern we are trying to avoid or keep to a minimum: Check in one function, pass an argument of a less-specific type, then cast in another function based on the knowledge that the check was done in the first.

[Aditya Keerthi]

(In reply to Darin Adler from comment #5)
> (In reply to Aditya Keerthi from comment #4)
> > Should I still move the cast to the caller or keep things the way they are
> > for more consistency in the RenderTheme::paint code?
> 
> The cast should go wherever the check is. I did’t completely follow the
> explanation. If there is somewhere that is doing a check, the cast should be
> right there.
> 
> The anti-pattern we are trying to avoid or keep to a minimum: Check in one
> function, pass an argument of a less-specific type, then cast in another
> function based on the knowledge that the check was done in the first.

The cast is currently performed in the same method as the check (no checks are performed earlier), so I will keep the cast as-is.

However, I will move the cast closer to the check for clarity (the check is at the top of RenderThemeIOS::paintProgressBar).

[Aditya Keerthi]

Created attachment 414275 [details]
Patch

[Aditya Keerthi]

This patch will apply once https://bugs.webkit.org/show_bug.cgi?id=218808 has landed.

[Wenson Hsieh]

Comment on attachment 414275 [details]
Patch

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

The new patch LGTM (just a minor comment).

It looks like Darin reviewed it already as well.

> Source/WebCore/rendering/RenderThemeIOS.h:116
> +    Seconds animationRepeatIntervalForProgressBar(RenderProgress&) const final;

Nit - can this take a `const RenderProgress&`?

[Aditya Keerthi]

Created attachment 414601 [details]
Patch for landing

[EWS]

commit-queue failed to commit attachment 414601 [details] to WebKit repository.

[EWS]

Committed r270065: <https://trac.webkit.org/changeset/270065>

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