[Webkit-unassigned] [Bug 25298] Ctrl + Right/Left arrow move forward/backward through document instead of right/left in RTL text

[bugzilla-daemon at webkit.org]

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





--- Comment #31 from Xiaomei Ji <xji at chromium.org>  2011-02-16 09:05:04 PST ---
Thanks for the review and comments!


(In reply to comment #30)
> (From update of attachment 78549 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78549&action=review
> 
> > Source/WebCore/editing/htmlediting.h:100
> > +// TextDirection on Node's enclosing block
> > +TextDirection directionOfEnclosingBlock(Node*);
> 
> The function’s name makes the comment redundant. Can it take a const Node*?

All underlying functions (including Position constructor) take a non-const Node*.

> 
> > Source/WebCore/editing/visible_units.cpp:1221
> > +enum WORDDIRECTION {
> > +    LEFTWORD,
> > +    RIGHTWORD
> > +};
> 
> Enums and their values should be in “CamelCase”, not uppercase. How does this enum differ from TextDirection?

This is only used to differentiate whether we are moving cursor left or right. It is introduced so that the leftWordPosition() and rightWordPosition() could share a common function. 

> 
> > Source/WebCore/editing/visible_units.cpp:1233
> > +        VisiblePosition wordBreak = ((direction == RIGHTWORD && box->direction() == LTR) || (direction == LEFTWORD && box->direction() == RTL) ? nextWordPosition(c) : previousWordPosition(c));
> 
> For example, here, if you used TextDirection instead of WORDDIRECTION you could have just written this expression as
>     (direction == boxDirection) ? nextWordPosition(c) : previousWordPosition(c);


It could. The problem is the meaning of the enum.

> 
> > Source/WebCore/editing/visible_units.cpp:1236
> > +        if (offset != boxOfWordBreak->caretMaxOffset() && offset != boxOfWordBreak->caretMinOffset() && box == boxOfWordBreak)
> 
> You’d want to move the box == boxOfWorkdBreak condition to the beginning, to avoid unnecessary calls to caret{Max,Min}Offset(). If you do this, then you’ll be getting box->caret{Max,Min}Offset(). Since you’ve already gotten them in line 1232, I suggest using local variables for them.

They are accessing different boxes, local variables wont help here.

> 
> > Source/WebCore/editing/visible_units.cpp:1256
> > +        Node* node = renderer->node();
> > +        if (!node)
> > +            break;
> 
> Can we do better than bail out in this case? How does this affect lines starting or ending with generated content?

I guess this should be a "continue" not a "break".


> 
> > Source/WebCore/editing/visible_units.cpp:1264
> > +        if ((direction == RIGHTWORD && directionOfBlock != box->direction())
> > +            || (direction == LEFTWORD && directionOfBlock == box->direction()))
> > +            wordBoundaries.append(previousPosition); 
> 
> This appends previousPosition to wordBoundaries but we don’t know yet that previousPosition is a word boundary. Why is this okay?


You are right. The code assumes box boundary as word breaker, so <div>who<span>ever</span><div> might be considered as 2 words "who" and "ever".
I will see whether I can get rid of it.

> 
> > Source/WebCore/editing/visible_units.cpp:1305
> > +    std::sort(wordBoundaries.begin(), wordBoundaries.end(), visiblePositionComparison);
> > +
> > +    VisiblePosition previousPosition = c;
> > +    while (true) {
> > +        VisiblePosition position = (direction == RIGHTWORD ? previousPosition.right(true) : previousPosition.left(true));
> > +        if (std::binary_search(wordBoundaries.begin(), wordBoundaries.end(), position, visiblePositionComparison))
> > +            return position;
> > +        if (position == previousPosition)
> > +            return position;
> > +        previousPosition = position;
> > +    }
> 
> This doesn’t look very efficient (and I am not convinced it’s correct). I would think that you could collect all word boundaries as (InlineBox*, offset) pairs (already ordered by box from left to right), find the nearest box to c in the direction you’re going, then find the word boundary offset nearest to c in that box (depending on the direction of that box). You’ll need to discard boundaries of the wrong kind (i.e. if going to the right, you want to keep boundaries where the word is to the left and the space is to the right).
> 

Good suggestion. I will try.

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