[webkit-reviews] review denied: [Bug 30946] Extra layout on keypress after a space (problem with rebalanceWhitespaceAt in InsertTextCommand) : [Attachment 43452] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Justin Garcia
<justin.garcia at apple.com>'s request for review:
Bug 30946: Extra layout on keypress after a space (problem with
rebalanceWhitespaceAt in InsertTextCommand)
https://bugs.webkit.org/show_bug.cgi?id=30946

Attachment 43452: patch
https://bugs.webkit.org/attachment.cgi?id=43452&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks pretty good. I have a few thoughts.

I am concerned about copying the function named isWhitespace in two different
source files. Can we put this into PlatformString.h or StringImpl.h instead?
What's the right name for it to distinguish it from other functions of similar
name?

> +void InsertTextCommand::insertTextIntoNodeAndRebalanceWhitespace(const
String& textToInsert, PassRefPtr<Text> node, unsigned offset)

This function does not take ownership of the text node, so the argument type
should be Text*, not PassRefPtr<Text>.

> +    unsigned whitespaceStart, whitespaceEnd;
> +    if (!extentOfWhitespaceAt(node.get(), offset, whitespaceStart,
whitespaceEnd)) {
> +    
> +	   VisiblePosition visiblePosition(Position(node.get(), offset));
> +	   String rebalancedText = stringWithRebalancedWhitespace(textToInsert,
isStartOfParagraph(visiblePosition), isEndOfParagraph(visiblePosition));
> +    
> +	   insertTextIntoNode(node.get(), offset, rebalancedText);
> +	   return;
> +    }

There should not be a blank line after the if statement.

> +    
> +    ASSERT(whitespaceEnd >= whitespaceStart);
> +    unsigned length = whitespaceEnd - whitespaceStart + 1;

Why the +1 here? It seems like the "end" value may be inclusive, which seems
like a mistake.

I can't tell by reading this.

> +    void insertTextIntoNodeAndRebalanceWhitespace(const String& text,
PassRefPtr<Text> node, unsigned offset);

As I said above, the type of the node should be Text*. The names "text" and
"node" for the arguments are justified here because of the clash of type names
with roles of the arguments. The argument names help make it clear.

> +// Finds the extent of whitespace around a particular offset, as long as it
is in a node with collapsable whitespace.

The word "collapsible" is misspelled here.

The comment could be clearer on what "as long as" means.

There are multiple modes of collapsible whitespace. For example, pre-wrap
collapses other whitespace, but not "\n". To handle those correctly, I think
RenderStyle::isCollapsibleWhiteSpace needs to be used instead of a local
isWhiteSpace function.

> +    if (node->length() == 0)
> +	   return false;

Do we really need a special case for empty text nodes? Is this a common case
that needs performance optimization? Does the code below fail for empty text
nodes?

I'd like to see an assertion about offset being <= node->length() and also get
a sense of whether callers really guarantee that or not.

> +    if (offset == text.length() || !isWhitespace(text[offset])) {

Because of how String indexing works, I don't think you need the "offset ==
text.length" check here. Getting a character past the end returns 0, and that
will return false from isWhitespace.

> +	   if (offset == 0 || !isWhitespace(text[offset - 1]))
> +	       // If neither text[offset] nor text[offset - 1] are some form of
whitespace, no rebalancing is necessary.
> +	       return false;

Need braces around this.

> +    while (start > 0 && isWhitespace(text[start - 1]))
> +	   start--;
> +	   
> +    for (unsigned i = end + 1; i < text.length(); i++) {
> +	   if (!isWhitespace(text[i]))
> +	       break;
> +	   end = i;
> +    }

I don't understand why these loops are structured differently, since they both
do the same thing. It would be slightly nicer to make them match.

I don't understand how this function can possibly do the right thing if there
are adjacent text nodes that also have whitespace in them.

review- because at least some of my comments seem worth doing, especially the
use of RenderStyle::isColllapsibleWhiteSpace

I'd like to see more test cases -- I worry that we are making performance
faster but correctness suffer in various edge cases.