[webkit-reviews] review denied: [Bug 58294] continue (3rd) experiment with moving caret by word in visual order : [Attachment 89513] 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 58294: continue (3rd) experiment with moving caret by word in visual order
https://bugs.webkit.org/show_bug.cgi?id=58294

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=89513&action=review

yay! we're almost there.

> Source/WebCore/ChangeLog:14
> +	      and the direction of the movement, looking for visually adjacent
word breaks.

Nit: s/looking for/look for/

> Source/WebCore/ChangeLog:15
> +	   2.1 If the offset is the lestmost offset of the box,

Do you mean the minimum offset of the box?

> Source/WebCore/ChangeLog:21
> +	       based on the directionality of the box. If the word break
position is also inside
> +	       the box, just return it. Otherwise, collect the word breaks
inside the box and

I don't think this part makes sense. Why would you collect word breaks inside
the box when there are no word break inside the box?  I don't think you meant
this.

> Source/WebCore/ChangeLog:36
> +	   2. Only boundaries of the right kind (i.e. for LTR block, the
boundaries where the word
> +	      is to the right and space is to the left, based on Firefox's
behavior) are collected.

I don't really understand the sentence inside the paragraph.

> Source/WebCore/ChangeLog:39
> +	      for LTR box, word boundaries are collected from visually right to
visually left;

I think it's better to say "word boundaries are collected from right to left
visually in a LTR box"

> Source/WebCore/ChangeLog:45
> +	   3. Same reason as above, to avoid manually check for space character

> +	      (which I do not think we should), some of the codes are
inefficient. 

What is same reason as above?

> Source/WebCore/ChangeLog:49
> +	      For example, given a LTR box in LTR block, if the movement is
right, 
> +	      nextWordPosition() returns the boundary of the wrong kind, 
> +	      nextWordPosition()'s nextWordPosition()'s previousWordPosition()
returns the right 
> +	      kind. 

I don't think calling something "wrong" and "right" is descriptive.

> Source/WebCore/ChangeLog:51
> +	   4. When collect word breaks inside a box, it first computes a start
position, then

Nit: when s/collect word breaks/collecting word breaks/

> Source/WebCore/ChangeLog:52
> +	      collect the right kind of word breaks until reach the end (or
beyond) the box.

Nit: s/until reach the end/until it reaches the end of/

> Source/WebCore/ChangeLog:55
> +	      have not consider the box's bidi level, which I think they
probably should.

Nit: s/have not consider/does not consider/

I don't think "which I think they probably should" adds any value here.  If
anything, you should file a bug instead.

> Source/WebCore/editing/visible_units.cpp:-1283
> -    
> -    if (wordBreak == previousWordBreak) {
> -	   isLastWordBreakInBox = true;
> -	   return VisiblePosition();
> -    }
> -
> -

This seems to imply that isLastWordBreakInBox is never true in
collectWordBreaksInBoxInsideBlockWithDifferntDirectionality.  We should get rid
of the argument altogether. r- because of this.

> Source/WebCore/editing/visible_units.cpp:1367
> +static VisiblePosition nextWordBoundaryInBox(const InlineBox* box, int
offset)
> +{

If you make the following two changes, then this function becomes identical to
previousWordBoundaryInBox except that this one calls
nextWordBreakInBoxInsideBlockWithDifferentDirectionality whereas
previousWordBoundaryInBox calls
previousWordBreakInBoxInsideBlockWithSameDirectionality. Maybe we should
combine these two functions? But naming them properly will be tricky.  Maybe
findWordBoundaryInBox?

> Source/WebCore/editing/visible_units.cpp:1374
> +	   if (wordBreak.isNotNull() && (offset == invalidOffset ||
offsetOfWordBreak != offset))
> +	       return wordBreak;

It seems to be that we should break when wordBreak is null.

> Source/WebCore/editing/visible_units.cpp:1376
> +	   if (isLastWordBreakInBox)
> +	       break;

Ditto; isLastWordBreakInBox is never true.

> Source/WebCore/editing/visible_units.cpp:1381
> +static VisiblePosition visuallyLastWordBoundaryInBox(const InlineBox* box,
int offset, TextDirection blockDirection)

I don't think visually last is a good name.  It gave me a wrong impression that
it's at either edge of a box (i.e. rightmost word boundary in LTR box and
leftmost word boundary in RTL box).  I think visuallyPreviousWordBondaryInBox
will be more appropriate.

> Source/WebCore/editing/visible_units.cpp:1455
> +	       if (!box->isLeftToRightDirection())

I'd prefer to have if (box->isLeftToRightDirection()) here and exchange the
order of previousWordBoundaryInBox and nextWordBoundaryInBox.