[webkit-reviews] review denied: [Bug 60529] Programmatically set selection should not have direction on Mac : [Attachment 103518] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Wyatt Carss <wcarss at chromium.org>'s
request for review:
Bug 60529: Programmatically set selection should not have direction on Mac
https://bugs.webkit.org/show_bug.cgi?id=60529

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

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


The patch looks great!	r- due to a bug in the test but I think we're almost
ready to land this patch.

>
LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.ht
ml:53
> +function runTestsAndVerify(currentNode, currentSelectionModifier,
expectedDirection, expectedText) {
> +    var currentTest = makeTest(currentNode, currentSelectionModifier,
expectedDirection, expectedText);

Nit: It seems odd to call arguments "current" since there's only one node
touched inside this function.
I suggest rename these two arguments to be name and selectionModifier.

>
LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.ht
ml:63
> +function makeTest(currentNode, runCurrentTest, expectedDirection,
expectedText) {

Nit: Ditto about currentNode.

Also, why are we calling the second argument as runCurrentTest?  That's a very
general name that can mean almost anything since the entire file is a test.
Please call it selectionModifier as done elsewhere.

>
LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.ht
ml:72
> +	       if (action == 'insertText' || action == 'forwardDelete')
> +		   window.getSelection().modify(action, 'word');

modify only accepts "move" or "extend" for the first argument. r- because of
this.  You probably meant execCommand(action, false, 'word');

But since it's always safe to pass the 3rd argument, I'd suggest you always
call document.execCommand(action, false, 'word') instead.

>
LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.ht
ml:85
> +function selectTestInElement(currentNode) {

I'd move this function above runTestsAndVerify so that all 3 functions are put
together.  Also I'd rename currentNode to node.

>
LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.ht
ml:94
> +function verifyResult(expectedDirection, expectedText, description) {

This function is only called in makeTest, and I'd rather see all of code inside
makeTest so that I can grasp the idea of what has been tested there.

>
LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.ht
ml:114
> +function runTestsOn(currentPlatform, currentNode) {

Nit: ditto about "current" in currentPlatform and currentNode.

>
LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.ht
ml:115
> +    var backwardOnMacAndForwardOtherwise = {'mac': 'backward', 'win':
'forward', 'unix': 'forward'}[currentPlatform];

Since win and unix will always have the same results in all these tests, I'd
define a local variable like macOrNonMac = platform == 'mac' ? 'non-mac'; to
reduce the duplication.

> Source/WebCore/ChangeLog:10
> +	   to make programmatic selection be directionless by default on mac.

s/mac/Mac/

> Source/WebCore/ChangeLog:12
> +	   Also modified endingSelection() in two spots to preserve
isDirectionality

Could you tells us where they were?