Summary: | Problems with Selection::appendTrailingWhitespace() | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Roman <eroman> | ||||||||
Component: | WebCore Misc. | Assignee: | Eric Roman <eroman> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | ||||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Eric Roman
2009-01-10 17:22:40 PST
Created attachment 26594 [details]
Rewrite appendTrailingWhitespace() using CharacterIterator.
* Modified LayoutTests/editing/selection/doubleclick-whitespace.html to include additional test cases.
* Added a separate layout test for the crasher.
Comment on attachment 26594 [details] Rewrite appendTrailingWhitespace() using CharacterIterator. > + 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. Created attachment 26627 [details] Rewrite appendTrailingWhitespace() using CharacterIterator. 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. Comment on attachment 26627 [details]
Rewrite appendTrailingWhitespace() using CharacterIterator.
r=me
Created attachment 26635 [details]
Rewrite appendTrailingWhitespace() using CharacterIterator.
Updated the patch to fix some indentation in doubleclick-whitespace.html.
There were two javascript functions where I had 2 space indentation instead of 4 space indentation.
This is the only change over the previous (reviewed) patch.
|