[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


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #139304|review?                     |review-
               Flag|                            |




--- Comment #8 from Ryosuke Niwa <rniwa at webkit.org>  2012-04-29 15:38:21 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.

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

> Source/WebCore/editing/EditingBehavior.h:74
> +    // But ctrl+right_arrow moves to the begin of word in Windows in a way that it eats space to the next word.
> +    // 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?

> Source/WebCore/editing/EditingBehavior.h:76
> +    // while ctrl(alt)+right_arrow moves to the end of word in Mac and Linux in a way that it does not eat space to the
> +    // next word. For example, the word break positions are "abc| def| hij| opq|".

Ditto. We should say before whitespace.

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

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

You can just do:
bool eatSpaceToNextWord = m_frame && m_frame->editor()->behavior().shouldEatSpaceToNextWord().

> Source/WebCore/editing/visible_units.cpp:371
> +    bool eatSpaceToNextWord)

I would call this variable skipsSpaceBeforeWord.

> Source/WebCore/editing/visible_units.cpp:425
> +            || (!eatSpaceToNextWord && direction == MoveLeft && box->direction() == LTR)
> +            || (!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?

> Source/WebCore/editing/visible_units.h:46
> +VisiblePosition rightWordPosition(const VisiblePosition&, bool);
> +VisiblePosition leftWordPosition(const VisiblePosition&, bool);

You need variable name here because it's not obvious what these two boolean arguments should be.

> LayoutTests/ChangeLog:7
> +

Please explain why expectations have changed.

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