[webkit-reviews] review denied: [Bug 16954] Support putImageData : [Attachment 19316] Here we go!

[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 19316: Here we go!
http://bugs.webkit.org/attachment.cgi?id=19316&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
I have a lot of comments, so review- for now.

 29 function dataToArray(data) {

You should put those function braces on new lines.

 43 function pixelShouldBe(ctx, x, y, colour) {

The shouldBe function from the JavaScript testing script can handle comparing
arrays and it colors the PASS and FAIL strings too. I wish you would use that.

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

Is size() really the right thing here? We normally try to base things on
whether the arguments are defined or not and never actually look at the size.
And we normally allow too few or too many parameters.

 350	 ExceptionCode ec;

If you're going to call setDOMException with the result from ec, then you need
to initialize the ExceptionCode to 0. The function you're calling has no
obligation to set the code to 0 if the function succeeds.

 1214	  if (!buffer || !isfinite(dx) || !isfinite(dy))
 1215	      return;

Seems a little strange to check dx and dy to see if they are finite *after*
checking the buffer. No big deal either way.

 1221	  ec = 0;

No need to set ec to 0. If there was a need to do so, we'd want to do that
before the first return. But you should just remove it.

The negative number stuff looks really crazy to me. Is that really the correct
logic? Are there test cases that cover all these?

 1243	  if (dirtyX + dirtyWidth > data->width())
 1244	      dirtyWidth = data->width() - dirtyX;

Could this be done more readably with the min function?

What prevents dirtyX from being > data->width()? Test case?

What prevents dirtyY from being > data->height()? Test case?

 1250	  if (!dirtyRect.width() || !dirtyRect.height())
 1251	      return;

There's an isEmptyFunction you can use for this.

I don't see any tests for all the different ways you do and don't get
exceptions with this function. I think we need those.

I don't see any tests for all the special cases for negative numbers. We need
those.

 73	    void putImageData(ImageData*, const IntPoint& targetPoint, const
IntRect& dirtyRect);

I don't think that "targetPoint" and "dirtyRect" are clear enough. Where
exactly does the image go? Is targetPoint the top/left of something?

 167	 int destx = targetPoint.x() + dirtyRect.x();

This can overflow, with undefined results. We need to check for that and make
sure we get well-defined results.

 171	     originx -= destx;

Maybe this can overflow too?

 174	 if (originx > dirtyRect.x() + dirtyRect.width())

The value x() + width() is called right(). But of course it can overflow.

 176	 int endx = targetPoint.x() + dirtyRect.x() + dirtyRect.width();

This can definitely overflow.

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

This comment is not followed by any code that contains -originy. Why the
comment then? What does the comment mean?

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

Do we have a guarantee that the buffer is suitably aligned? If not, then we
need to consider portability to processors that aren't able to do this
operation.