[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





--- Comment #13 from Ryosuke Niwa <rniwa at webkit.org>  2011-04-14 15:16:53 PST ---
(From update of attachment 89642)
View in context: https://bugs.webkit.org/attachment.cgi?id=89642&action=review

> Source/WebCore/ChangeLog:19
> +        2.3 When offset is inside the box (not at boundaries), first find the previousWordPosition or nextWordPosition

I think you need a line break before or after previousWordPosition.

> Source/WebCore/ChangeLog:21
> +            also inside (not at the bounadries) the same box, return it. 

Since you've already qualified the previous "inside the box", I don't think you need to repeat yourself here; i.e. remove "(not at the boundaries)"

> Source/WebCore/ChangeLog:23
> +            Otherwise (the nextWordPosition/previousWordPosition is not
> +in the same box or is at the box boundary),  collect all the

What happened here??

> Source/WebCore/ChangeLog:33
> +           need to save InlineBox in word boundaries. Instead, the word boundaries are saved
> +           as (VisiblePosition, offset) pair so there is no need to recompute VisiblePosition

I feel like this is too wordy.  Maybe you can rephrase it as "as a pair (VisiblePosition, offset) to avoid recomputing VisiblePosition"

> Source/WebCore/ChangeLog:36
> +           after we calcuated which "offset" is the closest one to the input offset.
> +           (I think recompute VisiblePosition from offset might be the same complexity as
> +            getInlineBoxAndOffset()).

These comments seem to only add noise.

> Source/WebCore/ChangeLog:39
> +        2. Only boundaries of the right kind are collected. By right kind, it means the 
> +           left boundary of a word in LTR block and right boundary of a word in RTL block. 

I think you can put the explanation of right kind in parenthesis following the pattern as in:
We only collect boundaries of the right kind (i.e. left boundary of a word in LTR block and right boundary of a word in RTL block).

> Source/WebCore/ChangeLog:43
> +           So when box directionality is the same as block directionality,
> +           for LTR box, word boundaries are collected from right to left visually;
> +           for RTL box, offsets collected from left to right visually.

As I wrote in my previous review, this sentence doesn't make sense.  You need to rephrase "for LTR box" and "for RTL box".  See my suggestion in the previous review.

> Source/WebCore/ChangeLog:53
> +        3. Because for LTR block, only left boundary of a word is collected (and for RTL block,
> +           only right boundary of a word is collected), to avoid manually check for space character 
> +           (which I do not think we should), some of the codes are inefficient. 
> +           For example, given a LTR box in LTR block, if the movement is left to right, 
> +           nextWordPosition() returns the right boundary of a word, which is not what we want,
> +           nextWordPosition()'s nextWordPosition()'s previousWordPosition() returns
> +           the left boundary of a word, and it is what we want. 

This whole paragraph is very confusing. I'd revise it to something along the like of:

To find the right kinds of word boundaries, we must move back and forth between words
in some situations. For example, if we're moving to the right in a LTR box in LTR block,
we cannot simply return nextWordPosition() because it would return the right boundary
of a word. Instead, we return nextWordPosition()'s nextWordPosition()'s previousWordPosition().

> Source/WebCore/ChangeLog:58
> +           position bases on the directionality of the box and block. Those computation

Nit: s/bases/based/

Nit: These computations do not or This computation does not.

> Source/WebCore/editing/visible_units.cpp:1371
> +    bool isLastWordBreakInBox;
> +    while (true) {

I'd initialize isLastWordBreakInBox to false and do while(!isLastWordBreakInBox).

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

So that I can get rid of this if statement.

> Source/WebCore/editing/visible_units.cpp:1385
> +    if (orderedWordBoundaries.size()) {

I would do early return instead.

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