UMA: Set Hardware model class on Mac.

UMA: Set Hardware model class on Mac.

Record the hardware model on Mac e.g. MacPro1,1 .

BUG=429487
Committed: https://crrev.com/ba4eca9bf6691e4ed55eee55a0bcac1a932b7688
Cr-Commit-Position: refs/heads/master@{#303054}

[jeremy, 2014-11-04 14:15:58]

jeremy@chromium.org changed reviewers:
+ asvitkine@chromium.org, mark@chromium.org

[jeremy, 2014-11-04 14:15:59]

Alexei: I hope I got this right, if there's a better place to set this please
let me know.
Mark: Can you review the changes in base please.

[Mark Mentovai, 2014-11-04 14:36:37]

https://codereview.chromium.org/699173002/diff/1/base/sys_info.cc
File base/sys_info.cc (right):

https://codereview.chromium.org/699173002/diff/1/base/sys_info.cc#newcode47
base/sys_info.cc:47: #if !defined(OS_MACOSX)
Why even bother implementing this for non-Mac? Nothing ever calls it. Better to
just wrap the declaration in the header in an #if defined(OS_MACOSX).

https://codereview.chromium.org/699173002/diff/1/base/sys_info_mac.cc
File base/sys_info_mac.cc (right):

https://codereview.chromium.org/699173002/diff/1/base/sys_info_mac.cc#newcode91
base/sys_info_mac.cc:91: if (!sysctlbyname("hw.model", model, &len, NULL, 0))
The ! makes things look like failure. Can you use == 0 instead?

https://codereview.chromium.org/699173002/diff/1/base/sys_info_unittest.cc
File base/sys_info_unittest.cc (right):

https://codereview.chromium.org/699173002/diff/1/base/sys_info_unittest.cc#ne...
base/sys_info_unittest.cc:71: base::SysInfo::HardwareModelName();
This would definitely fit on one line.

https://codereview.chromium.org/699173002/diff/1/base/sys_info_unittest.cc#ne...
base/sys_info_unittest.cc:72: EXPECT_GT(hardware_model.length(), 0U);
You can just EXPECT_FALSE(hardware_model.empty()).

[Alexei Svitkine (slow), 2014-11-04 16:22:12]

https://codereview.chromium.org/699173002/diff/1/base/sys_info.cc
File base/sys_info.cc (right):

https://codereview.chromium.org/699173002/diff/1/base/sys_info.cc#newcode50
base/sys_info.cc:50: return "";
Nit: std::string()

https://codereview.chromium.org/699173002/diff/1/base/sys_info.h
File base/sys_info.h (right):

https://codereview.chromium.org/699173002/diff/1/base/sys_info.h#newcode54
base/sys_info.h:54: // Returns a descriptive string for the current machine
model or "Unknown"
Maybe better to return an empty string if unknown?

https://codereview.chromium.org/699173002/diff/1/base/sys_info.h#newcode55
base/sys_info.h:55: // if an error occured.
Can you provide an example of a return value in the comment?

Also, mention which platforms it's implemented on.

https://codereview.chromium.org/699173002/diff/1/components/metrics/metrics_l...
File components/metrics/metrics_log.cc (right):

https://codereview.chromium.org/699173002/diff/1/components/metrics/metrics_l...
components/metrics/metrics_log.cc:359: #endif
FWIW, I'd prefer if we can avoid ifdefs in this code.

How about providing an implementation of base::SysInfo::HardwareModelName() on
all platforms (i.e. contrary to Mark's comment) that returns an empty string on
everything except for the Mac implementation. Document that the empty string
means unknown. Then, this code can simply call:

  hardware->set_hardware_class(base::SysInfo::HardwareModelName());

Without any ifdefs. WDYT?

[jeremy, 2014-11-05 05:38:06]

All fixed, thanks for the quick review in the previous round!!

https://codereview.chromium.org/699173002/diff/1/base/sys_info.cc
File base/sys_info.cc (right):

https://codereview.chromium.org/699173002/diff/1/base/sys_info.cc#newcode47
base/sys_info.cc:47: #if !defined(OS_MACOSX)
Leaving this in per-asvitkine's comment.

https://codereview.chromium.org/699173002/diff/1/base/sys_info.cc#newcode50
base/sys_info.cc:50: return "";
On 2014/11/04 16:22:12, Alexei Svitkine wrote:
> Nit: std::string()

Done.

https://codereview.chromium.org/699173002/diff/1/base/sys_info.h
File base/sys_info.h (right):

https://codereview.chromium.org/699173002/diff/1/base/sys_info.h#newcode54
base/sys_info.h:54: // Returns a descriptive string for the current machine
model or "Unknown"
On 2014/11/04 16:22:12, Alexei Svitkine wrote:
> Maybe better to return an empty string if unknown?

Done.

https://codereview.chromium.org/699173002/diff/1/base/sys_info.h#newcode55
base/sys_info.h:55: // if an error occured.
Example added.
I've never seen us mention supported platforms in a header comment before - if
you think I should, I have no problem adding it :)

https://codereview.chromium.org/699173002/diff/1/base/sys_info_mac.cc
File base/sys_info_mac.cc (right):

https://codereview.chromium.org/699173002/diff/1/base/sys_info_mac.cc#newcode91
base/sys_info_mac.cc:91: if (!sysctlbyname("hw.model", model, &len, NULL, 0))
On 2014/11/04 14:36:37, Mark Mentovai wrote:
> The ! makes things look like failure. Can you use == 0 instead?

Done.

https://codereview.chromium.org/699173002/diff/1/base/sys_info_unittest.cc
File base/sys_info_unittest.cc (right):

https://codereview.chromium.org/699173002/diff/1/base/sys_info_unittest.cc#ne...
base/sys_info_unittest.cc:71: base::SysInfo::HardwareModelName();
On 2014/11/04 14:36:37, Mark Mentovai wrote:
> This would definitely fit on one line.

Done.

https://codereview.chromium.org/699173002/diff/1/base/sys_info_unittest.cc#ne...
base/sys_info_unittest.cc:72: EXPECT_GT(hardware_model.length(), 0U);
On 2014/11/04 14:36:37, Mark Mentovai wrote:
> You can just EXPECT_FALSE(hardware_model.empty()).

Done.

https://codereview.chromium.org/699173002/diff/1/components/metrics/metrics_l...
File components/metrics/metrics_log.cc (right):

https://codereview.chromium.org/699173002/diff/1/components/metrics/metrics_l...
components/metrics/metrics_log.cc:359: #endif
On 2014/11/04 16:22:12, Alexei Svitkine wrote:
> FWIW, I'd prefer if we can avoid ifdefs in this code.
> 
> How about providing an implementation of base::SysInfo::HardwareModelName() on
> all platforms (i.e. contrary to Mark's comment) that returns an empty string
on
> everything except for the Mac implementation. Document that the empty string
> means unknown. Then, this code can simply call:
> 
>   hardware->set_hardware_class(base::SysInfo::HardwareModelName());
> 
> Without any ifdefs. WDYT?

Done.

[Alexei Svitkine (slow), 2014-11-05 15:47:21]

LGTM % comments

https://codereview.chromium.org/699173002/diff/1/base/sys_info.h
File base/sys_info.h (right):

https://codereview.chromium.org/699173002/diff/1/base/sys_info.h#newcode55
base/sys_info.h:55: // if an error occured.
On 2014/11/05 05:38:06, jeremy wrote:
> Example added.
> I've never seen us mention supported platforms in a header comment before - if
> you think I should, I have no problem adding it :)

My preference would be to add it - as it's helpful to users of the API when
they're looking at the header file. Less surprises, etc.

https://codereview.chromium.org/699173002/diff/20001/base/sys_info.cc
File base/sys_info.cc (right):

https://codereview.chromium.org/699173002/diff/20001/base/sys_info.cc#newcode49
base/sys_info.cc:49: NOTIMPLEMENTED() << "HardwareModelName() not supported.";
I don't think it's worth having a NOTIMPLEMENTED() here.

Since we're calling it all the time, it will just result in unnecessary logging
on other platforms. If a platform doesn't provide this field, I think it's fine.
Per the comment of the functions, callers should expect that an empty string
could be returned. The calling code can then decide what to do about such cases,
if they care.

https://codereview.chromium.org/699173002/diff/20001/base/sys_info_mac.cc
File base/sys_info_mac.cc (right):

https://codereview.chromium.org/699173002/diff/20001/base/sys_info_mac.cc#new...
base/sys_info_mac.cc:93: return "";
Nit: std::string()

[jeremy, 2014-11-06 14:06:14]

All fixed.

Mark: wdyt?

[Mark Mentovai, 2014-11-06 14:10:14]

LGTM otherwise

https://codereview.chromium.org/699173002/diff/40001/base/sys_info.h
File base/sys_info.h (right):

https://codereview.chromium.org/699173002/diff/40001/base/sys_info.h#newcode57
base/sys_info.h:57: // Only implemented on OS X.
This makes it sound like it’s an error to call on non-Mac OS X, but that’s not
the case.

[jeremy, 2014-11-06 16:31:41]

The CQ bit was checked by jeremy@chromium.org

[commit-bot: I haz the power, 2014-11-06 16:32:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699173002/60001

[commit-bot: I haz the power, 2014-11-06 16:41:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-06 16:41:32]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[jeremy, 2014-11-06 17:19:20]

The CQ bit was checked by jeremy@chromium.org

[commit-bot: I haz the power, 2014-11-06 17:19:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699173002/80001

[commit-bot: I haz the power, 2014-11-06 17:29:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-06 17:29:46]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[jeremy, 2014-11-06 17:41:03]

The CQ bit was checked by jeremy@chromium.org

[commit-bot: I haz the power, 2014-11-06 17:42:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699173002/100001

[commit-bot: I haz the power, 2014-11-06 18:47:20]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-06 18:47:52]

Patchset 6 (id:??) landed as
https://crrev.com/ba4eca9bf6691e4ed55eee55a0bcac1a932b7688
Cr-Commit-Position: refs/heads/master@{#303054}