Avoid re-parsing of string in fillText and measureText in Canvas

Avoid re-parsing of string in fillText and measureText calls of Canvas 2D.

Allowing "width" method in Font.cpp to handle space normalization.

R=eae@chromium.org
BUG=413225
TEST=fast/canvas/canvas-normalize-string.html

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

[h.joshi, 2014-09-11 12:59:27]

h.joshi@samsung.com changed reviewers:
- eae@chromium.org

[h.joshi, 2014-09-12 14:54:09]

h.joshi@samsung.com changed reviewers:
+ behdad@chromium.org, eae@chromium.org

[h.joshi, 2014-09-12 14:54:29]

Pls review

[eae, 2014-09-12 20:33:28]

This doesn't seem right. Could you please explain what you're trying to do here?

[h.joshi, 2014-09-13 14:35:06]

When fillText or measureText is called in Canvas 2D, we first call
"normalizeSpaces" in
which we loop through string and change Tabs, CR/LF/NL to "space" Unicode, then
this
new string is passed to "width" methods of Font object.
 In "width" method of Fonts, we again loop through complete string, with this
patch,
avoiding "normalizeSpaces" loop and shifting calculation into Font class
methods.

[h.joshi, 2014-09-13 20:25:53]

Submitted patch also shifts all text handling 
to font module, so that other layers using fonts 
does not have to do any special handling.

[eae, 2014-09-16 00:46:34]

Thanks for the explanation and test!

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Fo...
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Fo...
Source/platform/fonts/Font.cpp:572: glyphData = glyphDataForCharacter(character,
false, false, EmphasisMarkVariant);
bool normalizeSpace = false;

glyphData = glyphDataForCharacter(character, false, normalizeSpace,
EmphasisMarkVariant);

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Wi...
File Source/platform/fonts/WidthIterator.cpp (left):

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Wi...
Source/platform/fonts/WidthIterator.cpp:109: // FIXME: This does a little extra
work that could be avoided if
This change doesn't seem related to your space normalization patch at all.

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Wi...
File Source/platform/fonts/WidthIterator.cpp (right):

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Wi...
Source/platform/fonts/WidthIterator.cpp:185: bool isSpaceNormalize =
m_run.isSpaceNormalize();
s/isSPaceNormalize/normalizeSpace/

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Wi...
File Source/platform/fonts/WidthIterator.h (right):

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Wi...
Source/platform/fonts/WidthIterator.h:82: GlyphData
glyphDataForCharacter(CharacterData&, bool spaceNormalize = false);
normalizeSpace

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Wi...
Source/platform/fonts/WidthIterator.h:84: void cacheFallbackFont(const
SimpleFontData*, const SimpleFontData* primaryFont);
Why is the character param no longer needed here?

https://codereview.chromium.org/567543002/diff/20001/Source/platform/text/Tex...
File Source/platform/text/TextRun.h (right):

https://codereview.chromium.org/567543002/diff/20001/Source/platform/text/Tex...
Source/platform/text/TextRun.h:174: bool isSpaceNormalize() const { return
m_spaceNormalize; }
bool normalizeSpace()
void setNormalizeSpace

[h.joshi, 2014-09-16 05:48:11]

@Emil: Made suggested changes
Pls review

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Fo...
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Fo...
Source/platform/fonts/Font.cpp:572: glyphData = glyphDataForCharacter(character,
false, false, EmphasisMarkVariant);
On 2014/09/16 00:46:33, eae wrote:
> bool normalizeSpace = false;
> 
> glyphData = glyphDataForCharacter(character, false, normalizeSpace,
> EmphasisMarkVariant);

Done.

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Wi...
File Source/platform/fonts/WidthIterator.cpp (left):

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Wi...
Source/platform/fonts/WidthIterator.cpp:109: // FIXME: This does a little extra
work that could be avoided if
"glyphDataAndPageForCharacter" method takes care of NormalVariant and SmallCaps
variant, so passing of "character" is not required now.
 This change is not exactly related to current "normalize space" changes, I was
