[Webkit-unassigned] [Bug 108881] Canvas fillText and measureText handle ideographic spaces differently

[bugzilla-daemon at webkit.org]

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


Christophe Dumez <dchris at gmail.com> changed:

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




--- Comment #38 from Christophe Dumez <dchris at gmail.com>  2013-08-16 04:45:34 PST ---
(From update of attachment 208908)
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 :- '

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