Profile_Metrics integration with Keystone

chrome/browser/mac/keystone_glue.mm

Index: chrome/browser/mac/keystone_glue.mm
diff --git a/chrome/browser/mac/keystone_glue.mm b/chrome/browser/mac/keystone_glue.mm
index 2f0f19cddbd6e0d7a801f4b93d0d53f1808756a0..dfba11c7d4b27b5ead0276cb6a97cf31bf743f9c 100644
--- a/chrome/browser/mac/keystone_glue.mm
+++ b/chrome/browser/mac/keystone_glue.mm
@@ -24,6 +24,7 @@
 #include "base/threading/worker_pool.h"
 #include "build/build_config.h"
 #import "chrome/browser/mac/keystone_registration.h"
+#import "chrome/browser/mac/keystone_reporting_attribute.h"
 #include "chrome/browser/mac/obsolete_system.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_version_info.h"
@@ -119,6 +120,9 @@ class PerformBridge : public base::RefCountedThreadSafe<PerformBridge> {
 // Called when Keystone registration completes.
 - (void)registrationComplete:(NSNotification*)notification;
 
+// Set the registration active passing profile count parameters.
+- (void)setRegistrationActive:(KSRegistration*)ksr;
+
 // Called periodically to announce activity by pinging the Keystone server.
 - (void)markActive:(NSTimer*)timer;
 
@@ -491,6 +495,50 @@ NSString* const kVersionKey = @"KSVersion";
              nil];
 }
 
+- (void)setRegistrationActive:(KSRegistration*)ksr {
+  if (numProfiles_ == 0) {
+    [ksr setActive];
+    return;

[Mark Mentovai, 2014/10/06 18:35:13]

Every early return in this function is preceded by [ksr setActive]. Seems like
some light refactoring is in order. Like this:

- (BOOL)setRegistrationActiveWithReportingAttribute;

- (void)setRegistrationActive {
  if (![self setRegistrationActiveWithReportingAttribute]) {
    [registration_ setActive];
  }
}

[Boris Vidolov, 2014/10/06 20:25:52]

I'd rather put this as a requirement on the Keystone registration framework.
KSRegistration::SetActiveWithReportingAttributes should attempt to set as Active
first, even if reporting attributes fail. This would avoid all of the
registration framework users to write a complex code like this.

[Mike Lerman, 2014/10/09 20:41:01]

I understand this to mean that the SetActiveWithReportingAttributes (a) can
accept null's or invalid ReportingAttribute objects in the NSArray, and (b) can
be assumed to call [  setActive] if is it all possible, if either there are null
objects within the NSArray or if sending the ReportingAttribute fails. Is that
accurate, boris? if so, that's reflected now here.

[Boris Vidolov, 2014/10/10 22:31:40]

Well, it is. The idea is that you don't have to fall back to setActive
everywhere. Create a mutable dictionary, fill it in with all attributes you can
create, logging local errors whenever they show. At the end pass this dictionary
filled with whatever you have to setActiveWithReportingAttributes and if it
errors out, you log the error. This is it. No need for additional calls to
SetActive in case of error, no need to bail if one or more attributes cannot be
created - just log the error and continue with the rest of the attributes.

Also, don't forget my other comment about always sending these out, even if
numProfiles is 0. The issue is when they were not 0 before and just changed to
0. Keystone will continue reporting the non-zero amount as it does not see a
change in the attribute value.

+  }
+
+  NSError* reportingAttributeError = nil;
+  KSUnsignedReportingAttribute* numProfilesAttribute =
+      [KSUnsignedReportingAttribute
+          reportingAttributeWithValue:numProfiles_
+                                 name:@"_NumAccounts"
+                      aggregationType:kKSReportingAggregationSum
+                                error:&reportingAttributeError];
+  if (reportingAttributeError != nil) {
+    LOG(ERROR) << [reportingAttributeError localizedDescription];

[Mark Mentovai, 2014/10/06 18:35:13]

I don’t know that ERROR is the appropriate severity for these. They don’t really
affect the user or the running of the app. INFO, perhaps?

[Mike Lerman, 2014/10/09 20:41:01]

Changed to VLOG(INFO).
What do you think of also logging something via UMA?

[Mark Mentovai, 2014/10/09 21:26:20]

Mike Lerman wrote:
Doubt it’s necessary.

[Mike Lerman, 2014/10/10 15:32:29]

Acknowledged.

+    [ksr setActive];
+    return;
+  }
+
+  KSUnsignedReportingAttribute* numSignedInProfilesAttribute =
+      [KSUnsignedReportingAttribute
+          reportingAttributeWithValue:numSignedInProfiles_
+                                 name:@"_NumSignedIn"
+                      aggregationType:kKSReportingAggregationSum
+                                error:&reportingAttributeError];
+  if (reportingAttributeError != nil) {
+    LOG(ERROR) << [reportingAttributeError localizedDescription];
+    [ksr setActive];
+    return;
+  }
+
+  NSArray* profileCountsInformation =
+      [NSArray arrayWithObjects:numProfilesAttribute,
+                                numSignedInProfilesAttribute,
+                                nil];
+
+  NSError* setActiveError = nil;
+  if (![ksr setActiveWithReportingAttributes:profileCountsInformation
+                                       error:&setActiveError]) {
+    LOG(ERROR) << [setActiveError localizedDescription];
+    [ksr setActive];
+  }
+}
+
 - (void)registerWithKeystone {
   [self updateStatus:kAutoupdateRegistering version:nil];
 
@@ -512,7 +560,7 @@ NSString* const kVersionKey = @"KSVersion";
   // posted, and -registrationComplete: will be called.
 
   // Mark an active RIGHT NOW; don't wait an hour for the first one.
-  [registration_ setActive];
+  [self setRegistrationActive:registration_];
 
   // Set up hourly activity pings.
   timer_ = [NSTimer scheduledTimerWithTimeInterval:60 * 60  // One hour
@@ -541,8 +589,7 @@ NSString* const kVersionKey = @"KSVersion";
 }
 
 - (void)markActive:(NSTimer*)timer {
-  KSRegistration* ksr = [timer userInfo];
-  [ksr setActive];
+  [self setRegistrationActive:[timer userInfo]];
 }
 
 - (void)checkForUpdate {
@@ -1049,6 +1096,13 @@ NSString* const kVersionKey = @"KSVersion";
   return tagSuffix;
 }
 
+
+- (void)updateProfileCountsWithNumProfiles:(size_t)numProfiles
+                       numSignedInProfiles:(size_t)numSignedInProfiles {
+  numProfiles_ = numProfiles;
+  numSignedInProfiles_ = numSignedInProfiles;
+}
+
 @end  // @implementation KeystoneGlue
 
 namespace {