FATAL:DCHECK failed on BreakingContext::rewindToMidWordBreak()

FATAL:DCHECK failed on BreakingContext::rewindToMidWordBreak()

Fix for wrong line-breaking calculation on "word-break:break-all" case

BUG=613881
Committed: https://crrev.com/47e23e263b551925dccd72e0b170f64cf7ab8b9a
Cr-Commit-Position: refs/heads/master@{#421830}

[kyounga.ra, 2016-09-23 08:27:22]

kyounga.ra@gmail.com changed reviewers:
+ kojii@chromium.org

[kyounga.ra, 2016-09-23 08:27:22]

Hello, Kojii

I made a simple patch for bug=613881.
https://bugs.chromium.org/p/chromium/issues/detail?id=613881

Could you have a time to review this?

Thanks
Kyounga.

[kyounga.ra, 2016-09-26 00:09:07]

Description was changed from

==========
FATAL:DCHECK failed on BreakingContext::rewindToMidWordBreak()

Fix for wrong line-breaking calculation on "word-break:break-all" case

BUG=613883
==========

to

==========
FATAL:DCHECK failed on BreakingContext::rewindToMidWordBreak()

Fix for wrong line-breaking calculation on "word-break:break-all" case

BUG=613881
==========

[kyounga.ra, 2016-09-26 07:20:14]

kyounga.ra@gmail.com changed reviewers:
+ eae@chromium.org

[kyounga.ra, 2016-09-26 07:20:14]

Hello,

Could you have some time to review this patch?
It's small one. 

Thanks
Kyounga

[kojii, 2016-09-27 07:51:01]

Thank you for having a look on this issue. Please see comments inline below.

https://codereview.chromium.org/2363833003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/text/breaking-context-inline-crash.html
(right):

https://codereview.chromium.org/2363833003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/text/breaking-context-inline-crash.html:10:
</script>
Can you explain why we need this scripts?

https://codereview.chromium.org/2363833003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/text/breaking-context-inline-crash.html:23:
outline : solid 1px red;
Please be consistent on spaces. I think we generally add space after ":", and
not before ":".

https://codereview.chromium.org/2363833003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h
(right):

https://codereview.chromium.org/2363833003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:1009:
if (!m_ignoringSpaces && canBreakMidWord)
Probably this can be "else if"?

[kyounga.ra, 2016-09-28 01:11:56]

Applied your comments.

Additionaly, about test file,
I thought all layout test requires the testharness.js

Thanks.

On 2016/09/27 07:51:01, kojii wrote:
> Thank you for having a look on this issue. Please see comments inline below.
> 
>
https://codereview.chromium.org/2363833003/diff/1/third_party/WebKit/LayoutTe...
> File
third_party/WebKit/LayoutTests/fast/text/breaking-context-inline-crash.html
> (right):
> 
>
https://codereview.chromium.org/2363833003/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/fast/text/breaking-context-inline-crash.html:10:
> </script>
> Can you explain why we need this scripts?
> 
>
https://codereview.chromium.org/2363833003/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/fast/text/breaking-context-inline-crash.html:23:
> outline : solid 1px red;
> Please be consistent on spaces. I think we generally add space after ":", and
> not before ":".
> 
>
https://codereview.chromium.org/2363833003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h
> (right):
> 
>
https://codereview.chromium.org/2363833003/diff/1/third_party/WebKit/Source/c...
>
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:1009:
> if (!m_ignoringSpaces && canBreakMidWord)
> Probably this can be "else if"?

[kojii, 2016-09-28 01:45:39]

Non-owner LGTM, thank you for working on this.

eae@, PTAL.

[eae, 2016-09-28 01:57:00]

OK, LGTM

[kyounga.ra, 2016-09-28 02:10:07]

The CQ bit was checked by kyounga.ra@gmail.com

[commit-bot: I haz the power, 2016-09-28 02:10:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 03:40:21]

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

[commit-bot: I haz the power, 2016-09-28 03:40:23]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kyounga.ra, 2016-09-28 05:06:42]

On 2016/09/28 01:57:00, eae wrote:
> OK, LGTM

Hello, eae,

CQ test are failed with this patch. 

I have some questions for the failed test.

First, The failed case is
"fast/css3-text/css3-word-break/word-break-all-wrap-with-floats.html"

I'm not sure if the expected result is right. 
I mean, the only Chrome(without this patch) has different behavior from other
browsers. (FireFox / IE )
After this patch is applied, we got the same behavior (failed result)

I investigated the test's expected result but couldn't understand.
Where is the "non-breaking punctuation" ??? 
The message is below.
====
FAIL:
Expected 112 for height, but got 102. 

<p data-expected-height="112"><span></span>wrapbelowfloat)))))))))))))))))</p>

This test ensures we properly handle word-break: break-all in the context of
floats. The first two tests should wrap next to the float. The last contains
non-breaking punctuation too long to fit next to the float, and should be pushed
to below.
====

I think the "data-expected-height" should be change to 102. 
Should I?

Second, The failed case is "fast/text/breaking-context-inline-crash.html"
I guess the reason is "Ahem.ttf"
Is it OK if I change to load "ahem.js" instead of "Ahem.ttf"?

