[webkit-reviews] review denied: [Bug 130947] [iOS] Fix remaining misuses of abs() and fabsf() : [Attachment 238225] Patch v2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 130947: [iOS] Fix remaining misuses of abs() and fabsf()
https://bugs.webkit.org/show_bug.cgi?id=130947

Attachment 238225: Patch v2
https://bugs.webkit.org/attachment.cgi?id=238225&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=238225&action=review


> Source/WTF/ChangeLog:10
> +	   - to<T>(U) converts type U to type T with runtime bounds checks
> +	     in Debug builds for integral types.

Since by runtime bounds checks you mean assertions, I am a little puzzled. We
would only assert something that’s guaranteed. Since there are no uses of
to<int> it’s hard for me to argue one way or another whether there is a
guarantee, but this doesn’t sound great to me.

> Source/WebCore/platform/ios/ScrollAnimatorIOS.mm:132
> +		       float dragAngle =
atanf(to<float>(abs(deltaFromStart.height())) /
to<float>(abs(deltaFromStart.width())));

I don’t see any benefit to using to<float> here instead of static_cast<float>.
Is there a benefit I am missing?

> Source/WebCore/platform/ios/ScrollAnimatorIOS.mm:150
> +	   handled |= m_scrollableAreaForTouchSequence->scroll(delta < 0 ?
ScrollLeft : ScrollRight, ScrollByPixel, to<float>(abs(delta)));

Ditto.

> Source/WebCore/platform/ios/ScrollAnimatorIOS.mm:156
> +	   handled |= m_scrollableAreaForTouchSequence->scroll(delta < 0 ?
ScrollUp : ScrollDown, ScrollByPixel, to<float>(abs(delta)));

Ditto.

> Source/WebCore/platform/ios/wak/WKView.mm:252
>		   else if (origMarginsTotal == 0.0f 
> -			    || (abs(origMarginsTotal) == 1)) {
> +			    || (truncateTo<int>(std::abs(origMarginsTotal)) ==
1)) {

I don’t think that truncating is what we want to do here. Lets instead write
out the code in a way that makes it obvious what it actually does. Here's what
the code does:

    if (origMarginsTotal == 0 || (origMarginsTotal > -2 && origMarginsTotal <=
-1) || (origMarginsTotal >= 1 && origMarginsTotal < 2))

I think this rule is wrong, and rewriting it to use std::abs and truncateTo
just makes it seem like it makes sense, but obscures what is actually
happening. Writing the way the way I suggest above to make it clear what it
really does might make someone consider fixing it. One step in the right
direction would be to get rid of the strange special case for the range (0,1).
Like this:

    if (origMarginsTotal > -2 && origMarginsTotal < 2)

That seems like it might be a helpful change, although it is a change in
behavior.