[Webkit-unassigned] [Bug 67652] Negative width/height values not always treated properly by 2D canvas drawImage

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=67652


Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #219902|review?                     |review-
               Flag|                            |




--- Comment #40 from Daniel Bates <dbates at webkit.org>  2013-12-23 12:29:20 PST ---
(From update of attachment 219902)
View in context: https://bugs.webkit.org/attachment.cgi?id=219902&action=review

r- due to the lack of a citation to the Blink commit and the mac-wk2-ews layout test failure.

> Source/WebCore/ChangeLog:10
> +        According to the canvas spec - "When the source rectangle is outside the source image,
> +        the source rectangle must be clipped to the source image and the destination rectangle
> +        must be clipped in the same proportion."

Please reference the original Blink commit in the change log entry. We don't seem to have a standard format for such citations at the time of writing. You may either cite the original author of this patch, include the Blink revision number and a URL to the Blink revision as seen in the change log entry for <http://trac.webkit.org/changeset/158676> or you may simply provide a URL to the Blink Git commit as seen in the change log entry for <http://trac.webkit.org/changeset/160747>.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1226
> +static inline void clipRectsToImageRect(const FloatRect& imageRect, FloatRect* srcRect, FloatRect* dstRect)

This function assumes srcRect and dstRect are non-null. Can we make these arguments references instead of pointers? Then we can remove the use of the address-of operator in the caller and the dereference operator in the call to FloatRect::contains() (below).

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1232
> +    FloatSize scale(dstRect->size().width() / srcRect->size().width(), dstRect->size().height() / srcRect->size().height());

Although the compiler is probably smart enough, we may want to consider caching {dstRect, srcRect}->size().{height, width}() in a local variable to avoid extraneous dereferences.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1235
> +    FloatSize offset = dstRect->location() - scaledSrcLocation;

This variable is referenced exactly once (below) and I don't feel its name improves the readability of this code. I suggest we inline the value of this variable on line 1242.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.