Correct font not selected to calculate text run width

Correct font not selected to calculate text run width

Original patch: https://codereview.chromium.org/680213004

Breaking original patch into two parts for better tracking.

In RenderText.cpp, when width of text run is calculated sometimes
different fonts are used to get width of string which is different than
the font used to draw text.

This happens due to fallback font architecture which results in font
change. We need to make call to calculate width only when primary font 
has a glyph for the requested codepoint.

BUG=416425
TEST=fast/css/font-face-opentype.html, fast/text/line-break-after-question-mark.html

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

[h.joshi, 2014-12-08 13:01:05]

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

[Dominik Röttsches, 2014-12-15 09:48:59]

dominik.rottsches@intel.com changed reviewers:
+ dominik.rottsches@intel.com

[Dominik Röttsches, 2014-12-15 09:49:00]

Please update your change description to contain a one-line descriptive headline
with what functional change you're introducing.

https://codereview.chromium.org/763973003/diff/1/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/763973003/diff/1/LayoutTests/TestExpectations...
LayoutTests/TestExpectations:1452: # Test case needs to be updated
When you write "Test case needs to be updated", I understand that there might be
something wrong with the test itself. In that case, please explain. Otherwise,
the [ NeedsRebaseline ] tag already explains that you're requesting an update of
the baseline, in which case no comment is needed here.

https://codereview.chromium.org/763973003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderText.cpp (right):

https://codereview.chromium.org/763973003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderText.cpp:730: RenderStyle * renderStyle = style();
No space between type name and *.
http://www.chromium.org/blink/coding-style#TOC-Pointers-and-References

[h.joshi, 2014-12-16 06:47:46]

Patchset #2 (id:20001) has been deleted

[h.joshi, 2014-12-16 06:55:05]

Patchset #2 (id:40001) has been deleted

[h.joshi, 2014-12-16 07:02:20]

@Dominik: Thank you for the review.
Suggested changes are made.

https://codereview.chromium.org/763973003/diff/1/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/763973003/diff/1/LayoutTests/TestExpectations...
LayoutTests/TestExpectations:1452: # Test case needs to be updated
Done, removed comment.

https://codereview.chromium.org/763973003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderText.cpp (right):

https://codereview.chromium.org/763973003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderText.cpp:730: RenderStyle * renderStyle = style();
On 2014/12/15 09:49:00, Dominik Röttsches wrote:
> No space between type name and *.
> http://www.chromium.org/blink/coding-style#TOC-Pointers-and-References

Done.

[Dominik Röttsches, 2014-12-16 11:53:36]

"Correct font not selected to calculate text run width" works as a headline. 
Please put this as the first line in the description, since this is where the
commit message is taken from, then an empty line after that, then the remaining
explanations.

> In previous patch some issue was observed due to SVG fonts,
> SVG test cases are removed now.

What do you mean by this? Do you mean that the patch now does not break
SVG test cases anymore?

> When making calls to calculate width we need to check if Unicode
> is present in primary font or not.

Please rephrase this sentence, you probably mean: "...if the primary font
has a glyph for the requested codepoint."

[h.joshi, 2014-12-16 13:15:11]

> "Correct font not selected to calculate text run width" works as a headline. 
> Please put this as the first line in the description, since this is where the
> commit message is taken from, then an empty line after that, then the
remaining
> explanations.
Done 

> What do you mean by this? Do you mean that the patch now does not break
> SVG test cases anymore?
After original patch, few SVG Font test case were flaky, mainly on Windows bots.

All SVG font test cases were removed later in other patch as Blink no longer 
supports SVG font, due to low usage.
Output for those required to be updated due to change in text run width by this
patch.

 
> Please rephrase this sentence, you probably mean: "...if the primary font
> has a glyph for the requested codepoint."
Done

[Dominik Röttsches, 2014-12-16 14:21:24]

It's usually a good idea to trigger trybots during development of your patch. It
helps the reviewer see whether the patch applies cleanly and whether there are
any issues with failing tests. If you don't have trybot access, let me know, I
can request it for you on the committers mailing list.

LGTM

[h.joshi, 2014-12-16 16:59:01]

@Dominik: Thank you, 
I have trybots access, Will make sure to run trybots on all patches 
(this patch was taken from old patch so did not ran trybots this time).

[h.joshi, 2014-12-17 10:10:25]

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

[commit-bot: I haz the power, 2014-12-17 10:11:22]

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

[commit-bot: I haz the power, 2014-12-17 10:12:17]

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

[commit-bot: I haz the power, 2014-12-17 10:12:18]

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 1574.
  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
78faf8ab8bb31dc6351122c0001bd131dbddf751..8a70fef3afb3fec78de8bc4edb7e59a45385dd3f
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1574,3 +1574,6 @@ crbug.com/441743 [ Mac Win ] plugins/js-from-destroy.html
[ Timeout Crash ]
 
 crbug.com/439856 virtual/deferred/fast/images/pixelated-composited.html [
ImageOnlyFailure ]
 crbug.com/442181 virtual/slimmingpaint/fast/images/pixelated-composited.html [
ImageOnlyFailure ]
+
+crbug.com/416425 [ Win Mac ] fast/css/font-face-opentype.html [ NeedsRebaseline
]
+crbug.com/416425 [ Win Linux ] fast/text/line-break-after-question-mark.html [
NeedsRebaseline ]

[h.joshi, 2014-12-17 10:21:57]

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

[commit-bot: I haz the power, 2014-12-17 10:22:09]

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

[commit-bot: I haz the power, 2014-12-17 10:28:00]

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

[commit-bot: I haz the power, 2014-12-17 10:28:01]

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

[h.joshi, 2014-12-17 10:34:35]

@Emil: Pls review, getting following error 
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    Source/core/rendering/RenderText.cpp

[eae, 2014-12-17 17:08:04]

On 2014/12/17 10:34:35, h.joshi wrote:
> @Emil: Pls review, getting following error 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for these files:
>     Source/core/rendering/RenderText.cpp

LGTM, rubber-stamping based off Dominiks review

[eae, 2014-12-17 17:08:15]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-12-17 17:09:02]

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

[commit-bot: I haz the power, 2014-12-17 17:12:44]

Committed patchset #3 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187387