[webkit-reviews] review denied: [Bug 100026] inconsistency in drawImage with target rect negative dimensions. : [Attachment 171001] patch v2

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied arno. <arno at renevier.net>'s request
for review:
Bug 100026: inconsistency in drawImage with target rect negative dimensions.
https://bugs.webkit.org/show_bug.cgi?id=100026

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=171001&action=review


Looks quite good already. I am afraid, but you need to check the caller site as
well. There are quite some places that assume that we have the -1, -1 checks.

>> Source/WebCore/platform/graphics/GraphicsContext.cpp:442
>> +	if (!image)
> 
> Why is this check here?

This is from the last overload function. There you have !image check. And the
image is accessed now, tho check will be necessary. So I assume we can't be
sure that image is not null? I would rather replace the if(!image) conditions
with assertions. Should be wrong in the first place to call the function with
null. Can you investigate?

> Source/WebCore/platform/graphics/GraphicsContext.cpp:444
> +    drawImage(image, styleColorSpace, p, IntRect(IntPoint(0, 0),
image->size()), op, shouldRespectImageOrientation);

IntPoint() is enough. Please fix this on all functions.

>> Source/WebCore/platform/graphics/GraphicsContext.cpp:-478
>> -	    th = image->height();
> 
> This is a huge change.
> Are you sure that other parts of the WebKit code don't rely on this behavior
(ie CSS Filters)?

This actually looks correct.