[webkit-reviews] review denied: [Bug 110487] Ctrl+Shift+Right in Windows should select the spacing after the word : [Attachment 194615] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Claudio Saavedra
<csaavedra at igalia.com>'s request for review:
Bug 110487: Ctrl+Shift+Right in Windows should select the spacing after the
word
https://bugs.webkit.org/show_bug.cgi?id=110487

Attachment 194615: Patch
https://bugs.webkit.org/attachment.cgi?id=194615&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=194615&action=review


I would have preferred test changes to be submitted separately on a separate
bug so that we can assess the effect of the code change more closely.

> Source/WebCore/editing/FrameSelection.cpp:584
> +VisiblePosition FrameSelection::nextWordPositionForPlatform(const
VisiblePosition &c)

Please don't use one-letter variables like c.

> Source/WebCore/editing/FrameSelection.cpp:586
> +    VisiblePosition pos = nextWordPosition(c);

Please don't use abbreviations like "pos". Also, "position" is a very generic
name that doesn't really tell us the semantics.
How about "currentPosition"?

> Source/WebCore/editing/FrameSelection.cpp:590
> +    // In Windows, moving by word moves until the beginning of the
> +    // next word. We emulate that behavior here by moving twice
> +    // forward (intermediatePosition) and then once back.

I'm not sure if this comment is all that helpful. Given that this is an editing
behavior, it's no longer tied to Windows or that it may change in the future.
Given that the code already tells us we're moving forward twice and going
backwards once, we're probably repeating ourselves needlessly.
In WebKit, we try to avoid adding comments like this that merely explains what
the code does as much as possible.
If we could explain *why* we do it, that's useful.

> Source/WebCore/editing/FrameSelection.cpp:592
> +	   VisiblePosition intermediatePosition = nextWordPosition(pos);

"intermediate" is not a descriptive name either. Something like
positionAfterCurrentWord would better. In particular, this emphasizes the fact
it's not after the whitespace.

> Source/WebCore/editing/FrameSelection.cpp:597
> +	   // When moving back, it is possible that we end up in the
> +	   // start of the current word. In that case, just move until
> +	   // the intermediate position.

Instead of adding a long comment line this, we can give the variable a more
describe name.

> Source/WebCore/editing/FrameSelection.cpp:599
> +	   VisiblePosition wordStart =
previousWordPosition(nextWordPosition(c));
> +	   if (pos == wordStart)

e.g. we can do something like
bool movingBackwardsTwiceMovedPositionToStartOfCurrentWord =
intermiediatePosition == previousWordPosition(nextWordPosition(c));

> LayoutTests/ChangeLog:8
> +	   This is a massive review of all tests that use word selection for

Massive review? or massive rebaseline?

> LayoutTests/ChangeLog:28
> +	   uncovered a bug in execCommand(), reported now as wkb112240, that

Please use a regular URL like http://webkit.org/b/112240. "wkb" is not a thing.


> LayoutTests/ChangeLog:32
> +	   * editing/deleting/smart-editing-disabled-mac-expected.txt: Copied
from LayoutTests/editing/deleting/smart-editing-disabled-expected.txt.
> +	   * editing/deleting/smart-editing-disabled-mac.html: Copied from
LayoutTests/editing/deleting/smart-editing-disabled.html.

These lines are really long. Please line break somewhere. And ditto for the
following lines.

> LayoutTests/TestExpectations:12
> +# pending bug fix
> +webkit.org/b/112240
editing/execCommand/remove-format-multiple-elements-win.html [ Skip ]

Please don't skip an existing test on all platforms. r- because of this change.

You may add [ Failure ] expectation on some platform where this test fails but
it's not acceptable to disable a test on all platforms.
If we need to fix a test, then please fix it before submitting a patch on this
bug.