[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()
https://bugs.webkit.org/show_bug.cgi?id=23232
Attachment 26594: Rewrite appendTrailingWhitespace() using CharacterIterator.
https://bugs.webkit.org/attachment.cgi?id=26594&action=review
------- 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
understand?
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