[webkit-reviews] review denied: [Bug 53203] CSS 2.1 failure: bidi-breaking-002.htm test fails (PARAGRAPH SEPARATOR does not break line) : [Attachment 98059] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Levi Weintraub <leviw at chromium.org>'s
request for review:
Bug 53203: CSS 2.1 failure: bidi-breaking-002.htm test fails (PARAGRAPH
SEPARATOR does not break line)
https://bugs.webkit.org/show_bug.cgi?id=53203

Attachment 98059: Patch
https://bugs.webkit.org/attachment.cgi?id=98059&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=98059&action=review

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2117
>		   bool betweenWords = c == '\n' || (currWS != PRE && !atStart
&& isBreakable(lineBreakIteratorInfo.second, current.m_pos,
current.m_nextBreakablePosition, breakNBSP)
> -		       && (style->hyphens() != HyphensNone ||
(current.previousInSameNode() != softHyphen)));
> +		       && (style->hyphens() != HyphensNone ||
(current.previousInSameNode() != softHyphen))) || isUnicodeLineBreak(c);

So this would be better if the \n check was rolled into the unicode one.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2198
> +		       if ((c == '\n' && preserveNewline) ||
isUnicodeLineBreak(c)) {

We need a comment here as to why unicode linebreaks ignore preserveNewline.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2206
> -			   lineInfo.setPreviousLineBrokeCleanly(true);
> +			   lineInfo.setPreviousLineBrokeCleanly(c !=
WTF::Unicode::lineSeparator);

This needs a comment as well. :)

If this only affects the bidi algorithm, we should have a better name for this.
 Like bidiAlgorithResetsOnNextLine... (not really that much better, but I'm
sure we can come up with something better!)

> Source/WebCore/rendering/RenderText.cpp:706
> +	       while (i + linelen < len && text[i + linelen] != '\n' &&
!(isUnicodeLineBreak(text[i + linelen])))

Good use for a function which includes both \n and the unicode ones.

> Source/WebCore/rendering/RenderText.cpp:821
> +	   } else if (isUnicodeLineBreak(c)) {
> +	       m_hasBreak = true;
> +	       isNewline = true;
> +	       isSpace = false;

Seems OK.  Should we be rolling this into the \n check above?

> Source/WebCore/rendering/RenderText.cpp:852
> +	   while (c != '\n' && !isSpaceAccordingToStyle(c, style()) && c !=
'\t' && c != softHyphen && !(isUnicodeLineBreak(c))) {

This should again use an inline whihc checks all of \n and the unicode
newlines.  Checking \n first of course. :)

> Source/WebCore/rendering/break_lines.h:44
> +inline bool isUnicodeLineBreak(UChar c)
> +{
> +    return c == WTF::Unicode::lineSeparator || c ==
WTF::Unicode::paragraphSeparator;
> +}
> +

I believe we should have a isLineBreak inline which includes \n and possibly
\r.  And then use that everywhere instead of calling this unicode-only one in
addition to the explicit \n checks.

When you add a second function here, we may need to add comments about when to
use each of the two newline-checking-functions.