Bug 54429 – Refactor the extend-selection tests to be clearer what they're doing

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin (Ben) Kalman]

Refactor the extend-selection tests to be clearer what they're doing

[Ryosuke Niwa]

Should this bug block the bug 53756?

[Benjamin (Ben) Kalman]

(In reply to comment #1)
> Should this bug block the bug 53756?

In the middle of setting this bug up :-) yes.

[Benjamin (Ben) Kalman]

Created attachment 82410 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 82410 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82410&action=review

> LayoutTests/editing/selection/extend-selection-home-end-expected.txt:7
> -  Extending forward: "\nabc ABC xyz DEF def\n"[(1,1), (1,20)]
> -  Extending backward:  "\nabc ABC xyz DEF def\n"[(1,20), (1,1)]
> +  Extending forward:  "\nabc ABC xyz DEF def\n"[(1,1), (1,20)]
> +  Extending backward: "\nabc ABC xyz DEF def\n"[(1,20), (1,1)]

Where did this change come from?  We should avoid making these changes especially when we're refactoring test scripts even if new results are as good as old ones. r- because of this.

> LayoutTests/editing/selection/resources/extend-selection.js:8
> +    var s = window.getSelection();

Let's not use abbreviation.

> LayoutTests/editing/selection/resources/extend-selection.js:11
> +    var ltrNum = (granularity === "character") ? 5 : 1;
> +    var rtlNum = (granularity === "character") ? 15 : 3;

Ditto.  Please spell Number.  But ltrNumber isn't still descriptive.  This should be renamed such that what this variable stores is self-evident.

> LayoutTests/editing/selection/resources/extend-selection.js:18
> +    var reverse = (direction === "left") ? "right" : "left";

I don't think "reverse" is a good name. reversedDirection?  Also, you could just evaluate this inside s.modify() because I'm almost certain that JSC and V8 are smart enough not to evaluate it multiple times in the loop.

> LayoutTests/editing/selection/resources/extend-selection.js:34
> +    var s = window.getSelection();

Same comment about one-letter variable.

