[webkit-reviews] review denied: [Bug 85017] enable ctrl-arrow move by word visually in other platforms (besides Windows) : [Attachment 139304] patch w/ layout test

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Xiaomei Ji <xji at chromium.org>'s
request for review:
Bug 85017: enable ctrl-arrow move by word visually in other platforms (besides
Windows)
https://bugs.webkit.org/show_bug.cgi?id=85017

Attachment 139304: patch w/ layout test
https://bugs.webkit.org/attachment.cgi?id=139304&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.