[Webkit-unassigned] [Bug 110487] Ctrl+Shift+Right in Windows should select the spacing after the word

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Mar 25 17:17:24 PDT 2013


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


Ryosuke Niwa <rniwa at webkit.org> changed:

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




--- Comment #44 from Ryosuke Niwa <rniwa at webkit.org>  2013-03-25 17:19:48 PST ---
(From update of attachment 194615)
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.

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


More information about the webkit-unassigned mailing list