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 6960e70cb060a786d1be7162973e6069516fbe5b..34c4ba9c20fa9ee30c1a7013738407d8d5d066c7 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;

[Mark Mentovai, 2014/10/02 16:07:23]

Why is this taking ksr as a parameter when registration_ is already available?

[Mike Lerman, 2014/10/06 16:49:08]

when it's called from markActive (when the timer triggers), don't I need to get
the ksr from the timer's userInfo?

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

Mike Lerman wrote:
-markActive: is called on the KeystoneGlue instance, so why wouldn’t
registration_ be valid?

I don’t think we need to supply registration_ to the timer as userInfo at all.
If you’re going to supply it, all you should do is DCHECK that it’s the same as
registration_ in the -markActive: timer callback. But what’s the point?

I see no reason that -setRegistrationActive: needs to accept a KSRegistration*
argument at all.

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

Ok, removed it, now referencing the object's registration_.

+
 // 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;

[Boris Vidolov, 2014/09/26 20:57:00]

This will break in the following case:
1. User A (first user) signs in. numProfiles_ becomes equal to 1 and Keystone
remembers to report numProfiles_=1 for 28 days.
2. The user signs out, goes to privacy settings and clears all history.
numProfiles_ becomes 0, so Keystone is never updated.
3. Keystone will continue to report incorrectly numProfiles_ = 1 for 28 more
days.
I'd suggest that you remove this check for 0 altogether and always report the
number.

[Mike Lerman, 2014/10/06 16:49:08]

In step 2 here, after signing out, numAccounts will become 0, but numProfiles
will continue to be 1. You always have 1 profile - having 0 would be an error
condition for the profiles code.

+  }
+
+  NSError* reportingAttributeError = nil;
+  KSUnsignedReportingAttribute* numProfilesAttribute =
+      [KSUnsignedReportingAttribute
+          reportingAttributeWithValue:numProfiles_
+                                 name:@"_NumAccounts"
+                      aggregationType:kKSReportingAggregationSum
+                                error:&reportingAttributeError];
+  if (reportingAttributeError != nil) {
+    LOG(ERROR) << [reportingAttributeError localizedDescription];
+    [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 {
@@ -1045,6 +1092,13 @@ NSString* const kVersionKey = @"KSVersion";
   return tagSuffix;
 }
 
+
+- (void)updateProfileCountsWithNumProfiles:(size_t)numProfiles
+                       numSignedInProfiles:(size_t)numSignedInProfiles {
+  numProfiles_ = numProfiles;
+  numSignedInProfiles_ = numSignedInProfiles;
+}
+
 @end  // @implementation KeystoneGlue
 
 namespace {