Using fullstop Unicode if horizontal ellipsis not present

Using Fullstop Unicode if horizontal ellipsis not present in primary font.

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

In case of Ellipsis, we need to check if Ellipsis Unicode is present
in primary font or not, if not then use "Dot" (full-stop) Unicode, this is also
suggested in CSS3 (http://www.w3.org/TR/2003/CR-css3-text-20030514/)

BUG=416425
TEST=fast/text/ellipsis-platform-font-change.html

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

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

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

[Dominik Röttsches, 2014-12-15 09:37:40]

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

[Dominik Röttsches, 2014-12-15 09:37:41]

Thanks for your patch, good that you're trying to improve ellipsis rendering.
However, i see a couple of issues with this patch.

Also, it is a convention to format your CL description with a one-line
descriptive headline, an empty line, optionally followed by a meaningful
description.

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

https://codereview.chromium.org/781003003/diff/1/LayoutTests/TestExpectations...
LayoutTests/TestExpectations:1447: # Test case needs to be updated
What needs to be updated? The baselines, or the tests themselves?

https://codereview.chromium.org/781003003/diff/1/LayoutTests/fast/text/ellips...
File LayoutTests/fast/text/ellipsis-platform-font-change.html (right):

https://codereview.chromium.org/781003003/diff/1/LayoutTests/fast/text/ellips...
LayoutTests/fast/text/ellipsis-platform-font-change.html:7: src:
url(../../third_party/AndroidEmojiMonospace/AndroidEmoji.ttf.subset-uni203C.ttf)
format("truetype");
See comment on README.Chromium file. Could you explain the purpose of using this
font if the font does not contain any glyphs for the text "A,,,B" you are using
it for?

https://codereview.chromium.org/781003003/diff/1/LayoutTests/third_party/Andr...
File LayoutTests/third_party/AndroidEmojiMonospace/README.chromium (right):

https://codereview.chromium.org/781003003/diff/1/LayoutTests/third_party/Andr...
LayoutTests/third_party/AndroidEmojiMonospace/README.chromium:15: $ python
pyftsubset.py AndroidEmoji.ttf --unicodes=U+203C
I can't follow here. Can you explain why are you adding this font? You are
reducing the font to U+203C "DOUBLE EXCLAMATION MARK", but I do not see that
codepoint used anywhere. Typo "monosoace".

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

https://codereview.chromium.org/781003003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockLineLayout.cpp:1959: ellipsisStr =
fullstopCharacterStr;
Are you intentionally reassigning the value of the static local variable? I've
seen those variables mostly seen used in a const-way, this reads a little
confusing.

https://codereview.chromium.org/781003003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockLineLayout.cpp:1960: firstLineEllipsisWidth =
firstLineFont.width(constructTextRun(this, firstLineFont, &fullstopCharacter, 1,
firstLineStyle(), ellipsisDirection)) * 3;
Multiplying by three is not necessarily identical to taking the width of the
whole string. Why not use fullstopCharacterStr here?

https://codereview.chromium.org/781003003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockLineLayout.cpp:1968: ellipsisWidth =
font.width(constructTextRun(this, font, &fullstopCharacter, 1, style(),
ellipsisDirection)) * 3;
See above.

[h.joshi, 2014-12-24 06:30:59]

Added new test code and made suggested changes.

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

https://codereview.chromium.org/781003003/diff/1/LayoutTests/TestExpectations...
LayoutTests/TestExpectations:1447: # Test case needs to be updated
Done, removed comment. 
For mentioned test, rebase line is required.

https://codereview.chromium.org/781003003/diff/1/LayoutTests/fast/text/ellips...
File LayoutTests/fast/text/ellipsis-platform-font-change.html (right):

https://codereview.chromium.org/781003003/diff/1/LayoutTests/fast/text/ellips...
LayoutTests/fast/text/ellipsis-platform-font-change.html:7: src:
url(../../third_party/AndroidEmojiMonospace/AndroidEmoji.ttf.subset-uni203C.ttf)
format("truetype");
Pls refer to comment in README.Chromium

https://codereview.chromium.org/781003003/diff/1/LayoutTests/third_party/Andr...
File LayoutTests/third_party/AndroidEmojiMonospace/README.chromium (right):

https://codereview.chromium.org/781003003/diff/1/LayoutTests/third_party/Andr...
LayoutTests/third_party/AndroidEmojiMonospace/README.chromium:15: $ python
pyftsubset.py AndroidEmoji.ttf --unicodes=U+203C
This font and test was made for old patch
(https://codereview.chromium.org/680213004).
In which was trying to test Monochrome font for which "widthFromCache" is called
and 
we were calculating width using wrong font.

 Later that patch was split into two for better handling of patch, I was busy
with OTS patch
thus did not got time to update font and test case, that was the reason I did
not raised patch 
review request.

Now new font and new test case is made.

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

https://codereview.chromium.org/781003003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockLineLayout.cpp:1959: ellipsisStr =
fullstopCharacterStr;
Made changes to the code and avoided reassigning.

https://codereview.chromium.org/781003003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockLineLayout.cpp:1960: firstLineEllipsisWidth =
firstLineFont.width(constructTextRun(this, firstLineFont, &fullstopCharacter, 1,
firstLineStyle(), ellipsisDirection)) * 3;
Made suggested changes.

[h.joshi, 2014-12-24 06:32:00]

Currently Windows bot (win_blink_rel) is failing.
I tried 2-3 times and every times its failing in different test case.

[h.joshi, 2015-01-08 10:03:47]

Pls review

[Dominik Röttsches, 2015-01-08 13:02:25]

https://codereview.chromium.org/781003003/diff/60001/LayoutTests/fast/text/el...
File LayoutTests/fast/text/ellipsis-platform-font-change.html (right):

https://codereview.chromium.org/781003003/diff/60001/LayoutTests/fast/text/el...
LayoutTests/fast/text/ellipsis-platform-font-change.html:25: <!--
Please do not submit unused/commented-out code.

https://codereview.chromium.org/781003003/diff/60001/LayoutTests/fast/text/el...
LayoutTests/fast/text/ellipsis-platform-font-change.html:28: <div
id="div">ABAB</div>
Can the test case be extended to cover the firstline case that's special-cased
in the code?

https://codereview.chromium.org/781003003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/781003003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockLineLayout.cpp:1966: else
I think you might be missing useHorizontalEllipsis = false; here - or you might
even need two variables of the kind, if the firstLineFont != font and one has an
ellipsis glyph and the other one doesn't (see below). 

But instead of introducing more booleans, it may be better to not use the
boolean but instead use one additional AtomicString& reference to denote the
string that gets used - more similar to your first approach but without
overriding a static local.

https://codereview.chromium.org/781003003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockLineLayout.cpp:1993: totalLogicalWidth =
curr->placeEllipsis(ellipsisStr, ltr, blockLeftEdge.toFloat(),
blockRightEdge.toFloat(), width.toFloat());
Does this code need to differentiate between firstLineFont and font?

[Dominik Röttsches, 2015-01-08 13:06:07]

>
https://codereview.chromium.org/781003003/diff/1/LayoutTests/fast/text/ellips...
> LayoutTests/fast/text/ellipsis-platform-font-change.html:7: src:
>
url(../../third_party/AndroidEmojiMonospace/AndroidEmoji.ttf.subset-uni203C.ttf)
> format("truetype");
> Pls refer to comment in README.Chromium

I am not sure whether this addresses my comment. You seem to have updated the
README.Chromium - but the filename still indicates the subset only contains an
exclamation mark.

[Dominik Röttsches, 2015-01-08 13:07:49]

Ah, never mind this one, sorry - I see the filename has changed.

[Dominik Röttsches, 2015-01-14 08:18:01]

LGTM with nit: Please remove the unused boolean.

https://codereview.chromium.org/781003003/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/781003003/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockLineLayout.cpp:1952: bool useHorizontalEllipsis
= true;
This shouldn't be needed anymore.

https://codereview.chromium.org/781003003/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockLineLayout.cpp:1959: useHorizontalEllipsis =
false;
Dito.

[h.joshi, 2015-01-14 08:19:58]

@Dominik: thank you for reviewing.
Was busy with some other stuff and was not able to reply to comments after 
updated code.
 Will fix all nit comments.
Thank u again for review.

[h.joshi, 2015-01-14 10:17:27]

Made suggested nit changes.

https://codereview.chromium.org/781003003/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/781003003/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockLineLayout.cpp:1952: bool useHorizontalEllipsis
= true;
On 2015/01/14 08:18:01, Dominik Röttsches wrote:
> This shouldn't be needed anymore.

Done.

https://codereview.chromium.org/781003003/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockLineLayout.cpp:1959: useHorizontalEllipsis =
false;
On 2015/01/14 08:18:01, Dominik Röttsches wrote:
> Dito.

Done.

https://codereview.chromium.org/781003003/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockLineLayout.cpp:1959: useHorizontalEllipsis =
false;
On 2015/01/14 08:18:01, Dominik Röttsches wrote:
> Dito.

Done.

[h.joshi, 2015-01-14 12:36:00]

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

[commit-bot: I haz the power, 2015-01-14 12:37:02]

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

[commit-bot: I haz the power, 2015-01-14 12:43:14]

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

[commit-bot: I haz the power, 2015-01-14 12:43:15]

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, 2015-01-17 09:38:20]

On 2015/01/14 12:43:15, I haz the power (commit-bot) wrote:
> 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, 2015-01-17 09:39:18]

