[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


Ryosuke Niwa <rniwa at webkit.org> changed:

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




--- Comment #16 from Ryosuke Niwa <rniwa at webkit.org>  2012-03-15 09:48:16 PST ---
(From update of attachment 131956)
View in context: https://bugs.webkit.org/attachment.cgi?id=131956&action=review

r-. Let's do one more iteration.

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

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

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

> Source/WebCore/editing/visible_units.cpp:79
> +    Node* n = child ? child->nextLeafNode() : node->lastDescendant()->nextLeafNode();

Ditto about one-letter variable.

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

Same comment.

> Source/WebCore/editing/visible_units.cpp:110
> +    while (previousNode && enclosingBlockNode == enclosingNodeWithNonInlineRenderer(previousNode))

Please put a blank line before while for the better readability.

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

> Source/WebCore/editing/visible_units.cpp:166
> +    leafBoxesInLogicalOrder.clear();
> +    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.

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

> Source/WebCore/editing/visible_units.cpp:172
> +        for (size_t i = 0; i < leafBoxesInLogicalOrder.size(); ++i) {

Shouldn't we be decrementing i here?

> Source/WebCore/editing/visible_units.cpp:180
> +    for (int j = previousBoxIndex; j >= 0; --j) {

We can use i here again.

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

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

Ditto about the name.

> Source/WebCore/editing/visible_units.cpp:339
> +enum CursorMovementDirection {
> +    MoveLeft, MoveRight,
> +};

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,
};

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

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

> Source/WebCore/editing/visible_units.cpp:411
> +    // 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);
> +    }

Why don't we just call honor* here?

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