[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





--- Comment #26 from MORITA Hajime <morrita at google.com>  2011-01-17 21:34:41 PST ---
Ryosuke, thank you for continuing the  review! I updated the patch.

(In reply to comment #24)
> (From update of attachment 79131 [details])
> 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?
I didn't notice  that. But I tried tests under fast/form and they passed.
Fortunately, the change looks  orthogonal, at least conceptually. 

> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:398
> > +static inline bool isWhitespaces(const String& text)
> 
> How about containsOnlyWhitespace as in JavaScriptCore/wtf/text/StringImpl.h?
> 
Looks nice, So Replaced this with String::containsOnlyWhitespace()

> > 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.
Renamed to rebalanceWhitespaceOnTextSubstring() with changing signature as 
your feedback below.

> 
> 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.
Good point. I extracted the guard code and invoke it from call sites.

> 
> > 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.
> 
I'd like to keep this code removed because it will pull back off-by-one tricks...

> > 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?
Agreed and filed Bug 52608. I'll do it sometime soon.

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