Implement SysInfo::CPUArchitecture() on Windows.

Implement SysInfo::CPUArchitecture() on Windows. Try to make Windows and POSIX versions return consistent architecture names.

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

[Mark Mentovai, 2012-09-13 12:40:19]

LGTM but consider this.

https://codereview.chromium.org/10910247/diff/8001/base/sys_info_posix.cc
File base/sys_info_posix.cc (right):

https://codereview.chromium.org/10910247/diff/8001/base/sys_info_posix.cc#new...
base/sys_info_posix.cc:86: arch = "x86_64";
For Mac and Linux x86_*, this returns the kernel’s bitness, which may not be the
same thing as the underlying hardware’s actual capability (32-bit kernel running
on 64-bit CPU) and may not be the same thing as the running Chrome’s bitness
(32-bit Chrome running on 64-bit kernel, or for that matter on the Mac, 64-bit
Chrome running on 32-bit kernel—except there currently is no 64-bit Mac Chrome).

Is all of this OK? Is it comparable to the behavior on Windows? The comment in
the header doesn’t explain which architecture of (CPU, kernel, app) this is
expected to return.

[Lei Zhang, 2012-09-21 21:10:21]

There's ~5 uses of CPUArchitecture(). Most of it is for logging / metrics. I've
CC'd the folks who wrote the callers in case they have any concerns.

sergeyu: remoting/host/server_log_entry.cc
isherman: chrome/browser/metrics/metrics_log.cc
jleyba: chrome/test/webdriver/webdriver_dispatch.cc
mnissler: chrome/browser/policy/device_management_service.cc
hongbo: chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc

https://chromiumcodereview.appspot.com/10910247/diff/8001/base/sys_info_posix.cc
File base/sys_info_posix.cc (right):

https://chromiumcodereview.appspot.com/10910247/diff/8001/base/sys_info_posix...
base/sys_info_posix.cc:86: arch = "x86_64";
On 2012/09/13 12:40:19, Mark Mentovai wrote:
> For Mac and Linux x86_*, this returns the kernel’s bitness, which may not be
the
> same thing as the underlying hardware’s actual capability (32-bit kernel
running
> on 64-bit CPU) and may not be the same thing as the running Chrome’s bitness
> (32-bit Chrome running on 64-bit kernel, or for that matter on the Mac, 64-bit
> Chrome running on 32-bit kernel—except there currently is no 64-bit Mac
Chrome).
>
> Is all of this OK? Is it comparable to the behavior on Windows? The comment in
> the header doesn’t explain which architecture of (CPU, kernel, app) this is
> expected to return.

Right, I think CPUArchitecture() should be renamed to OSArch(). The behavior on
Windows matches the behavior on Linux. I've added additional comments in the
header.

[Ilya Sherman, 2012-09-22 00:01:25]

No objections from me :)  Thanks for implementing this!

[Hongbo Min, 2012-09-22 00:55:09]

On 2012/09/22 00:01:25, Ilya Sherman wrote:
> No objections from me :)  Thanks for implementing this!

Good to me too:)

[jleyba, 2012-09-22 02:33:46]

On 2012/09/22 00:55:09, Hongbo Min wrote:
> On 2012/09/22 00:01:25, Ilya Sherman wrote:
> > No objections from me :)  Thanks for implementing this!
> 
> Good to me too:)

LGTM

[Mattias Nissler (ping if slow), 2012-09-22 04:33:58]

Thumbs up from my side as well.

[commit-bot: I haz the power, 2012-09-24 02:27:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/10910247/11001

[commit-bot: I haz the power, 2012-09-24 02:45:14]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Sergey Ulanov, 2012-09-24 17:50:00]

+simonmorris
Simon - will this break logging for chromoting?

[simonmorris, 2012-09-24 19:55:15]

On 2012/09/24 17:50:00, sergeyu wrote:
> +simonmorris
> Simon - will this break logging for chromoting?

LGTM for remoting.

[Lei Zhang, 2012-09-25 22:55:02]

Thanks for the feedback everyone.

Patch set 3 adds the right namespace to fix the Windows build.

[commit-bot: I haz the power, 2012-09-25 22:55:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/10910247/23001

[commit-bot: I haz the power, 2012-09-26 00:09:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/10910247/23001

[commit-bot: I haz the power, 2012-09-26 02:49:44]

Change committed as 158726