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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.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 209259: Patch
https://bugs.webkit.org/attachment.cgi?id=209259&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.