Bug 155482 - <attachment> on iOS should paint its progress indicator instead of a green square
Summary: <attachment> on iOS should paint its progress indicator instead of a green sq...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-14 18:56 PDT by Tim Horton
Modified: 2016-03-16 14:47 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.17 KB, patch)
2016-03-14 18:57 PDT, Tim Horton
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2016-03-14 18:56:38 PDT
<attachment> on iOS should paint its progress indicator instead of a green square
Comment 1 Tim Horton 2016-03-14 18:57:12 PDT
Created attachment 274068 [details]
Patch
Comment 2 Tim Horton 2016-03-14 21:10:42 PDT
http://trac.webkit.org/changeset/198191
Comment 3 Daniel Bates 2016-03-14 21:17:34 PDT
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.
Comment 4 Tim Horton 2016-03-14 21:22:40 PDT
(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.
Comment 5 Darin Adler 2016-03-16 14:47:05 PDT
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));