making changes and need to pass "normalize space flag" to this method also. But
as "character" is not required in this method so modified calling parameters.

 Pls suggest, if I need to make separate patch for this?

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Wi...
File Source/platform/fonts/WidthIterator.cpp (right):

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Wi...
Source/platform/fonts/WidthIterator.cpp:185: bool isSpaceNormalize =
m_run.isSpaceNormalize();
Done, changed to normalizeSpace

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Wi...
File Source/platform/fonts/WidthIterator.h (right):

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Wi...
Source/platform/fonts/WidthIterator.h:82: GlyphData
glyphDataForCharacter(CharacterData&, bool spaceNormalize = false);
On 2014/09/16 00:46:34, eae wrote:
> normalizeSpace

Done.

https://codereview.chromium.org/567543002/diff/20001/Source/platform/fonts/Wi...
Source/platform/fonts/WidthIterator.h:84: void cacheFallbackFont(const
SimpleFontData*, const SimpleFontData* primaryFont);
"glyphDataAndPageForCharacter" method takes care of NormalVariant and SmallCaps
variant, so passing of "character" is not required now.
 This change is not exactly related to current "normalize space" changes, I was
making changes and need to pass "normalize space flag" to this method also. But
as "character" is not required in this method so modified calling parameters.

https://codereview.chromium.org/567543002/diff/20001/Source/platform/text/Tex...
File Source/platform/text/TextRun.h (right):

https://codereview.chromium.org/567543002/diff/20001/Source/platform/text/Tex...
Source/platform/text/TextRun.h:174: bool isSpaceNormalize() const { return
m_spaceNormalize; }
On 2014/09/16 00:46:34, eae wrote:
> bool normalizeSpace()
> void setNormalizeSpace

Done.

[h.joshi, 2014-09-18 05:11:28]

@Emil: Pls review this patch.

[h.joshi, 2014-09-23 05:08:29]

@Emil: PTAL

[eae, 2014-09-24 07:51:17]

LGTM

[eae, 2014-09-24 07:51:38]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-09-24 07:52:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/567543002/60001

[commit-bot: I haz the power, 2014-09-24 09:16:14]

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

[commit-bot: I haz the power, 2014-09-24 09:16:15]

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

[h.joshi, 2014-09-24 10:33:04]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-09-24 10:33:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/567543002/80001

[h.joshi, 2014-09-24 10:36:41]

Thank you Emil.

[commit-bot: I haz the power, 2014-09-24 12:41:33]

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

[commit-bot: I haz the power, 2014-09-24 12:41:48]

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

[commit-bot: I haz the power, 2014-09-24 12:41:50]

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

[h.joshi, 2014-09-25 09:07:25]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-09-25 09:08:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/567543002/80001

[commit-bot: I haz the power, 2014-09-25 09:08:49]

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

[commit-bot: I haz the power, 2014-09-25 09:08:50]

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 1321.
  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
7006f5bf7c40d9fb3b31161c31348b4e5968c401..34f6eb63587025026b06345f5587fc0b8e24c1d1
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1321,3 +1321,7 @@ crbug.com/417182 [ Mac ]
compositing/geometry/fixed-position-composited-page-sca
 crbug.com/417182 [ Mac ]
compositing/geometry/fixed-position-transform-composited-page-scale-down.html [
NeedsRebaseline ]
 
 crbug.com/416846 inspector/elements/styles/styles-add-new-rule.html [ Pass
Failure ]
+
+# Need to rebased after https://codereview.chromium.org/567543002/
+crbug.com/413225 [ Mac ] fast/canvas/canvas-normalize-string.html [
NeedsRebaseline ]
+crbug.com/413225 [ Mac ] virtual/gpu/fast/canvas/canvas-normalize-string.html [
NeedsRebaseline ]

[h.joshi, 2014-09-25 10:27:25]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-09-25 10:28:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/567543002/100001

[commit-bot: I haz the power, 2014-09-25 11:36:05]

Committed patchset #6 (id:100001) as 182675