Looking into following two error in Linux Debug bots which are not
reported on my machine:
virtual/slimmingpaint/fast/text/ellipsis-stroked.html
virtual/slimmingpaint/fast/text/emphasis-ellipsis-complextext.html

[h.joshi, 2015-01-20 09:27:15]

@Emil: Pls review
Linux bit issues are fixed now, they require update in expectation file.

[Dominik Röttsches, 2015-01-20 11:49:28]

Usually minor TestExpectations updates do not require a new review. LGTM, you
can go ahead and commit.

[h.joshi, 2015-01-20 12:15:19]

Thank you Dominik.

[h.joshi, 2015-01-20 12:15:25]

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

[commit-bot: I haz the power, 2015-01-20 12:16:17]

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

[commit-bot: I haz the power, 2015-01-20 12:21:58]

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

[commit-bot: I haz the power, 2015-01-20 12:21:59]

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, 2015-01-20 13:07:21]

Getting following error:

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    Source/core/rendering/RenderBlockLineLayout.cpp
    Source/wtf/unicode/CharacterNames.h

[h.joshi, 2015-01-20 13:11:29]

h.joshi@samsung.com changed reviewers:
+ jchaffraix@chromium.org

[h.joshi, 2015-01-20 13:11:56]

+Adding jchaffraix@chromium.org for CharacterNames.h
Pls review

[h.joshi, 2015-01-22 06:19:31]

@Emil, jchaffraix: Pls review

[tkent, 2015-02-02 00:36:55]

CharacterNames.h lgtm

[eae, 2015-02-02 00:39:43]

LGTM

[h.joshi, 2015-06-10 07:12:50]

@Emil: Pls review
somehow I forgot to commit this patch and now RenderBlockLineLayout is changed
to LayoutBlockFlowLine 
due to which rebase was required.

[eae, 2015-06-10 15:28:54]

LGTM

[h.joshi, 2015-06-11 04:11:51]

Thank you Emil

[h.joshi, 2015-06-11 04:11:58]

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

[h.joshi, 2015-06-11 04:11:58]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
dominik.rottsches@intel.com
Link to the patchset: https://codereview.chromium.org/781003003/#ps160001
(title: "Rebase patch")

[commit-bot: I haz the power, 2015-06-11 04:12:03]

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

[commit-bot: I haz the power, 2015-06-11 05:27:56]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196912