Add Desktop iOS promotion logging to chrome ios app.

ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm

Index: ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm
diff --git a/ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm b/ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm
new file mode 100644
index 0000000000000000000000000000000000000000..ecfe8365d1a9315d955850ba1a5c5262a6185504
--- /dev/null
+++ b/ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm
@@ -0,0 +1,85 @@
+// 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_macros.h"
+#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/desktop_promotion/histogram_macros.h"
+#include "ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory.h"
+
+namespace {
+
+const char* kDesktopIOSPromotionEntrypointHistogramPrefix[] = {
+    "SavePasswordsNewBubble", "BookmarksNewBubble", "BookmarksExistingBubble",
+    "HistoryPage"};
+}
+
+DesktopPromotionSyncObserver::DesktopPromotionSyncObserver(
+    ios::ChromeBrowserState* browser_state)
+    : browser_state_(browser_state) {
+  browser_sync::ProfileSyncService* sync_service =

[sdefresne, 2017/01/25 09:52:52]

Unless there is no other way around passing the service depended on in the
constructor is better than passing a browser_state and then fetching the service
via the factory. For one thing it make the code much easier to test as there is
no need to create a test chrome browser state, set up a fake factory to inject a
fake/mock implementation on the depended upon service.

Another reason is that it easier to reason about the code by looking at the
header. If the object take a browser state, it has access to a lot of thing. If
it only takes a profile sync service, then I know it access much less things.
You'll probably also have to pass the pref service (but for the same reason, it
is better to do this).

The last reason is that when/if turning this class into a KeyedService, the
dependencies need to be explicit in the factory, and passing the service in the
constructor is a good reminder to the reviewer to ask for the proper dependency
declaration in the factory.

So I would recommend changing this constructor to:

DesktopPromotionSyncObserver::DesktopPromotionSyncObserver(
    PrefService* pref_service,
    browser_sync::ProfileSyncService* sync_service)
    : pref_service_(pref_service), sync_service_(sync_service) {
  DCHECK(pref_service_);
  DCHECK(sync_service_);
  sync_service_->AddObserver(this);
}

[mrefaat1, 2017/01/26 16:51:19]

Acknowledged.

+      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_ &&

[sdefresne, 2017/01/25 09:58:22]

Unrelated to KeyedService, I think you should swap your conditions and return
early as it makes the code easier to read IMO:

void DesltopPromotionSyncObserver::OnStateChanged() {
  if (desktop_metrics_logger_initiated_ ||
      !sync_service_->GetActiveDataTypes().Has(syncer::PRIORITY_PREFERENCES)) {
    return;
  }

  desktop_metrics_logger_initiated_ = true;
  bool done_logging =
      pref_service_->GetBoolean(prefs::kDesktopIOSPromotionDone);
  double last_impression =
      pref_service_->GetDouble(prefs::kDesktopIOSPromotionLastImpression);
  base::TimeDelta delta =
      base::Time::Now() - base::Time::FromDoubleT(last_impression);
  if (done_logging || delta.InDays() >= 7)
    return;

  // This user have seen the promotion in the last 7 days so it may be a
  // reason of the installation.
  int sms_entrypoint =
      pref_service_->GetInteger(prefs::kDesktopIOSPromotionSMSEntryPoint);
  ...

[mrefaat1, 2017/01/26 16:51:19]

Acknowledged.

+      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].
+      int entrypoint_prefixes_count =
+          arraysize(kDesktopIOSPromotionEntrypointHistogramPrefix);
+      for (int i = 1; i < entrypoint_prefixes_count + 1; i++) {
+        if (sms_entrypoint == i) {
+          UMA_HISTOGRAM_ENUMERATION(
+              "DesktopIOSPromotion.SMSSent.IOSSigninReason", i,
+              entrypoint_prefixes_count + 1);
+          UMA_HISTOGRAM_COUNT_1000_NO_CACHE(
+              base::StringPrintf(
+                  "DesktopIOSPromotion.%s.CompletionTime",
+                  kDesktopIOSPromotionEntrypointHistogramPrefix[i - 1]),
+              delta.InHours());
+        } else {
+          // If the user saw this promotion type, log that it could be a reason
+          // for the signin.
+          if ((1 << i) & targeted_entrypoints)
+            UMA_HISTOGRAM_ENUMERATION(
+                "DesktopIOSPromotion.NoSMS.IOSSigninReason", i,
+                entrypoint_prefixes_count + 1);
+        }
+      }
+      prefs->SetBoolean(prefs::kDesktopIOSPromotionDone, true);
+    }
+  }
+}