text-combine should scale rather than fall back to none when wide

text-combine should scale rather than fall back to none when wide

This patch follows the two spec change in 2014 CR:
1. Compress rather than fallback when width > ~1em.
   Old WD said to fallback to none.
2. For line breaking before and after the composition, it is treated as a
   regular inline with its actual contents.
   Old WD said it should be treated as U+FFFC.

CR: http://www.w3.org/TR/css-writing-modes-3/#text-combine
ED: http://dev.w3.org/csswg/css-writing-modes/#text-combine

In addition to the changes to RenderCombineText, this spec change has a
small but pleasant side effect to our implementation. The
RenderCombinedText delays the width calculation until it's needed. But the
old spec said that it has to be treated as an U+FFFC OBJECT REPLACEMENT
CHARACTER to line breakers when combined.

From these two, line breakers used be aware of RenderCombinedText and
updated it (combineText()) when needed. Also, the fact that combineText()
could change the length of text brought additional complexity. We had a
few calls to combineText() in line breakers, and still has some flakiness
such as bug 312606. The new spec eliminates this complexity, so our line
breakers are a bit cleaner, faster, and less flaky.

fast/text/font-variant-width.html had to be rewritten because it relied on
the behavior of text-combine to fallback when wide.

BUG=433176, 312606
TEST=fast/writing-mode/text-combine-compress.html

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

[kojii, 2014-12-29 19:08:08]

Patchset #1 (id:1) has been deleted

[kojii, 2014-12-29 19:08:16]

Patchset #1 (id:20001) has been deleted

[kojii, 2014-12-29 19:08:23]

Patchset #1 (id:40001) has been deleted

[kojii, 2014-12-29 21:32:13]

Patchset #1 (id:60001) has been deleted

[kojii, 2014-12-30 10:56:31]

Patchset #2 (id:100001) has been deleted

[kojii, 2015-01-21 01:08:43]

kojii@chromium.org changed reviewers:
+ kouhei@chromium.org

[kojii, 2015-01-21 01:08:44]

kouhei@, PTAL!

[kouhei (in TOK), 2015-01-21 04:25:04]

mostly lg

https://codereview.chromium.org/799123003/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderCombineText.cpp (right):

https://codereview.chromium.org/799123003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderCombineText.cpp:34: , m_scaleX(0)
I prefer this default to 1.0f.

https://codereview.chromium.org/799123003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderCombineText.cpp:75: auto renderingWidth =
m_combinedTextWidth;
float

[kojii, 2015-01-21 07:00:24]

both done, PTAL!

https://codereview.chromium.org/799123003/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderCombineText.cpp (right):

https://codereview.chromium.org/799123003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderCombineText.cpp:34: , m_scaleX(0)
On 2015/01/21 04:25:04, kouhei wrote:
> I prefer this default to 1.0f.

Done.

https://codereview.chromium.org/799123003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderCombineText.cpp:75: auto renderingWidth =
m_combinedTextWidth;
On 2015/01/21 04:25:04, kouhei wrote:
> float

Done.

[kouhei (in TOK), 2015-01-21 08:43:34]

https://codereview.chromium.org/799123003/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderCombineText.cpp (right):

https://codereview.chromium.org/799123003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderCombineText.cpp:76: if (m_scaleX)
Please update logics too.

no if needed if default is m_scaleX.

https://codereview.chromium.org/799123003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderCombineText.cpp:83: ASSERT(!m_needsFontUpdate);
+ ASSERT(isTransformNeeded());

https://codereview.chromium.org/799123003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderCombineText.cpp:148: m_scaleX = 0;
1.0f;

https://codereview.chromium.org/799123003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderCombineText.cpp:167: m_scaleX = 0;
1.0f;

https://codereview.chromium.org/799123003/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderCombineText.h (right):

https://codereview.chromium.org/799123003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderCombineText.h:35: bool isTransformNeeded() const {
return m_scaleX; }
m_scaleX != 1.0f;

[Dominik Röttsches, 2015-01-21 13:07:50]

Just mentioning, as a reply to your changelog: Note that I changed the character
that's used for width calculation when drawing emphasis marks in
https://codereview.chromium.org/605953002 since I thought doing it this way was
minimal and less intrusive than other solutions. I don't think that conflicts
with the intent of the spec or overlaps with your change here - just mostly fyi.

[kojii, 2015-01-21 15:55:33]

On 2015/01/21 13:07:50, Dominik Röttsches wrote:
> Just mentioning, as a reply to your changelog: Note that I changed the
character
> that's used for width calculation when drawing emphasis marks in
> https://codereview.chromium.org/605953002 since I thought doing it this way
was
> minimal and less intrusive than other solutions. I don't think that conflicts
> with the intent of the spec or overlaps with your change here - just mostly
fyi.

Thank you for letting me know. One of CJK code point looks safer than U+FFFC to
me too.

I quickly tested some fonts and it looks good. I actually worried a bit, since
you commented
to use space but actually used FullStop, and FullStop is a code point sometimes
proportional.

But quick test results look good, and spec-wise, I do not see any problems
either. Thank you!

[kojii, 2015-01-21 18:04:02]

Apologies, how I could miss these...all fixed, PTAL.

https://codereview.chromium.org/799123003/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderCombineText.cpp (right):

https://codereview.chromium.org/799123003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderCombineText.cpp:76: if (m_scaleX)
On 2015/01/21 08:43:34, kouhei wrote:
> Please update logics too.
> 
> no if needed if default is m_scaleX.

Done.

https://codereview.chromium.org/799123003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderCombineText.cpp:83: ASSERT(!m_needsFontUpdate);
On 2015/01/21 08:43:34, kouhei wrote:
> + ASSERT(isTransformNeeded());

