[webkit-reviews] review denied: [Bug 108881] Canvas fillText and measureText handle ideographic spaces differently : [Attachment 208523] 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 208523: Patch
https://bugs.webkit.org/attachment.cgi?id=208523&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=208523&action=review


> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:97
> +    // 0x000B should be treated as space for backward compatibility and for
matching the behavior of Firefox 23.0

For matching is bad grammar; it's not clear why matching Firefox 23.0 is a
goal, so not sure why citing it here is helpful.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2127
> +    // According to spec all space characters should be replaced with +U0020
space character

+U0020 is not the correct syntax. Missing period here. “According to spec” is
ambiguous. Please briefly name the spec you are talking bout.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2129
> +    replaceCharacterInString(normalizedText, isHTMLSpaceOrVerticalTab, " ");


The algorithm used in this replaceCharacterInString function is highly
inefficient and we should not use it. There are major problems:

1) No algorithm that loops through a string should modify the string with
multiple calls to String member functions. Each of those String member
functions allocates a new string for the result. So if the string has 100
spaces in it, this will reallocate the string 100 times, for a total of about
200 expensive calls to malloc. One good way to avoid this problem is to use a
StringBuilder or Vector<UChar> instead of using the string replace function. We
would only do that work if there is at least one character that needs to be
replaced.

2) This algorithm finds existing U+0020 spaces in the string and replaces each
one. I'm sure it's common to have strings with spaces in them. We should not be
finding those one at a time and then doing a costly replace-with-self operation
that has no effect but causes multiple memory allocations. One way to avoid
this problem is to have the character matching function return false for
U+0020.

3) The replace function called here is an extremely expensive way to replace a
single character without changing the string. Just calling the function
requires creating and destroying a string, which means we are doing memory
allocation even when measuring a string that has no spaces in it. And the
function is a general purpose one that can change string length, which is far
more expensive than it should be.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2173
> -    replaceCharacterInString(normalizedText, isSpaceOrNewline, " ");
> +    replaceCharacterInString(normalizedText, isHTMLSpaceOrVerticalTab, " ");


This is just as a bad as the new call site. Not sure how this made it through
patch review before!