[webkit-reviews] review denied: [Bug 78856] visual word movement: Using ICU break iterator to simplify implementation : [Attachment 131956] patch w/ layout test

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Xiaomei Ji <xji at chromium.org>'s
request for review:
Bug 78856: visual word movement: Using ICU break iterator to simplify
implementation
https://bugs.webkit.org/show_bug.cgi?id=78856

Attachment 131956: patch w/ layout test
https://bugs.webkit.org/attachment.cgi?id=131956&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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?