[webkit-reviews] review denied: [Bug 54429] Refactor the extend-selection tests to be clearer what they're doing : [Attachment 82414] 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 82414: Patch
https://bugs.webkit.org/attachment.cgi?id=82414&action=review

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