[eae, 2016-09-28 19:37:55]

On 2016/09/28 05:06:42, kyounga.ra wrote:
> On 2016/09/28 01:57:00, eae wrote:
> > OK, LGTM
> 
> Hello, eae,
> 
> CQ test are failed with this patch. 
> 
> I have some questions for the failed test.
> 
> First, The failed case is
> "fast/css3-text/css3-word-break/word-break-all-wrap-with-floats.html"
> 
> I'm not sure if the expected result is right. 
> I mean, the only Chrome(without this patch) has different behavior from other
> browsers. (FireFox / IE )
> After this patch is applied, we got the same behavior (failed result)
> 
> I investigated the test's expected result but couldn't understand.
> Where is the "non-breaking punctuation" ??? 
> The message is below.
> ====
> FAIL:
> Expected 112 for height, but got 102. 
> 
> <p data-expected-height="112"><span></span>wrapbelowfloat)))))))))))))))))</p>
> 
> This test ensures we properly handle word-break: break-all in the context of
> floats. The first two tests should wrap next to the float. The last contains
> non-breaking punctuation too long to fit next to the float, and should be
pushed
> to below.
> ====
> 
> I think the "data-expected-height" should be change to 102. 
> Should I?

I think you're right. check-layout is really not the right type of test for
this. Would be better to change it to a reference test but that's a discussion
for a different time. Just change it to 102 for now.

> 
> Second, The failed case is "fast/text/breaking-context-inline-crash.html"
> I guess the reason is "Ahem.ttf"
> Is it OK if I change to load "ahem.js" instead of "Ahem.ttf"?

Sure, go ahead!

[kyounga.ra, 2016-09-28 23:35:20]

The CQ bit was checked by kyounga.ra@gmail.com

[kyounga.ra, 2016-09-28 23:35:20]

The patchset sent to the CQ was uploaded after l-g-t-m from kojii@chromium.org,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2363833003/#ps40001
(title: "FATAL:DCHECK failed on BreakingContext::rewindToMidWordBreak()")

[commit-bot: I haz the power, 2016-09-28 23:35:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 00:38:16]

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

[commit-bot: I haz the power, 2016-09-29 00:38:17]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kyounga.ra, 2016-09-29 01:20:04]

On 2016/09/28 01:45:39, kojii wrote:
> Non-owner LGTM, thank you for working on this.
> 
> eae@, PTAL.

Hello, kojii,

I found out the why we need the script("testharness.js").

My patch has failed result by TryBot.
The log says that "-expected.txt" is missing
If we use "testharness.js" we don't need "expected.txt".

I am asked to use "testharness.js" instead of "expected.txt" in my previous
commit
So, if you agree with my comment, I will use
"breaking-context-inline-crash.html" from patch-set-1.

Thanks

[kojii, 2016-09-29 02:56:58]

> I found out the why we need the script("testharness.js").

Yes, that looks good, sorry for the trouble.

The change to word-break-all-wrap-with-floats.html doesn't look good though. I
think the test is correct. You will need to find the solution without changing
that.

Sorry I should have noticed test failures before the review response.

[kyounga.ra, 2016-09-29 04:12:05]

On 2016/09/29 02:56:58, kojii wrote:
> > I found out the why we need the script("testharness.js").
> 
> Yes, that looks good, sorry for the trouble.
> 
> The change to word-break-all-wrap-with-floats.html doesn't look good though. I
> think the test is correct. You will need to find the solution without changing
> that.

