Bug 108881 – Canvas fillText and measureText handle ideographic spaces differently

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Jason Parrott]

Created attachment 186505 [details]
Demo showing bug.

This is a bug first created over on chromium's issue tracker here: http://code.google.com/p/chromium/issues/detail?id=171639

Steps to reproduce the problem:
1. Call canvas 2d context's measureText() twice. Once with \u0020 (ascii space) and once with ideographic space (\u3000).
2. Notice the width returned are different.
3. Call canvas 2d context's fillText() twice. Once with \u0020 (ascii space) and once with ideographic space (\u3000).
4. Notice how the results are the same. Both are rendered as a normal ascii space.

What is the expected behavior?
The width of the space shown with fillText should be the same as the width calculated by measureText.

What went wrong?
There are actually several things going on here.

Specification-wise, in the current stable and dev w3c versions of Canvas,
( http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas/#text-preparation-algorithm )
text passed in to both measureText and fillText needs to have all space characters replaced with ASCII space characters.
However the definition of 'space character' is undefined.

In whatwg,
( http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#text-preparation-algorithm )
The spec says to replace space characters again, however this time with a definition.
( http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#space-character )
Note that ideographic space (U+3000) is not in that definition.

Both measureText and fillText are supposed to use this same algorithm for replacing text and should therefore have the same result when doing things with space widths.

Code-wise the problem can be easily found (if I'm reading this correctly).

http://git.chromium.org/gitweb/?p=external/Webkit.git;a=blob;f=Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp;hb=HEAD#l2218

measureText is not trying to replace anything. Which in itself is a bug.

http://git.chromium.org/gitweb/?p=external/Webkit.git;a=blob;f=Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp;hb=HEAD#l2266

drawTextInternal does replace spaces using replaceCharacterInString(normalizedText, isSpaceOrNewline, " ");
However, isSpaceOrNewline (if I'm following the code correctly), matches all BIDI WS characters, including ideographic space.
This will replace all white space characters with ASCII space which is both specification-wise incorrect and language-wise annoying.

In Japanese the standard space is the ideographic space and if you replace that with ASCII space in a sentence the sentence becomes difficult to read as it is not what people are used to (as it is half the size).

At any rate, measureText and drawTextInternal are doing things differently while they should be doing the same thing.

Please see the attached file for a demo of this breaking.
Note that this uses the Japanese Osaka font to demonstrate easily.

[Rashmi Shyamasundar]

Created attachment 208243 [details]
Patch

[Martin Robinson]

Comment on attachment 208243 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208243&action=review

> Source/WebCore/ChangeLog:16
> +        No new tests (OOPS!).

This is a hint that your patch needs a test. :)

> Source/WTF/wtf/text/StringImpl.h:1359
> +static inline bool shouldSpaceBeReplaced(UChar c)
> +{
> +    return (c == 0x0009 || c == 0x000A || c == 0x000C || c == 0x000D) ? true : false;
> +}
> +

It seems like a layering violation for details of the canvas spec to be encoded into the WTFString class.

[Rashmi Shyamasundar]

Currently, fillText is using the function isSpaceOrNewline which is defined in StringImpl.h . The function isSpaceOrNewline returns true for any space character including the ideographic space character U+3000. And, we need a function which returns true only for those space characters mentioned in the spec. An ideographic space should not be replaced by U+0020.

The function shouldSpaceBeReplaced() which I have defined in StringImpl.h, supports the text preparation algorithm defined in the spec. This text preparation algorithms could be used by other html elements as well, right? With the above idea in mind, I defined this function here. Do you think that this should be moved to CanvasRenderingContext2D?

[Martin Robinson]

(In reply to comment #3)

> The function shouldSpaceBeReplaced() which I have defined in StringImpl.h, supports the text preparation algorithm defined in the spec. This text preparation algorithms could be used by other html elements as well, right? With the above idea in mind, I defined this function here. Do you think that this should be moved to CanvasRenderingContext2D?

Yes. I also think you should write it in terms of the existing isSpaceOrNewline which handles most cases.

[Rashmi Shyamasundar]

The function isSpaceOrNewline returns true for the below characters

1. Ascii characters - U+0020, 0xD, 0xC, 0xB, 0xA,  0x9
2. All unicode white space characters

According to http://en.wikipedia.org/wiki/Mapping_of_Unicode_characters#Whitespace_characters :-

Unicode designates the legacy control characters U+0009 through U+000D and U+0085 as whitespace characters, as well as all characters whose General Category property value is Separator. There are 26 total whitespace characters as of Unicode 6.0.0.

Hence U+3000(ideographic space character) is a white space character according to Unicode. And, the function isSpaceOrNewline() returns true for U+3000.

But, the character U+3000 should not be replaced with U+0020 according to the text preparation algorithm - http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#text-preparation-algorithm . 

Hence, I have defined a new function :-

The function isSpaceOrNewline returns true for the below characters

1. Ascii characters - U+0020, 0xD, 0xC, 0xB, 0xA,  0x9
2. All unicode white space characters

According to http://en.wikipedia.org/wiki/Mapping_of_Unicode_characters#Whitespace_characters :-

Unicode designates the legacy control characters U+0009 through U+000D and U+0085 as whitespace characters, as well as all characters whose General Category property value is Separator. There are 26 total whitespace characters as of Unicode 6.0.0.

Hence U+3000(ideographic space character) is a white space character according to Unicode. And, the function isSpaceOrNewline() returns true for U+3000.

But, the character U+3000 should not be replaced with U+0020 according to the text preparation algorithm - http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#text-preparation-algorithm . 

Hence, I have defined a new function :-

static inline bool shouldSpaceBeReplaced(UChar c)
{
return (c == 0x0009 || c == 0x000A || c == 0x000C || c == 0x000D) ? true : false;
}

[Martin Robinson]

(In reply to comment #5)
> The function isSpaceOrNewline returns true for the below characters
> 
> 1. Ascii characters - U+0020, 0xD, 0xC, 0xB, 0xA,  0x9
> 2. All unicode white space characters
> 
> According to http://en.wikipedia.org/wiki/Mapping_of_Unicode_characters#Whitespace_characters :-

Thanks for the detailed explanation. It matches my understanding of the situation. 

I think that shouldSpaceBeReplaced belongs in canvas class since its behavior is specific to the canvas spec. I also recommend basing it off of isSpaceOrNewline to avoid having to repeat the logic of deciding which characters are spaces.

For instance:

return c != 0x3000 && isSpaceOrNewline;

[Rashmi Shyamasundar]

According to the spec - http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#space-character :-

The space characters, for the purposes of this specification, are U+0020 SPACE, U+0009 CHARACTER TABULATION (tab), U+000A LINE FEED (LF), U+000C FORM FEED (FF), and U+000D CARRIAGE RETURN (CR).

The statement  "return c != 0x3000 && isSpaceOrNewline;" will return true for all the 26 unicode white space characters which should not be replaced by U+0020.(According to spec http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#white_space :- The White_Space characters are those that have the Unicode property "White_Space" in the Unicode PropList.txt data file.)

Please correct if my understanding is wrong.

[Rashmi Shyamasundar]

This test case was failing before committing the changeset http://trac.webkit.org/changeset/125575, to fix the bug https://bugs.webkit.org/show_bug.cgi?id=92974 .

[Rashmi Shyamasundar]

(In reply to comment #8)
> This test case was failing before committing the changeset http://trac.webkit.org/changeset/125575, to fix the bug https://bugs.webkit.org/show_bug.cgi?id=92974 .


The test case to be referred :- LayoutTests/canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html

Have raised a bug saying that the above test case is against the spec -
https://bugs.webkit.org/show_bug.cgi?id=119567

[Chris Dumez]

(In reply to comment #9)
> (In reply to comment #8)
> > This test case was failing before committing the changeset http://trac.webkit.org/changeset/125575, to fix the bug https://bugs.webkit.org/show_bug.cgi?id=92974 .
> 
> 
> The test case to be referred :- LayoutTests/canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html
> 
> Have raised a bug saying that the above test case is against the spec -
> https://bugs.webkit.org/show_bug.cgi?id=119567

As commented on that bug, the test passes for WebKit / Blink / Gecko so I would not change it.

Maybe you can use the demo in attachment, make it a ref test and include it in your patch. The demo seems to render differently in Gecko than WebKit.

[Rashmi Shyamasundar]

Created attachment 208414 [details]
Patch

[Chris Dumez]

Comment on attachment 208414 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208414&action=review

> Source/WebCore/ChangeLog:12
> +        This patch modifies the canvas functions drawTextInternal and measureText to conform to the above spec.

Does it match Firefox as well?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:93
>  

Would be nice to add a link to the specification in comment:
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#space-character

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

This function has nothing to do with replacing. Maybe we can call it isHTMLSpace() ?

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

return (c == 0x0009 || c == 0x000A || c == 0x000B || c == 0x000C || c == 0x000D);

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2111
> +    const size_t replacementLength = replacement.length();

length() returns an 'unsigned' type. I think it is best to use the same type.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2112
> +    size_t index = 0;

No need to initialize to 0.

> LayoutTests/ChangeLog:11
> +        The below listed layout tests veriy the conformance to above spec.

verify

> LayoutTests/fast/canvas/canvas-fillText-ideographicSpace-expected.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

[Chris Dumez]

Comment on attachment 208414 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208414&action=review

> Source/WebCore/ChangeLog:14
> +        Reviewed by NOBODY (OOPS!).

The reviewed by line should come before the changelog, not after.

> LayoutTests/ChangeLog:13
> +        Reviewed by NOBODY (OOPS!).

The reviewed by line should come before the changelog, not after.

[Rashmi Shyamasundar]

Created attachment 208421 [details]
Patch

[EFL EWS Bot]

Comment on attachment 208421 [details]
Patch

Attachment 208421 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1359437

[Early Warning System Bot]

Comment on attachment 208421 [details]
Patch

Attachment 208421 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1401180

[Early Warning System Bot]

Comment on attachment 208421 [details]
Patch

Attachment 208421 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1414319

[EFL EWS Bot]

Comment on attachment 208421 [details]
Patch

Attachment 208421 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1385259

[Rashmi Shyamasundar]

Created attachment 208423 [details]
Patch

[Build Bot]

Comment on attachment 208423 [details]
Patch

Attachment 208423 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1399183

New failing tests:
canvas/philip/tests/2d.text.measure.width.space.html
fast/canvas/canvas-fillText-ideographicSpace.html

[Build Bot]

Created attachment 208427 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 208423 [details]
Patch

Attachment 208423 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1405211

New failing tests:
canvas/philip/tests/2d.text.measure.width.space.html
fast/canvas/canvas-fillText-ideographicSpace.html

[Build Bot]

Created attachment 208432 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Darin Adler]

Comment on attachment 208423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208423&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:98
> +// Spec : http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#space-character
> +static inline bool isHTMLSpace(UChar c)
> +{
> +    return (c == 0x0009 || c == 0x000A || c == 0x000B || c == 0x000C || c == 0x000D);
> +}

This function already exists in HTMLParserIdioms.h and you should use the version from there instead of adding a new one.

[Rashmi Shyamasundar]

(In reply to comment #24)
> (From update of attachment 208423 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208423&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:98
> > +// Spec : http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#space-character
> > +static inline bool isHTMLSpace(UChar c)
> > +{
> > +    return (c == 0x0009 || c == 0x000A || c == 0x000B || c == 0x000C || c == 0x000D);
> > +}
> 
> This function already exists in HTMLParserIdioms.h and you should use the version from there instead of adding a new one.

The function isHTMLSpace() defined in HTMLParserIdioms.h does not behave exactly as mentioned in Spec : http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#space-character . 

It does not return true for the below characters :-
1. line feed - U+000A
2. line tabulation - U+000B (Please check the discussion at https://bugs.webkit.org/show_bug.cgi?id=119567)  

It returns true for  '\n' ( U+2424) which is not required.

[Rashmi Shyamasundar]

(In reply to comment #24)
> (From update of attachment 208423 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208423&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:98
> > +// Spec : http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#space-character
> > +static inline bool isHTMLSpace(UChar c)
> > +{
> > +    return (c == 0x0009 || c == 0x000A || c == 0x000B || c == 0x000C || c == 0x000D);
> > +}
> 
> This function already exists in HTMLParserIdioms.h and you should use the version from there instead of adding a new one.

Using the function isHTMLSpace() defined in HTMLParserIdioms.h, will cause the below layout test to fail -

LayoutTests/canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html

[Rashmi Shyamasundar]

Created attachment 208514 [details]
Patch

[Chris Dumez]

isHTMLSpace() behaves exactly according to spec so it is fine to use it. Regarding 0x000B which is not in the spec. I think we can keep it as a special case in Canvas to maintain backward compatibility and compatibility with Firefox.

So the code could look like (isHTMLSpace(ch) || ch == '0x000B') with a comment explaining why we also treat 0x000B as a space.

[Rashmi Shyamasundar]

Created attachment 208518 [details]
Patch

[Chris Dumez]

Comment on attachment 208518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208518&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:95
> +// Spec : http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#space-character

This link is not needed.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:98
> +    return (isHTMLSpace(c) || c == 0x000B);

You need to add a comment to explain why we treat 0x000B as a space here (backward compatibility and consistency with Firefox).

[Rashmi Shyamasundar]

Created attachment 208523 [details]
Patch

[Darin Adler]

Comment on attachment 208523 [details]
Patch

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!

[Rashmi Shyamasundar]

Please comment on the below function "replaceHTMLSpace()", which can be used instead of the existing function "replaceCharacterInString()"

static void replaceHTMLSpace(String& text)
{	
	unsigned int textLength = text.length();
	std::vector<char> charVector(textLength);

	unsigned int i=0;
   	while (i != textLength)
   	{
   	        ch = text.characterStartingAt(i);
		if (!isHTMLSpace(ch))
			charVector[i] = ch;
		else	
			charVector[i] = ' ';
		i++;
        }
	charVector[i] = '\0';
	string tempText(charVector.begin(), charVector.end());
	String finalText(tempText.c_str(),textLength);
        text = finalText;
}


As suggested by Mr. Darin Adler, the above approach avoids the expensive memory re-allocations unlike the function replaceCharacterInString().

[Chris Dumez]

(In reply to comment #33)
> Please comment on the below function "replaceHTMLSpace()", which can be used instead of the existing function "replaceCharacterInString()"
> 
> static void replaceHTMLSpace(String& text)
> {    
>     unsigned int textLength = text.length();
>     std::vector<char> charVector(textLength);
> 
>     unsigned int i=0;
>        while (i != textLength)
>        {
>                ch = text.characterStartingAt(i);
>         if (!isHTMLSpace(ch))
>             charVector[i] = ch;
>         else    
>             charVector[i] = ' ';
>         i++;
>         }
>     charVector[i] = '\0';
>     string tempText(charVector.begin(), charVector.end());
>     String finalText(tempText.c_str(),textLength);
>         text = finalText;
> }

A few comments comments:
- Looks like you could use a for loop.
- You can use String::adopt() to avoid copying the input Vector
- You don't handle vertical spaces anymore
- As Darin advised, you should not replace ' ' (U+0020) chars by ' '. The space matching function should return false for those
- Use a WTF::Vector<UChar>, not a std::vector<char>
- iirc Darin wanted to allocate memory only if there is at least 1 char that needs replacing
- You can use operator[] instead of characterStartingAt()

[Rashmi Shyamasundar]

(In reply to comment #33)
> Please comment on the below function "replaceHTMLSpace()", which can be used instead of the existing function "replaceCharacterInString()"
> 
> static void replaceHTMLSpace(String& text)
> {    
>     unsigned int textLength = text.length();
>     std::vector<char> charVector(textLength);
> 
>     unsigned int i=0;
>        while (i != textLength)
>        {
>                ch = text.characterStartingAt(i);
>         if (!isHTMLSpace(ch))
>             charVector[i] = ch;
>         else    
>             charVector[i] = ' ';
>         i++;
>         }
>     charVector[i] = '\0';
>     string tempText(charVector.begin(), charVector.end());
>     String finalText(tempText.c_str(),textLength);
>         text = finalText;
> }
> 
> 
> As suggested by Mr. Darin Adler, the above approach avoids the expensive memory re-allocations unlike the function replaceCharacterInString().

Re-wrote the function replaceHTMLSpace() as below :-

static void replaceHTMLSpace(String& text)
{    
    unsigned int textLength = text.length();
    const UChar *textArray = text.characters();
    UChar *finalTextArray = new UChar[textLength];

    UChar ch;
    unsigned int i=0;
       while (i != textLength)
       {
           ch = textArray[i];
        if (!isHTMLSpace(ch))
            finalTextArray[i] = ch;
        else    
            finalTextArray[i] = ' ';
        i++;
        }

    String finalText(finalTextArray, textLength);
    text = finalText;

}

[Rashmi Shyamasundar]

Created attachment 208908 [details]
Patch

[Rashmi Shyamasundar]

With this patch, the below layout test will fail, as discussed at https://bugs.webkit.org/show_bug.cgi?id=108881#c32 and https://bugs.webkit.org/show_bug.cgi?id=119567

Layout test :-
LayoutTests/canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html

[Chris Dumez]

Comment on attachment 208908 [details]
Patch

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 :- '

[Rashmi Shyamasundar]

Please comment on below piece of code :-

static void normalizeSpaces(String& text)
{
    unsigned textLength = text.length();

    unsigned i = 0;
    for (; i != textLength; ++i) {
        if (spaceNeedsReplacing(text[i]))
            break;
    }

    if (i == textLength)
        return;

    WTF::Vector<UChar> charVector(textLength);
    memcpy(&charVector, &text, i * sizeof(UChar));
    charVector[i] = ' ';
    ++i;

    for (; i != textLength; ++i) {
        if (spaceNeedsReplacing(text[i]))
            charVector[i] = ' ';
        else
            charVector[i] = text[i];
    }

    text.adopt(charVector);
}

[Chris Dumez]

(In reply to comment #39)
> Please comment on below piece of code :-
> 
> static void normalizeSpaces(String& text)
> {
>     unsigned textLength = text.length();
> 
>     unsigned i = 0;
>     for (; i != textLength; ++i) {
>         if (spaceNeedsReplacing(text[i]))
>             break;
>     }
> 
>     if (i == textLength)
>         return;
> 
>     WTF::Vector<UChar> charVector(textLength);
>     memcpy(&charVector, &text, i * sizeof(UChar));
>     charVector[i] = ' ';
>     ++i;
> 
>     for (; i != textLength; ++i) {
>         if (spaceNeedsReplacing(text[i]))
>             charVector[i] = ' ';
>         else
>             charVector[i] = text[i];
>     }
> 
>     text.adopt(charVector);
> }

Looks like you could use find in the beginning:
size_t find(CharacterMatchFunctionPtr matchFunction, unsigned start = 0) const;

> charVector[i] = ' ';
> ++i;

could be charVector[i++] = ' ';

[Chris Dumez]

(In reply to comment #40)
> (In reply to comment #39)
> > Please comment on below piece of code :-
> > 
> > static void normalizeSpaces(String& text)
> > {
> >     unsigned textLength = text.length();
> > 
> >     unsigned i = 0;
> >     for (; i != textLength; ++i) {
> >         if (spaceNeedsReplacing(text[i]))
> >             break;
> >     }
> > 
> >     if (i == textLength)
> >         return;
> > 
> >     WTF::Vector<UChar> charVector(textLength);

N WTF:: is not needed.
 
> >     memcpy(&charVector, &text, i * sizeof(UChar));

If you use String::charactersWithNullTermination(), you won't need to memcpy.

> >     charVector[i] = ' ';
> >     ++i;
> > 
> >     for (; i != textLength; ++i) {
> >         if (spaceNeedsReplacing(text[i]))
> >             charVector[i] = ' ';
> >         else
> >             charVector[i] = text[i];
If you use String::charactersWithNullTermination() you only need to replace spaces, no write non-spaces.

[Rashmi Shyamasundar]

In the case of find and memcpy, we are parsing a part of the string twice.  In the case of using for loop to find the first occurrence of space, we are parsing the string only once. So, for loop should be better to find the first occurrence of space?

[Chris Dumez]

(In reply to comment #42)
> In the case of find and memcpy, we are parsing a part of the string twice.  In the case of using for loop to find the first occurrence of space, we are parsing the string only once. So, for loop should be better to find the first occurrence of space?

I suggested to use String::charactersWithNullTermination() to initialize the Vector to avoid the memcpy already.

Regarding the find(), it returns as soon as it finds a replaceable space so it behaves exactly the same as your first loop. Duplicating code is not a good idea.

[Rashmi Shyamasundar]

Created attachment 209259 [details]
Patch

[Build Bot]

Comment on attachment 209259 [details]
Patch

Attachment 209259 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1512877

New failing tests:
canvas/philip/tests/2d.text.measure.width.space.html

[Build Bot]

Created attachment 209261 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Chris Dumez]

Comment on attachment 209259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209259&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2112
> +    int textLength = text.length();

This can be moved down a bit to avoid doing this if there is not space that needs replacing.
Also, length() returns an unsigned, not an int.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2114
> +    int i = text.find(spaceNeedsReplacing);

find() returns a size_t, not an int

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2116
> +    if (i == -1)

-1 -> use usually use notFound instead (from NotFound.h).

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2122
> +    for (; i != textLength; ++i) {

I believe 'i < textLength' is more common.

> LayoutTests/fast/canvas/canvas-measureText-ideographicSpace.html:9
> +<span style="padding: 0px; font-size: 12px; font-family: Osaka; display: inline; visibility: hidden">a&#x3000;b&#x3000;c</span>

Looks like this test is failing on ews?

[Build Bot]

Comment on attachment 209259 [details]
Patch

Attachment 209259 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1519694

New failing tests:
canvas/philip/tests/2d.text.measure.width.space.html

[Build Bot]

Created attachment 209284 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Rashmi Shyamasundar]

The failing test is : canvas/philip/tests/2d.text.measure.width.space.html. 

This test calls canvas.measureText for a string which contains the characters \x09, \x0a, \x0b, \x0c and \x0d. This test fails even without my patch. 

The diff reported by the Build Bot is :- 

Expected :- Failed assertion ctx.measureText('A \x09\x0a\x0c\x0d \x09\x0a\x0c\x0dB').width === 150 (got 450[number], expected 150[number]) 

Actual :- Failed assertion ctx.measureText('A \x09\x0a\x0c\x0d \x09\x0a\x0c\x0dB').width === 150 (got 650[number], expected 150[number])

This test crashes when opened on Mininbrowser on efl port. I wonder why this crash is not getting reported when I run the layout tests using command "./Tools/Scripts/run-webkit-tests --efl --debug LayoutTests/canvas/philip/ -2" .

The crash is resolved on efl, if I replace "\x09\x0a\x0c\x0d" with "\x0009\x000a\x000c\x000d" in the test html.

[Darin Adler]

Comment on attachment 209259 [details]
Patch

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.

[Rashmi Shyamasundar]

Created attachment 209748 [details]
Patch

[Rashmi Shyamasundar]

In the latest patch, I have incorporated all the comments from Mr. Christophe Dumez and Mr. Darin Adler except that I am using chractersWithNullTermination(). This is because, if I use "memcpy(vector.data(), text.characters(), text.length());" then, the test Layouttests/canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html fails since the text is not properly copied into vector.

[Chris Dumez]

(In reply to comment #53)
> In the latest patch, I have incorporated all the comments from Mr. Christophe Dumez and Mr. Darin Adler except that I am using chractersWithNullTermination(). This is because, if I use "memcpy(vector.data(), text.characters(), text.length());" then, the test Layouttests/canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html fails since the text is not properly copied into vector.

Have you tried?
memcpy(vector.data(), text.characters(), text.length() * sizeof(UChar));

I believe UChar has size 2, not one (unlike char) so you end up copying half the vector.

[Rashmi Shyamasundar]

Created attachment 209760 [details]
Patch

[Rashmi Shyamasundar]

(In reply to comment #54)
> (In reply to comment #53)
> > In the latest patch, I have incorporated all the comments from Mr. Christophe Dumez and Mr. Darin Adler except that I am using chractersWithNullTermination(). This is because, if I use "memcpy(vector.data(), text.characters(), text.length());" then, the test Layouttests/canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html fails since the text is not properly copied into vector.
> 
> Have you tried?
> memcpy(vector.data(), text.characters(), text.length() * sizeof(UChar));
> 
> I believe UChar has size 2, not one (unlike char) so you end up copying half the vector.


Yes. That works!

[Build Bot]

Comment on attachment 209760 [details]
Patch

Attachment 209760 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1586290

New failing tests:
canvas/philip/tests/2d.text.measure.width.space.html

[Build Bot]

Created attachment 209765 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 209760 [details]
Patch

Attachment 209760 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1589289

New failing tests:
canvas/philip/tests/2d.text.measure.width.space.html

[Build Bot]

Created attachment 209768 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Darin Adler]

Comment on attachment 209760 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209760&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:69
> +#include <wtf/NotFound.h>

No need to add this include. It’s already included as a side effect of other headers we include.

[Rashmi Shyamasundar]

Created attachment 209848 [details]
Patch

[Rashmi Shyamasundar]

In this new patch -

1. I have removed #include <wtf/NotFound.h> from CanvasRenderingContext2D.cpp
2. Removed the unnecessary comments from measureText() and drawTextInternal() in CanvasRenderingContext2D.cpp
3. Modified the file "LayoutTests/platform/mac/canvas/philip/tests/2d.text.measure.width.space-expected.txt". 

Without my patch, "ctx.measureText('A \x09\x0a\x0c\x0d \x09\x0a\x0c\x0dB')" was returning 450. With my patch it returns 650. The test case fails both with and without my patch.

[Build Bot]

Comment on attachment 209848 [details]
Patch

Attachment 209848 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1592515

New failing tests:
fast/canvas/canvas-fillText-ideographicSpace.html

[Build Bot]

Created attachment 209852 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Rashmi Shyamasundar]

(In reply to comment #64)
> (From update of attachment 209848 [details])
> Attachment 209848 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/1592515
> 
> New failing tests:
> fast/canvas/canvas-fillText-ideographicSpace.html

This test is passing in my MAC PC. I wonder why it is failing on Build-bot. I have used font "Arial" in the test.

[Chris Dumez]

(In reply to comment #66)
> (In reply to comment #64)
> > (From update of attachment 209848 [details] [details])
> > Attachment 209848 [details] [details] did not pass mac-wk2-ews (mac-wk2):
> > Output: http://webkit-queues.appspot.com/results/1592515
> > 
> > New failing tests:
> > fast/canvas/canvas-fillText-ideographicSpace.html
> 
> This test is passing in my MAC PC. I wonder why it is failing on Build-bot. I have used font "Arial" in the test.

Are you sure you tried WebKit2? (Passing "-2" to run-webkit-tests)

[Rashmi Shyamasundar]

(In reply to comment #67)
> (In reply to comment #66)
> > (In reply to comment #64)
> > > (From update of attachment 209848 [details] [details] [details])
> > > Attachment 209848 [details] [details] [details] did not pass mac-wk2-ews (mac-wk2):
> > > Output: http://webkit-queues.appspot.com/results/1592515
> > > 
> > > New failing tests:
> > > fast/canvas/canvas-fillText-ideographicSpace.html
> > 
> > This test is passing in my MAC PC. I wonder why it is failing on Build-bot. I have used font "Arial" in the test.
> 
> Are you sure you tried WebKit2? (Passing "-2" to run-webkit-tests)

Yes. I tried both, with and without "-2" option to run-webkit-tests.

[Darin Adler]

What is the nature of the failure on the build bot? What result are we getting there?

[Darin Adler]

(In reply to comment #66)
> I have used font "Arial" in the test.

Why did you chose that font in particular? I don’t think we have a guarantee that’s on all machines running our layout tests.

[Chris Dumez]

(In reply to comment #70)
> (In reply to comment #66)
> > I have used font "Arial" in the test.
> 
> Why did you chose that font in particular? I don’t think we have a guarantee that’s on all machines running our layout tests.

Arial seemed like a common font. Which font would you suggest?

I took a look at the diff on the bot and it is not obvious what the problem is. The spacing between the letters look identical, the font looks identical or extremely similar. The only noticeable difference is that the expected text is slightly more bold than the actual one.

[Darin Adler]

(In reply to comment #71)
> (In reply to comment #70)
> > (In reply to comment #66)
> > > I have used font "Arial" in the test.
> > 
> > Why did you chose that font in particular? I don’t think we have a guarantee that’s on all machines running our layout tests.
> 
> Arial seemed like a common font. Which font would you suggest?
> 
> I took a look at the diff on the bot and it is not obvious what the problem is. The spacing between the letters look identical, the font looks identical or extremely similar. The only noticeable difference is that the expected text is slightly more bold than the actual one.

My guess is that there’s a difference in anti-aliasing between the canvas and non-canvas text. Simon Fraser might have tips about how to avoid that issue.

[Darin Adler]

It might be as simple as painting an opaque background in the canvas instead of drawing on a transparent background.

[Rashmi Shyamasundar]

The test "canvas-fillText-ideographicSpace.html" is failing on MAC. The test "canvas-measureText-ideographicSpace.html" is passing on both EFL and MAC. Both of these tests use the same function "isSpaceThatNeedsReplacing()". So, the test "canvas-measureText-ideographicSpace.html" alone should suffice to validate my patch. Right?

[Rashmi Shyamasundar]

Created attachment 211185 [details]
Patch

[Darin Adler]

(In reply to comment #74)
> The test "canvas-fillText-ideographicSpace.html" is failing on MAC. The test "canvas-measureText-ideographicSpace.html" is passing on both EFL and MAC. Both of these tests use the same function "isSpaceThatNeedsReplacing()". So, the test "canvas-measureText-ideographicSpace.html" alone should suffice to validate my patch. Right?

No. We can’t check in a patch that makes a test start failing without taking some kind of action to mitigate the failure.

[Chris Dumez]

(In reply to comment #76)
> (In reply to comment #74)
> > The test "canvas-fillText-ideographicSpace.html" is failing on MAC. The test "canvas-measureText-ideographicSpace.html" is passing on both EFL and MAC. Both of these tests use the same function "isSpaceThatNeedsReplacing()". So, the test "canvas-measureText-ideographicSpace.html" alone should suffice to validate my patch. Right?
> 
> No. We can’t check in a patch that makes a test start failing without taking some kind of action to mitigate the failure.

I don't understand your comment. It does not look like this patch makes any test start failing.
This patch initially introduced 2 tests, one ref test and one regular test. The regular test is passing everywhere but the ref test is failing on mac-wk2 for some reason (aliasing?). So Rashmi is proposing to include only the test that is passing in his patch and remove the ref test. According to him, it provides good enough coverage for his fix.

[Darin Adler]

OK, that makes sense. I’d love to add the ref. test later if we can get it to work.

[Darin Adler]

Comment on attachment 211185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=211185&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2133
> +    unsigned textLength = text.length();
> +    Vector<UChar> charVector(textLength);
> +    memcpy(charVector.data(), text.characters(), textLength * sizeof(UChar));
> +
> +    charVector[i++] = ' ';
> +
> +    for (; i < textLength; ++i) {
> +        if (isSpaceThatNeedsReplacing(charVector[i]))
> +            charVector[i] = ' ';
> +    }
> +    text = String::adopt(charVector);

If it ever matters, some day we might want to return to this and make an optimized path for 8-bit character strings; this implementation as is will have a side effect of expanding any strings with a space that needs replacing to 16-bit characters.

[WebKit Commit Bot]

Comment on attachment 211185 [details]
Patch

Clearing flags on attachment: 211185

Committed r155596: <http://trac.webkit.org/changeset/155596>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Corrected the test in <http://trac.webkit.org/r155650> - location of js-test scripts changed since the patch was posted. Also removed <meta charcode>, because that's not a thing.