[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


Darin Adler <darin at apple.com> changed:

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




--- Comment #51 from Darin Adler <darin at apple.com>  2013-08-22 09:40:04 PST ---
(From update of attachment 209259)
View in context: https://bugs.webkit.org/attachment.cgi?id=209259&action=review

review- because the use of int instead of size_t is incorrect and because we need the test to pass

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

I know we’re renamed this many times. An even better name than this latest one is isSpaceThatNeedsReplacing.

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

Needs a comment to explain where this set came from. Specifically, why does this include 0xB even though isHTMLSpace does not.

Also, no need for the parentheses here.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2116
>> +    if (i == -1)
> 
> -1 -> use usually use notFound instead (from NotFound.h).

Must use it; it’s incorrect to use -1 like this.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2119
> +    Vector<UChar> charVector = text.charactersWithNullTermination();

Why with null termination? We should use a length, not a null terminator. We only use null termination when forced to by platform APIs. I can see how it’s tempting to use the function just because it happens to return a Vector, but you can also copy a string into a Vector simply by constructing the vector with the appropriate size and then using memcopy(vector.data(), text.characters(), text.length());

I am not asking you to do this in the initial patch, but as follow-up it would be nice to take the removeCharacters from from String and StringImpl as a pattern and make a replaceCharacters function that replaces characters with a new character. It can use templates so it won’t turn an 8-bit string into a 16-bit string and can be even more efficient than the Vector-based version.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2137
> +    // According to specification all space characters should be replaced with 0x0020 space character.
> +    // http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#text-preparation-algorithm

I don’t think this is a helpful comment. Most of what we do is according to specifications, and we don’t need to quote them as we implement them.

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