[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 #79131|review?                     |review-
               Flag|                            |




--- Comment #24 from Ryosuke Niwa <rniwa at webkit.org>  2011-01-17 12:00:45 PST ---
(From update of attachment 79131)
View in context: https://bugs.webkit.org/attachment.cgi?id=79131&action=review

I was looking at trac logs and found out that enrica attempted a fix at a related bug 30946 (her patch was rolled out in http://trac.webkit.org/changeset/55271). Have you taken a look at that bug?

> Source/WebCore/editing/CompositeEditCommand.cpp:398
> +static inline bool isWhitespaces(const String& text)

How about containsOnlyWhitespace as in JavaScriptCore/wtf/text/StringImpl.h?

> Source/WebCore/editing/CompositeEditCommand.cpp:419
> +void CompositeEditCommand::rebalanceWhitespaceBetween(const Position& startPosition, const Position& endPosition)

"between" is kind of misleading because you're extending positions to include all surrounding whitespace.

How about rebalanceSurroundingWhitespace?

> Source/WebCore/editing/CompositeEditCommand.cpp:422
> +    if (startPosition.anchorType() != Position::PositionIsOffsetInAnchor || !node || !node->isTextNode())

You need to do the same check for endPosition. In fact, don't you need to assert that startPosition and endPosition point to the same text node? r- for this.

Maybe it's better to move this check into caller and have them pass a RefPtr to the text node, startOffset, and endOffset. You don't need to check this condition in InsertTextCommand::input for example.  Although if we're fixing this function to consider surrounding text nodes, taking two positions might make more sense.

> Source/WebCore/editing/CompositeEditCommand.cpp:-421
> -    int offset = position.deprecatedEditingOffset();
> -    // If neither text[offset] nor text[offset - 1] are some form of whitespace, do nothing.
> -    if (!isWhitespace(text[offset])) {
> -        offset--;
> -        if (offset < 0 || !isWhitespace(text[offset]))
> -            return;
> -    }

If you make the above change, then we can move this optimization into CompositeEditCommand::rebalanceWhitespaceAt.  But I'm still not sure that'll make sense if we extended this function to walk surrounding text nodes though.

> Source/WebCore/page/EventHandler.cpp:2656
> -bool EventHandler::handleTextInputEvent(const String& text, Event* underlyingEvent, bool isLineBreak, bool isBackTab)
> +bool EventHandler::handleTextInputEvent(const String& text, Event* underlyingEvent, bool isLineBreak, bool isBackTab, bool isComposition)

I really don't like the fact this function takes 3! boolean arguments.  Would you mind cleaning that up in a separate patch?

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