About the word-break-all-wrap-with-floats.html, 
I thought the test is not correct and @eae agreed with my thought.(comment #15)

The test result message says below.
==
 This test ensures we properly handle word-break: break-all in the context of
 floats. The first two tests should wrap next to the float. The last contains
 non-breaking punctuation too long to fit next to the float, and should be
pushed
== 
But I cannot find the "non-breaking punctuation" in the test case. 

Could you explain where is non-breaking punctuation?

> 
> Sorry I should have noticed test failures before the review response.

No problem.

Thanks for your time.

[kojii, 2016-09-29 04:48:53]

Apologies and please forgive us giving you inconsistent feedback, but please
also understand this is the value of reviews. By having multiple eyes, we
improve our accuracy.

On 2016/09/29 at 04:12:05, kyounga.ra wrote:
> 
> About the word-break-all-wrap-with-floats.html, 
> I thought the test is not correct and @eae agreed with my thought.(comment
#15)
> 
> The test result message says below.
> ==
>  This test ensures we properly handle word-break: break-all in the context of
>  floats. The first two tests should wrap next to the float. The last contains
>  non-breaking punctuation too long to fit next to the float, and should be
> pushed
> == 
> But I cannot find the "non-breaking punctuation" in the test case.

break-word and break-all are different that, break-word allows break anywhere
where break-all allows only between certain characters. Please have a look at
the spec for these values and compare:

break-all: https://drafts.csswg.org/css-text-3/#valdef-word-break-break-all
break-word: https://drafts.csswg.org/css-text-3/#valdef-overflow-wrap-break-word

> Could you explain where is non-breaking punctuation?

In this case, break-all cannot break before ")", and thus the long sequence of
")" should be pushed next to the float.

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

https://codereview.chromium.org/2363833003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:1008:
} else if (!m_ignoringSpaces && canBreakMidWord) {
Maybe replacing "canBreakMidWord" with "breakWords" can solve?

[kyounga.ra, 2016-09-29 06:10:53]

On 2016/09/29 04:48:53, kojii wrote:
> Apologies and please forgive us giving you inconsistent feedback, but please
> also understand this is the value of reviews. By having multiple eyes, we
> improve our accuracy.
> 
> On 2016/09/29 at 04:12:05, kyounga.ra wrote:
> > 
> > About the word-break-all-wrap-with-floats.html, 
> > I thought the test is not correct and @eae agreed with my thought.(comment
> #15)
> > 
> > The test result message says below.
> > ==
> >  This test ensures we properly handle word-break: break-all in the context
of
> >  floats. The first two tests should wrap next to the float. The last
contains
> >  non-breaking punctuation too long to fit next to the float, and should be
> > pushed
> > == 
> > But I cannot find the "non-breaking punctuation" in the test case.
> 
> break-word and break-all are different that, break-word allows break anywhere
> where break-all allows only between certain characters. Please have a look at
> the spec for these values and compare:
> 
> break-all: https://drafts.csswg.org/css-text-3/#valdef-word-break-break-all
> break-word:
https://drafts.csswg.org/css-text-3/#valdef-overflow-wrap-break-word
> 
> > Could you explain where is non-breaking punctuation?
> 
> In this case, break-all cannot break before ")", and thus the long sequence of
> ")" should be pushed next to the float.
> 
>
https://codereview.chromium.org/2363833003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h
> (right):
> 
>
https://codereview.chromium.org/2363833003/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:1008:
> } else if (!m_ignoringSpaces && canBreakMidWord) {
> Maybe replacing "canBreakMidWord" with "breakWords" can solve?

It's my mis-understanding. 
I just saw other browser's behavior without understanding CSS standard. 

For the sure, I have question. 
The 3rd test word has 10 more ")" characters. Does "non-breaking puncutation"
mean ")" character ?

Your solution works! :-) 
I'll upload.
Thanks.

[kojii, 2016-09-29 08:26:26]

On 2016/09/29 at 06:10:53, kyounga.ra wrote:
> 
> It's my mis-understanding. 
> I just saw other browser's behavior without understanding CSS standard.

Yeah, Gecko and WebKit does not follow the spec on this regard, I believe
Edge/Trident does.

> For the sure, I have question. 
> The 3rd test word has 10 more ")" characters. Does "non-breaking puncutation"
mean ")" character ?

Yes. Historically, "break-all" was designed to use for CJK, so it breaks normal
words but breaking before ")" or "." looks strange.

> Your solution works! :-) 
> I'll upload.
> Thanks.

Great, look forward to it. Thank you for working on this again.

[kyounga.ra, 2016-09-29 08:39:26]

kojii@, PTAL on patch-set 5.

"breakWords" solution works for word-break-all-wrap-with-floats.html.
But, does not work for my test case.

I added one more condition, more strictly.

[kyounga.ra, 2016-09-29 08:45:07]

Patchset #4 (id:60001) has been deleted

[kojii, 2016-09-29 08:59:18]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 08:59:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 11:39:17]

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

[commit-bot: I haz the power, 2016-09-29 11:39:19]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-09-29 15:45:01]

lgtm, thank you for working on this.

[kojii, 2016-09-29 15:45:19]

The CQ bit was checked by kojii@chromium.org

[kojii, 2016-09-29 15:45:19]

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/2363833003/#ps80001
(title: "FATAL:DCHECK failed on BreakingContext::rewindToMidWordBreak()")

[commit-bot: I haz the power, 2016-09-29 15:45:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 15:51:09]

Description was changed from

==========
FATAL:DCHECK failed on BreakingContext::rewindToMidWordBreak()

Fix for wrong line-breaking calculation on "word-break:break-all" case

BUG=613881
==========

to

==========
FATAL:DCHECK failed on BreakingContext::rewindToMidWordBreak()

Fix for wrong line-breaking calculation on "word-break:break-all" case

BUG=613881
==========

[commit-bot: I haz the power, 2016-09-29 15:51:13]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-09-29 15:53:30]

Description was changed from

==========
FATAL:DCHECK failed on BreakingContext::rewindToMidWordBreak()

Fix for wrong line-breaking calculation on "word-break:break-all" case

BUG=613881
==========

to

==========
FATAL:DCHECK failed on BreakingContext::rewindToMidWordBreak()

Fix for wrong line-breaking calculation on "word-break:break-all" case

BUG=613881

Committed: https://crrev.com/47e23e263b551925dccd72e0b170f64cf7ab8b9a
Cr-Commit-Position: refs/heads/master@{#421830}
==========

[commit-bot: I haz the power, 2016-09-29 15:53:32]

Patchset 4 (id:??) landed as
https://crrev.com/47e23e263b551925dccd72e0b170f64cf7ab8b9a
Cr-Commit-Position: refs/heads/master@{#421830}