[Webkit-unassigned] [Bug 5241] Space and tab characters "sent" by an input method give totally different results than typing them directly

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=5241


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #78646|review?                     |review-
               Flag|                            |




--- Comment #13 from Ryosuke Niwa <rniwa at webkit.org>  2011-01-11 21:38:40 PST ---
(From update of attachment 78646)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.