[Webkit-unassigned] [Bug 60529] Programmatically set selection should not have direction on Mac

[bugzilla-daemon at webkit.org]

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


Ryosuke Niwa <rniwa at webkit.org> changed:

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




--- Comment #57 from Ryosuke Niwa <rniwa at webkit.org>  2011-08-10 15:55:21 PST ---
(From update of attachment 103518)
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.html: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.html: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.html: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.html: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.html: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.html:114
> +function runTestsOn(currentPlatform, currentNode) {

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

> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html: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?

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