Checks the age of device before initializing HaTS Notification

chrome/browser/chromeos/hats/hats_notification_controller.cc

 // Copyright (c) 2016 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/chromeos/hats/hats_notification_controller.h"
 
 #include "ash/common/system/system_notifier.h"
 #include "base/feature_list.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/hats/hats_dialog.h"
+#include "chrome/browser/chromeos/login/startup_utils.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/notifications/notification_ui_manager.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/network/network_state.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/browser_thread.h"
 #include "grit/ash_strings.h"
@@ skipping 12 matching lines @@
 bool DidShowSurveyToProfileRecently(Profile* profile,
                                     base::TimeDelta threshold) {
   int64_t serialized_timestamp =
       profile->GetPrefs()->GetInt64(prefs::kHatsLastInteractionTimestamp);
 
   base::Time previous_interaction_timestamp =
       base::Time::FromInternalValue(serialized_timestamp);
   return (previous_interaction_timestamp + threshold) > base::Time::Now();
 }
 
+// Returns true if atleast |threshold| time units have passed since OOBE. This
+// is an indirect measure of whether the owner has used the device for atleast
+// |threshold| amount of time.
+bool IsNewDevice(base::TimeDelta threshold) {
+  const base::TimeDelta time_since_oobe =
+      chromeos::StartupUtils::GetTimeSinceOobeFlagFileCreation();
+  return time_since_oobe <= threshold;
+}
+
 }  // namespace
 
 namespace chromeos {
 
 // static
 const char HatsNotificationController::kDelegateId[] = "hats_delegate";
 
 // static
 const char HatsNotificationController::kNotificationId[] = "hats_notification";
 
 // static
 const base::TimeDelta HatsNotificationController::kHatsThresholdTime =
     base::TimeDelta::FromDays(90);
 
+// static
+const base::TimeDelta HatsNotificationController::kHatsNewDeviceThresholdTime =
+    base::TimeDelta::FromDays(7);
+
 HatsNotificationController::HatsNotificationController(Profile* profile)
-    : profile_(profile) {
-  // Add self as an observer to be notified when an internet connection is
-  // available.
-  network_portal_detector::GetInstance()->AddAndFireObserver(this);
+    : profile_(profile), weak_pointer_factory_(this) {
+  base::PostTaskAndReplyWithResult(
+      content::BrowserThread::GetBlockingPool(), FROM_HERE,
+      base::Bind(&IsNewDevice, kHatsNewDeviceThresholdTime),
+      base::Bind(&HatsNotificationController::Initialize,
+                 weak_pointer_factory_.GetWeakPtr()));
 }
 
 HatsNotificationController::~HatsNotificationController() {
   network_portal_detector::GetInstance()->RemoveObserver(this);
 }
 
+void HatsNotificationController::Initialize(bool is_new_device) {
+  if (is_new_device) {

[jdufault, 2016/06/29 19:26:10]

It'd be nice if we could move this is_new_device logic to
ShouldShowSurveyToProfile so that Initialize only does one thing.

Instead of updating the last interaction time, the new device threshold time
could be set to kHatsThresholdTime.

The problem though is that the OOBE creation time read needs to happen on a
different thread. You could make ShouldShowSurveyToProfile an async function
that takes a callback that is invoked with the result. It looks like
UserSessionManager already has a WeakPtrFactory so it shouldn't be too much work
to change.

[malaykeshav, 2016/06/29 19:38:29]

We want to consider the case of a new device as if the user has attempted the
survey to reduce clustering of data point at the same point in time, that is 7
days after getting a device.(Part of requirements discussed offline with
+omrilio@ and +abodenha@)
'ShouldShowSurveyToProfile' manages the check for whether to display the
notification for a given _profile_. Also moving it adds more complexity and hats
related logic to UserSessionManager class. Not sure if thats desirable.

[jdufault, 2016/06/29 22:27:03]

After offline discussion keeping the existing structure lgtm.

+    // If the device is new, then we do not show any survey or notificaiton.

[jdufault, 2016/06/29 22:27:03]

Can you update the comment to be more specific, ie,

// This device has been chosen for a survey, but it is too new. Instead 
// of showing the user the survey, just mark it as completed.

[malaykeshav, 2016/06/29 22:32:34]

+1 
Done

+    // We also update the preference flag as if the user just attempted the
+    // survey.
+    UpdateLastInteractionTime();
+    return;
+  }
+  // Add self as an observer to be notified when an internet connection is

[jdufault, 2016/06/29 19:26:10]

nit: newline before comment

[malaykeshav, 2016/06/29 19:38:29]

Done

+  // available.
+  network_portal_detector::GetInstance()->AddAndFireObserver(this);
+}
+
 // static
 // TODO(malaykeshav): Add check for @google accounts.
 bool HatsNotificationController::ShouldShowSurveyToProfile(Profile* profile) {
   // Do not show the survey if the HaTS feature is disabled for the device.

[jdufault, 2016/06/29 22:27:03]

Please update this comment to explain that the feature flag is controlled by
finch and is what lets us run the experiments.

[malaykeshav, 2016/06/29 22:32:33]

Done.

   if (!base::FeatureList::IsEnabled(features::kHappininessTrackingSystem))
     return false;
 
   // Do not show survey if this is a guest session.
   if (profile->IsGuestSession())
     return false;
 
   // Do not show the survey if the current user is not an owner.
   if (!ProfileHelper::IsOwnerProfile(profile))
     return false;
@@ skipping 76 matching lines @@
       GURL() /* Send an empty invalid url */, kNotificationId, optional, this);
 }
 
 void HatsNotificationController::UpdateLastInteractionTime() {
   PrefService* pref_service = profile_->GetPrefs();
   pref_service->SetInt64(prefs::kHatsLastInteractionTimestamp,
                          base::Time::Now().ToInternalValue());
 }
 
 }  // namespace chromeos