[Webkit-unassigned] [Bug 58294] continue (3rd) experiment with moving caret by word in visual order

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=58294


Ryosuke Niwa <rniwa at webkit.org> changed:

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




--- Comment #6 from Ryosuke Niwa <rniwa at webkit.org>  2011-04-14 10:40:36 PST ---
(From update of attachment 89513)
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.

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