[webkit-reviews] review denied: [Bug 25298] Ctrl + Right/Left arrow move forward/backward through document instead of right/left in RTL text : [Attachment 78549] patch w/ layout test

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Xiaomei Ji <xji at chromium.org>'s request for review:
Bug 25298: Ctrl + Right/Left arrow move forward/backward through document
instead of right/left in RTL text
https://bugs.webkit.org/show_bug.cgi?id=25298

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

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=78549&action=review

> Source/WebCore/ChangeLog:22
> +	   * editing/SelectionController.cpp:
> +	   (WebCore::SelectionController::modifyExtendingRight):
> +	   (WebCore::SelectionController::modifyMovingRight):
> +	   (WebCore::SelectionController::modifyExtendingLeft):
> +	   (WebCore::SelectionController::modifyMovingLeft):
> +	   * editing/SelectionController.h:
> +	   * editing/htmlediting.cpp:
> +	   (WebCore::directionOfEnclosingBlock):
> +	   * editing/htmlediting.h:
> +	   * editing/visible_units.cpp:
> +	   (WebCore::visiblePositionComparison):
> +	   (WebCore::wordBoundary):
> +	   (WebCore::leftWordPosition):
> +	   (WebCore::rightWordPosition):
> +	   * editing/visible_units.h:

The change log should describe the changes, not just list the changed
functions.

> Source/WebCore/editing/SelectionController.cpp:-354
> -TextDirection SelectionController::directionOfEnclosingBlock()
> -{
> -    Node* enclosingBlockNode = enclosingBlock(m_selection.extent().node());
> -    if (!enclosingBlockNode)
> -	   return LTR;
> -    RenderObject* renderer = enclosingBlockNode->renderer();
> -    if (renderer)
> -	   return renderer->style()->direction();
> -    return LTR;
> -}
> -

Seems useful to leave this function here (perhaps changed to call the new one
in htmlediting) rather than change all call sites.

> Source/WebCore/editing/htmlediting.cpp:335
> +TextDirection directionOfEnclosingBlock(Node* n)

It’s better to use a word like “node” rather than a single letter for a
variable name.

> Source/WebCore/editing/htmlediting.h:100
> +// TextDirection on Node's enclosing block
> +TextDirection directionOfEnclosingBlock(Node*);

The function’s name makes the comment redundant. Can it take a const Node*?

> Source/WebCore/editing/visible_units.cpp:1221
> +enum WORDDIRECTION {
> +    LEFTWORD,
> +    RIGHTWORD
> +};

Enums and their values should be in “CamelCase”, not uppercase. How does this
enum differ from TextDirection?

> Source/WebCore/editing/visible_units.cpp:1223
> +static VisiblePosition wordBoundary(enum WORDDIRECTION direction, const
VisiblePosition& c)

No need to qualify WORDDIRECTION with the “enum” keyword. “c” is a poor name
for a VisiblePosition. How about “visiblePosition” or “position”?

> Source/WebCore/editing/visible_units.cpp:1233
> +	   VisiblePosition wordBreak = ((direction == RIGHTWORD &&
box->direction() == LTR) || (direction == LEFTWORD && box->direction() == RTL)
? nextWordPosition(c) : previousWordPosition(c));

For example, here, if you used TextDirection instead of WORDDIRECTION you could
have just written this expression as
    (direction == boxDirection) ? nextWordPosition(c) :
previousWordPosition(c);

> Source/WebCore/editing/visible_units.cpp:1236
> +	   if (offset != boxOfWordBreak->caretMaxOffset() && offset !=
boxOfWordBreak->caretMinOffset() && box == boxOfWordBreak)

You’d want to move the box == boxOfWorkdBreak condition to the beginning, to
avoid unnecessary calls to caret{Max,Min}Offset(). If you do this, then you’ll
be getting box->caret{Max,Min}Offset(). Since you’ve already gotten them in
line 1232, I suggest using local variables for them.

> Source/WebCore/editing/visible_units.cpp:1242
> +    Vector<VisiblePosition> wordBoundaries;

You should give this Vector some inline capacity to avoid heap allocation in
most cases.

> Source/WebCore/editing/visible_units.cpp:1256
> +	   Node* node = renderer->node();
> +	   if (!node)
> +	       break;

Can we do better than bail out in this case? How does this affect lines
starting or ending with generated content?

> Source/WebCore/editing/visible_units.cpp:1264
> +	   if ((direction == RIGHTWORD && directionOfBlock != box->direction())

> +	       || (direction == LEFTWORD && directionOfBlock ==
box->direction()))
> +	       wordBoundaries.append(previousPosition); 

This appends previousPosition to wordBoundaries but we don’t know yet that
previousPosition is a word boundary. Why is this okay?

> Source/WebCore/editing/visible_units.cpp:1267
> +	   while (1) {

WebKit style is to use while (true)

> Source/WebCore/editing/visible_units.cpp:1305
> +    std::sort(wordBoundaries.begin(), wordBoundaries.end(),
visiblePositionComparison);
> +
> +    VisiblePosition previousPosition = c;
> +    while (true) {
> +	   VisiblePosition position = (direction == RIGHTWORD ?
previousPosition.right(true) : previousPosition.left(true));
> +	   if (std::binary_search(wordBoundaries.begin(), wordBoundaries.end(),
position, visiblePositionComparison))
> +	       return position;
> +	   if (position == previousPosition)
> +	       return position;
> +	   previousPosition = position;
> +    }

This doesn’t look very efficient (and I am not convinced it’s correct). I would
think that you could collect all word boundaries as (InlineBox*, offset) pairs
(already ordered by box from left to right), find the nearest box to c in the
direction you’re going, then find the word boundary offset nearest to c in that
box (depending on the direction of that box). You’ll need to discard boundaries
of the wrong kind (i.e. if going to the right, you want to keep boundaries
where the word is to the left and the space is to the right).

> Source/WebCore/editing/visible_units.cpp:1309
> +VisiblePosition leftWordPosition(const VisiblePosition& c)

Please consider renaming “c”.

> Source/WebCore/editing/visible_units.cpp:1314
> +VisiblePosition rightWordPosition(const VisiblePosition& c)

Ditto.

> Source/WebCore/editing/visible_units.h:45
> +VisiblePosition rightWordPosition(const VisiblePosition &);
> +VisiblePosition leftWordPosition(const VisiblePosition &);

There shouldn’t be spaces before the ampersands here.