[webkit-reviews] review denied: [Bug 108881] Canvas fillText and measureText handle ideographic spaces differently : [Attachment 208414] Patch

Fri Aug 9 04:27:18 PDT 2013

Christophe Dumez <dchris at gmail.com> has denied Rashmi Shyamasundar
<rashmi.s2 at samsung.com>'s request for review:
Bug 108881: Canvas fillText and measureText handle ideographic spaces
differently
https://bugs.webkit.org/show_bug.cgi?id=108881

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

------- Additional Comments from Christophe Dumez <dchris at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=208414&action=review

> Source/WebCore/ChangeLog:12
> +	   This patch modifies the canvas functions drawTextInternal and
measureText to conform to the above spec.

Does it match Firefox as well?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:93
>  

Would be nice to add a link to the specification in comment:
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxe
s.html#space-character

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:94
> +static inline bool shouldSpaceBeReplaced(UChar c)

This function has nothing to do with replacing. Maybe we can call it
isHTMLSpace() ?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:96
> +    return (c == 0x0009 || c == 0x000A || c == 0x000B || c == 0x000C || c ==
0x000D) ? true : false;

return (c == 0x0009 || c == 0x000A || c == 0x000B || c == 0x000C || c ==
0x000D);

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2111
> +    const size_t replacementLength = replacement.length();

length() returns an 'unsigned' type. I think it is best to use the same type.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2112
> +    size_t index = 0;

No need to initialize to 0.

> LayoutTests/ChangeLog:11
> +	   The below listed layout tests veriy the conformance to above spec.

verify

> LayoutTests/fast/canvas/canvas-fillText-ideographicSpace-expected.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>