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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 12 02:01:33 PST 2009

Eric Roman <eroman at chromium.org> has asked  for review:
Bug 23232: Problems with Selection::appendTrailingWhitespace()

Attachment 26627: Rewrite appendTrailingWhitespace() using CharacterIterator.

------- 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.

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.


> 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.

Changed to:

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

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

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

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

More information about the webkit-reviews mailing list