[Webkit-unassigned] [Bug 57806] continue experiment with moving caret by word in visual order

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Xiaomei Ji <xji at chromium.org>  2011-04-06 10:00:25 PST ---
(From update of attachment 88319)
View in context: https://bugs.webkit.org/attachment.cgi?id=88319&action=review

>> Source/WebCore/editing/visible_units.cpp:1232
>> +static bool positionIsVisuallyOrderedInBox(const VisiblePosition& wordBreak, const InlineBox* box, int& offsetOfWordBreak)
> 
> I don't think positionIsVisuallyOrderedInBox is a descriptive name here.  It explains what the function is checking but it doesn't explain anything in the context of nextWordBreakInBoxInsideBlockWithDifferentDirectionality.  Please rename.

The function is named as what it does, does not matter what the context is. As to it is only needed when block and box has different directionality, I think that should be explained in the caller, as in the current code.
Or suggestion on a better name?

>> Source/WebCore/editing/visible_units.cpp:1400
>> +    VisiblePosition nextNext = nextWordPosition(next);
> 
> next and nextNext don't make any sense to me.  Next what?  It's much clearer to call next as beforeNextWord because it signifies the fact nextWordPosition returns the position between the next word and the whitespace immediately before the next word.  It then becomes clear from your other comment that we must move forward again to obtain beforeWordAfterNextWord or afterNextWord & backward in order obtain the position after the current word.

nextWordPosition does not return the position between the next word and the white space immediately before the next word. It returns the position after the current word, before the white space immediately before the next word.

This function returns the position between the next word and the white space immediately before the next word.

Then, the function name should be "positionBeforeNextWord"? 
and suggestion on "next"/"nextNext"? Themselves are nothing special. If follows your naming convention, they probably should be named as "positionAfterWord" and "positionAfterNextWord".

>> Source/WebCore/editing/visible_units.cpp:1406
>> +static VisiblePosition positionBeforePreviousWord(const VisiblePosition& position)
> 
> Ditto. This should be named as positionBeforeWord

the function returns position after previous word, by after, I mean logically. should it be named as "positionAfterPreviousWord"?
how about "previous" and "previousPrevious"? any suggestion? "positionBeforeWord" and "positionBeforePreviousWord"?

> Source/WebCore/editing/visible_units.cpp:1448
> +    collectWordBreaksInBox(box, blockDirection, wordBoundaries);

Mitz suggested inline capacity to avoid heap allocation in most cases.

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