Cross Device Promo - Main Eligibility Flow

chrome/browser/signin/cross_device_promo.cc

Index: chrome/browser/signin/cross_device_promo.cc
diff --git a/chrome/browser/signin/cross_device_promo.cc b/chrome/browser/signin/cross_device_promo.cc
new file mode 100644
index 0000000000000000000000000000000000000000..fff0d943bb607bbbbed0e074c995d7814012f2ee
--- /dev/null
+++ b/chrome/browser/signin/cross_device_promo.cc
@@ -0,0 +1,427 @@
+// Copyright 2015 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.
+
+#include "chrome/browser/signin/cross_device_promo.h"
+
+#include "base/metrics/histogram_macros.h"
+#include "base/prefs/pref_service.h"
+#include "base/rand_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/time/time.h"
+#include "chrome/common/pref_names.h"
+#include "components/signin/core/browser/signin_client.h"
+#include "components/signin/core/browser/signin_manager.h"
+#include "components/signin/core/browser/signin_metrics.h"
+#include "components/variations/variations_associated_data.h"
+#include "net/cookies/canonical_cookie.h"
+
+namespace {
+
+const int kDelayUntilGettingDeviceActivityInMS = 3000;
+const int kDefaultBrowsingSessionDurationInMinutes = 15;
+
+// Helper method to set a parameter based on a particular variable from the
+// configuration. Returns false if the parameter was not found or the
+// converstion could not succeed.

[Roger Tawa OOO till Jul 10th, 2015/05/15 16:01:39]

typo: converstion

[Mike Lerman, 2015/05/15 20:47:34]

Done.

+bool SetParameterFromVariation(
+    std::string variation_parameter,
+    base::TimeDelta& local_parameter,

[Roger Tawa OOO till Jul 10th, 2015/05/15 16:01:40]

const & for line 28, pointer for out arg on line 29.

[Mike Lerman, 2015/05/15 20:47:34]

Done.

+    base::Callback<base::TimeDelta(int)> conversion) {
+  std::string parameter_as_string = variations::GetVariationParamValue(
+      "CrossDevicePromo", variation_parameter);
+  if (parameter_as_string.empty())
+    return false;
+
+  int parameter_as_int;
+  if (!base::StringToInt(parameter_as_string, &parameter_as_int))
+    return false;
+
+  local_parameter = conversion.Run(parameter_as_int);
+  return true;
+}
+
+}  // namespace
+
+CrossDevicePromo::CrossDevicePromo(
+    SigninManager* signin_manager,
+    GaiaCookieManagerService* cookie_manager_service,
+    SigninClient* signin_client,
+    PrefService* pref_service)
+  : signin_manager_(signin_manager),
+    cookie_manager_service_(cookie_manager_service),
+    prefs_(pref_service),
+    signin_client_(signin_client),
+    start_last_browsing_session_(base::Time()),
+    initialized_(false),
+    track_metrics_(true) {
+  VLOG(1) << "CrossDevicePromo::CrossDevicePromo.";

[Roger Tawa OOO till Jul 10th, 2015/05/15 16:01:40]

nit: DCHECK pointers are good?

[Mike Lerman, 2015/05/15 20:47:33]

Done.

+  Init();
+}
+
+CrossDevicePromo::~CrossDevicePromo() {}
+
+void CrossDevicePromo::Shutdown() {
+  VLOG(1) << "CrossDevicePromo::Shutdown.";
+  UnregisterForCookieChanges();
+  if (start_last_browsing_session_ != base::Time())
+    signin_metrics::LogBrowsingSessionDuration(start_last_browsing_session_);
+}
+
+
+void CrossDevicePromo::AddObserver(CrossDevicePromo::Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void CrossDevicePromo::RemoveObserver(CrossDevicePromo::Observer* observer) {
+  observer_list_.RemoveObserver(observer);
+}
+
+bool CrossDevicePromo::IsPromoActive() {
+  return prefs_->GetBoolean(prefs::kCrossDevicePromoActive);
+}
+
+void CrossDevicePromo::Init() {
+  DCHECK(!initialized_);
+  // We need a default value for this as it is referenced early in
+  // UpdateLastActiveTime and we want to gather as many stats about Browsing
+  // Sessions as possible.
+  inactivity_between_browsing_sessions_ = base::TimeDelta::FromMinutes(
+      kDefaultBrowsingSessionDurationInMinutes);
+
+  if (prefs_->GetBoolean(prefs::kCrossDevicePromoOptedOut)) {
+    UMA_HISTOGRAM_BOOLEAN("Signin.XDevicePromo.Initialized", false);
+    return;
+  }
+
+  if (!SetParameterFromVariation("HoursBetweenSyncDeviceChecks",
+                                 delay_until_next_list_devices_,
+                                 base::Bind(&base::TimeDelta::FromHours)) ||
+      !SetParameterFromVariation("DaysToVerifySingleUserProfile",
+                                 single_account_duration_threshold_,
+                                 base::Bind(&base::TimeDelta::FromDays)) ||
+      !SetParameterFromVariation("MinutesBetweenBrowsingSessions",
+                                 inactivity_between_browsing_sessions_,
+                                 base::Bind(&base::TimeDelta::FromMinutes)) ||
+      !SetParameterFromVariation("MinutesMaxContextSwitchDuration",
+                                 context_switch_duration_,
+                                 base::Bind(&base::TimeDelta::FromMinutes))) {
+    UMA_HISTOGRAM_BOOLEAN("Signin.XDevicePromo.Initialized", false);

[Roger Tawa OOO till Jul 10th, 2015/05/15 16:01:40]

Is it worth making this histogram an enum to differentiate between line 109 and
line 93 and line 116?  How will we use this metric?  Should the UMA call be put
into a static helper in this file?

[Mike Lerman, 2015/05/15 20:47:34]

I'll split it into three: OPTED_OUT, NO_VARIATIONS_CONFIG, INITIALIZED.

This will be helpful in helping us figure out the state of people out in the
wild: We'll want to slice the population into what %age opted out, %age that's
signed in, %age that's multi-account... and %age that didn't initialize since
they couldn't get a finch config.

+    return;
+  }
+
+  std::string throttle = variations::GetVariationParamValue(
+    "CrossDevicePromo", "RPCThrottle");
+  if (throttle.empty() || !base::StringToUint64(throttle, &rpc_throttle_)) {
+    UMA_HISTOGRAM_BOOLEAN("Signin.XDevicePromo.Initialized", false);
+    return;
+  }
+
+  VLOG(1) << "CrossDevicePromo::Init. Service initialized. Parameters: "
+          << "Hour between RPC checks: "
+          << delay_until_next_list_devices_.InHours()
+          << "Days to verify an account in the cookie: "
+          << single_account_duration_threshold_.InDays()
+          << "Minutes between browsing sessions: "
+          << inactivity_between_browsing_sessions_.InMinutes()
+          << "Window (in minutes) for a context switch: "
+          << context_switch_duration_.InMinutes()
+          << "Throttle rate for RPC calls: "
+          << rpc_throttle_;
+  RegisterForCookieChanges();
+  initialized_ = true;
+  UMA_HISTOGRAM_BOOLEAN("Signin.XDevicePromo.Initialized", true);
+  return;
+}
+
+void CrossDevicePromo::OptOut() {
+  VLOG(1) << "CrossDevicePromo::OptOut.";
+  UnregisterForCookieChanges();
+  prefs_->SetBoolean(prefs::kCrossDevicePromoOptedOut, true);
+  MarkPromoInactive();

[Roger Tawa OOO till Jul 10th, 2015/05/15 16:01:40]

Stop any fetchers too?

[Mike Lerman, 2015/05/15 20:47:34]

Yes. Added a Stop method to the DeviceActivityFetcher.

+}
+
+bool CrossDevicePromo::VerifyPromoEligibleReadOnly() {
+  if (!initialized_)
+    return false;
+
+  if (prefs_->GetBoolean(prefs::kCrossDevicePromoOptedOut))
+    return false;
+
+  if (!prefs_->HasPrefPath(prefs::kCrossDevicePromoObservedSingleAccountCookie))
+    return false;
+
+  if (base::Time::FromInternalValue(prefs_->GetInt64(
+          prefs::kCrossDevicePromoObservedSingleAccountCookie)) +
+          single_account_duration_threshold_ >
+              base::Time::Now()) {
+    return false;
+  }
+
+  return true;
+}
+
+bool CrossDevicePromo::CheckPromoEligibility() {
+  if (!initialized_) {
+    // In tests the variations may not be present when Init() was first called.
+    Init();
+    if (!initialized_)
+      return false;
+  }
+
+  if (prefs_->GetBoolean(prefs::kCrossDevicePromoOptedOut)) {
+    signin_metrics::LogXDevicePromoEligible(signin_metrics::OPTED_OUT);
+    return false;
+  }
+
+  if (!signin_manager_->GetAuthenticatedUsername().empty()) {

[Roger Tawa OOO till Jul 10th, 2015/05/15 16:01:40]

Use IsAuthenticated() instead of !GetAuthenticatedUsername().empty()

[Mike Lerman, 2015/05/15 20:47:34]

Done.

+    signin_metrics::LogXDevicePromoEligible(signin_metrics::SIGNED_IN);
+    return false;
+  }
+
+  base::Time cookie_has_one_account_since = base::Time::FromInternalValue(
+      prefs_->GetInt64(
+          prefs::kCrossDevicePromoObservedSingleAccountCookie));
+  if (!prefs_->HasPrefPath(prefs::kCrossDevicePromoObservedSingleAccountCookie)
+      || cookie_has_one_account_since + single_account_duration_threshold_ >
+          base::Time::Now()) {
+    signin_metrics::LogXDevicePromoEligible(
+        signin_metrics::NOT_SINGLE_GAIA_ACCOUNT);
+    return false;
+  }
+
+  // This is the first time the promo's being run; determine when to call the
+  // DeviceActivityFetcher.
+  if (!prefs_->HasPrefPath(prefs::kCrossDevicePromoNextFetchListDevicesTime)) {
+    int minutes_until_next_list_devices = base::RandGenerator(
+        delay_until_next_list_devices_.InMinutes());
+    prefs_->SetInt64(prefs::kCrossDevicePromoNextFetchListDevicesTime,
+        (base::Time::Now() +
+        base::TimeDelta::FromMinutes(minutes_until_next_list_devices)).
+            ToInternalValue());
+    signin_metrics::LogXDevicePromoEligible(
+        signin_metrics::UNKNOWN_COUNT_DEVICES);
+    return false;
+  }
+
+  // We have no knowledge of other device activity yet.
+  if (!prefs_->HasPrefPath(prefs::kCrossDevicePromoNumDevices)) {
+    base::Time time_next_list_devices = base::Time::FromInternalValue(
+        prefs_->GetInt64(prefs::kCrossDevicePromoNextFetchListDevicesTime));
+    // Not time yet to poll the list of devices.
+    if (time_next_list_devices > base::Time::Now()) {
+      signin_metrics::LogXDevicePromoEligible(
+          signin_metrics::UNKNOWN_COUNT_DEVICES);
+      return false;
+    } else {
+      // We're not eligible... but might be! Track metrics in the results.
+      GetDevicesActivityForAccountInCookie();
+      return false;
+    }
+  }
+
+  int num_devices = prefs_->GetInteger(prefs::kCrossDevicePromoNumDevices);
+  base::Time time_next_list_devices = base::Time::FromInternalValue(
+      prefs_->GetInt64(prefs::kCrossDevicePromoNextFetchListDevicesTime));
+  if (time_next_list_devices < base::Time::Now()) {
+    GetDevicesActivityForAccountInCookie();
+  } else if (num_devices == 0) {
+    signin_metrics::LogXDevicePromoEligible(signin_metrics::ZERO_DEVICES);
+    return false;
+  }
+
+  DCHECK(VerifyPromoEligibleReadOnly());
+  return true;
+}
+
+void CrossDevicePromo::UpdateLastActiveTime(

[Roger Tawa OOO till Jul 10th, 2015/05/15 16:01:40]

nit: should this be called something like UpdateStartLastBrowsingSessionTime(),
or maybe MarkPromoActiveIf()?

[Mike Lerman, 2015/05/15 20:47:34]

I opted for: MaybeBrowsingSessionStarted

+    const base::Time& previous_last_active) {
+  // In tests, or the first call for a profile, don't pass go.
+  if (previous_last_active == base::Time())
+    return;
+
+  // Determine how often this method is called. Need an estimate for QPS.
+  UMA_HISTOGRAM_CUSTOM_COUNTS(
+      "Signin.XDevicePromo.BrowsingSessionDurationComputed",
+      (base::Time::Now() - previous_last_active).InMinutes(),
+      1, base::TimeDelta::FromDays(30).InMinutes(), 50);
+
+  // Check if this is a different browsing session since the last call.
+  if (base::Time::Now() - previous_last_active <=
+      inactivity_between_browsing_sessions_) {
+    VLOG(1) << "CrossDevicePromo::UpdateLastActiveTime. Same browsing session "
+               "as the last call.";
+    return;
+  }
+
+  if (start_last_browsing_session_ != base::Time())
+    signin_metrics::LogBrowsingSessionDuration(previous_last_active);
+
+  start_last_browsing_session_ = base::Time::Now();
+
+  if (!CheckPromoEligibility()) {
+    VLOG(1) << "CrossDevicePromo::UpdateLastActiveTime; Ineligible for promo.";
+    if (IsPromoActive())
+      MarkPromoInactive();
+    return;
+  }
+
+  // Check if there is a record of recent browsing activity on another device.
+  // If there is none, we set a timer to update the records after a small delay
+  // to ensure server-side data is synchronized.
+  base::Time device_last_active = base::Time::FromInternalValue(
+      prefs_->GetInt64(prefs::kCrossDevicePromoLastDeviceActiveTime));
+  if (base::Time::Now() - device_last_active < context_switch_duration_) {
+    VLOG(1) << "CrossDevicePromo::UpdateLastActiveTime; promo active.";
+    signin_metrics::LogXDevicePromoEligible(signin_metrics::ELIGIBLE);
+    MarkPromoActive();
+    return;
+  }
+
+  // Check for recency of device activity unless a check is already being
+  // executed because the number of devices is being updated.
+  if (!device_activity_fetcher_.get()) {
+    VLOG(1) << "CrossDevicePromo::UpdateLastActiveTime; Check device activity.";
+    device_activity_timer_.Start(
+        FROM_HERE,
+        base::TimeDelta::FromMilliseconds(kDelayUntilGettingDeviceActivityInMS),
+        this,
+        &CrossDevicePromo::GetDevicesActivityForAccountInCookie);
+  }
+}
+
+void CrossDevicePromo::GetDevicesActivityForAccountInCookie() {
+  // Don't start a fetch while one is processing.
+  if (device_activity_fetcher_.get())
+    return;
+
+  if (rpc_throttle_ && base::RandGenerator(100) < rpc_throttle_) {

[Roger Tawa OOO till Jul 10th, 2015/05/15 16:01:40]

Should be calculated each time or only once per chrome run?

[Mike Lerman, 2015/05/15 20:47:34]

Once per Chrome/per client makes sense, that way we correctly can test for
browser sessions and device activity within a Chrome session. This is done up in
Init().

+    if (track_metrics_) {
+      signin_metrics::LogXDevicePromoEligible(
+          signin_metrics::THROTTLED_FETCHING_DEVICE_ACTIVITY);
+    }
+    return;
+  }
+
+  VLOG(1) << "CrossDevicePromo::GetDevicesActivityForAccountInCookie. Start.";
+  DCHECK(VerifyPromoEligibleReadOnly());
+  device_activity_fetcher_.reset(
+      new DeviceActivityFetcher(signin_client_, this));
+  device_activity_fetcher_->Start();
+}
+
+void CrossDevicePromo::OnFetchDeviceActivitySuccess(
+    const std::vector<DeviceActivityFetcher::DeviceActivity>& devices) {
+  VLOG(1) << "CrossDevicePromo::OnFetchDeviceActivitySuccess. "
+          << devices.size() << " devices.";
+  prefs_->SetInt64(prefs::kCrossDevicePromoNextFetchListDevicesTime,
+      (base::Time::Now() +
+      delay_until_next_list_devices_).ToInternalValue());
+  prefs_->SetInteger(prefs::kCrossDevicePromoNumDevices, devices.size());
+
+  if (devices.empty()) {
+    if (track_metrics_)
+      signin_metrics::LogXDevicePromoEligible(signin_metrics::ZERO_DEVICES);
+    return;
+  }
+
+  base::Time most_recent_last_active = base::Time();
+  for (size_t i = 0; i < devices.size(); i++) {
+    if (devices[i].last_active > most_recent_last_active)
+      most_recent_last_active = devices[i].last_active;
+  }
+
+  prefs_->SetInt64(prefs::kCrossDevicePromoLastDeviceActiveTime,
+                  most_recent_last_active.ToInternalValue());

[Roger Tawa OOO till Jul 10th, 2015/05/15 16:01:40]

nit: extra space.

[Mike Lerman, 2015/05/15 20:47:34]

Done.

+
+  if (base::Time::Now() - most_recent_last_active < context_switch_duration_) {
+    // Make sure eligibility wasn't lost while executing the remote call.
+    if (!VerifyPromoEligibleReadOnly())
+      return;
+
+    // The context switch will only be valid for so long. Schedule another
+    // DeviceActivity check for when our switch would expire to check for more
+    // recent activity.
+    if (!device_activity_timer_.IsRunning()) {
+      base::TimeDelta time_to_next_check = most_recent_last_active +
+          context_switch_duration_ - base::Time::Now();
+      device_activity_timer_.Start(
+          FROM_HERE,
+          time_to_next_check,
+          this,
+          &CrossDevicePromo::GetDevicesActivityForAccountInCookie);
+    }
+
+    if (track_metrics_)
+      signin_metrics::LogXDevicePromoEligible(signin_metrics::ELIGIBLE);

[Roger Tawa OOO till Jul 10th, 2015/05/15 16:01:40]

It seems that not all calls to uma logs are protected with if (track_metrics_). 
What's the reason for that?

[Mike Lerman, 2015/05/15 20:47:34]

Ah, I don't need track_metrics_ anymore! It's never set to false.

This was needed because there were flows in CheckPromoEligibility() where I log
an UMA stat and, in the same flow, fire a fetch for DeviceActivity. After a
recent refactoring, there's no scenario where that happens.

+    MarkPromoActive();
+  } else {
+    if (track_metrics_) {
+      signin_metrics::LogXDevicePromoEligible(
+          signin_metrics::NO_ACTIVE_DEVICES);
+    }
+    MarkPromoInactive();
+  }
+  track_metrics_ = true;
+  device_activity_fetcher_.reset();
+}
+
+void CrossDevicePromo::OnFetchDeviceActivityFailure() {
+  VLOG(1) << "CrossDevicePromo::OnFetchDeviceActivityFailure.";
+  if (track_metrics_) {
+    signin_metrics::LogXDevicePromoEligible(
+        signin_metrics::ERROR_FETCHING_DEVICE_ACTIVITY);
+  }
+  device_activity_fetcher_.reset();
+}
+
+void CrossDevicePromo::RegisterForCookieChanges() {
+  cookie_manager_service_->AddObserver(this);
+}
+
+void CrossDevicePromo::UnregisterForCookieChanges() {
+  cookie_manager_service_->RemoveObserver(this);
+}
+
+void CrossDevicePromo::OnGaiaAccountsInCookieUpdated(
+      const std::vector<std::pair<std::string, bool> >& accounts,
+      const GoogleServiceAuthError& error) {

[Roger Tawa OOO till Jul 10th, 2015/05/15 16:01:40]

Too many spaces?

[Mike Lerman, 2015/05/15 20:47:34]

Done.

+  VLOG(1) << "CrossDevicePromo::OnGaiaAccountsInCookieUpdated. "
+          << accounts.size() << " accounts with auth error " << error.state();
+  if (error.state() != GoogleServiceAuthError::State::NONE)
+    return;
+
+  // Multiple accounts seen - clear the pref.
+  if (accounts.size() != 1 && prefs_->HasPrefPath(
+      prefs::kCrossDevicePromoObservedSingleAccountCookie)) {
+    prefs_->ClearPref(prefs::kCrossDevicePromoObservedSingleAccountCookie);
+    MarkPromoInactive();
+  // Single account seen. Note if this is the first time we've seen this.
+  } else if (accounts.size() == 1 && !prefs_->HasPrefPath(
+      prefs::kCrossDevicePromoObservedSingleAccountCookie)){

[Roger Tawa OOO till Jul 10th, 2015/05/15 16:01:40]

nit: space before {

[Mike Lerman, 2015/05/15 20:47:34]

Done.

+    prefs_->SetInt64(prefs::kCrossDevicePromoObservedSingleAccountCookie,
+        base::Time::Now().ToInternalValue());
+  }
+}
+
+void CrossDevicePromo::MarkPromoActive() {
+  VLOG(1) << "CrossDevicePromo::MarkPromoActive.";
+  DCHECK(!prefs_->GetBoolean(prefs::kCrossDevicePromoOptedOut));
+
+  if (!prefs_->GetBoolean(prefs::kCrossDevicePromoActive)) {
+    prefs_->SetBoolean(prefs::kCrossDevicePromoActive, true);
+    FOR_EACH_OBSERVER(CrossDevicePromo::Observer,
+                      observer_list_,
+                      OnPromoActivationChanged(true));
+  }
+}
+
+void CrossDevicePromo::MarkPromoInactive() {
+  VLOG(1) << "CrossDevicePromo::MarkPromoInactive.";
+  if (prefs_->GetBoolean(prefs::kCrossDevicePromoActive)) {
+    prefs_->SetBoolean(prefs::kCrossDevicePromoActive, false);
+    FOR_EACH_OBSERVER(CrossDevicePromo::Observer,
+                      observer_list_,
+                      OnPromoActivationChanged(false));
+  }
+}