Lump client and server versions of Windows together.

Lump client and server versions of Windows together.

Previously, Server 2008 R2 and Server 2012 had been treated like Server 2008, which was bad. I debated adding enum values for these two, but I can't think of a compelling argument for it. The big argument against doing so would be that any code that currently has some logic based on the version bearing some relation to VERSION_WIN7, for example, would be broken one way or another on a 2008 R2 box

BUG=129217
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142539

[grt (UTC plus 2), 2012-05-23 04:32:19]

Carlos for correctness, Brett for OWNERS.  Thanks.

[cpu_(ooo_6.6-7.5), 2012-05-23 18:21:08]

I like this change. One other question, in how many places do we check / compare
with server versions?, I mean callers of this function?

http://codereview.chromium.org/10408084/diff/2001/base/win/windows_version.cc
File base/win/windows_version.cc (right):

http://codereview.chromium.org/10408084/diff/2001/base/win/windows_version.cc...
base/win/windows_version.cc:46: break;
is server 2008 also vista? I mean, if we are folding the server versions we
might as well fold them all.

http://codereview.chromium.org/10408084/diff/2001/base/win/windows_version.cc...
base/win/windows_version.cc:52: DCHECK_EQ(version_number_.minor, 2);
I rather have case 2: and a dcheck on the default:

[brettw, 2012-05-23 19:51:42]

Owners LGTM but I didn't look. I'll defer to Carlos' review.

[grt (UTC plus 2), 2012-05-23 21:43:49]

In practice, we pretty much don't test specifically for server versions.  The
one exception that isn't purely textual is this:

http://code.google.com/searchframe#OAMlx_jo-ck/src/base/win/pe_image_unittest...

I haven't looked into why the test wouldn't be supported in Server 2003 alone.

http://codereview.chromium.org/10408084/diff/2001/base/win/windows_version.cc
File base/win/windows_version.cc (right):

http://codereview.chromium.org/10408084/diff/2001/base/win/windows_version.cc...
base/win/windows_version.cc:46: break;
On 2012/05/23 18:21:08, cpu wrote:
> is server 2008 also vista? I mean, if we are folding the server versions we
> might as well fold them all.

Sure, why not.  Is it reasonable to lump all of these in with VERSION_XP:

XP Professional x64 Edition, Home Server, Server 2003, Server 2003 R2

http://codereview.chromium.org/10408084/diff/2001/base/win/windows_version.cc...
base/win/windows_version.cc:52: DCHECK_EQ(version_number_.minor, 2);
On 2012/05/23 18:21:08, cpu wrote:
> I rather have case 2: and a dcheck on the default:

But then when Windows 9 comes out, version_ will be VERSION_PRE_XP until someone
gets around to updating the code.  The way I have it, version_ will have what is
likely a more sane value to start with.

[rvargas (doing something else), 2012-06-05 02:21:44]

I actually don't like the change in the enum... as much as it makes little sense
for production code to use server versions when checking for some feature, base
code should be general enough while still being targeted to what we use (that is
to say, it doesn't have to support every possible scenario). For instance,
getting the correct version for a string is a valid use case (and again, not
really critical as to support the feature by itself). That is to say, having
server versions laid out at the right place seems totally fine to me.

On the other hand, now that I've seen the cl, I actually like how the cc code
looks. It removes an axis of the code.

https://chromiumcodereview.appspot.com/10408084/diff/11001/base/win/windows_v...
File base/win/windows_version.cc (right):

https://chromiumcodereview.appspot.com/10408084/diff/11001/base/win/windows_v...
base/win/windows_version.cc:51: default:  // case 2 appears to be win8.
nit: send the comment to the next line

[grt (UTC plus 2), 2012-06-15 00:05:43]

PTAL.  I plan to eventually change pe_image_unittest so that it tests a known
image file rather than a moving target included in the OS.

[grt (UTC plus 2), 2012-06-15 19:47:52]

ping

[cpu_(ooo_6.6-7.5), 2012-06-15 19:55:00]

lgtm

[commit-bot: I haz the power, 2012-06-15 22:01:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/10408084/16001

[commit-bot: I haz the power, 2012-06-15 22:01:15]

Presubmit check for 10408084-16001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui

Presubmit checks took 1.3s to calculate.

[grt (UTC plus 2), 2012-06-15 22:29:14]

+estade for OWNERS review of webui

[Evan Stade, 2012-06-15 22:57:50]

lgtm

[commit-bot: I haz the power, 2012-06-15 23:02:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/10408084/16001

[commit-bot: I haz the power, 2012-06-16 00:34:40]

Change committed as 142539