Clean up HaTS code.

- Better explain ownership semantics.
- DCHECK the code runs on the UI thread.
- Use base::TimeDelta in more places.
- Pass non-POD objects by const-reference.
- Add an OWNERS file.
- Fix nits.

Change-Id: Iaa41d368882c08a56977ee418416c31e2bdc46dc
Reviewed-on: https://chromium-review.googlesource.com/c/1496538Reviewed-by: Malay Keshav <malaykeshav@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636900}

chrome/browser/chromeos/hats/OWNERS

+malaykeshav@chromium.org

chrome/browser/chromeos/hats/hats_dialog.cc

@@ -18,6 +18,7 @@
 #include "components/language/core/common/locale_util.h"
 #include "components/prefs/pref_service.h"
 #include "components/version_info/version_info.h"
+#include "content/public/browser/browser_thread.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/geometry/size.h"
@@ -55,8 +56,8 @@ enum class DeviceInfoKey : unsigned int {
 
 // Returns the local HaTS HTML file as a string with the correct Hats script
 // URL.
-std::string LoadLocalHtmlAsString(std::string site_id,
-                                  std::string site_context) {
+std::string LoadLocalHtmlAsString(const std::string& site_id,
+                                  const std::string& site_context) {
   std::string html_data;
   ui::ResourceBundle::GetSharedInstance()
       .GetRawDataResource(IDR_HATS_HTML)
@@ -98,7 +99,7 @@ const std::string KeyEnumToString(DeviceInfoKey key) {
 // 'STOP' is used as a token to identify the end of a key value pair. This is
 // done since GCS only allows the use of alphanumeric characters to be passed as
 // a site context.
-std::string GetFormattedSiteContext(std::string user_locale,
+std::string GetFormattedSiteContext(const std::string& user_locale,
                                     base::StringPiece join_keyword) {
   std::vector<std::string> pairs;
   pairs.push_back(KeyEnumToString(DeviceInfoKey::BROWSER) +
@@ -119,6 +120,8 @@ std::string GetFormattedSiteContext(std::string user_locale,
 
 // static
 void HatsDialog::CreateAndShow(bool is_google_account) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   Profile* profile = ProfileManager::GetActiveUserProfile();
   std::string user_locale =
       profile->GetPrefs()->GetString(language::prefs::kApplicationLocale);
@@ -126,30 +129,36 @@ void HatsDialog::CreateAndShow(bool is_google_account) {
   if (!user_locale.length())
     user_locale = kDefaultProfileLocale;
 
-  std::unique_ptr<HatsDialog> hats_dialog(new HatsDialog);
-
   base::PostTaskWithTraitsAndReplyWithResult(
       FROM_HERE, {base::MayBlock(), base::TaskPriority::BEST_EFFORT},
-      base::Bind(&GetFormattedSiteContext, user_locale, kDeviceInfoStopKeyword),
-      base::Bind(&HatsDialog::Show, base::Passed(&hats_dialog),
-                 is_google_account ? kGooglerSiteID : kSiteID));
+      base::BindOnce(&GetFormattedSiteContext, user_locale,
+                     kDeviceInfoStopKeyword),
+      base::BindOnce(&HatsDialog::Show, is_google_account));
 }
 
 // static
-void HatsDialog::Show(std::unique_ptr<HatsDialog> hats_dialog,
-                      std::string site_id,
-                      std::string site_context) {
+void HatsDialog::Show(bool is_google_account, const std::string& site_context) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   // Load and set the html data that needs to be displayed in the dialog.
-  hats_dialog->html_data_ = LoadLocalHtmlAsString(site_id, site_context);
+  std::string site_id = is_google_account ? kGooglerSiteID : kSiteID;
+  std::string html_data = LoadLocalHtmlAsString(site_id, site_context);
+
+  // Self deleting.
+  auto* hats_dialog = new HatsDialog(html_data);
 
   chrome::ShowWebDialog(
       nullptr, ProfileManager::GetActiveUserProfile()->GetOffTheRecordProfile(),
-      hats_dialog.release());
+      hats_dialog);
 }
 
-HatsDialog::HatsDialog() {}
+HatsDialog::HatsDialog(const std::string& html_data) : html_data_(html_data) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+}
 
-HatsDialog::~HatsDialog() {}
+HatsDialog::~HatsDialog() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+}
 
 ui::ModalType HatsDialog::GetDialogModalType() const {
   return ui::MODAL_TYPE_SYSTEM;

chrome/browser/chromeos/hats/hats_dialog.h

@@ -14,19 +14,19 @@ namespace chromeos {
 
 // Happiness tracking survey dialog. Sometimes appears after login to ask the
 // user how satisfied they are with their Chromebook.
+// This class lives on the UI thread and is self deleting.
 class HatsDialog : public ui::WebDialogDelegate {
  public:
-  HatsDialog();
-  ~HatsDialog() override;
-
   // Creates an instance of HatsDialog and posts a task to load all the relevant
   // device info before displaying the dialog.
   static void CreateAndShow(bool is_google_account);
 
  private:
-  static void Show(std::unique_ptr<HatsDialog> hats_dialog,
-                   std::string site_id,
-                   std::string site_context);
+  static void Show(bool is_google_account, const std::string& site_context);
+
+  // Use CreateAndShow() above.
+  explicit HatsDialog(const std::string& html_data);
+  ~HatsDialog() override;
 
   // ui::WebDialogDelegate implementation.
   ui::ModalType GetDialogModalType() const override;
@@ -45,7 +45,7 @@ class HatsDialog : public ui::WebDialogDelegate {
   bool HandleContextMenu(content::RenderFrameHost* render_frame_host,
                          const content::ContextMenuParams& params) override;
 
-  std::string html_data_;
+  const std::string html_data_;
 
   DISALLOW_COPY_AND_ASSIGN(HatsDialog);
 };

chrome/browser/chromeos/hats/hats_notification_controller.cc

@@ -24,6 +24,7 @@
 #include "chromeos/constants/chromeos_switches.h"
 #include "chromeos/network/network_state.h"
 #include "components/prefs/pref_service.h"
+#include "content/public/browser/browser_thread.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -39,34 +40,35 @@ const char kNotifierHats[] = "ash.hats";
 
 // Minimum amount of time before the notification is displayed again after a
 // user has interacted with it.
-const int kHatsThresholdDays = 90;
+constexpr base::TimeDelta kHatsThreshold = base::TimeDelta::FromDays(90);
 
-// The threshold for a googler is less.
-const int kHatsGooglerThresholdDays = 30;
+// The threshold for a Googler is less.
+constexpr base::TimeDelta kHatsGooglerThreshold = base::TimeDelta::FromDays(30);
 
 // Minimum amount of time after initial login or oobe after which we can show
 // the HaTS notification.
-const int kHatsNewDeviceThresholdDays = 7;
+constexpr base::TimeDelta kHatsNewDeviceThreshold =
+    base::TimeDelta::FromDays(7);
 
 // Returns true if the given |profile| interacted with HaTS by either
-// dismissing the notification or taking the survey within a given threshold
-// days |threshold_days|.
-bool DidShowSurveyToProfileRecently(Profile* profile, int threshold_days) {
+// dismissing the notification or taking the survey within a given
+// |threshold_time|.
+bool DidShowSurveyToProfileRecently(Profile* profile,
+                                    base::TimeDelta threshold_time) {
   int64_t serialized_timestamp =
       profile->GetPrefs()->GetInt64(prefs::kHatsLastInteractionTimestamp);
 
   base::Time previous_interaction_timestamp =
       base::Time::FromInternalValue(serialized_timestamp);
-  return (previous_interaction_timestamp +
-          base::TimeDelta::FromDays(threshold_days)) > base::Time::Now();
+  return previous_interaction_timestamp + threshold_time > base::Time::Now();
 }
 
-// Returns true if at least |threshold_days| days have passed since OOBE. This
-// is an indirect measure of whether the owner has used the device for at least
-// |threshold_days| days.
-bool IsNewDevice(int threshold_days) {
+// Returns true if at least |kHatsNewDeviceThreshold| time have passed since
+// OOBE. This is an indirect measure of whether the owner has used the device
+// for at least |kHatsNewDeviceThreshold| time.
+bool IsNewDevice() {
   return chromeos::StartupUtils::GetTimeSinceOobeFlagFileCreation() <=
-         base::TimeDelta::FromDays(threshold_days);
+         kHatsNewDeviceThreshold;
 }
 
 bool IsGoogleUser(std::string username) {
@@ -88,19 +90,25 @@ const char HatsNotificationController::kNotificationId[] = "hats_notification";
 
 HatsNotificationController::HatsNotificationController(Profile* profile)
     : profile_(profile), weak_pointer_factory_(this) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   base::PostTaskWithTraitsAndReplyWithResult(
       FROM_HERE, {base::MayBlock(), base::TaskPriority::BEST_EFFORT},
-      base::Bind(&IsNewDevice, kHatsNewDeviceThresholdDays),
-      base::Bind(&HatsNotificationController::Initialize,
-                 weak_pointer_factory_.GetWeakPtr()));
+      base::BindOnce(&IsNewDevice),
+      base::BindOnce(&HatsNotificationController::Initialize,
+                     weak_pointer_factory_.GetWeakPtr()));
 }
 
 HatsNotificationController::~HatsNotificationController() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   if (network_portal_detector::IsInitialized())
     network_portal_detector::GetInstance()->RemoveObserver(this);
 }
 
 void HatsNotificationController::Initialize(bool is_new_device) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   if (is_new_device && !IsTestingEnabled()) {
     // 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.
@@ -115,6 +123,8 @@ void HatsNotificationController::Initialize(bool is_new_device) {
 
 // static
 bool HatsNotificationController::ShouldShowSurveyToProfile(Profile* profile) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   if (IsTestingEnabled())
     return true;
 
@@ -146,12 +156,12 @@ bool HatsNotificationController::ShouldShowSurveyToProfile(Profile* profile) {
   if (!hats_finch_helper.IsDeviceSelectedForCurrentCycle())
     return false;
 
-  int threshold_days = IsGoogleUser(profile->GetProfileUserName())
-                           ? kHatsGooglerThresholdDays
-                           : kHatsThresholdDays;
+  base::TimeDelta threshold_time = IsGoogleUser(profile->GetProfileUserName())
+                                       ? kHatsGooglerThreshold
+                                       : kHatsThreshold;
   // Do not show survey to user if user has interacted with HaTS within the past
-  // |kHatsThresholdTime| time delta.
-  if (DidShowSurveyToProfileRecently(profile, threshold_days))
+  // |threshold_time| time delta.
+  if (DidShowSurveyToProfileRecently(profile, threshold_time))
     return false;
 
   return true;
@@ -160,9 +170,11 @@ bool HatsNotificationController::ShouldShowSurveyToProfile(Profile* profile) {
 void HatsNotificationController::Click(
     const base::Optional<int>& button_index,
     const base::Optional<base::string16>& reply) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   UpdateLastInteractionTime();
 
-  // The dialog deletes itslef on close.
+  // The dialog deletes itself on close.
   HatsDialog::CreateAndShow(IsGoogleUser(profile_->GetProfileUserName()));
 
   // Remove the notification.
@@ -172,6 +184,8 @@ void HatsNotificationController::Click(
 
 // message_center::NotificationDelegate override:
 void HatsNotificationController::Close(bool by_user) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   if (by_user)
     UpdateLastInteractionTime();
 }
@@ -180,6 +194,7 @@ void HatsNotificationController::Close(bool by_user) {
 void HatsNotificationController::OnPortalDetectionCompleted(
     const NetworkState* network,
     const NetworkPortalDetector::CaptivePortalState& state) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   VLOG(1) << "HatsController::OnPortalDetectionCompleted(): "
           << "network=" << (network ? network->path() : "") << ", "
           << "state.status=" << state.status << ", "
@@ -209,6 +224,8 @@ void HatsNotificationController::OnPortalDetectionCompleted(
 }
 
 void HatsNotificationController::UpdateLastInteractionTime() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   PrefService* pref_service = profile_->GetPrefs();
   pref_service->SetInt64(prefs::kHatsLastInteractionTimestamp,
                          base::Time::Now().ToInternalValue());

chrome/browser/chromeos/hats/hats_notification_controller.h

@@ -19,6 +19,7 @@ namespace chromeos {
 
 // Happiness tracking survey (HaTS) notification controller is responsible for
 // managing the HaTS notification that is displayed to the user.
+// This class lives on the UI thread.
 class HatsNotificationController : public message_center::NotificationDelegate,
                                    public NetworkPortalDetector::Observer {
  public:
@@ -57,7 +58,7 @@ class HatsNotificationController : public message_center::NotificationDelegate,
 
   void UpdateLastInteractionTime();
 
-  Profile* profile_;
+  Profile* const profile_;
   base::WeakPtrFactory<HatsNotificationController> weak_pointer_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(HatsNotificationController);