Only track profile inactivity for non-mobile platforms.

Only track profile inactivity for non-mobile platforms.

Profile_metrics doesn't log signed-in metrics for inactive profiles.
However, since mobile platforms never log active times, the signed-in
metrics never get logged.

Reference CLs: (introduced issue as of M35)
https://codereview.chromium.org/134863002/
https://codereview.chromium.org/141663004

BUG=392948
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284486

[Mike Lerman, 2014-07-17 20:44:02]

Hi Monica,

PTAL? Also, I'm very open to ideas about how to make the syntax around the if
{}'s and #ifdef look nicer. I can't find any combination I like.

Thanks!

[Mike Lerman, 2014-07-17 20:46:36]

Brian,

Your thoughts are also useful here!

[bcwhite, 2014-07-18 01:13:59]

https://codereview.chromium.org/405473004/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/405473004/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:122: base::Time oldest =
base::Time::Now() -
This is now calculating this value for every profile instead of just once at the
top.  May have to have another ifdef above.

https://codereview.chromium.org/405473004/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:125: if
(info_cache.GetProfileActiveTimeAtIndex(i) < oldest) {
I think the better choice would be to set the active time for these other OS.

https://codereview.chromium.org/405473004/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:129: {
This duplicate open brace will break editors' auto-brace-matching; they don't
recognize the ifdef.

[Mike Lerman, 2014-07-18 15:50:09]

Hi Brian and Monica,

I rewrote the code, PTAL.

I agree there would be benefits to having the inactivity of mobile profiles
logged. I left a TODO for that. Also given mobile platforms only have one
profile, I think this would correlate directly to overall install-base activity,
as measured by Omaha.

Thanks,
Mike

[bcwhite, 2014-07-18 16:38:44]

https://codereview.chromium.org/405473004/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/405473004/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:74: if
(info_cache.GetProfileActiveTimeAtIndex(index) < active_limit) {
Can the entire condition test be moved inside the #ifdef?

https://codereview.chromium.org/405473004/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:75: // TODO(mlerman): iOS and Android
should set an ActiveTime in the
Put the comment inside the appropriate #ifdef and indent to match code.

[Mike Lerman, 2014-07-18 17:44:52]

https://codereview.chromium.org/405473004/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/405473004/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:74: if
(info_cache.GetProfileActiveTimeAtIndex(index) < active_limit) {
On 2014/07/18 16:38:44, bcwhite wrote:
> Can the entire condition test be moved inside the #ifdef?

You can! Done.

https://codereview.chromium.org/405473004/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:75: // TODO(mlerman): iOS and Android
should set an ActiveTime in the
On 2014/07/18 16:38:44, bcwhite wrote:
> Put the comment inside the appropriate #ifdef and indent to match code.

Acknowledged.

[bcwhite, 2014-07-21 13:21:52]

lgtm

[noms (inactive), 2014-07-21 13:49:15]

Ah, it's much nicer now. lgtm

[Mike Lerman, 2014-07-21 16:43:06]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-07-21 16:44:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/405473004/40001

[commit-bot: I haz the power, 2014-07-21 19:48:17]

Change committed as 284486