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

[bugzilla-daemon at webkit.org]

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 208908: Patch
https://bugs.webkit.org/attachment.cgi?id=208908&action=review

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


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

How about spaceNeedsReplacing()? Since we don't include U+0020.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2106
> +    return (c == 0x0009 || c == 0x000A || c == 0x000C || c == 0x000D);

where is 0B?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2109
> +static void replaceSpace(String& text)

Maybe normalizeSpaces()?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2113
> +    UChar* finalTextArray = new UChar[textLength];

Allocating memory even if there is no space that needs replacing :( i.e. You
should abort early if there is space that needs replacing.

Not to mention that this is leaking... Please use a Vector<UChar> to avoid such
issue, you can then use String::adopt() to avoid copying the Vector.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2115
> +    UChar ch;

Should be inside the loop

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2116
> +    for (unsigned i = 0; i != textLength; i++) {

We prefer ++i in WebKit.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2124
> +    String finalText(finalTextArray, textLength);

See comment above.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2134
> +    // According to whatwg spec all space characters should be replaced with
0x0020 space character.

According to the specification...

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2135
> +    // Spec :-
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.
html#text-preparation-algorithm

You can remove : 'Spec :- '