[webkit-reviews] review granted: [Bug 218864] [iOS][FCR] Add new look for progress bars : [Attachment 413956] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Aditya Keerthi <akeerthi at apple.com>'s
request for review:
Bug 218864: [iOS][FCR] Add new look for progress bars
https://bugs.webkit.org/show_bug.cgi?id=218864

Attachment 413956: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 413956
  --> https://bugs.webkit.org/attachment.cgi?id=413956
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?