Integrate SMS service with Desktop iOS promotion

chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc

Index: chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc
diff --git a/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc b/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc
index b4cba3f3b0609a62b09c4e79114e780cf215f3ed..ea9e2760198bda5bfcd445484ae6ec055c0e33af 100644
--- a/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc
+++ b/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc
@@ -4,14 +4,102 @@
 
 #include "chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h"
 
-DesktopIOSPromotionController::DesktopIOSPromotionController() {}
+#include "base/bind.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/time/time.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h"
+#include "chrome/browser/ui/desktop_ios_promotion/sms_service.h"
+#include "chrome/browser/ui/desktop_ios_promotion/sms_service_factory.h"
+#include "components/prefs/pref_service.h"
+
+DesktopIOSPromotionController::DesktopIOSPromotionController(
+    Profile* profile,
+    desktop_ios_promotion::PromotionEntryPoint entry_point)
+    : weak_ptr_factory_(this),

[sky, 2017/02/16 00:13:35]

Order of member initialize list should match that of declaration order.

[mrefaat, 2017/02/16 21:02:39]

Done.

+      recovery_number_(std::string()),

[sky, 2017/02/16 00:13:35]

Don't bother initializing members that have empty constructors.

[mrefaat, 2017/02/16 21:02:39]

Done.

+      entry_point_(entry_point),
+      profile_prefs_(profile->GetPrefs()) {
+  sms_service_ = SMSServiceFactory::GetForProfile(profile);

[sky, 2017/02/16 00:13:36]

Move to member initializer.

[mrefaat, 2017/02/16 21:02:39]

Done.

+  sms_service_->QueryPhoneNumber(
+      base::Bind(&DesktopIOSPromotionController::OnQueryPhoneNumber,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
 
 DesktopIOSPromotionController::~DesktopIOSPromotionController() {}
 
 void DesktopIOSPromotionController::OnSendSMSClicked() {
-  // TODO(crbug.com/676655): Call the growth api to send sms.
+  // TODO(crbug.com/676655): Get the SMS message id from the finch group.
+  std::string sms_message_id = "19001507";
+  sms_service_->SendSMS(sms_message_id,
+                        base::Bind(&DesktopIOSPromotionController::OnSendSMS,
+                                   weak_ptr_factory_.GetWeakPtr()));
+
+  // Update Profile prefs.
+  profile_prefs_->SetInteger(prefs::kIOSPromotionSMSEntryPoint,
+                             (int)entry_point_);

[sky, 2017/02/16 00:13:35]

Use c++ casts.

[mrefaat, 2017/02/16 21:02:39]

Done.

+
+  // Update histograms.
+  desktop_ios_promotion::LogDismissalReason(
+      desktop_ios_promotion::PromotionDismissalReason::SEND_SMS, entry_point_);
+}
+
+void DesktopIOSPromotionController::OnPromotionShown() {
+  // update the impressions count.
+  PrefService* local_state = g_browser_process->local_state();
+  int impressions = local_state->GetInteger(
+      desktop_ios_promotion::kEntryPointLocalPrefs[(int)entry_point_ - 1][(

[sky, 2017/02/16 00:13:36]

Again with the casts, also, use a constant for the -1 everywhere. Probably the
constant should be part of the enum to make it obvious.

[mrefaat, 2017/02/16 21:02:39]

made a constant but i think it shouldn't be in the enum - just to avoid doing 1
more cast.

+          int)desktop_ios_promotion::EntryPointLocalPrefType::IMPRESSIONS]);
+  impressions++;
+  local_state->SetInteger(
+      desktop_ios_promotion::kEntryPointLocalPrefs[(int)entry_point_ - 1][(
+          int)desktop_ios_promotion::EntryPointLocalPrefType::IMPRESSIONS],
+      impressions);
+
+  // Update synced profile prefs.
+  int shown_entrypoints =
+      profile_prefs_->GetInteger(prefs::kIOSPromotionShownEntryPoints);
+  shown_entrypoints |= 1 << (int)entry_point_;
+  profile_prefs_->SetInteger(prefs::kIOSPromotionShownEntryPoints,
+                             shown_entrypoints);
+
+  // If the promo is seen then it means the SMS was not sent on the last 7 days,
+  // reset the pref.
+  profile_prefs_->SetInteger(prefs::kIOSPromotionSMSEntryPoint, 0);
+
+  double last_impression = base::Time::NowFromSystemTime().ToDoubleT();
+  profile_prefs_->SetDouble(prefs::kIOSPromotionLastImpression,
+                            last_impression);
+  // Update histograms.
+  UMA_HISTOGRAM_ENUMERATION("DesktopIOSPromotion.ImpressionFromEntryPoint",
+                            (int)entry_point_, 5);

[sky, 2017/02/16 00:13:36]

Use a constant for 5.

[mrefaat, 2017/02/16 21:02:39]

Done.

 }
 
 void DesktopIOSPromotionController::OnNoThanksClicked() {
-  // TODO(crbug.com/676655): Handle logging & update sync.
+  PrefService* local_state = g_browser_process->local_state();
+  local_state->SetBoolean(
+      desktop_ios_promotion::kEntryPointLocalPrefs[(int)entry_point_ - 1][(
+          int)desktop_ios_promotion::EntryPointLocalPrefType::DISMISSED],
+      true);
+
+  desktop_ios_promotion::LogDismissalReason(
+      desktop_ios_promotion::PromotionDismissalReason::NO_THANKS, entry_point_);
+}
+
+void DesktopIOSPromotionController::OnQueryPhoneNumber(
+    SMSService::Request* request,
+    bool success,
+    const std::string& number) {
+  if (success) {

[sky, 2017/02/16 00:13:36]

no {}

[mrefaat, 2017/02/16 21:02:39]

Done.

+    recovery_number_ = number;

[sky, 2017/02/16 00:13:36]

AFAICT this isn't used.

Based on my understanding it looks like you show the bubble, and then query for
the phone number. Given that the query can fail, this seems bad. You should
query first and only show the bubble if you can get the number, right?

[mrefaat, 2017/02/16 21:02:39]

Our Targeting set will only have the user if their phone number is on the
account.
But anyway we initially put the bubble with message that doesn't have the number
if the number isn't ready, but after the number is ready we update the bubble.

[sky, 2017/02/17 00:04:12]

Not sure I follow. That sort of implies success is always true. Is that right?
If it is possible for success to be false, then again, you should verify you
really have a number before showing the bubble. Otherwise if you can't actually
get the number, then why show the bubble?

+  }
+  UMA_HISTOGRAM_BOOLEAN("DesktopIOSPromotion.QueryPhoneNumberSucceeded",
+                        success);
+}
+
+void DesktopIOSPromotionController::OnSendSMS(SMSService::Request* request,
+                                              bool success,
+                                              const std::string& number) {
+  UMA_HISTOGRAM_BOOLEAN("DesktopIOSPromotion.SendSMSSucceeded", success);
 }