I usually recommend against core changes just to make it easier for the GUI. In this case, the logic "is a test net?" is only used in the outer function to determine whether to tint the client green or not.
NACK on moving it to ChainParams.

Thanks for the feedback. Thinking more about it, doesn't really make much sense.
Another related idea would be to move TestNet() and RegTest() to be methods of CChainParams.

TestNet() may seem more convenient than Params().testNet() or Params().isTestNet() but makes the code less readable. Actually my preference would be params.isTestNet(), using a simple instance instead of a const global-ish function, but it's probably better to refactor a little bit at a time.
Could I make that change?

The intention was to get rid of TestNet/RegTest eventually.

The idea behind CChainParams is to encompass the properties "quacks like a duck" of the chain.

TestNet() / RegTest() are "is this specific chain the ..." identify functions. Moving them to the object is a bit strange in my eyes as by definition there is only one TestNet and one RegTest.

The idea would be to split the TestNet() / RegTest() up into specific properties of the appropriate networks. However the few calls to TestNet/RegTest left are very specialized properties like "switch difficulty to min after nTargetSpacing*2 time between blocks" for which it's likely not worth making individual queries (come up with a name for that!).

Ok, that makes sense. So, yes, the proposed change is not a big move in that direction.
I'm also noticing that there's no consisntency in the use of "Params().NetworkID() == CChainParams::TESTNET" vs "TestNet()".
I now have a clearer idea of the changes I can do to like this more. Hopefully you will like my next push more. I'll reuse this pull request and change the tittle.

Any idea why this could have failed?

I'm going back and forth on whether "Params().NetworkID() ==" or TestNet() is better but we should certainly only have one or the other and be consistent. My initial thought that was "Params().NetworkID() ==" was different from "Params().NetworkID() !=" and harder to grep, but that could be easily solved with Params().isNetwork(network) or something like that.

Anyway, now it's easier for me to get rid of Params().isRegtest() which is really my goal here (moving towards the "quack like a duck").

So this is definitely not prepared for merging yet, but it's good to have some feedback in the meantime.

Now REGTEST "quacks like a duck".
I'm thinking that the test errors may be related to the windows or mac build. Hopefully restoring NetworkID will make it disappear?

It's useful for me to destroy and restore things when refactoring, but maybe is not so good for merging?
Feedback on that welcomed as well

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/992f5be1e106affc55109749ac7940cd11b5cef7 for binaries and test log.

This could happen for one of several reasons:

1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
4. The test suite fails on either Linux i386 or Win32
5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

Ok, it seems I'm not building on top of the latest changes, I'll start again. Closing.