[webkit-reviews] review denied: [Bug 40272] The 2d.imageData.object.round and 2d.imageData.object.wrap canvas tests are failing : [Attachment 62028] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Helder Correia
<heldercorreia at codeaurora.org>'s request for review:
Bug 40272: The 2d.imageData.object.round and 2d.imageData.object.wrap canvas
tests are failing
https://bugs.webkit.org/show_bug.cgi?id=40272

Attachment 62028: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=62028&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -	       if (value & ~0xFF) {
> -		   if (value < 0)
> -		       value = 0;
> -		   else
> -		       value = 255;
> -	       }
> +	       if (value & ~0xFF)
> +		   value %= 256;
>	       m_storage->data()[i] = static_cast<unsigned char>(value);

To me this looks like an inefficient way to do what the static_cast will
already do. For what values does this give a different result than if the %=
was left out?

> -	       if (!(value > 0)) // Clamp NaN to 0
> -		   value = 0;
> -	       else if (value > 255)
> -		   value = 255;
> -	       m_storage->data()[i] = static_cast<unsigned char>(value + 0.5);
> +	       long long intValue = static_cast<long long>(value);
> +	       if (intValue & ~0xFF)
> +		   intValue %= 256;
> +	       m_storage->data()[i] = static_cast<unsigned char>(intValue);

Why does this require the use of a 64-bit integer? I don’t understand why it’s
important or helpful to convert the double into a long long. If 32 bits aren’t
enough, then why are 64 bits enough? We need to code this in an efficient way,
and converting a double to a 64-bit integer is a particularly slow operation.
Perhaps this operation also can be done by simply doing a single static_cast.

Is there a test covering the behavior of NaN, Infinity, and -Infinity?

review- because I would like the questions above answered. If this is indeed
the most efficient way to do the correct thing, then you can put it up for
review again.