Add Desktop iOS promotion logging to chrome ios app.

ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.h"
+
+#include <memory>
+
+#include "base/metrics/histogram.h"

[Mark P, 2017/01/19 23:35:48]

nit: use histogram_macros.h

[mrefaat, 2017/01/20 19:14:59]

Done.

+#include "base/strings/stringprintf.h"
+#include "base/time/time.h"
+#include "components/browser_sync/profile_sync_service.h"
+#include "components/prefs/pref_service.h"
+#include "ios/chrome/browser/browser_state/chrome_browser_state.h"
+#include "ios/chrome/browser/desktop_promotion/desktop_promotion_prefs.h"
+#include "ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory.h"
+
+namespace {
+
+// Needed by UMA_HISTOGRAM_ENUM as the max value that shouldn't be exceeded.
+const int kMaxEntryPoints = 5;

[Mark P, 2017/01/19 23:35:48]

Can't you do arraysize(kDesktopIOSPromotionEntrypointHistogramPrefix) + 1?

[mrefaat, 2017/01/20 19:14:59]

Acknowledged.

+const char* kDesktopIOSPromotionEntrypointHistogramPrefix[] = {
+    "SavePasswordsNewBubble", "BookmarksNewBubble", "BookmarksExistingBubble",
+    "HistoryPage"};
+const char kSMSSentIOSSigninReasonHistogram[] =

[Mark P, 2017/01/19 23:35:48]

nit: only used in one place; don't need a special variable for it.
ditto below

[mrefaat, 2017/01/20 19:14:59]

Acknowledged.

+    "DesktopIOSPromotion.SMSSent.IOSSigninReason";

[Mark P, 2017/01/19 23:35:48]

These two histograms aren't listed in histograms.xml.
(DesktopIOSPromotion.IOSSigninReason is.)

[mrefaat, 2017/01/20 19:14:59]

they are defined by a suffix in histograms.xml

[Mark P, 2017/01/20 19:40:05]

Ah, I forgot about histogram_suffixes supporting putting stuff in the middle. 
Thanks.

[mrefaat, 2017/01/24 19:57:19]

Acknowledged.

+const char kNoSMSIOSSigninReasonHistogram[] =
+    "DesktopIOSPromotion.NoSMS.IOSSigninReason";
+}
+
+DesktopPromotionSyncObserver::DesktopPromotionSyncObserver(
+    ios::ChromeBrowserState* browser_state)
+    : browser_state_(browser_state) {
+  browser_sync::ProfileSyncService* sync_service =
+      IOSChromeProfileSyncServiceFactory::GetForBrowserState(browser_state_);
+  sync_service->AddObserver(this);
+}
+
+DesktopPromotionSyncObserver::~DesktopPromotionSyncObserver() {
+  browser_sync::ProfileSyncService* sync_service =
+      IOSChromeProfileSyncServiceFactory::GetForBrowserState(browser_state_);
+  sync_service->RemoveObserver(this);
+}
+
+void DesktopPromotionSyncObserver::OnStateChanged() {
+  browser_sync::ProfileSyncService* sync_service =
+      IOSChromeProfileSyncServiceFactory::GetForBrowserState(browser_state_);
+  if (!desktop_metrics_logger_initiated_ &&
+      sync_service->GetActiveDataTypes().Has(syncer::PRIORITY_PREFERENCES)) {
+    desktop_metrics_logger_initiated_ = true;
+    PrefService* prefs = browser_state_->GetPrefs();
+    bool done_logging = prefs->GetBoolean(prefs::kDesktopIOSPromotionDone);
+    double last_impression =
+        prefs->GetDouble(prefs::kDesktopIOSPromotionLastImpression);
+    base::TimeDelta delta =
+        base::Time::Now() - base::Time::FromDoubleT(last_impression);
+    if (!done_logging && delta.InDays() < 7) {
+      // This user have seen the promotion in the last 7 days so it may be a
+      // reason of the installation.
+      int sms_entrypoint =
+          prefs->GetInteger(prefs::kDesktopIOSPromotionSMSEntryPoint);
+      int targeted_entrypoints =
+          prefs->GetInteger(prefs::kDesktopIOSPromotionTargetedEntryPoints);
+      // Entry points are represented on the preference by integers [1..4].
+      for (int i = 1; i < kMaxEntryPoints; i++) {
+        if (sms_entrypoint == i) {
+          UMA_HISTOGRAM_ENUMERATION(kSMSSentIOSSigninReasonHistogram, i,
+                                    kMaxEntryPoints);
+          UMA_HISTOGRAM_COUNTS(
+              base::StringPrintf(

[Mark P, 2017/01/19 23:35:48]

You cannot use a UMA_HISTOGRAM macro with a not-statically-defined string. 
Please see the comments histogram_macros.h.  There is an alternate construction
that should work for you.

[mrefaat, 2017/01/20 19:14:59]

From histogram_macros.h: // All of these macros must be called with |name| as a
runtime constant.
If i understand this sentence right i believe its okay to not use a statically
defined string.

[Mark P, 2017/01/20 19:40:05]

You cannot use a StringPrintf here.  From histogram_macros.h:
"it must be the same string on all calls from a particular call site."  At this
call site, you are having multiple different strings at the same call site.

You need to use FactoryGet, not a macro here.

[mrefaat, 2017/01/24 19:57:19]

Thanks for the advice, will update that

+                  "DesktopIOSPromotion.%s.CompletionTime",
+                  kDesktopIOSPromotionEntrypointHistogramPrefix[i - 1]),
+              delta.InHours());
+        } else {
+          if ((1 << i) & targeted_entrypoints)
+            UMA_HISTOGRAM_ENUMERATION(kNoSMSIOSSigninReasonHistogram, i,

[Mark P, 2017/01/19 23:35:48]

I know this histogram isn't yet documented.  Yet, I find the fact that it's
going to be emitted to in a loop surprising.  Listing "no sign in" to multiple
entry points sounds like it'll be hard to explain.  I look forward to reading
the histograms.xml description for this and seeing what it's intended to
measure. :-)

[mrefaat, 2017/01/20 19:14:59]

Acknowledged.

+                                      kMaxEntryPoints);
+        }
+      }
+      prefs->SetBoolean(prefs::kDesktopIOSPromotionDone, true);
+    }
+  }
+}