Defer effects of HandleViewed/HandleClosed until prefs are loaded again.

This CL changes NotificationPromo's HandleViewed/HandleClosed methods to
not update the current instance variables and just save the updated
values to prefs. This is needed because HandleViewed can be called
in the middle of the chain of CanShow calls made during layout of the
NTP, causing some CanShow calls to return true while others to return
false during the same layout. By not saving the updated values to the
current instance variables, the CanShow calls will remain consistent
during the same layout. To facilitate this change, this CL also changes
the return type of HandleViewed to void since the return value is never
used.

BUG=625192

Review-Url: https://codereview.chromium.org/2140173009
Cr-Commit-Position: refs/heads/master@{#407218}

ios/chrome/browser/notification_promo.cc

@@ -232,18 +232,17 @@ bool NotificationPromo::CanShow() const {
 
 void NotificationPromo::HandleClosed() {
   if (!closed_) {
-    closed_ = true;
-    WritePrefs();
+    WritePrefs(promo_id_, first_view_time_, views_, true);
   }
 }
 
-bool NotificationPromo::HandleViewed() {
-  ++views_;
-  if (first_view_time_ == 0) {
-    first_view_time_ = base::Time::Now().ToDoubleT();
+void NotificationPromo::HandleViewed() {
+  int views = views_ + 1;
+  double first_view_time = first_view_time_;
+  if (first_view_time == 0) {
+    first_view_time = base::Time::Now().ToDoubleT();
   }
-  WritePrefs();
-  return ExceedsMaxViews() || ExceedsMaxSeconds();
+  WritePrefs(promo_id_, first_view_time, views, closed_);
 }
 
 bool NotificationPromo::ExceedsMaxViews() const {

ios/chrome/browser/notification_promo.h

@@ -57,9 +57,8 @@ class NotificationPromo {
 
   // Mark the promo as closed when the user dismisses it.
   void HandleClosed();
-  // Mark the promo has having been viewed. This returns true if views
-  // exceeds the maximum allowed.
-  bool HandleViewed();
+  // Mark the promo has having been viewed.
+  void HandleViewed();
 
   const std::string& promo_text() const { return promo_text_; }
   PromoType promo_type() const { return promo_type_; }

ios/chrome/browser/notification_promo_unittest.cc

@@ -162,9 +162,7 @@ class NotificationPromoTest : public testing::Test {
     NotificationPromo first_promo(&local_state_);
     first_promo.InitFromVariations();
     first_promo.InitFromPrefs(promo_type_);
-
-    first_promo.HandleViewed();
-    EXPECT_EQ(first_promo.max_views_ - 1, first_promo.views_);
+    EXPECT_EQ(first_promo.max_views_ - 2, first_promo.views_);
     EXPECT_TRUE(first_promo.CanShow());
     first_promo.HandleViewed();
 
@@ -173,19 +171,26 @@ class NotificationPromoTest : public testing::Test {
     NotificationPromo second_promo(&local_state_);
     second_promo.InitFromVariations();
     second_promo.InitFromPrefs(promo_type_);
-    EXPECT_EQ(second_promo.max_views_, second_promo.views_);
-    EXPECT_FALSE(second_promo.CanShow());
+    EXPECT_EQ(second_promo.max_views_ - 1, second_promo.views_);
+    EXPECT_TRUE(second_promo.CanShow());
+    second_promo.HandleViewed();
+
+    NotificationPromo third_promo(&local_state_);
+    third_promo.InitFromVariations();
+    third_promo.InitFromPrefs(promo_type_);
+    EXPECT_EQ(third_promo.max_views_, third_promo.views_);
+    EXPECT_FALSE(third_promo.CanShow());
 
     // Test out of range views.
     for (int i = max_views_; i < max_views_ * 2; ++i) {
-      second_promo.views_ = i;
-      EXPECT_FALSE(second_promo.CanShow());
+      third_promo.views_ = i;
+      EXPECT_FALSE(third_promo.CanShow());
     }
 
     // Test in range views.
     for (int i = 0; i < max_views_; ++i) {
-      second_promo.views_ = i;
-      EXPECT_TRUE(second_promo.CanShow());
+      third_promo.views_ = i;
+      EXPECT_TRUE(third_promo.CanShow());
     }
 
     // Reset prefs to default.
@@ -200,10 +205,7 @@ class NotificationPromoTest : public testing::Test {
     first_promo.InitFromPrefs(promo_type_);
     EXPECT_FALSE(first_promo.closed_);
     EXPECT_TRUE(first_promo.CanShow());
-
     first_promo.HandleClosed();
-    EXPECT_TRUE(first_promo.closed_);
-    EXPECT_FALSE(first_promo.CanShow());
 
     // Initialize another promo to test that the the closing of the promo was
     // recorded correctly in prefs.
@@ -274,14 +276,11 @@ class NotificationPromoTest : public testing::Test {
   void TestFirstViewTimeRecorded() {
     EXPECT_EQ(0, notification_promo_.first_view_time_);
     notification_promo_.HandleViewed();
-    EXPECT_NE(0, notification_promo_.first_view_time_);
-    double first_viewed = notification_promo_.first_view_time_;
 
     NotificationPromo temp_promo(&local_state_);
     temp_promo.InitFromVariations();
     temp_promo.InitFromPrefs(promo_type_);
-
-    EXPECT_EQ(first_viewed, temp_promo.first_view_time_);
+    EXPECT_NE(0, temp_promo.first_view_time_);
 
     notification_promo_.views_ = 0;
     notification_promo_.first_view_time_ = 0;