[Webkit-unassigned] [Bug 85017] enable ctrl-arrow move by word visually in other platforms (besides Windows)

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Xiaomei Ji <xji at chromium.org>  2012-04-30 15:13:10 PST ---
(From update of attachment 139304)
View in context: https://bugs.webkit.org/attachment.cgi?id=139304&action=review

>> Source/WebCore/ChangeLog:7
>> +
> 
> You need to explain what kind of changes we're making in this patch. r- due to lack of explanation in the change log.

done.

>> Source/WebCore/editing/EditingBehavior.h:71
>> +    // Based on native behavior, when using ctrl(alt)+arrow to move word, ctrl(alt)+left_arrow moves to begin of word
> 
> "to move word" sounds awkward. "to move between words" maybe?
> Nit: to the beginning of word.

changed to "to move caret by word". 
changed to "to the beginning of word".

>> Source/WebCore/editing/EditingBehavior.h:74
>> +    // For example, the word break positions are: "abc |def |hij |opq".
> 
> Instead of saying "it eats space", can we just say that the caret should be after the whitespace?

done.

>> Source/WebCore/editing/EditingBehavior.h:76
>> +    // next word. For example, the word break positions are "abc| def| hij| opq|".
> 
> Ditto. We should say before whitespace.

done.

>> Source/WebCore/editing/FrameSelection.cpp:644
>> +#if !OS(WINCE) && !PLATFORM(QT)
> 
> Why are we disabling this feature on Qt? Is that because ICU is not available on Qt? This should be documented in the change log.

done.

>> Source/WebCore/editing/FrameSelection.cpp:648
>> +            eatSpaceToNextWord = true;
> 
> You can just do:
> bool eatSpaceToNextWord = m_frame && m_frame->editor()->behavior().shouldEatSpaceToNextWord().

done.

>> Source/WebCore/editing/visible_units.cpp:371
>> +    bool eatSpaceToNextWord)
> 
> I would call this variable skipsSpaceBeforeWord.

done.

>> Source/WebCore/editing/visible_units.cpp:425
>> +            || (!eatSpaceToNextWord && direction == MoveRight && box->direction() == RTL)) {
> 
> Can we define booleans for "(direction == MoveLeft && box->direction() == LTR) || (direction == MoveRight && box->direction() == RTL)" as well?
> Maybe movingBackward?

done.

>> Source/WebCore/editing/visible_units.h:46
>> +VisiblePosition leftWordPosition(const VisiblePosition&, bool);
> 
> You need variable name here because it's not obvious what these two boolean arguments should be.

done.

>> LayoutTests/ChangeLog:7
>> +
> 
> Please explain why expectations have changed.

done.

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