Bug 108881 - Canvas fillText and measureText handle ideographic spaces differently
Summary: Canvas fillText and measureText handle ideographic spaces differently
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-04 17:20 PST by Jason Parrott
Modified: 2013-09-12 14:39 PDT (History)
20 users (show)

See Also:


Attachments
Demo showing bug. (916 bytes, text/html)
2013-02-04 17:20 PST, Jason Parrott
no flags Details
Patch (6.10 KB, patch)
2013-08-07 01:41 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Patch (9.33 KB, patch)
2013-08-09 03:56 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Patch (9.41 KB, patch)
2013-08-09 05:45 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Patch (9.42 KB, patch)
2013-08-09 05:59 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (561.64 KB, application/zip)
2013-08-09 06:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (544.93 KB, application/zip)
2013-08-09 07:12 PDT, Build Bot
no flags Details
Patch (9.42 KB, patch)
2013-08-12 00:49 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Patch (9.64 KB, patch)
2013-08-12 01:51 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Patch (9.64 KB, patch)
2013-08-12 03:08 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Patch (9.42 KB, patch)
2013-08-16 04:06 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Patch (9.43 KB, patch)
2013-08-21 03:31 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (485.82 KB, application/zip)
2013-08-21 04:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (817.41 KB, application/zip)
2013-08-21 10:24 PDT, Build Bot
no flags Details
Patch (10.24 KB, patch)
2013-08-27 05:10 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Patch (10.29 KB, patch)
2013-08-27 06:09 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (449.37 KB, application/zip)
2013-08-27 07:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (512.09 KB, application/zip)
2013-08-27 07:25 PDT, Build Bot
no flags Details
Patch (10.69 KB, patch)
2013-08-27 22:53 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (811.55 KB, application/zip)
2013-08-28 00:45 PDT, Build Bot
no flags Details
Patch (9.06 KB, patch)
2013-09-10 01:58 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Parrott 2013-02-04 17:20:55 PST
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.
Comment 1 Rashmi Shyamasundar 2013-08-07 01:41:27 PDT
Created attachment 208243 [details]
Patch
Comment 2 Martin Robinson 2013-08-07 01:45:45 PDT
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.
Comment 3 Rashmi Shyamasundar 2013-08-07 02:13:36 PDT
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?
Comment 4 Martin Robinson 2013-08-07 02:21:32 PDT
(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.
Comment 5 Rashmi Shyamasundar 2013-08-07 05:05:34 PDT
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;
}
Comment 6 Martin Robinson 2013-08-07 05:17:53 PDT
(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;
Comment 7 Rashmi Shyamasundar 2013-08-07 05:53:52 PDT
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.
Comment 8 Rashmi Shyamasundar 2013-08-08 02:38:41 PDT
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 .
Comment 9 Rashmi Shyamasundar 2013-08-08 03:28:01 PDT
(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
Comment 10 Chris Dumez 2013-08-08 04:55:48 PDT
(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.
Comment 11 Rashmi Shyamasundar 2013-08-09 03:56:43 PDT
Created attachment 208414 [details]
Patch
Comment 12 Chris Dumez 2013-08-09 04:26:57 PDT
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>
Comment 13 Chris Dumez 2013-08-09 04:28:09 PDT
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.
Comment 14 Rashmi Shyamasundar 2013-08-09 05:45:37 PDT
Created attachment 208421 [details]
Patch
Comment 15 EFL EWS Bot 2013-08-09 05:49:28 PDT
Comment on attachment 208421 [details]
Patch

Attachment 208421 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1359437
Comment 16 Early Warning System Bot 2013-08-09 05:50:13 PDT
Comment on attachment 208421 [details]
Patch

Attachment 208421 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1401180
Comment 17 Early Warning System Bot 2013-08-09 05:50:25 PDT
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
Comment 18 EFL EWS Bot 2013-08-09 05:52:40 PDT
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
Comment 19 Rashmi Shyamasundar 2013-08-09 05:59:53 PDT
Created attachment 208423 [details]
Patch
Comment 20 Build Bot 2013-08-09 06:25:47 PDT
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
Comment 21 Build Bot 2013-08-09 06:25:51 PDT
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
Comment 22 Build Bot 2013-08-09 07:12:38 PDT
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
Comment 23 Build Bot 2013-08-09 07:12:43 PDT
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
Comment 24 Darin Adler 2013-08-09 13:42:44 PDT
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.
Comment 25 Rashmi Shyamasundar 2013-08-12 00:17:25 PDT
(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.
Comment 26 Rashmi Shyamasundar 2013-08-12 00:31:03 PDT
(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
Comment 27 Rashmi Shyamasundar 2013-08-12 00:49:41 PDT
Created attachment 208514 [details]
Patch
Comment 28 Chris Dumez 2013-08-12 01:06:44 PDT
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.
Comment 29 Rashmi Shyamasundar 2013-08-12 01:51:18 PDT
Created attachment 208518 [details]
Patch
Comment 30 Chris Dumez 2013-08-12 02:55:18 PDT
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).
Comment 31 Rashmi Shyamasundar 2013-08-12 03:08:24 PDT
Created attachment 208523 [details]
Patch
Comment 32 Darin Adler 2013-08-12 09:56:28 PDT
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!
Comment 33 Rashmi Shyamasundar 2013-08-13 05:59:50 PDT
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().
Comment 34 Chris Dumez 2013-08-13 06:27:05 PDT
(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()
Comment 35 Rashmi Shyamasundar 2013-08-13 06:31:40 PDT
(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;

}
Comment 36 Rashmi Shyamasundar 2013-08-16 04:06:28 PDT
Created attachment 208908 [details]
Patch
Comment 37 Rashmi Shyamasundar 2013-08-16 04:40:51 PDT
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
Comment 38 Chris Dumez 2013-08-16 04:45:34 PDT
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 :- '
Comment 39 Rashmi Shyamasundar 2013-08-16 06:27:25 PDT
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);
}
Comment 40 Chris Dumez 2013-08-16 06:31:12 PDT
(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++] = ' ';
Comment 41 Chris Dumez 2013-08-16 06:39:24 PDT
(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.
Comment 42 Rashmi Shyamasundar 2013-08-16 07:45:27 PDT
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?
Comment 43 Chris Dumez 2013-08-16 08:20:13 PDT
(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.
Comment 44 Rashmi Shyamasundar 2013-08-21 03:31:41 PDT
Created attachment 209259 [details]
Patch
Comment 45 Build Bot 2013-08-21 04:40:34 PDT
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
Comment 46 Build Bot 2013-08-21 04:40:38 PDT
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
Comment 47 Chris Dumez 2013-08-21 06:20:56 PDT
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?
Comment 48 Build Bot 2013-08-21 10:24:28 PDT
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
Comment 49 Build Bot 2013-08-21 10:24:34 PDT
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
Comment 50 Rashmi Shyamasundar 2013-08-21 22:55:22 PDT
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.
Comment 51 Darin Adler 2013-08-22 09:40:04 PDT
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.
Comment 52 Rashmi Shyamasundar 2013-08-27 05:10:33 PDT
Created attachment 209748 [details]
Patch
Comment 53 Rashmi Shyamasundar 2013-08-27 05:16:08 PDT
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.
Comment 54 Chris Dumez 2013-08-27 05:26:01 PDT
(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.
Comment 55 Rashmi Shyamasundar 2013-08-27 06:09:41 PDT
Created attachment 209760 [details]
Patch
Comment 56 Rashmi Shyamasundar 2013-08-27 06:11:20 PDT
(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!
Comment 57 Build Bot 2013-08-27 07:01:09 PDT
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
Comment 58 Build Bot 2013-08-27 07:01:17 PDT
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
Comment 59 Build Bot 2013-08-27 07:25:12 PDT
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
Comment 60 Build Bot 2013-08-27 07:25:19 PDT
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
Comment 61 Darin Adler 2013-08-27 09:52:37 PDT
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.
Comment 62 Rashmi Shyamasundar 2013-08-27 22:53:17 PDT
Created attachment 209848 [details]
Patch
Comment 63 Rashmi Shyamasundar 2013-08-27 23:01:49 PDT
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.
Comment 64 Build Bot 2013-08-28 00:45:44 PDT
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
Comment 65 Build Bot 2013-08-28 00:45:50 PDT
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
Comment 66 Rashmi Shyamasundar 2013-08-28 04:21:09 PDT
(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.
Comment 67 Chris Dumez 2013-08-28 04:32:15 PDT
(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)
Comment 68 Rashmi Shyamasundar 2013-08-28 04:48:16 PDT
(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.
Comment 69 Darin Adler 2013-08-28 09:24:18 PDT
What is the nature of the failure on the build bot? What result are we getting there?
Comment 70 Darin Adler 2013-08-28 09:36:16 PDT
(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.
Comment 71 Chris Dumez 2013-08-28 09:38:51 PDT
(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.
Comment 72 Darin Adler 2013-08-28 09:49:51 PDT
(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.
Comment 73 Darin Adler 2013-08-28 09:50:44 PDT
It might be as simple as painting an opaque background in the canvas instead of drawing on a transparent background.
Comment 74 Rashmi Shyamasundar 2013-09-10 01:48:18 PDT
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?
Comment 75 Rashmi Shyamasundar 2013-09-10 01:58:33 PDT
Created attachment 211185 [details]
Patch
Comment 76 Darin Adler 2013-09-10 12:25:01 PDT
(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.
Comment 77 Chris Dumez 2013-09-10 12:35:40 PDT
(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.
Comment 78 Darin Adler 2013-09-10 12:36:44 PDT
OK, that makes sense. I’d love to add the ref. test later if we can get it to work.
Comment 79 Darin Adler 2013-09-10 12:39:12 PDT
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.
Comment 80 WebKit Commit Bot 2013-09-11 23:20:47 PDT
Comment on attachment 211185 [details]
Patch

Clearing flags on attachment: 211185

Committed r155596: <http://trac.webkit.org/changeset/155596>
Comment 81 WebKit Commit Bot 2013-09-11 23:20:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 82 Alexey Proskuryakov 2013-09-12 14:39:56 PDT
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.