[webkit-reviews] review denied: [Bug 17427] Line breaking opportunities at the end of a text node are missed : [Attachment 192388] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Glenn Adams <glenn at skynav.com>'s
request for review:
Bug 17427: Line breaking opportunities at the end of a text node are missed
https://bugs.webkit.org/show_bug.cgi?id=17427

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=192388&action=review


Please look into the single-character-node issue I mention below. That’s the
only reason I gave this a review- instead of review+.

> Source/WebCore/platform/text/TextBreakIterator.h:63
> +	   , m_lastChar(0)
> +	   , m_lastLastChar(0)

I know that our existing code calls these lastCh and lastLastCh. But, in WebKit
code we normally prefer to use words rather than abbreviations and there is a
normal English phrase for the “last last character” that I think is clear. I
would prefer m_lastCharacter, m_secondToLastCharacter, setLastTwoCharacters,
and resetLastTwoCharacters. All those seem easy to read and less like jargon.

> Source/WebCore/platform/text/TextBreakIterator.h:95
> +    inline UChar lastChar() const { return m_lastChar; }
> +    inline UChar lastLastChar() const { return m_lastLastChar; }
> +    inline void setLastChars(UChar last, UChar lastLast)
> +    {
> +	   m_lastChar = last;
> +	   m_lastLastChar = lastLast;
> +    }
> +    inline void resetLastChars()
> +    {
> +	   m_lastChar = 0;
> +	   m_lastLastChar = 0;
> +    }

The “inline” here are redundant and have no effect. All function definitions
that are inside a class definition are treated as if “inline” is specified
automatically.

Please omit them.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2873
> +	       UChar lastCharInNode = 0;

lastCharacterInNode, please

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2874
> +	       UChar lastLastCharInNode = 0;

secondToLastCharacterInNode, please

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2917
> +			       goto nextChar;

nextCharacter, please

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:3083
> +	       renderTextInfo.m_lineBreakIterator.setLastChars(lastCharInNode,
lastLastCharInNode);

This does not seem to correctly handle a text node that has only one character
in it (or zero characters, for that matter). I think that in that case it would
overwrite a characters from the second to last node with a zero.

> Source/WebCore/rendering/break_lines.cpp:157
> +    CharacterType lastLastCh = pos > 1 ? str[pos - 2] :
CharacterType(lazyBreakIterator.lastLastChar());
> +    CharacterType lastCh = pos > 0 ? str[pos - 1] :
CharacterType(lazyBreakIterator.lastChar());

Please use static_cast<CharacterType>(x) instead of CharacterType(x).