[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


mitz at webkit.org changed:

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




--- Comment #30 from mitz at webkit.org  2011-02-13 21:41:30 PST ---
(From update of attachment 78549)
View in context: https://bugs.webkit.org/attachment.cgi?id=78549&action=review

> Source/WebCore/ChangeLog:22
> +        * editing/SelectionController.cpp:
> +        (WebCore::SelectionController::modifyExtendingRight):
> +        (WebCore::SelectionController::modifyMovingRight):
> +        (WebCore::SelectionController::modifyExtendingLeft):
> +        (WebCore::SelectionController::modifyMovingLeft):
> +        * editing/SelectionController.h:
> +        * editing/htmlediting.cpp:
> +        (WebCore::directionOfEnclosingBlock):
> +        * editing/htmlediting.h:
> +        * editing/visible_units.cpp:
> +        (WebCore::visiblePositionComparison):
> +        (WebCore::wordBoundary):
> +        (WebCore::leftWordPosition):
> +        (WebCore::rightWordPosition):
> +        * editing/visible_units.h:

The change log should describe the changes, not just list the changed functions.

> Source/WebCore/editing/SelectionController.cpp:-354
> -TextDirection SelectionController::directionOfEnclosingBlock()
> -{
> -    Node* enclosingBlockNode = enclosingBlock(m_selection.extent().node());
> -    if (!enclosingBlockNode)
> -        return LTR;
> -    RenderObject* renderer = enclosingBlockNode->renderer();
> -    if (renderer)
> -        return renderer->style()->direction();
> -    return LTR;
> -}
> -

Seems useful to leave this function here (perhaps changed to call the new one in htmlediting) rather than change all call sites.

> Source/WebCore/editing/htmlediting.cpp:335
> +TextDirection directionOfEnclosingBlock(Node* n)

It’s better to use a word like “node” rather than a single letter for a variable name.

> 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*?

> 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?

> Source/WebCore/editing/visible_units.cpp:1223
> +static VisiblePosition wordBoundary(enum WORDDIRECTION direction, const VisiblePosition& c)

No need to qualify WORDDIRECTION with the “enum” keyword. “c” is a poor name for a VisiblePosition. How about “visiblePosition” or “position”?

> 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);

> 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.

> Source/WebCore/editing/visible_units.cpp:1242
> +    Vector<VisiblePosition> wordBoundaries;

You should give this Vector some inline capacity to avoid heap allocation in most cases.

> 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?

> 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?

> Source/WebCore/editing/visible_units.cpp:1267
> +        while (1) {

WebKit style is to use while (true)

> 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).

> Source/WebCore/editing/visible_units.cpp:1309
> +VisiblePosition leftWordPosition(const VisiblePosition& c)

Please consider renaming “c”.

> Source/WebCore/editing/visible_units.cpp:1314
> +VisiblePosition rightWordPosition(const VisiblePosition& c)

Ditto.

> Source/WebCore/editing/visible_units.h:45
> +VisiblePosition rightWordPosition(const VisiblePosition &);
> +VisiblePosition leftWordPosition(const VisiblePosition &);

There shouldn’t be spaces before the ampersands here.

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