Bug 155482 – <attachment> on iOS should paint its progress indicator instead of a green square

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

<attachment> on iOS should paint its progress indicator instead of a green square

[Tim Horton]

Created attachment 274068 [details]
Patch

[Tim Horton]

http://trac.webkit.org/changeset/198191

[Daniel Bates]

Comment on attachment 274068 [details]
Patch

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

I know that this was already reviewed and landed. I had a suggestion to improve the readability of this code.

> Source/WebCore/rendering/RenderThemeIOS.mm:1619
> +    progressPath.addArc(center, info.progressRect.width() / 2, -M_PI_2, info.progress * 2 * M_PI - M_PI_2, 0);

The last parameter of Path::addArc() is a boolean as to whether the arc should be drawn clockwise. Although 0 will be implicitly cast to boolean false I suggest that we either define a local boolean variable, say clockwise, that has value false and pass this variable for the last argument of Path::addArc() or pass false and add some kind of inline comment that conveys that the purpose of this argument is to draw the arc counterclockwise.

[Tim Horton]

(In reply to comment #3)
> Comment on attachment 274068 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274068&action=review
> 
> I know that this was already reviewed and landed. I had a suggestion to
> improve the readability of this code.
> 
> > Source/WebCore/rendering/RenderThemeIOS.mm:1619
> > +    progressPath.addArc(center, info.progressRect.width() / 2, -M_PI_2, info.progress * 2 * M_PI - M_PI_2, 0);
> 
> The last parameter of Path::addArc() is a boolean as to whether the arc
> should be drawn clockwise. Although 0 will be implicitly cast to boolean
> false I suggest that we either define a local boolean variable, say
> clockwise, that has value false and pass this variable for the last argument
> of Path::addArc()

That seems a bit excessive.

> or pass false and add some kind of inline comment that
> conveys that the purpose of this argument is to draw the arc
> counterclockwise.

More reasonable, but the proper fix would be to change addArc to take an enum instead of a boolean.

[Darin Adler]

Comment on attachment 274068 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:1481
> +    progress = std::max<float>(std::min<float>(progressString.toFloat(&validProgress), 1), 0);

Tiny style comment: When clamping like this I like to put the lower bound on the left and the upper bound on the right, like this:

    progress = std::max(0.f, std::min(progressString.toFloat(&validProgress), 1.f));