[webkit-reviews] review denied: [Bug 16954] Support putImageData : [Attachment 19329] Now with betterer testcases!!!

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 16954: Support putImageData
http://bugs.webkit.org/show_bug.cgi?id=16954

Attachment 19329: Now with betterer testcases!!!
http://bugs.webkit.org/attachment.cgi?id=19329&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks pretty good, but there are still some things I'd like to see fixed.

 351	 if (args.size() == 7)

I think this should be ">= 7". There are no test cases passing "undefined". You
should include those.

 1237	  IntRect sourceRect(static_cast<int>(clipRect.x()) + destX,
static_cast<int>(clipRect.y()) + destY, 
 1238			     static_cast<int>(clipRect.width()),
static_cast<int>(clipRect.height()));

Why exactly does this use casts to int rather than the enclosingIntRect
function or ceil or floor?

 1235	  int destX = static_cast<int>(dx);
 1236	  int destY = static_cast<int>(dy);

The code would come out better if this was an IntSize.

 1237	  IntRect sourceRect(static_cast<int>(clipRect.x()) + destX,
static_cast<int>(clipRect.y()) + destY, 
 1238			     static_cast<int>(clipRect.width()),
static_cast<int>(clipRect.height()));

If destX/Y was an IntSize, this could be an assignment followed by a
IntRect::move call.

 1234	  clipRect.intersect(IntRect(0, 0, data->width(), data->height()));
 1239	  sourceRect.intersect(IntRect(0, 0, buffer->size().width(),
buffer->size().height()));

I think there's a nicer way to write these using the IntPoint and IntSize
version of the constructor. We might even want to add a constructor that takes
only an IntSize.

 1243	  sourceRect.setX(sourceRect.x() - destX);
 1244	  sourceRect.setY(sourceRect.y() - destY);

IntRect has a "move" function for making this kind of change. It could be
sourceRect.move(-destOffset).

I don't think the test cases with negative values for "dirty width" and "dirty
height" are good enough. They seem to test only cases that are entirely clipped
out; I'm still confused by the meanings of negative numbers for these given the
code implementing it and I'd really like to see test cases to clarify what
we're doing is right.

I think we are using "float" way too much in CanvasRenderingContext2D. We
should use double instead of float as much as possible, since JavaScript always
uses doubles. The conversion to float should be inside the GraphicsContext
class. When we get to 64-bit, it's going to be double even in there.

 161	 ASSERT(sourceRect.width() > 0);
 162	 ASSERT(sourceRect.height() > 0);

Should be ASSERT(!sourceRect.isEmpty()).

 171	 int endx = destPoint.x() + sourceRect.x() + sourceRect.width();

Should use sourceRect.right().

 181	 ASSERT(originy <= sourceRect.x() + sourceRect.height());

x + height is obviously wrong. This should use bottom().

 183	 int endy = destPoint.y() + sourceRect.y() + sourceRect.height();

This should use bottom().

 190	 // -originy to handle the accursed flipped y axis

We still have a comment that says "-originy", but no code that mentions it. Why
is the Y axis flipped? Don't we always do everything to make it not be flipped?


 202		     reinterpret_cast<uint32_t*>(destRows + x * 4)[0] =
reinterpret_cast<uint32_t*>(srcRows + x * 4)[0];

You didn't respond to my comments about alignment.