[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 #17 from Xiaomei Ji <xji at chromium.org>  2012-03-15 12:21:26 PST ---
(From update of attachment 131956)
View in context: https://bugs.webkit.org/attachment.cgi?id=131956&action=review

>> Source/WebCore/editing/visible_units.cpp:52
>> +// The first leaf before node that has the same editability as node.
> 
> I think this comment is more confusing than useful.

Ah,  the first 4 functions are moved over by me without any functionality change. I updated the styles accordingly.
Removed the comment.

>> Source/WebCore/editing/visible_units.cpp:56
>> +    Node* n = node->previousLeafNode();
> 
> Can't we just modify node here? We try not to introduce new one-letter variable in WebKit.

done.

>> Source/WebCore/editing/visible_units.cpp:67
>> +    for (Node* p = n; p; p = p->parentNode()) {
> 
> Ditto about p & n. Just rename n to node and re-use that.

done.

>> Source/WebCore/editing/visible_units.cpp:94
>> +    Node* n = node->nextLeafNode();
> 
> Same comment.

done.

>> Source/WebCore/editing/visible_units.cpp:110
>> +    while (previousNode && enclosingBlockNode == enclosingNodeWithNonInlineRenderer(previousNode))
> 
> Please put a blank line before while for the better readability.

done.

>> Source/WebCore/editing/visible_units.cpp:160
>> +static const InlineTextBox* previousBoxInOneLine(const RootInlineBox* root, const InlineTextBox* box, Vector<InlineBox*>& leafBoxesInLogicalOrder)
> 
> I think "One" is redundant here because we don't say "lines".

done.

>> Source/WebCore/editing/visible_units.cpp:166
>> +    root->collectLeafBoxesInLogicalOrder(leafBoxesInLogicalOrder);
> 
> Wait... I don't think this is right. The only time we need to clear the cache is when we move onto a new root inline box.

the function only called when we move onto a  new root inline box. And it called continuously when moving to different root inline box so need to clear the vector.

>> Source/WebCore/editing/visible_units.cpp:169
>> +    // Otherwise, root is box's RootInlineBox, and previousBox is the previous logical box in the same line.
> 
> I think this second line is more confusing than helpful, and sort of repeats what the code says.

removed.

>> Source/WebCore/editing/visible_units.cpp:172
>> +        for (size_t i = 0; i < leafBoxesInLogicalOrder.size(); ++i) {
> 
> Shouldn't we be decrementing i here?

This is for looking for the current box's index, and it is the logical index we are looking for, so, it does not matter whether it is start from and incrementing or start from size-1 and decrementing.

>> Source/WebCore/editing/visible_units.cpp:180
>> +    for (int j = previousBoxIndex; j >= 0; --j) {
> 
> We can use i here again.

this one can not. Since prevBoxIndex could be -1 and can not be assigned to size_t. Also because of the name scope.

>> Source/WebCore/editing/visible_units.cpp:320
>> +static bool logicallyStartOfWord(TextBreakIterator* iter, int position, bool hardLineBreak)
> 
> Nit: logicalStartOfWord.
> By the way, I like isLogicalStartOfWord better.

done.

>> Source/WebCore/editing/visible_units.cpp:331
>> +static bool logicallyEndOfWord(TextBreakIterator* iter, int position, bool hardLineBreak)
> 
> Ditto about the name.

done.

>> Source/WebCore/editing/visible_units.cpp:339
>> +};
> 
> I think we normally put them in one line, or put each value in the enum in a separate line.
> i.e.
> enum A { V1, V2 };
> or
> enum A {
>     V1,
>     V2,
> };

done.

>> Source/WebCore/editing/visible_units.cpp:352
>> +        VisiblePosition adjacentCharacter = direction == MoveRight ? current.right(true) : current.left(true); 
> 
> Ugh... sorry, on my second thought, adjacentCharacter is confusing since it sounds as if it's a character.
> Maybe adjacentCharacterPosition or adjacentPosition?

done.

>> Source/WebCore/editing/visible_units.cpp:369
>> +        bool previousBoxInDifferentRenderer = false;
> 
> Maybe it's better to name this variable previousBoxIsInDifferentBlock or previousBoxIsInDifferentParagraph to clarify the semantics?

done.

>> Source/WebCore/editing/visible_units.cpp:411
>> +    }
> 
> Why don't we just call honor* here?

honor* is called above which enforces the returned visible position is in the same editing boundary.
The 'if' block is used to return the edge position of editable content.
For example, "abc def", since the word position we are looking for is *start* of word, it covers "|abc def" and "abc |def". But we also want to reach the edges of the text, the 'if' here and in rightWordPosition covers the "|abc def" and "abc def|".

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