[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 #5 from Xiaomei Ji <xji at chromium.org>  2011-04-05 12:39:14 PST ---
(From update of attachment 88162)
View in context: https://bugs.webkit.org/attachment.cgi?id=88162&action=review

>> Source/WebCore/editing/visible_units.cpp:1206
>> +    const InlineBox* box, const VisiblePosition& previousWordBreak, int& offsetOfWordBreak, bool& last)
> 
> what does last mean here?  last in the box?
> 
> Also, this function has very few blank lines.  It's hard to grasp the logic when there is one giant block of code.  Please insert blank lines as needed to separate blocks of logic.  I'm likely to have to come back to this function again later because I'm yet to fully understand how it works.  But extracting functions and refactoring a bit should help.

changed to isLastWordBreakInBox.

>> Source/WebCore/editing/visible_units.cpp:1233
>> +        return wordBreak;
> 
> I'd extract this condition as a function instead of adding comment here like this.

I could extract. but I'd leave the comments there. Without them, it is very hard to understand why the code is doing that.

>> Source/WebCore/editing/visible_units.cpp:1240
>> +    VisiblePosition boundaryPosition;
> 
> What does boundaryPosition mean? I don't think it's descriptive.

it is the leftmost or rightmost position in box. suggestion for a better name?

> Source/WebCore/editing/visible_units.cpp:1282
> +    

it is more about granularity. I feel split them is more clear.

>> Source/WebCore/editing/visible_units.cpp:1312
>> +    } else {
> 
> return invalidOffset here instead so that you don't need the else clause.

I like the "if"/else branch, and I feel it is clearer. But no strong objection.

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