Done.

https://codereview.chromium.org/799123003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderCombineText.cpp:148: m_scaleX = 0;
On 2015/01/21 08:43:34, kouhei wrote:
> 1.0f;

Done.

https://codereview.chromium.org/799123003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderCombineText.cpp:167: m_scaleX = 0;
On 2015/01/21 08:43:34, kouhei wrote:
> 1.0f;

Done.

https://codereview.chromium.org/799123003/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderCombineText.h (right):

https://codereview.chromium.org/799123003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderCombineText.h:35: bool isTransformNeeded() const {
return m_scaleX; }
On 2015/01/21 08:43:34, kouhei wrote:
> m_scaleX != 1.0f;

Done.

[kouhei (in TOK), 2015-01-21 18:05:55]

almost there

https://codereview.chromium.org/799123003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderCombineText.h (right):

https://codereview.chromium.org/799123003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderCombineText.h:35: bool isTransformNeeded() const {
return m_scaleX < 1.0f; }
Not sure here, if m_scaleX > 1.0f, wouldn't it still need transform?

[kojii, 2015-01-22 02:05:51]

On 2015/01/21 18:05:55, kouhei wrote:
> almost there
> 
>
https://codereview.chromium.org/799123003/diff/160001/Source/core/rendering/R...
> File Source/core/rendering/RenderCombineText.h (right):
> 
>
https://codereview.chromium.org/799123003/diff/160001/Source/core/rendering/R...
> Source/core/rendering/RenderCombineText.h:35: bool isTransformNeeded() const {
> return m_scaleX < 1.0f; }
> Not sure here, if m_scaleX > 1.0f, wouldn't it still need transform?

The spec requires shrink if > 1em, but render as is when < 1em.
|updateIsCombinedAndText()| sets |m_scaleX| only when |m_combinedTextWidth <=
emWidth|, so on the current code base, |m_scaleX| should be always |<= 1.0f| and
whichever works.

The spec specifically says "fits within 1em by compressing" and does not allow
expanding, so I felt this might express what the spec says, but since both are
the same, I'm fine either way.
http://dev.w3.org/csswg/css-writing-modes-3/#text-combine-compression

[kouhei (in TOK), 2015-01-22 03:05:10]

On 2015/01/22 02:05:51, koji wrote:
> On 2015/01/21 18:05:55, kouhei wrote:
> > almost there
> > 
> >
>
https://codereview.chromium.org/799123003/diff/160001/Source/core/rendering/R...
> > File Source/core/rendering/RenderCombineText.h (right):
> > 
> >
>
https://codereview.chromium.org/799123003/diff/160001/Source/core/rendering/R...
> > Source/core/rendering/RenderCombineText.h:35: bool isTransformNeeded() const
{
> > return m_scaleX < 1.0f; }
> > Not sure here, if m_scaleX > 1.0f, wouldn't it still need transform?
> 
> The spec requires shrink if > 1em, but render as is when < 1em.
> |updateIsCombinedAndText()| sets |m_scaleX| only when |m_combinedTextWidth <=
> emWidth|, so on the current code base, |m_scaleX| should be always |<= 1.0f|
and
> whichever works.
> 
> The spec specifically says "fits within 1em by compressing" and does not allow
> expanding, so I felt this might express what the spec says, but since both are
> the same, I'm fine either way.
> http://dev.w3.org/csswg/css-writing-modes-3/#text-combine-compression

Ah, makes sense. Thanks for clarification.
Would you add a comment pointing to the spec? This is not very obvious.

[kouhei (in TOK), 2015-01-22 03:05:25]

lgtm

[kojii, 2015-01-22 06:02:38]

kojii@chromium.org changed reviewers:
+ leviw@chromium.org

[kojii, 2015-01-22 06:02:39]

leviw@, PTAL!

Spec comments & link added.

[kojii, 2015-01-30 06:05:53]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2015-01-30 06:06:44]

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

[kojii, 2015-01-30 06:07:10]

On 2015/01/22 06:02:39, koji wrote:
> leviw@, PTAL!
> 
> Spec comments & link added.

I somehow misunderstood that I need another lgtm than from kouhei@, but after
talking with him offline, it looks like I'm ok to go. Sorry for bothering.

[commit-bot: I haz the power, 2015-01-30 06:10:26]

Committed patchset #5 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189235

[vsevik, 2015-01-30 09:47:37]

A revert of this CL (patchset #5 id:180001) has been created in
https://codereview.chromium.org/881133003/ by vsevik@chromium.org.

The reason for reverting is: Makes fast/text/text-combine-selection-crash.html
crash on mac:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fas....

[vsevik, 2015-01-30 10:05:05]

Reverted in https://codereview.chromium.org/890853002/

[kojii, 2015-01-30 10:06:04]

On 2015/01/30 09:47:37, vsevik wrote:
> A revert of this CL (patchset #5 id:180001) has been created in
> https://codereview.chromium.org/881133003/ by mailto:vsevik@chromium.org.
> 
> The reason for reverting is: Makes fast/text/text-combine-selection-crash.html
> crash on mac:
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fas....

Thanks, it looks like something was changed since the last test run. I'll look
into this.

[kojii, 2015-02-01 15:42:19]

kojii@chromium.org changed reviewers:
- leviw@chromium.org

[kojii, 2015-02-01 16:15:51]

Patchset #6 (id:200001) has been deleted

[kojii, 2015-02-02 07:00:37]

kohei@, PTAL!

[kouhei (in TOK), 2015-02-03 01:33:58]

still lgtm

[kojii, 2015-02-03 04:19:06]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2015-02-03 04:20:08]

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

[commit-bot: I haz the power, 2015-02-03 04:23:44]

Committed patchset #10 (id:300001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189382