[webkit-reviews] review requested: [Bug 23232] Problems with Selection::appendTrailingWhitespace() : [Attachment 26627] Rewrite appendTrailingWhitespace() using CharacterIterator.

[bugzilla-daemon at webkit.org]

Eric Roman <eroman at chromium.org> has asked  for review:
Bug 23232: Problems with Selection::appendTrailingWhitespace()
https://bugs.webkit.org/show_bug.cgi?id=23232

Attachment 26627: Rewrite appendTrailingWhitespace() using CharacterIterator.
https://bugs.webkit.org/attachment.cgi?id=26627&action=review

------- Additional Comments from Eric Roman <eroman at chromium.org>
Thanks for the review comments!


> WebKit coding style requires that you do Node* n rather than Node *n.

Fixed.
Sorry about that.
I lifted this code from visible_units.cpp (nextBoundary), which must be using
an older style.

> There's no need for .get() here. You can use the ! operator with a RefPtr
> without calling it.

Done.

> There's no need to check atEnd() here. Once you're at the end, length is
> guaranteed to return 0. In WebKit style we'd normally code this without the
">
> 0" also.

Done.
Changed to:

  for (; charIt.length(); charIt.advance(1)) {

> Maybe the should be written the other way around, with the break first, and
the
> m_end part outside the if statement, to make the looping logic easier to
> understand?

Done.
Moved the break case to be first, so no "else" is needed.
Much cleaner, thanks for the suggestion.

As for moving the m_end outside of the loop, I have not done that since it
hurts readability some, for the case where there is no whitespace (and m_end is
not to be changed).

> Is there a reason the tab character isn't included here? As you can see in
> RenderStyle::isCollapsibleWhiteSpace, it's considered a form of space.

The tab character is in fact included -- it is matched by
"isSpaceOrNewline(c)".

Added "test6" to "doubleclick-whitespace.html",
to ensure that tabs (0x9) are included in the selection.