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

Wed Mar 14 16:37:58 PDT 2012

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

--- Comment #14 from Xiaomei Ji <xji at chromium.org>  2012-03-14 16:37:55 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.

done.

>> Source/WebCore/editing/visible_units.cpp:173
>> +            break;
> 
> If you do that, you can just return 0.

it actually should return previousBox. But not applicable anymore since the function changed.

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

it actually should not change. But not applicable anymore.

>> Source/WebCore/editing/visible_units.cpp:182
>> +    textBox->root()->collectLeafBoxesInLogicalOrder(leafBoxesInLogicalOrder);
> 
> I guess we can cache these vector for the current root inline box to avoid calling collectLeafBoxesInLogicalOrder on the same root inline box repeatedly when a single word is spread across multiple inline boxes but that can probably be done in a separate patch since these functions are only used when user presses ctrl+alt+left/right, and not used by any internal functions.

will do in a separate patch.

>> Source/WebCore/editing/visible_units.cpp:192
>> +            }
> 
> It seems like searchPreviousBoxInSameLine should also have this loop.
> It'll be great if we could add a test case for searchPreviousBoxInSameLine.

the original logic in searchPreviousBoxInSameLine is correct since previousBox is updated on every iteration. So, the above loop is not needed.
But I've changed these functions, and the new logic is always looking for current textbox first, then, iterate to find the previous or next Inline *text* box.

added test case to test non InlineTextBox.

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

done.

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

done.

>> Source/WebCore/editing/visible_units.cpp:311
>> +enum ECursorMovementDirection {
> 
> I think the new style is not to prefix enum types with E.

done.

>> Source/WebCore/editing/visible_units.cpp:312
>> +    MOVE_LEFT, MOVE_RIGHT,
> 
> We should use MoveLeft and MoveRight for the consistency.

done.

>> Source/WebCore/editing/visible_units.cpp:322
>> +    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.

done.

>> Source/WebCore/editing/visible_units.cpp:331
>> +static VisiblePosition wordPosition(const VisiblePosition& visiblePosition, ECursorMovementDirection direction)
> 
> We should probably call this function visualWordPosition.

done.

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

done.

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

renamed to offsetInBox.

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

done.

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

done.

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

done.

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

done.

>> Source/WebCore/editing/visible_units.cpp:372
>> +            isStartOfWord(iter, offset - textBox->start() + previousBoxLen,
> 
> Please define descriptive local variables for offset - textBox->start() + previousBoxLen.

done.

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

done.

>> Source/WebCore/editing/visible_units.cpp:392
>> +    }
> 
> I think it's okay to ignore editing boundaries in wordPosition function and then just call honorEditingBoundaryAtOrBefore here.

added honorEditingBoundaryAtOrBefore to reinforce.

>> LayoutTests/editing/selection/move-by-word-visually-inline-block-positioned-element.html:21
>> +[d_7, 16][d_7, 8][d_7, 5][d_7, 0][d_6, 8][d_6, 5][d_6, 0][d_5, 8][d_5, 5][d_5, 0][d_4, 8][d_4, 5][d_4, 0][d_3, 4][d_3, 0][d_2, 4][d_2, 0][d_1, 6][d_1, 0]" 
> 
> Maybe we want to change the format to make it more semantically clear. Of course, in a separate patch :)
> It's really hard to understand what's going on here.

I need to think a way to make the format more readable (although there is a comment about the format in each HTML file). I can prepend "elment_id", "position_offset", and "child_node_index" in each field, but that will make the long string even longer. Fortunately, the format in expected result is readable.

>> LayoutTests/editing/selection/move-by-word-visually-multi-line-expected.txt:40
>> +"abc def ghi jkl mn "[0, 3, 8, 11, 16], "opq rst uvw xyz"[0, 3, 8, 11, 15]
> 
> It's great to see this test case passing but can we print PASS in these passing cases? (in a separate patch, of course).

An alternative would be to print nothing for those PASS (And print a PASS at the end if all tests pass). Will do next.

>> LayoutTests/editing/selection/move-by-word-visually-multi-line.html:57
>> +<div contenteditable dir=rtl id="ml_8" class="test_move_by_word fix_width" title="[ml_8, 15, 5][ml_8, 11, 5][ml_8, 8, 5][ml_8, 3, 5][ml_8, 0, 5][ml_8, 16][ml_8, 11][ml_8, 8][ml_8, 3][ml_8, 0]|[ml_8, 0][ml_8, 3][ml_8, 8][ml_8, 11][ml_8, 16][ml_8, 0, 5][ml_8, 3, 5][ml_8, 8, 5][ml_8, 11, 5][ml_8, 15, 5]">abc def ghi jkl mn <div><br/></div><div><br/></div><div><br/></div>opq rst uvw xyz</div>
> 
> Please explain the change in the change log as done in person :)
> I understand that this is an improvement after talking with you, but it'll be nice to document it here so that other contributors like ap and mitz can understand it as well without having to talk with you in person.

done.

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