[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 #102399|1                           |0
        is obsolete|                            |




--- Comment #34 from Ryosuke Niwa <rniwa at webkit.org>  2011-08-01 14:16:54 PST ---
(From update of attachment 102399)
View in context: https://bugs.webkit.org/attachment.cgi?id=102399&action=review

r- due to various nits.

> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:41
> +    range.setStart(container, container.data.search('test'));
> +    range.setEnd(container, range.startOffset + 'test'.length);

Since you're selecting the first appearance of "test" instead of the forth word, it might be better to call this function selectTestInDiv.

> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:46
> +function selectFourthWordInSecondLineOfTextField(container) {

Ditto about the function name.

> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:79
> +    macIsBackwardWinAndUnixAreForward = {'mac': 'backward', 'win': 'forward', 'unix': 'forward'}[currentPlatform];

I still think this variable name isn't describe.  It should be backwardOnMacAndForwardOtherwise or backwardOnMacAndForwardOnWinAndUnix.
Also, you're missing "var".

> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:81
> +    allPlatformsAreBackward = 'backward';

It appears that this variable is never used.

> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:86
> +    extendSelectionLeftByCharacter = makeSelectionModifier('left', 'character');
> +    extendSelectionRightByCharacter = makeSelectionModifier('right', 'character');
> +    extendSelectionLeftByLine = makeSelectionModifier('left', 'line');
> +    extendSelectionRightByLine = makeSelectionModifier('right', 'line');

Missing var :(

> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:92
> +        expectedResult = {'mac': 'The test', 'win': 'The ', 'unix': 'The '};

Missing "var" again.

> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:96
> +                          'win': 'second sentence. Three sentences.\nAnother sentence. A ', 
> +                          'unix': 'second sentence. Three sentences.\nAnother sentence. A '};

Nit: we don't deep-indent to align lines.  This line should be intended by 4 spaces in addition to the indentation before the previous line.

> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:100
> +                          'win': 'econd sentence. Three sentences.\nAnother sentence. A ', 
> +                          'unix': 'econd sentence. Three sentences.\nAnother sentence. A '};

Ditto about the deep indentation.

> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:111
> +    runTestAndVerify(currentNode, extendSelectionRightByLine, allPlatformsAreForward, expectedResult);

Just pass 'forward' here.

> LayoutTests/platform/chromium-win/editing/deleting/delete-at-start-or-end-expected.txt:4
> -EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > BODY > HTML > #document to 0 of DIV > BODY > HTML > #document toDOMRange:range from 0 of DIV > BODY > HTML > #document to 0 of DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE

It's kind of odd that we don't have webViewDidChangeSelection after calling shouldChangeSelectedDOMRange.  It's not like the editor client is canceling it.

> Source/WebCore/ChangeLog:9
> +        to 'false', and added a parameter to one VisibleSelection constructor

I don't think the term 'parameter' is used in C++ to mean an argument.

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

s/isDirectionality/isDirectional/.

> Source/WebCore/ChangeLog:15
> +        Many more spots likely exist where preserving directionality is a good 
> +        idea, and some of those will be found by forthcoming undo tests.

Are we adding new undo tests?  If so, where?  We should file a bug and put the bug number here.
On the other hand, if we're not doing this, then this comment seems redundant.

> Source/WebCore/editing/VisibleSelection.h:45
> +    VisibleSelection(const Position&, EAffinity, bool = false);

We should probably say what this new variable is because "bool = false" doesn't tell us any semantics.

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