[webkit-reviews] review denied: [Bug 54429] Refactor the extend-selection tests to be clearer what they're doing : [Attachment 82410] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Benjamin (Ben) Kalman
<kalman at chromium.org>'s request for review:
Bug 54429: Refactor the extend-selection tests to be clearer what they're doing
https://bugs.webkit.org/show_bug.cgi?id=54429

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.