Remove promotion-type-specific JSON handling from NotificationPromo

chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc

Index: chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc
diff --git a/chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc b/chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..5749130249e3044336867ab6c1e68bb8a45db692
--- /dev/null
+++ b/chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc
@@ -0,0 +1,270 @@
+// Copyright (c) 2012 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 <vector>
+
+#include "base/json/json_reader.h"
+#include "base/message_loop.h"
+#include "base/string_number_conversions.h"
+#include "base/time.h"
+#include "base/utf_string_conversions.h"
+#include "base/values.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/prefs/browser_prefs.h"
+#include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/web_resource/notification_promo.h"
+#include "chrome/browser/web_resource/promo_resource_service.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "chrome/test/base/testing_pref_service.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/notification_service.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+#include "chrome/browser/web_resource/notification_promo_mobile_ntp.h"
+
+class PromoResourceServiceMobileNtpTest : public testing::Test {
+ public:
+  PromoResourceServiceMobileNtpTest()
+      : local_state_(static_cast<TestingBrowserProcess*>(g_browser_process)),
+        web_resource_service_(new PromoResourceService(&profile_)) {
+  }
+
+ protected:
+  TestingProfile profile_;
+  ScopedTestingLocalState local_state_;
+  scoped_refptr<PromoResourceService> web_resource_service_;
+  MessageLoop loop_;
+};
+
+class NotificationPromoMobileNtpTest {
+ public:
+  explicit NotificationPromoMobileNtpTest(Profile* profile)
+      : profile_(profile),
+        prefs_(profile->GetPrefs()),
+        notification_promo_(profile),
+        received_notification_(false),
+        start_(0.0),
+        end_(0.0),
+        closed_(false) {
+  }
+
+  void Init(const std::string& json,
+            const std::string& promo_text,
+            const std::string& promo_text_long,
+            const std::string& promo_action_type,
+            const std::string& promo_action_arg0,
+            const std::string& promo_action_arg1,
+            double start, double end) {
+    Value* value(base::JSONReader::Read(json));
+    ASSERT_TRUE(value);

[Dan Beam, 2012/08/21 03:53:49]

can you use

  EXPECT_NE(value, NULL);

for these cases (and throughout) or does something complain about some ostream
operator not existing?

[aruslan, 2012/08/21 15:57:55]

Waiting till NULL is nullptr.
Otherwise
./testing/gtest/include/gtest/gtest.h: In function 'testing::AssertionResult
testing::internal::CmpHelperNE(char const*, char const*, const T1&, const T2&)
[wit
h T1 = base::Value*, T2 = int]':
./testing/gtest/include/gtest/gtest.h:1532:1: error: ISO C++ forbids comparison
between pointer and integer [-fpermissive]

+    DictionaryValue* dict = NULL;
+    value->GetAsDictionary(&dict);
+    ASSERT_TRUE(dict);
+    test_json_.reset(dict);
+
+    promo_type_ = NotificationPromo::MOBILE_NTP_SYNC_PROMO;
+
+    promo_text_ = promo_text;
+
+    promo_text_long_ = promo_text_long;
+    promo_action_type_ = promo_action_type;
+    promo_action_args_.push_back(promo_action_arg0);
+    promo_action_args_.push_back(promo_action_arg1);
+
+    start_ = start;
+    end_ = end;
+
+
+    closed_ = false;
+    received_notification_ = false;
+  }
+
+  void InitPromoFromJson(bool should_receive_notification) {
+    notification_promo_.InitFromJson(*test_json_, promo_type_);
+    EXPECT_EQ(should_receive_notification,
+              notification_promo_.new_notification());
+
+    // Test the fields.
+    TestNotification();
+  }
+
+  void TestNotification() {
+    // Check values.
+    EXPECT_EQ(notification_promo_.promo_text_, promo_text_);
+
+    NotificationPromoMobileNtp mobile_promo(notification_promo_);
+    EXPECT_TRUE(mobile_promo.valid());
+    EXPECT_EQ(mobile_promo.text(), promo_text_);
+    EXPECT_EQ(mobile_promo.text_long(), promo_text_long_);
+    EXPECT_EQ(mobile_promo.action_type(), promo_action_type_);
+    EXPECT_TRUE(mobile_promo.action_args() != NULL);
+    EXPECT_EQ(std::size_t(2), promo_action_args_.size());

[Dan Beam, 2012/08/21 03:53:49]

hmmm, I guess I'm used to seeing 2U here instead. is there any reason to do one
over the other?

[aruslan, 2012/08/21 15:57:55]

Just a habit (2u is not size_t).
You are right -- grep says that everyone is using 2u.
On 2012/08/21 03:53:49, Dan Beam wrote:

+    EXPECT_EQ(mobile_promo.action_args()->GetSize(),
+              promo_action_args_.size());
+    for (std::size_t i = 0; i < promo_action_args_.size(); ++i) {
+      std::string value;
+      EXPECT_TRUE(mobile_promo.action_args()->GetString(i, &value));
+      EXPECT_EQ(value, promo_action_args_[i]);
+    }
+
+    EXPECT_EQ(notification_promo_.start_, start_);
+    EXPECT_EQ(notification_promo_.end_, end_);
+    EXPECT_EQ(notification_promo_.closed_, closed_);
+  }
+
+  // Create a new NotificationPromo from prefs and compare to current
+  // notification.
+  void TestInitFromPrefs() {
+    NotificationPromo prefs_notification_promo(profile_);
+    prefs_notification_promo.InitFromPrefs(promo_type_);
+
+    EXPECT_EQ(notification_promo_.prefs_,

[Dan Beam, 2012/08/21 03:53:49]

is there a way you could use the public interface instead of the private one
here?

[aruslan, 2012/08/21 15:57:55]

Done.

+              prefs_notification_promo.prefs_);
+    EXPECT_EQ(notification_promo_.promo_text_,
+              prefs_notification_promo.promo_text_);
+    EXPECT_EQ(notification_promo_.start_,
+              prefs_notification_promo.start_);
+    EXPECT_EQ(notification_promo_.end_,
+              prefs_notification_promo.end_);
+    EXPECT_EQ(notification_promo_.views_,
+              prefs_notification_promo.views_);
+    EXPECT_EQ(notification_promo_.closed_,
+              prefs_notification_promo.closed_);
+
+    NotificationPromoMobileNtp prefs_mobile_promo(notification_promo_);
+    EXPECT_TRUE(prefs_mobile_promo.valid());
+    NotificationPromoMobileNtp mobile_promo(prefs_notification_promo);
+    EXPECT_TRUE(mobile_promo.valid());
+    EXPECT_EQ(prefs_mobile_promo.text(),
+              mobile_promo.text());
+    EXPECT_EQ(prefs_mobile_promo.text_long(),
+              mobile_promo.text_long());
+    EXPECT_EQ(prefs_mobile_promo.action_type(),
+              mobile_promo.action_type());
+    EXPECT_TRUE(mobile_promo.action_args() != NULL);
+    EXPECT_EQ(prefs_mobile_promo.action_args()->GetSize(),
+              mobile_promo.action_args()->GetSize());
+    for (std::size_t i = 0;
+         i < prefs_mobile_promo.action_args()->GetSize();
+         ++i) {
+      std::string promo_value;
+      std::string prefs_value;
+      EXPECT_TRUE(
+          prefs_mobile_promo.action_args()->GetString(i, &promo_value));
+      EXPECT_TRUE(
+          mobile_promo.action_args()->GetString(

[Dan Beam, 2012/08/21 03:53:49]

why does this need to be wrapped so much?

[aruslan, 2012/08/21 15:57:55]

Done.

+              i, &prefs_value));
+      EXPECT_EQ(promo_value, prefs_value);
+    }
+  }
+
+  void TestClosed() {
+    NotificationPromo new_promo(profile_);
+    new_promo.InitFromPrefs(promo_type_);
+    EXPECT_FALSE(new_promo.closed_);
+    EXPECT_TRUE(new_promo.CanShow());
+
+    NotificationPromo::HandleClosed(profile_, promo_type_);
+    new_promo.InitFromPrefs(promo_type_);
+    EXPECT_TRUE(new_promo.closed_);
+    EXPECT_FALSE(new_promo.CanShow());
+
+    new_promo.closed_ = false;

[Dan Beam, 2012/08/21 03:53:49]

I'm confused as to what this last part is testing?

[aruslan, 2012/08/21 15:57:55]

My fault -- these two tests are non-mobile-specific and are already in the
NotificationPromo test.
On 2012/08/21 03:53:49, Dan Beam wrote:

+    EXPECT_TRUE(new_promo.CanShow());
+    new_promo.WritePrefs();
+  }
+
+  void TestPromoText() {

[Dan Beam, 2012/08/21 03:53:49]

so this tests that we're not going to show an empty promo? if so, can you update
the name of this test to reflect that?

[aruslan, 2012/08/21 15:57:55]

Removed (tested in NotificationPromo).

+    notification_promo_.promo_text_.clear();
+    EXPECT_FALSE(notification_promo_.CanShow());
+
+    notification_promo_.promo_text_ = promo_text_;
+    EXPECT_TRUE(notification_promo_.CanShow());
+  }
+
+ private:
+  Profile* profile_;
+  PrefService* prefs_;
+  NotificationPromo notification_promo_;
+  bool received_notification_;
+  scoped_ptr<DictionaryValue> test_json_;
+
+  NotificationPromo::PromoType promo_type_;
+  std::string promo_text_;
+  std::string promo_text_long_;
+  std::string promo_action_type_;
+  std::vector<std::string> promo_action_args_;
+
+  double start_;
+  double end_;
+  bool closed_;
+};
+
+TEST_F(PromoResourceServiceMobileNtpTest, NotificationPromoMobileNtpTest) {
+  // Check that prefs are set correctly.
+  PrefService* prefs = profile_.GetPrefs();
+  ASSERT_TRUE(prefs != NULL);
+
+  NotificationPromoMobileNtpTest promo_test(&profile_);
+
+  // Set up start and end dates and promo line in a Dictionary as if parsed
+  // from the service.
+  promo_test.Init(
+      "{"
+      "  \"mobile_ntp_sync_promo\": ["
+      "    {"
+      "      \"date\":"
+      "        ["
+      "          {"
+      "            \"start\":\"3 Aug 1999 9:26:06 GMT\","
+      "            \"end\":\"7 Jan 2013 5:40:75 PST\""
+      "          }"
+      "        ],"
+      "      \"strings\":"
+      "        {"
+      "          \"MOBILE_PROMO_CHROME_SHORT_TEXT\":"
+      "          \"Like Chrome? Go http://www.google.com/chrome/\","
+      "          \"MOBILE_PROMO_CHROME_LONG_TEXT\":"
+      "          \"It\'s simple. Go http://www.google.com/chrome/\","
+      "          \"MOBILE_PROMO_EMAIL_BODY\":\"This is the body.\","
+      "          \"XXX\":\"XXX value\""
+      "        },"
+      "      \"payload\":"
+      "        {"
+      "          \"payload_format_version\":3,"
+      "          \"gplus_required\":false,"
+      "          \"promo_message_long\":"
+      "              \"MOBILE_PROMO_CHROME_LONG_TEXT\","
+      "          \"promo_message_short\":"
+      "              \"MOBILE_PROMO_CHROME_SHORT_TEXT\","
+      "          \"promo_action_type\":\"ACTION_EMAIL\","
+      "          \"promo_action_args\":[\"MOBILE_PROMO_EMAIL_BODY\",\"XXX\"]"
+      "        },"
+      "      \"max_views\":30"
+      "    }"
+      "  ]"
+      "}",
+      "Like Chrome? Go http://www.google.com/chrome/",
+      "It\'s simple. Go http://www.google.com/chrome/",

[Dan Beam, 2012/08/21 03:53:49]

nit: why is there a \' ? also, shouldn't it be go to <link>?

[aruslan, 2012/08/21 15:57:55]

Removed unnecessary escaping.

+      "ACTION_EMAIL", "This is the body.", "XXX value",
+      // We hardcode the unix epoch time to make sure our parsing works.
+      // The starting date is in 1999 to make tests pass on Android devices
+      // with incorrect or unset date/time.
+      933672366,   // unix epoch for 3 Aug 1999 9:26:06 GMT.
+      1357566075); // unix epoch for 7 Jan 2013 5:40:75 PST.
+
+  promo_test.InitPromoFromJson(true);
+
+  // Second time should not trigger a notification.
+  promo_test.InitPromoFromJson(false);
+
+  promo_test.TestInitFromPrefs();
+
+  // Test various conditions of CanShow.
+  promo_test.TestClosed();
+  promo_test.TestPromoText();
+}