> LayoutTests/editing/selection/resources/extend-selection.js:38
> +    var nextPosition = {};
> +    while (nextPosition.node !== s.extentNode || nextPosition.end !== s.extentOffset) {
> +        nextPosition = { node: s.extentNode, begin: s.baseOffset, end: s.extentOffset };

Isn't do-while more appropriate here?

> LayoutTests/editing/selection/resources/extend-selection.js:76
> +function compareInDirection(positions, comparisonPositions, inReverse)

I don't think this function name is descriptive enough. This function not only compares but also logs errors.  How about compareTwoPositionListsAndLogErrors?

> LayoutTests/editing/selection/resources/extend-selection.js:95
> +        if (pos.begin !== comparison.begin) {
> +            log("WARNING: positions should be the same, but begin are not the same " + pos.begin + " vs. " + comparison.begin + "\n");
> +            break;
> +        }
> +        if (pos.end !== comparison.end) {

I'm not sure "break" here is that helpful.  Why don't you do if (~~) log(~~) else if (~~) so that you don't need curly brackets.  Here, you're changing the behavior to print only the first warning.  Is that really a good change?  Don't we want to see at least a couple of error logs so that we can diagnose the problem even if test failed on some port we don't have access?

> LayoutTests/editing/selection/resources/extend-selection.js:104
> +    compareInDirection(positions, comparisonPositions, true);

I feel like you can just reverse the order of positions here so that we don't need that extra parameter in compareInDirection.  I don't think it'll add that much overhead.

> LayoutTests/editing/selection/resources/extend-selection.js:114
> +    var positions = extendFn(direction, granularity);

What does Fn stand for?  How about "extender" or functionToExtendSelection?

> LayoutTests/editing/selection/resources/extend-selection.js:130
> +function runSelectionTestsWithGranularity(tests, granularity)

I don't think "tests" is a good name for this parameter.  It should be testNodes instead.

> LayoutTests/editing/selection/resources/extend-selection.js:199
> +function createNode(innerHTML, style)

I'm not sure if the argument should be named innerHTML because it's not innerHTML of any node.  IMO, it should be named content.

> LayoutTests/editing/selection/resources/extend-selection.js:266
> +function setupForLayoutTest()
> +{
> +    if (!window.layoutTestController)
> +        throw "Cannot find layoutTestController, running from outside test?";
> +    layoutTestController.dumpAsText();
> +    document.body.removeChild(getTestNodeContainer());
> +}

Can't you attach this as an load event listener so that you don't have to manually call it in all tests?  Also, you're calling this function only if window.layouTestController is defined so throw statement is useless now.

[Ryosuke Niwa]

Comment on attachment 82410 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82410&action=review

> LayoutTests/editing/selection/extend-selection-home-end.html:18
> +        log("  Extending forward:  ");
> +        extendAndLogSelectionToEnd("forward", "lineBoundary");
> +        log("  Extending backward: ");

Okay, it seems like this is what changed the outputs.  I see that this change aligns the output columns nicely but let's make this change in a separate patch.

[Benjamin (Ben) Kalman]

Comment on attachment 82410 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82410&action=review

>> LayoutTests/editing/selection/extend-selection-home-end-expected.txt:7
>> +  Extending backward: "\nabc ABC xyz DEF def\n"[(1,20), (1,1)]
> 
> Where did this change come from?  We should avoid making these changes especially when we're refactoring test scripts even if new results are as good as old ones. r- because of this.

I will revert this change and add it back later.

>> LayoutTests/editing/selection/resources/extend-selection.js:38
>> +        nextPosition = { node: s.extentNode, begin: s.baseOffset, end: s.extentOffset };
> 
> Isn't do-while more appropriate here?

perhaps.  I find do-while aesthetically ugly, but I've made the change.

>> LayoutTests/editing/selection/resources/extend-selection.js:76
>> +function compareInDirection(positions, comparisonPositions, inReverse)
> 
> I don't think this function name is descriptive enough. This function not only compares but also logs errors.  How about compareTwoPositionListsAndLogErrors?

Well, the "two" is redundant since it's implied by the number of arguments, and the "log errors"... well I would have thought that it's self-evident and not worth making the function name even longer for?

But I've removed this method as suggested, and renamed checkSameOrder to checkPositionLists and the other to checkPositionListsInReverse

>> LayoutTests/editing/selection/resources/extend-selection.js:95
>> +        if (pos.end !== comparison.end) {
> 
> I'm not sure "break" here is that helpful.  Why don't you do if (~~) log(~~) else if (~~) so that you don't need curly brackets.  Here, you're changing the behavior to print only the first warning.  Is that really a good change?  Don't we want to see at least a couple of error logs so that we can diagnose the problem even if test failed on some port we don't have access?

if (..) else if (..) would actually be even more different to how it currently is.  not "break"ing makes it similar to how it was originally but it now compares all positions rather than early exiting on the first.

>> LayoutTests/editing/selection/resources/extend-selection.js:104
>> +    compareInDirection(positions, comparisonPositions, true);
> 
> I feel like you can just reverse the order of positions here so that we don't need that extra parameter in compareInDirection.  I don't think it'll add that much overhead.

nice

>> LayoutTests/editing/selection/resources/extend-selection.js:266
>> +}
> 
> Can't you attach this as an load event listener so that you don't have to manually call it in all tests?  Also, you're calling this function only if window.layouTestController is defined so throw statement is useless now.

nice

[Benjamin (Ben) Kalman]

Created attachment 82414 [details]
Patch

[Ryosuke Niwa]

(In reply to comment #6)
> >> LayoutTests/editing/selection/resources/extend-selection.js:76
> >> +function compareInDirection(positions, comparisonPositions, inReverse)
> > 
> > I don't think this function name is descriptive enough. This function not only compares but also logs errors.  How about compareTwoPositionListsAndLogErrors?
> 
> Well, the "two" is redundant since it's implied by the number of arguments, and the "log errors"... well I would have thought that it's self-evident and not worth making the function name even longer for?

When a function is named compareX, I'd think that it compares two Xs and then return 0, 1, or -1.

> >> LayoutTests/editing/selection/resources/extend-selection.js:95
> >> +        if (pos.end !== comparison.end) {
> > 
> > I'm not sure "break" here is that helpful.  Why don't you do if (~~) log(~~) else if (~~) so that you don't need curly brackets.  Here, you're changing the behavior to print only the first warning.  Is that really a good change?  Don't we want to see at least a couple of error logs so that we can diagnose the problem even if test failed on some port we don't have access?
> 
> if (..) else if (..) would actually be even more different to how it currently is.  not "break"ing makes it similar to how it was originally but it now compares all positions rather than early exiting on the first.

I think printing all warnings is fine.

[Ryosuke Niwa]

Comment on attachment 82414 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82414&action=review

This patch is much nicer but still r- because of nits.

> LayoutTests/editing/selection/resources/extend-selection.js:81
> +function comparePositions(positions, comparisonPositions)

I still don't think comparePositions is a good name.  Of course this function compares two lists of positions but that's almost implementation detail.  What a caller of this function cares is that it logs error when they mismatch, and the name comparePositions doesn't convey that behavior.

> LayoutTests/editing/selection/resources/extend-selection.js:124
> +function extendAndLogSelectionWithinBlock(direction, granularity)
> +{
> +    return extendAndLogSelection(extendSelectionWithinBlock, direction, granularity);
> +}
> +
> +function extendAndLogSelectionToEnd(direction, granularity)
>  {
> -    for (var i = 0; i < tests.length; ++i) {
> -        tests[i].style.direction = "ltr";
> -        log("Test " + (i + 1) + ", LTR:\n  Extending right: ");
> -        sel.setPosition(tests[i], 0);
> -        var ltrRightPos = extendingSelection(sel, "right", granularity, 0);
> +    return extendAndLogSelection(extendSelectionToEnd, direction, granularity);
> +}

I'm not if these one line functions are that helpful.

> LayoutTests/editing/selection/resources/extend-selection.js:264
> +        document.body.removeChild(getTestNodeContainer());

It'll be nice if this code was right next to getTestNodeContainer().  Can we move the definition of getTestNodeContainer right above this if block?

[Benjamin (Ben) Kalman]

Created attachment 82421 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 82421 [details]
Patch

r+ but you might want to  consider my other comments.

[Benjamin (Ben) Kalman]

Comment on attachment 82414 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82414&action=review

>> LayoutTests/editing/selection/resources/extend-selection.js:81
>> +function comparePositions(positions, comparisonPositions)
> 
> I still don't think comparePositions is a good name.  Of course this function compares two lists of positions but that's almost implementation detail.  What a caller of this function cares is that it logs error when they mismatch, and the name comparePositions doesn't convey that behavior.

good point, thanks

>> LayoutTests/editing/selection/resources/extend-selection.js:124
>> +}
> 
> I'm not if these one line functions are that helpful.

They're actually used in the tests (.html files) which is why I gave them actual names.  I'd probably inline it otherwise.

>> LayoutTests/editing/selection/resources/extend-selection.js:264
>> +        document.body.removeChild(getTestNodeContainer());
> 
> It'll be nice if this code was right next to getTestNodeContainer().  Can we move the definition of getTestNodeContainer right above this if block?

Well, not that I run this through jlint, but if I did this would cause it to fail since create*Nodes relies on createNode relies on getTestNodeContainer.

[Ryosuke Niwa]

Comment on attachment 82421 [details]
Patch

(In reply to comment #12)
> (From update of attachment 82414 [details])
> >> LayoutTests/editing/selection/resources/extend-selection.js:124
> >> +}
> > 
> > I'm not if these one line functions are that helpful.
> 
> They're actually used in the tests (.html files) which is why I gave them actual names.  I'd probably inline it otherwise.
> 
> >> LayoutTests/editing/selection/resources/extend-selection.js:264
> >> +        document.body.removeChild(getTestNodeContainer());
> > 
> > It'll be nice if this code was right next to getTestNodeContainer().  Can we move the definition of getTestNodeContainer right above this if block?
> 
> Well, not that I run this through jlint, but if I did this would cause it to fail since create*Nodes relies on createNode relies on getTestNodeContainer.

OK.

[WebKit Commit Bot]

Comment on attachment 82421 [details]
Patch

Clearing flags on attachment: 82421

Committed r78565: <http://trac.webkit.org/changeset/78565>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.