Sanitize simpletest.js checkbox code

More accurate title, as it also touches a few lines in the expand/collapse function.

Ok, the five-liner dependency has been applied, now it's the turn of this patch. I do hope that not killing kittens pays off, especially since my other patch for #305150: Keep running tests checked depends on this clean-up.

@Dries:
Not that I'm experienced with JavaScript/jQuery in any way, but if I remember correctly, $(this) denotes the current context (for the elements that $(...) selects) whereas self is declared in the "top context" and stays the same also inside the other functions in all the sub-contexts. Thus, $(this) is different than self in all functions except the one where self is declared. At least, that's how I got it? Might be wrong here, tell me if I am.

On the other hand, you're right that there are a few instances of $(this) that could safely be replaced by self, I just don't know which way is more easily understandable.

@webchick:
True, but on the other hand "trs" accurately describes what the variable holds, other variable names would be more meta than that. Suggestions for a better name? "test_rows" might be workable, maybe.

Better, yes. Seems to me like using self consistently is the only choice here, as using $(this) where appropriate is confusing even to Dries. So let's go all the way. Reworked the comments a bit, too.

I noticed that the select-everything checkbox at the top doesn't work correctly with this patch applied (it also didn't exist when I first wrote the patch), so that should probably be fixed. Steps to reproduce:

1. Select the IP address test in the Bootstap group. (Yeah, just an example, you can do it with any other test too.)
2. Run the test. When completed, the same test should be checked again.
3. Click the select-everything checkbox to select all tests, and click it again to uncheck all of them.
4. Check the IP address test and one more test in the Bootstrap category. The Bootstrap group checkbox will be checked although one test is still missing from the group.

In other words, checking or unchecking the select-all checkbox doesn't update the 'simpletest-checked-tests' counter. Which makes me wonder if we should use such a counter at all... it should totally be possible to check this stuff on the fly, shouldn't it?

Oh wow, this is just so much better. Getting rid of status tracking and self in the checkbox handling part, which resolves all possible bugs there might ever be. Or something. Please review and commit.

Ok, so #305150: Keep running tests checked has been committed for quite a while now, but as people love features more than janitorial stuff, this issue got ignored as soon as the other one was in. That means that the bugs in the simpletest.js checkbox code do now actually manifest, because some test checkboxes are checked initially but ignored by simpletest.js's internal "selected checkboxes per group" counter.

The group collapsing function has been cleaned up by some other patch, so there's no need for cleaning that one up anymore. Therefore, let's focus on the actual purpose of this issue and get the checkbox code a) readable, and b) bug free. Rerolled patch attached.

In case you don't believe how buggy this is, here's a few steps to reproduce:

1. Choose your favorite test group, and check all tests (doesn't matter if you're doing it with the group checkbox or by checking each test separately).
2. Run those tests. When the test run has finished, the group checkbox will not be checked (although all tests in the group are), and will neither be synchronized correctly when you check/uncheck the tests.
3. When you check or uncheck the group checkbox itself, those problems will disappear as the internal counter is being reset.

You might also try this one (slight variation, same cause):

1. Choose your favorite test group with two or three tests (for the sake of click avoidance, but yeah, it's also buggy for groups with 15 tests or stuff).
2. Select one test in that group, and run it. When the test run has finished, its checkbox is still checked.
3. Check the other tests in the same group, and notice the group checkbox not to sync with the test checkboxes correctly. Again, clicking on the group checkboxes will make the problem disappear.

Please get this committed, it's just a vast improvement over the current state and I'd be disappointed if I need to reroll it again in another 3 months.

er, the actual patch.

Boy, nothing motivates someone to commit a patch quite like a lecture. :P~~~

True, sorry for making it sound slightly bitter. Actually it's more like the opposite; I've been doing this patch for fun with no personal gain whatsoever - I have no need for it to be committed, do with it whatever you like. And sure, I know that someone needs to RTBC it before it gets committed, it's just that I won't run after anyone for that to happen. It's all yours!

Any reason for double negative?

Makes a boolean (true) out of a non-boolean (1 / "checked" / whatever the value was, I don't remember anymore). Makes a boolean (false) out of null, or whatever that value was. Not sure which one applied to that specific case - it's been 4 months since I wrote that stuff - but better safe than sorry.

Any reason for using ++checkedTests instead of checkedTests++? Seems like post-incramenting is standard when there is no reason for them to pre-incramenting.

If post-incrementing is the standard somehow then I'm totally fine with using it. I usually use the pre-increment because the post-increment is normally implemented as x = value; ++value; return x; which is kind of an unnecessary indirection, even if only a minor one. (Disclaimer: I haven't actually checked the ECMAScript specs or an implementation of it to make sure this is also the case here. Should apply, though.)

The script still takes a long time (2-3 seconds) when I check/uncheck the main checkbox, any thoughts?

"Still takes a long time" like in, "same as before", or like in "performance regression"? (It's just like a third second at my place, but that obviously depends a lot on the machine and JS interpreter.) Also, where is that "select all" checkbox being implemented anyways? I imagine that it probably checks all the test checkboxes which triggers some unnecessary calculation. As the version in my patch does not need to keep track of checkbox status, we could save performance if the JavaScript behavior of that function was turned off before (un)checking the boxes, and turned on after that's done.