[Webkit-unassigned] [Bug 78856] visual word movement: Using ICU break iterator to simplify implementation

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from Ryosuke Niwa <rniwa at webkit.org>  2012-03-13 14:00:20 PST ---
(From update of attachment 131476)
View in context: https://bugs.webkit.org/attachment.cgi?id=131476&action=review

> Source/WebCore/editing/visible_units.cpp:166
> +static void searchPreviousBoxInSameLine(const InlineTextBox* textBox, InlineTextBox*& previousBox)

I think it's better for this function to just return the box.
Also we don't normally call these functions "search" blah. It's probably better to call it something along the like of logicallyPreviousBoxInSameLine.

> Source/WebCore/editing/visible_units.cpp:173
> +            break;

If you do that, you can just return 0.

> Source/WebCore/editing/visible_units.cpp:175
> +            previousBox = toInlineTextBox(leafBoxesInLogicalOrder[i]);

And will be returning toInlineTextBox(leafBoxesInLogicalOrder[i]) here.

> Source/WebCore/editing/visible_units.cpp:192
> +            while (j < leafBoxesInLogicalOrder.size()) {
> +                if (leafBoxesInLogicalOrder[j]->isInlineTextBox()) {
> +                    nextBox = toInlineTextBox(leafBoxesInLogicalOrder[j]);
> +                    break;
> +                }
> +            }

It seems like searchPreviousBoxInSameLine should also have this loop.
It'll be great if we could add a test case for searchPreviousBoxInSameLine.

> Source/WebCore/editing/visible_units.cpp:200
> +static void searchPreviousBoxInPreviousRootInlineBox(const VisiblePosition& visiblePosition, const InlineTextBox* textBox,
> +    InlineTextBox*& previousBox, bool& previousBoxInDifferentLine)
> +{

It'll be great if you can combine this function with searchPreviousBoxInSameLine.

> Source/WebCore/editing/visible_units.cpp:261
> +    TextBreakIterator*& iter, int& previousBoxLen, bool& previousBoxInDifferentLine)

Please spell out length in previousBoxLen. It's probably still better for this function to return TextBreakIterator*.

> Source/WebCore/editing/visible_units.cpp:265
> +    // TODO: how should we handle non-text box?

s/TODO/FIXME/. Again, it's probably better to say "handle the case when we don't have an inline box".

> Source/WebCore/editing/visible_units.cpp:311
> +enum ECursorMovementDirection {

I think the new style is not to prefix enum types with E.

> Source/WebCore/editing/visible_units.cpp:312
> +    MOVE_LEFT, MOVE_RIGHT,

We should use MoveLeft and MoveRight for the consistency.

> Source/WebCore/editing/visible_units.cpp:322
> +    textBreakFollowing(iter, position);
> +    return isWordTextBreak(iter);

Maybe we need a comment explaining that ICU's isWordTextBreak returns true after moving across a word and false after moving across a punctuation/whitespace.

> Source/WebCore/editing/visible_units.cpp:331
> +static VisiblePosition wordPosition(const VisiblePosition& visiblePosition, ECursorMovementDirection direction)

We should probably call this function visualWordPosition.

> Source/WebCore/editing/visible_units.cpp:342
> +        VisiblePosition adjacentByCharacter = direction == MOVE_RIGHT ? current.right(true) : current.left(true); 

We should probably call this adjacentCharacter.

> Source/WebCore/editing/visible_units.cpp:347
> +        int offset;

Maybe we want to clarify that this offset is in box because there are two offsets here: one in box and one in iterator.

> Source/WebCore/editing/visible_units.cpp:350
> +        // TODO: how should we handle non text box?

We don't use TODO. Use FIXME instead. I think it's probably better to say "Handle the case when we don't have an inline box" instead of asking "how".

> Source/WebCore/editing/visible_units.cpp:355
> +        int previousBoxLen = 0;

Please spell out length.

> Source/WebCore/editing/visible_units.cpp:364
> +            if (previouslyVisitedBox != box) {

Maybe you want to a define a boolean like movingIntoNewBox = previouslyVisitedBox != box to clarify the semantics.

> Source/WebCore/editing/visible_units.cpp:368
> +            previouslyVisitedBox = box;

This statement can be moved into the if statement so that you can just have one else if instead of nested else and if.

> Source/WebCore/editing/visible_units.cpp:372
> +            isStartOfWord(iter, offset - textBox->start() + previousBoxLen,

Please define descriptive local variables for offset - textBox->start() + previousBoxLen.

> Source/WebCore/editing/visible_units.cpp:373
> +                offset == (int)textBox->start() && previousBoxInDifferentLine) :

Please use static_cast<int>(~).

> Source/WebCore/editing/visible_units.cpp:392
> +    // FIXME: How should we handle a non-editable position?
> +    if (leftWordBreak.isNull() && isEditablePosition(visiblePosition.deepEquivalent())) {
> +        TextDirection blockDirection = directionOfEnclosingBlock(visiblePosition.deepEquivalent());
> +        leftWordBreak = blockDirection == LTR ? startOfEditableContent(visiblePosition) : endOfEditableContent(visiblePosition);
> +    }

I think it's okay to ignore editing boundaries in wordPosition function and then just call honorEditingBoundaryAtOrBefore here.

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