Canvas fillText and measureText handle ideographic spaces differently.

Canvas fillText and measureText handle ideographic spaces differently.

According to specification[1] all space characters should be replaced with
0x0020 space character. (U+0009, U+000A, U+000C, U+000D -> U+0020)
BTW, ideographic space(U+3000) isn't space characters what the spec[2] says.
This change aligns behavior with Gecko and WebKit.

[1] text preparation algorithm:
  http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#text-preparation-algorithm

[2] space characters
  http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#space-character

This patch is based on following patch from WebKit:
  https://bugs.webkit.org/show_bug.cgi?id=108881

BUG=171639
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174663

[zino, 2014-05-19 11:17:55]

Please take a look.

Thank you :)

[Justin Novosad, 2014-05-20 18:29:50]

https://codereview.chromium.org/293653002/diff/1/LayoutTests/fast/canvas/canv...
File LayoutTests/fast/canvas/canvas-text-ideographic-space.html (right):

https://codereview.chromium.org/293653002/diff/1/LayoutTests/fast/canvas/canv...
LayoutTests/fast/canvas/canvas-text-ideographic-space.html:29: var abc = 'a' +
'\u3000' + 'b' + '\u3000' + 'c';
Please add more tests to cover cases that contain newline chracters and cases
with consecutive space chracters.

https://codereview.chromium.org/293653002/diff/1/Source/core/html/canvas/Canv...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/293653002/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2125:
replaceCharacterInString(normalizedText, isSpaceCharacters, " ");
What exactly is wrong with isSpaceOrNewline? Why are you defining a new function
instead of correcting isSpaceOrNewline?

[zino, 2014-05-21 15:08:01]

On 2014/05/20 18:29:50, junov wrote:
>
https://codereview.chromium.org/293653002/diff/1/LayoutTests/fast/canvas/canv...
> File LayoutTests/fast/canvas/canvas-text-ideographic-space.html (right):
> 
>
https://codereview.chromium.org/293653002/diff/1/LayoutTests/fast/canvas/canv...
> LayoutTests/fast/canvas/canvas-text-ideographic-space.html:29: var abc = 'a' +
> '\u3000' + 'b' + '\u3000' + 'c';
> Please add more tests to cover cases that contain newline chracters and cases
> with consecutive space chracters.

Thanks for pointing out.
Actually, there was a discrepancy between the length of before string and the
length of replaced string.

> 
>
https://codereview.chromium.org/293653002/diff/1/Source/core/html/canvas/Canv...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/293653002/diff/1/Source/core/html/canvas/Canv...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:2125:
> replaceCharacterInString(normalizedText, isSpaceCharacters, " ");
> What exactly is wrong with isSpaceOrNewline? Why are you defining a new
function
> instead of correcting isSpaceOrNewline?

isSpaceOrNewline covers space characters and all BIDI WS characters, including
ideographic space.
But the character U+3000 should not be replaced with U+0020 when drawing text on
canvas
according to the text preparation algorithm.
(isSpaceOrNewline is defined in StringImpl.h and it seems to be used more
generally)

I've just uploaded the new patch set 3 which includes the new function to
substitute replaceCharacterInString().
The algorithm used in the old function is highly inefficient and we should not
use it.

If you want more details, please refer to
https://bugs.webkit.org/show_bug.cgi?id=108881.

Thank you!

[Justin Novosad, 2014-05-21 15:14:32]

Thanks for the explanation.

lgtm

[Justin Novosad, 2014-05-21 15:14:46]

https://codereview.chromium.org/293653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/293653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2027: static inline bool
isSpaceCharacters(UChar c)
Remove 's' at the end of name. There is just one character.

[zino, 2014-05-21 15:19:14]

On 2014/05/21 15:14:46, junov wrote:
>
https://codereview.chromium.org/293653002/diff/40001/Source/core/html/canvas/...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/293653002/diff/40001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:2027: static inline bool
> isSpaceCharacters(UChar c)
> Remove 's' at the end of name. There is just one character.

I addressed it. Thank you.

[Justin Novosad, 2014-05-21 19:44:47]

On 2014/05/21 15:19:14, zino wrote:
> On 2014/05/21 15:14:46, junov wrote:
> >
>
https://codereview.chromium.org/293653002/diff/40001/Source/core/html/canvas/...
> > File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> > 
> >
>
https://codereview.chromium.org/293653002/diff/40001/Source/core/html/canvas/...
> > Source/core/html/canvas/CanvasRenderingContext2D.cpp:2027: static inline
bool
> > isSpaceCharacters(UChar c)
> > Remove 's' at the end of name. There is just one character.
> 
> I addressed it. Thank you.

lgtm

[zino, 2014-05-21 23:43:47]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-05-21 23:46:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/293653002/60001

[commit-bot: I haz the power, 2014-05-21 23:46:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-21 23:46:22]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 994.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
b7c87bbb013dbe4e504e9b1e5f1714f9b5bb0c81..b9635e1e2e8d501554ddd7ca5e08f2c7bc6cccbb
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -994,3 +994,6 @@ crbug.com/366442 svg/custom/masking-clipping-hidpi.svg [
Pass Failure ]
 crbug.com/375565 [ Mac ] fast/repaint/caret-with-transformation.html [ Pass
Failure ]
 
 crbug.com/375567 web-animations-api/w3c/simple-keyframes.html [ Pass Failure ]
+
+crbug.com/171639 fast/canvas/canvas-text-space-characters.html [
NeedsRebaseline ]
+crbug.com/171639 fast/canvas/canvas-text-ideographic-space.html [
NeedsRebaseline ]

[zino, 2014-05-22 09:34:57]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-05-22 09:35:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/293653002/80001

[zino, 2014-05-22 10:23:37]

The CQ bit was unchecked by jinho.bang@samsung.com

[zino, 2014-05-23 02:20:50]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-05-23 02:21:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/293653002/100001

[commit-bot: I haz the power, 2014-05-23 03:42:00]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/9461)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9262)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/8712)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/8813)

[commit-bot: I haz the power, 2014-05-23 04:24:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-23 04:24:00]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/8723)

[zino, 2014-05-23 07:03:58]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-05-23 07:04:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/293653002/150001

[commit-bot: I haz the power, 2014-05-23 08:26:48]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/8854)

[commit-bot: I haz the power, 2014-05-23 10:16:38]

Change committed as 174663