[webkit-reviews] review denied: [Bug 5241] Space and tab characters "sent" by an input method give totally different results than typing them directly : [Attachment 78646] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied MORITA Hajime <morrita at google.com>'s
request for review:
Bug 5241: Space and tab characters "sent" by an input method give totally
different results than typing them directly
https://bugs.webkit.org/show_bug.cgi?id=5241

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78646&action=review

>
LayoutTests/editing/inserting/script-tests/insert-composition-whitespace.js:34
> +test("div", "A  B", "A \xA0B");
> +test("div", "A   B", "A \xA0 B");
> +test("div", "A    B", "A \xA0 \xA0B"); 

Mn... nbsp is 0xC2 0xA0 in UTF-8 but I guess it doesn't matter here.  But watch
out for test failures when you're landing this.

> Source/WebCore/ChangeLog:24
> +	   * editing/CompositeEditCommand.cpp:
> +	   (WebCore::hasWhitespaceList):
> +	   (WebCore::CompositeEditCommand::shouldBalanceWhitespaceAt):
> +	   (WebCore::CompositeEditCommand::rebalanceWhitespaceAt):
> +	   (WebCore::CompositeEditCommand::rebalanceWhitespaceBetweenWordsAt):
Added.
> +	   * editing/CompositeEditCommand.h:
> +	   * editing/Editor.cpp:
> +	   (WebCore::Editor::confirmComposition):
> +	   (WebCore::Editor::setComposition):

Nit: Since you're making quite few modifications, it'll be nice if you also
added inline description to each function.

> Source/WebCore/editing/CompositeEditCommand.cpp:411
> +bool CompositeEditCommand::shouldBalanceWhitespaceAt(const Position&
position, bool willInsert)

I'm asking this function "should I balance whitespace at this position?"  What
does willInsert mean in that semantics?

> Source/WebCore/editing/CompositeEditCommand.cpp:424
> +    if (node->isTextNode()) {
> +	   Text* textNode = static_cast<Text*>(node);
> +	   if (!textNode->length() && !willInsert)
> +	       return false;
> +    } else {
> +	   if (!willInsert)
> +	       return false;
> +    }

Can't you just do:
if ((!node->isTextNode() || !static_cast<Text*>(node)->length()) &&
!willInsert)
    return false;

> Source/WebCore/editing/CompositeEditCommand.cpp:436
> +    if (!hasWhitespaceList(text) || !shouldBalanceWhitespaceAt(position,
true))
> +	   return text;

Per recent discussion on webkit-dev, we don't want to be calling a function
with true/false. r- for this.
Consider making the argument enum type.  But also reconsider the name
willInsert.  I don't think it's descriptive enough.

> Source/WebCore/editing/CompositeEditCommand.cpp:469
> +    String balancedText;
> +    unsigned whiteCount = 0;
> +
> +    for (unsigned i = 0; i < text.length(); ++i) {
> +	   bool isWhitespace = text[i] == ' ';
> +	   if (isWhitespace) {
> +	       whiteCount++;
> +
> +	       if (i + 1 < text.length())
> +		   continue;
> +	       // Preserves trailing whitespace, which will be balanced by
rebalanceWhitespaceAt()
> +	       balancedText.append(text.characters() + text.length() -
whiteCount, whiteCount);
> +	       break;
> +	   }
> +
> +	   // Preseves leading whitespace, which will be balanced by
rebalanceWhitespaceAt()
> +	   if (whiteCount == i)
> +	       balancedText.append(text.characters(), whiteCount);
> +	   else {
> +	       // Interleaves nbsp and whitespace to preserve
> +	       // space characters and line-breakability.
> +	       if (1 == whiteCount)
> +		   balancedText.append(' ');
> +	       else {
> +		   for (unsigned whiteIndex = 0; whiteIndex < whiteCount;
++whiteIndex)
> +		       balancedText.append(static_cast<UChar>(whiteIndex%2 ?
noBreakSpace : ' '));
> +	       }
> +	   }
> +
> +	   balancedText.append(text.characters()[i]);
> +	   whiteCount = 0;
> +    }

It's really unfortunate that we can't share any code with
rebalanceWhitespaceAt.	Is there any way we can add more general version of the
two and call that one in rebalanceWhitespaceAt and
rebalanceWhitespaceBetweenWordsAt?