[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 #208523|review?                     |review-
               Flag|                            |




--- Comment #32 from Darin Adler <darin at apple.com>  2013-08-12 09:56:28 PST ---
(From update of attachment 208523)
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!

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