[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 #10 from Ryosuke Niwa <rniwa at webkit.org>  2011-04-06 10:11:40 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?

I disagree. We should name a function to explain for what purpose the function is used especially because this function is only used in one place.

>>> 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".

Ah, I see. Thanks for the clarification.  Yes, the function name should be positionBeforeNextWord and variables should be named along the lines of positionAfterCurrentWord and positionAfterNextWord respectively.

>>> 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"?

I'd name them positionAfterPreviousWord, positionBeforeCurrentWord, and positionBeforePreviousWord respectively.

>> Source/WebCore/editing/visible_units.cpp:1448
>> +    collectWordBreaksInBox(box, blockDirection, wordBoundaries);
> 
> Mitz suggested inline capacity to avoid heap allocation in most cases.

I feel like that's a pre-mature optimization.  If anything, we should make a benchmark and figure out if that'll affect the performance at all.

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