Enable hyphens: auto and none in BreakingContext

Enable hyphens: auto and none in BreakingContext

This patch enables "hyphens: auto"[1] (automatic hyphenation using the
dictionaries) and "hyphens: no" (disables soft-hyphens for heading and
a11y) in BreakingContext, along with a mock hyphenation dictionary for
testing.

The code to support hyphenation in InlineTextBox, LayoutText,
LayoutTreeAsText, etc. were still there and reused in this code.

LayoutText::computePreferredLogicalWidths() needs to support the
"hyphens" property to compute the min-content width[2]. This will be in
following patches.

Platform support for dictionaries will also be in following patches.

Detailed control of hyphenation defined in CSS Text Level 4[3] is not
included in this phase.

[1] https://drafts.csswg.org/css-text-3/#hyphens-property
[2] https://drafts.csswg.org/css-sizing-3/#min-content
[3] https://drafts.csswg.org/css-text-4/#hyphenation

BUG=605840
Committed: https://crrev.com/52bb31c6d49c5376103577363720410994e55113
Cr-Commit-Position: refs/heads/master@{#393972}

[kojii, 2016-05-16 04:11:27]

Description was changed from

==========
WIP: hyphens

BUG=605840
==========

to

==========
Enable hyphens: auto and none in BreakingContext

This patch enables "hyphens: auto"[1] (automatic hyphenation using the
dictionaries) and "hyphens: no" (disables soft-hyphens for heading and
a11y) in BreakingContext, along with a mock hyphenation dictionary.

The code to support hyphenation in InlineTextBox, LayoutText,
LayoutTreeAsText, etc. were still there and reused in this code.

LayoutText::computePreferredLogicalWidths() needs to support the
"hyphens" property to compute the min-content width[3]. This will be in
following patches.

Platform support for dictionaries will also in following patches.

Detailed control of hyphenation defined in CSS Text Level 4[2] is not
included in this phase.

[1] https://drafts.csswg.org/css-text-3/#hyphens-property
[2] https://drafts.csswg.org/css-text-4/#hyphenation
[3] https://drafts.csswg.org/css-sizing-3/#min-content

BUG=605840
==========

[kojii, 2016-05-16 04:12:40]

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

[kojii, 2016-05-16 04:12:41]

PTAL.

cc: drott@ as this include one argument removal from the API surface, and the
impl of the mock.

[kojii, 2016-05-16 04:15:47]

Description was changed from

==========
Enable hyphens: auto and none in BreakingContext

This patch enables "hyphens: auto"[1] (automatic hyphenation using the
dictionaries) and "hyphens: no" (disables soft-hyphens for heading and
a11y) in BreakingContext, along with a mock hyphenation dictionary.

The code to support hyphenation in InlineTextBox, LayoutText,
LayoutTreeAsText, etc. were still there and reused in this code.

LayoutText::computePreferredLogicalWidths() needs to support the
"hyphens" property to compute the min-content width[3]. This will be in
following patches.

Platform support for dictionaries will also in following patches.

Detailed control of hyphenation defined in CSS Text Level 4[2] is not
included in this phase.

[1] https://drafts.csswg.org/css-text-3/#hyphens-property
[2] https://drafts.csswg.org/css-text-4/#hyphenation
[3] https://drafts.csswg.org/css-sizing-3/#min-content

BUG=605840
==========

to

==========
Enable hyphens: auto and none in BreakingContext

This patch enables "hyphens: auto"[1] (automatic hyphenation using the
dictionaries) and "hyphens: no" (disables soft-hyphens for heading and
a11y) in BreakingContext, along with a mock hyphenation dictionary.

The code to support hyphenation in InlineTextBox, LayoutText,
LayoutTreeAsText, etc. were still there and reused in this code.

LayoutText::computePreferredLogicalWidths() needs to support the
"hyphens" property to compute the min-content width[3]. This will be in
following patches.

Platform support for dictionaries will also be in following patches.

Detailed control of hyphenation defined in CSS Text Level 4[2] is not
included in this phase.

[1] https://drafts.csswg.org/css-text-3/#hyphens-property
[2] https://drafts.csswg.org/css-text-4/#hyphenation
[3] https://drafts.csswg.org/css-sizing-3/#min-content

BUG=605840
==========

[kojii, 2016-05-16 04:16:15]

Description was changed from

==========
Enable hyphens: auto and none in BreakingContext

This patch enables "hyphens: auto"[1] (automatic hyphenation using the
dictionaries) and "hyphens: no" (disables soft-hyphens for heading and
a11y) in BreakingContext, along with a mock hyphenation dictionary.

The code to support hyphenation in InlineTextBox, LayoutText,
LayoutTreeAsText, etc. were still there and reused in this code.

LayoutText::computePreferredLogicalWidths() needs to support the
"hyphens" property to compute the min-content width[3]. This will be in
following patches.

Platform support for dictionaries will also be in following patches.

Detailed control of hyphenation defined in CSS Text Level 4[2] is not
included in this phase.

[1] https://drafts.csswg.org/css-text-3/#hyphens-property
[2] https://drafts.csswg.org/css-text-4/#hyphenation
[3] https://drafts.csswg.org/css-sizing-3/#min-content

BUG=605840
==========

to

==========
Enable hyphens: auto and none in BreakingContext

This patch enables "hyphens: auto"[1] (automatic hyphenation using the
dictionaries) and "hyphens: no" (disables soft-hyphens for heading and
a11y) in BreakingContext, along with a mock hyphenation dictionary.

The code to support hyphenation in InlineTextBox, LayoutText,
LayoutTreeAsText, etc. were still there and reused in this code.

LayoutText::computePreferredLogicalWidths() needs to support the
"hyphens" property to compute the min-content width[2]. This will be in
following patches.

Platform support for dictionaries will also be in following patches.

Detailed control of hyphenation defined in CSS Text Level 4[3] is not
included in this phase.

[1] https://drafts.csswg.org/css-text-3/#hyphens-property
[2] https://drafts.csswg.org/css-sizing-3/#min-content
[3] https://drafts.csswg.org/css-text-4/#hyphenation

BUG=605840
==========

[kojii, 2016-05-16 04:16:57]

Description was changed from

==========
Enable hyphens: auto and none in BreakingContext

This patch enables "hyphens: auto"[1] (automatic hyphenation using the
dictionaries) and "hyphens: no" (disables soft-hyphens for heading and
a11y) in BreakingContext, along with a mock hyphenation dictionary.

The code to support hyphenation in InlineTextBox, LayoutText,
LayoutTreeAsText, etc. were still there and reused in this code.

LayoutText::computePreferredLogicalWidths() needs to support the
"hyphens" property to compute the min-content width[2]. This will be in
following patches.

Platform support for dictionaries will also be in following patches.

Detailed control of hyphenation defined in CSS Text Level 4[3] is not
included in this phase.

[1] https://drafts.csswg.org/css-text-3/#hyphens-property
[2] https://drafts.csswg.org/css-sizing-3/#min-content
[3] https://drafts.csswg.org/css-text-4/#hyphenation

BUG=605840
==========

to

==========
Enable hyphens: auto and none in BreakingContext

This patch enables "hyphens: auto"[1] (automatic hyphenation using the
dictionaries) and "hyphens: no" (disables soft-hyphens for heading and
a11y) in BreakingContext, along with a mock hyphenation dictionary for
testing.

The code to support hyphenation in InlineTextBox, LayoutText,
LayoutTreeAsText, etc. were still there and reused in this code.

LayoutText::computePreferredLogicalWidths() needs to support the
"hyphens" property to compute the min-content width[2]. This will be in
following patches.

Platform support for dictionaries will also be in following patches.

Detailed control of hyphenation defined in CSS Text Level 4[3] is not
included in this phase.

[1] https://drafts.csswg.org/css-text-3/#hyphens-property
[2] https://drafts.csswg.org/css-sizing-3/#min-content
[3] https://drafts.csswg.org/css-text-4/#hyphenation

BUG=605840
==========

[drott, 2016-05-16 08:58:19]

drott@chromium.org changed reviewers:
+ drott@chromium.org

[drott, 2016-05-16 08:58:20]

Looks well written and well tested to me, but I don't understand this part of
the layout code well enough to do a full review. Hence, I only have two minor
nits.

https://codereview.chromium.org/1978683002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h
(right):

https://codereview.chromium.org/1978683002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:103:
bool canBreakAtWhitespace(bool breakWords, WordMeasurement&, bool
stoppedIgnoringSpaces, bool& hyphenated, float charWidth, bool
disableSoftHyphen, float& hyphenWidth, bool betweenWords, bool midWordBreak,
bool canBreakMidWord, bool previousCharacterIsSpace, float lastWidthMeasurement,
const LineLayoutText&, const Font&, bool applyWordSpacing, float wordSpacing);
Would it make sense to bring all the hyphenation related arguments together, for
example by moving charWidth before hyphenated?
- bool& hyphenated
- bool disableSoftHyphen
- float& hyphenWidth,

https://codereview.chromium.org/1978683002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:646:
if (maxPrefixWidth <= font.getFontDescription().computedSize() * 5 / 4)
Could you perhaps explain the 5/4 magic number and move it to a constant?

[kojii, 2016-05-16 10:21:53]

https://codereview.chromium.org/1978683002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h
(right):

https://codereview.chromium.org/1978683002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:103:
bool canBreakAtWhitespace(bool breakWords, WordMeasurement&, bool
stoppedIgnoringSpaces, bool& hyphenated, float charWidth, bool
disableSoftHyphen, float& hyphenWidth, bool betweenWords, bool midWordBreak,
bool canBreakMidWord, bool previousCharacterIsSpace, float lastWidthMeasurement,
const LineLayoutText&, const Font&, bool applyWordSpacing, float wordSpacing);
On 2016/05/16 at 08:58:20, drott wrote:
> Would it make sense to bring all the hyphenation related arguments together,
for example by moving charWidth before hyphenated?
> - bool& hyphenated
> - bool disableSoftHyphen
> - float& hyphenWidth,

Done.

https://codereview.chromium.org/1978683002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:646:
if (maxPrefixWidth <= font.getFontDescription().computedSize() * 5 / 4)
On 2016/05/16 at 08:58:20, drott wrote:
> Could you perhaps explain the 5/4 magic number and move it to a constant?

I guess it's a heuristic value introduced in WebKit. It was there when we
removed hyphenation in crbug.com/107111 (crrev.com/20526006) and I brought it
back. Added const's.

[eae, 2016-05-16 16:43:26]

https://codereview.chromium.org/1978683002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h
(right):

https://codereview.chromium.org/1978683002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:640:
float maxPrefixWidth = m_width.availableWidth() + LayoutUnit::epsilon()
Do we need epsilon here? If we can ceil a floor to layout unit instead
LayoutUnit::fromFloatCeil that might allow us to not have to add epsilon.

https://codereview.chromium.org/1978683002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:645:
const int minPrefixWidthNumerator = 5;
Where did you get these numbers from?

https://codereview.chromium.org/1978683002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:663:
FloatRect rect = font.selectionRectForText(run, FloatPoint(), 0, 0,
prefixLength);
Calling shaper.getCharacterRange instead of selection rect would be better as
that gives access to the actual character positions and doesn't do the snapping
or height adjustments.

[kojii, 2016-05-16 17:29:34]

https://codereview.chromium.org/1978683002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h
(right):

https://codereview.chromium.org/1978683002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:640:
float maxPrefixWidth = m_width.availableWidth() + LayoutUnit::epsilon()
On 2016/05/16 at 16:43:26, eae wrote:
> Do we need epsilon here? If we can ceil a floor to layout unit instead
LayoutUnit::fromFloatCeil that might allow us to not have to add epsilon.

When tests use ch/em that should exactly fit, this is needed. And also this is
aligned with LineWidth::fitsOnLine() adds epsilon. In break-all, without
epsilon, we can fit slightly less in existing tests.

But from the test in this CL, it doesn't fit when it's "3ch" (so the test has
"3.1ch"), and this is a new code path, so this can be removed if I adjust tests
to have slight buffer for other widths too. PS7 removes epsilon without
modifying tests, will add ".1ch" to tests if any fail.

https://codereview.chromium.org/1978683002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:645:
const int minPrefixWidthNumerator = 5;
On 2016/05/16 at 16:43:25, eae wrote:
> Where did you get these numbers from?

From our removal patch (crrev.com/20526006). ToT WebKit has the same number.

https://codereview.chromium.org/1978683002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:663:
FloatRect rect = font.selectionRectForText(run, FloatPoint(), 0, 0,
prefixLength);
On 2016/05/16 at 16:43:25, eae wrote:
> Calling shaper.getCharacterRange instead of selection rect would be better as
that gives access to the actual character positions and doesn't do the snapping
or height adjustments.

Done.

[eae, 2016-05-16 17:32:38]

LGTM

https://codereview.chromium.org/1978683002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h
(right):

https://codereview.chromium.org/1978683002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:640:
float maxPrefixWidth = m_width.availableWidth() + LayoutUnit::epsilon()
On 2016/05/16 17:29:34, kojii wrote:
> On 2016/05/16 at 16:43:26, eae wrote:
> > Do we need epsilon here? If we can ceil a floor to layout unit instead
> LayoutUnit::fromFloatCeil that might allow us to not have to add epsilon.
> 
> When tests use ch/em that should exactly fit, this is needed. And also this is
> aligned with LineWidth::fitsOnLine() adds epsilon. In break-all, without
> epsilon, we can fit slightly less in existing tests.
> 
> But from the test in this CL, it doesn't fit when it's "3ch" (so the test has
> "3.1ch"), and this is a new code path, so this can be removed if I adjust
tests
> to have slight buffer for other widths too. PS7 removes epsilon without
> modifying tests, will add ".1ch" to tests if any fail.

Thanks. We're trying to remove epsilon from LineWidth::fitsOnLine.

https://codereview.chromium.org/1978683002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:645:
const int minPrefixWidthNumerator = 5;
On 2016/05/16 17:29:34, kojii wrote:
> On 2016/05/16 at 16:43:25, eae wrote:
> > Where did you get these numbers from?
> 
> From our removal patch (crrev.com/20526006). ToT WebKit has the same number.

Ok, could you add a quick comment explaining the numbers?

https://codereview.chromium.org/1978683002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:663:
FloatRect rect = font.selectionRectForText(run, FloatPoint(), 0, 0,
prefixLength);
On 2016/05/16 17:29:34, kojii wrote:
> On 2016/05/16 at 16:43:25, eae wrote:
> > Calling shaper.getCharacterRange instead of selection rect would be better
as
> that gives access to the actual character positions and doesn't do the
snapping
> or height adjustments.
> 
> Done.

Thanks!`

[kojii, 2016-05-16 18:29:28]

On 2016/05/16 at 17:32:38, eae wrote:
> > PS7 removes epsilon without
> > modifying tests, will add ".1ch" to tests if any fail.
> 
> Thanks. We're trying to remove epsilon from LineWidth::fitsOnLine.

Some tests failed, adjusted. Thank you for the background.

> > > Where did you get these numbers from?
> > 
> > From our removal patch (crrev.com/20526006). ToT WebKit has the same number.
> 
> Ok, could you add a quick comment explaining the numbers?

Done, this was added in http://wkb.ug/45606 back in 2010.

[kojii, 2016-05-16 23:08:42]

The CQ bit was checked by kojii@chromium.org

[kojii, 2016-05-16 23:08:42]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1978683002/#ps140001
(title: "Add comments for heuristic, adjust widths of hyphens-auto-mock test")

[commit-bot: I haz the power, 2016-05-16 23:09:12]

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

[commit-bot: I haz the power, 2016-05-16 23:14:22]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-16 23:16:40]

Description was changed from

==========
Enable hyphens: auto and none in BreakingContext

This patch enables "hyphens: auto"[1] (automatic hyphenation using the
dictionaries) and "hyphens: no" (disables soft-hyphens for heading and
a11y) in BreakingContext, along with a mock hyphenation dictionary for
testing.

The code to support hyphenation in InlineTextBox, LayoutText,
LayoutTreeAsText, etc. were still there and reused in this code.

LayoutText::computePreferredLogicalWidths() needs to support the
"hyphens" property to compute the min-content width[2]. This will be in
following patches.

Platform support for dictionaries will also be in following patches.

Detailed control of hyphenation defined in CSS Text Level 4[3] is not
included in this phase.

[1] https://drafts.csswg.org/css-text-3/#hyphens-property
[2] https://drafts.csswg.org/css-sizing-3/#min-content
[3] https://drafts.csswg.org/css-text-4/#hyphenation

BUG=605840
==========

to

==========
Enable hyphens: auto and none in BreakingContext

This patch enables "hyphens: auto"[1] (automatic hyphenation using the
dictionaries) and "hyphens: no" (disables soft-hyphens for heading and
a11y) in BreakingContext, along with a mock hyphenation dictionary for
testing.

The code to support hyphenation in InlineTextBox, LayoutText,
LayoutTreeAsText, etc. were still there and reused in this code.

LayoutText::computePreferredLogicalWidths() needs to support the
"hyphens" property to compute the min-content width[2]. This will be in
following patches.

Platform support for dictionaries will also be in following patches.

Detailed control of hyphenation defined in CSS Text Level 4[3] is not
included in this phase.

[1] https://drafts.csswg.org/css-text-3/#hyphens-property
[2] https://drafts.csswg.org/css-sizing-3/#min-content
[3] https://drafts.csswg.org/css-text-4/#hyphenation

BUG=605840

Committed: https://crrev.com/52bb31c6d49c5376103577363720410994e55113
Cr-Commit-Position: refs/heads/master@{#393972}
==========

[commit-bot: I haz the power, 2016-05-16 23:16:41]

Patchset 8 (id:??) landed as
https://crrev.com/52bb31c6d49c5376103577363720410994e55113
Cr-Commit-Position: refs/heads/master@{#393972}