Bug 54429

Summary: Refactor the extend-selection tests to be clearer what they're doing
Product: WebKit Reporter: Benjamin (Ben) Kalman <kalman>
Component: New BugsAssignee: Benjamin (Ben) Kalman <kalman>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 53756    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Benjamin (Ben) Kalman
Reported 2011-02-14 19:44:47 PST
Refactor the extend-selection tests to be clearer what they're doing
Attachments
Patch (62.26 KB, patch)
2011-02-14 19:54 PST, Benjamin (Ben) Kalman
no flags
Patch (25.02 KB, patch)
2011-02-14 22:25 PST, Benjamin (Ben) Kalman
no flags
Patch (24.86 KB, patch)
2011-02-14 23:37 PST, Benjamin (Ben) Kalman
no flags
Ryosuke Niwa
Comment 1 2011-02-14 19:46:14 PST
Should this bug block the bug 53756?
Benjamin (Ben) Kalman
Comment 2 2011-02-14 19:52:03 PST
(In reply to comment #1) > Should this bug block the bug 53756? In the middle of setting this bug up :-) yes.
Benjamin (Ben) Kalman
Comment 3 2011-02-14 19:54:43 PST
Ryosuke Niwa
Comment 4 2011-02-14 21:28:21 PST
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 5 2011-02-14 21:33:41 PST
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 6 2011-02-14 22:24:23 PST
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
Comment 7 2011-02-14 22:25:40 PST
Ryosuke Niwa
Comment 8 2011-02-14 22:40:19 PST
(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 9 2011-02-14 22:48:26 PST
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
Comment 10 2011-02-14 23:37:35 PST
Ryosuke Niwa
Comment 11 2011-02-14 23:43:45 PST
Comment on attachment 82421 [details] Patch r+ but you might want to consider my other comments.
Benjamin (Ben) Kalman
Comment 12 2011-02-14 23:56:24 PST
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 13 2011-02-15 00:00:01 PST
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 14 2011-02-15 08:15:03 PST
Comment on attachment 82421 [details] Patch Clearing flags on attachment: 82421 Committed r78565: <http://trac.webkit.org/changeset/78565>
WebKit Commit Bot
Comment 15 2011-02-15 08:15:09 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.