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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jan 11 08:39:05 PST 2009

Darin Adler <darin at apple.com> has denied Eric Roman <eroman at chromium.org>'s
request for review:
Bug 23232: Problems with Selection::appendTrailingWhitespace()

Attachment 26594: Rewrite appendTrailingWhitespace() using CharacterIterator.

------- Additional Comments from Darin Adler <darin at apple.com>
> +    Node *n = pos.node();

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

> +    Document *d = n->document();
> +    Node *de = d->documentElement();

Same here.

> +    Node *boundary = n->enclosingBlockFlowElement();

And here.

> +    if (!searchRange.get())

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

> +    for (; !charIt.atEnd() && charIt.length() > 0; charIt.advance(1)) {

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.

> +	   if (isSpaceOrNewline(c) || c == noBreakSpace)
> +	       m_end = charIt.range()->endPosition();
> +	   else
> +	       break;

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

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.

Do you need to handle newlines that actually render as hard line breaks
differently from newlines that are treated as general whitespace? I guess your
test 4 is supposed to be implementing that.

Patch looks good and it's *almost* a review+, but I'd like you to resolve the
issues I mention above and post a new patch if you don't mind.

More information about the webkit-reviews mailing list