[webkit-reviews] review granted: [Bug 66574] Canvas 2D - Source rectangles that overlap the source image boundary, not supported by drawImage : [Attachment 144560] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Justin Novosad <junov at chromium.org>'s
request for review:
Bug 66574: Canvas 2D - Source rectangles that overlap the source image
boundary, not supported by drawImage
https://bugs.webkit.org/show_bug.cgi?id=66574

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

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


Looks OK.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1260
> +static inline bool normalizeAndClipRects(const FloatRect &imageRect,
FloatRect& srcRect, FloatRect& dstRect, ExceptionCode& ec)

The & for imageRect is in the wrong place for WebKit coding style.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1264
> +    if (!isfinite(dstRect.x()) || !isfinite(dstRect.y()) ||
!isfinite(dstRect.width()) || !isfinite(dstRect.height())
> +	   || !isfinite(srcRect.x()) || !isfinite(srcRect.y()) ||
!isfinite(srcRect.width()) || !isfinite(srcRect.height()))
> +	   return false;

I’d like to see a helper function that does this kind of check on a FloatRect
so we can write this if statement in a more compact way.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1267
> +    FloatRect origSrcRect = srcRect;

I suggest the whole word “original” rather than the abbreviation “orig”.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1269
> +    if (!srcRect.width() || !srcRect.height()) {

I think it would be better to write this as:

    if (srcRect.isEmpty())

Or srcRect.isZero().

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1275
> +    if (!dstRect.width() || !dstRect.height())
> +	   return false;

I think it would be better to write this as:

    if (dstRect.isEmpty())

Or dstRect.isZero().

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1347
> +    if (!normalizeAndClipRects(imageRect, normalizedSrcRect,
normalizedDstRect, ec)) {
>	   return;
>      }

Single line if statement bodies don’t get braces in WebKit coding style.