Remove promotion-type-specific JSON handling from NotificationPromo

chrome/browser/web_resource/notification_promo.cc

Index: chrome/browser/web_resource/notification_promo.cc
diff --git a/chrome/browser/web_resource/notification_promo.cc b/chrome/browser/web_resource/notification_promo.cc
index 9a9ddbbfe446e433e3272c599ae538019ba37311..eef196a2d3e7fae54b71a86cdd6c40ccd4bdd99d 100644
--- a/chrome/browser/web_resource/notification_promo.cc
+++ b/chrome/browser/web_resource/notification_promo.cc
@@ -35,13 +35,12 @@ const int kDefaultGroupSize = 100;
 
 const char promo_server_url[] = "https://clients3.google.com/crsignal/client";
 
+// TODO(achuith): Shouldn't this be in pref_names?

[achuithb, 2012/08/20 19:28:31]

Let's not have these be in pref_names. These should only be accessed through
NotificationPromo. Please remove this comment.

[aruslan, 2012/08/20 22:38:17]

Done.

 const char kPrefPromoObject[] = "promo";
+
+// Keys into the kPrefPromoObject dictionary; used only here.

[Dan Beam, 2012/08/20 20:58:12]

nit: Keys in the

[aruslan, 2012/08/20 22:38:17]

Done.

 const char kPrefPromoText[] = "text";
-#if defined(OS_ANDROID)
-const char kPrefPromoTextLong[] = "text_long";
-const char kPrefPromoActionType[] = "action_type";
-const char kPrefPromoActionArgs[] = "action_args";
-#endif
+const char kPrefPromoPayload[] = "payload";
 const char kPrefPromoStart[] = "start";
 const char kPrefPromoEnd[] = "end";
 const char kPrefPromoNumGroups[] = "num_groups";
@@ -55,10 +54,6 @@ const char kPrefPromoViews[] = "views";
 const char kPrefPromoClosed[] = "closed";
 const char kPrefPromoGPlusRequired[] = "gplus_required";
 
-#if defined(OS_ANDROID)
-const int kCurrentMobilePayloadFormatVersion = 3;
-#endif  // defined(OS_ANDROID)
-
 // Returns a string suitable for the Promo Server URL 'osname' value.
 std::string PlatformString() {
 #if defined(OS_WIN)
@@ -133,21 +128,6 @@ void ClearDeprecatedPrefs(PrefService* prefs) {
                             std::string(),
                             PrefService::UNSYNCABLE_PREF);
   prefs->ClearPref(prefs::kNtpPromoLine);
-#if defined(OS_ANDROID)
-  prefs->RegisterStringPref(prefs::kNtpPromoLineLong,
-                            std::string(),
-                            PrefService::UNSYNCABLE_PREF);
-  prefs->RegisterStringPref(prefs::kNtpPromoActionType,
-                            std::string(),
-                            PrefService::UNSYNCABLE_PREF);
-  prefs->RegisterListPref(prefs::kNtpPromoActionArgs,
-                          new base::ListValue,
-                          PrefService::UNSYNCABLE_PREF);
-  prefs->ClearPref(prefs::kNtpPromoLineLong);
-  prefs->ClearPref(prefs::kNtpPromoActionType);
-  prefs->ClearPref(prefs::kNtpPromoActionArgs);
-#endif  // defined(OS_ANDROID)
-
   prefs->RegisterDoublePref(prefs::kNtpPromoStart,
                             0,
                             PrefService::UNSYNCABLE_PREF);
@@ -203,15 +183,71 @@ void ClearDeprecatedPrefs(PrefService* prefs) {
   prefs->ClearPref(prefs::kNtpPromoGplusRequired);
 }
 
+// Deep-copies a node, replacing any "value" that is a key
+// into "strings" dictionary with its value from "strings".
+// E.g. for
+//   {promo_action_args:['MSG_SHORT']} + strings:{MSG_SHORT:'yes'}
+// it will return
+//   {promo_action_args:['yes']}
+// |node| - a value to be deep copied and resolved.
+// |strings| - a dictionary of strings to be used for resolution.
+// Returns a _new_ object that is a deep copy with replacements.
+base::Value* DeepCopyAndResolveStrings(
+    const base::Value* node,
+    const base::DictionaryValue* strings) {
+  switch (node->GetType()) {
+    case base::Value::TYPE_LIST: {
+      const base::ListValue* list = static_cast<const base::ListValue*>(node);
+      base::ListValue* copy = new base::ListValue;
+      for (base::ListValue::const_iterator it = list->begin();

[Dan Beam, 2012/08/20 20:58:12]

* wishes we could say

  for (auto it = list->begin(); it != list->end(); ++it) {

:(

[aruslan, 2012/08/20 22:38:17]

I hope we could "for (auto it : list) {}" soon! :)

On 2012/08/20 20:58:12, Dan Beam wrote:

+           it != list->end();

[Dan Beam, 2012/08/20 20:58:12]

nit: move ++it; up a line to match style below

[aruslan, 2012/08/20 22:38:17]

Done.

+           ++it) {
+        base::Value* child_copy = DeepCopyAndResolveStrings(*it, strings);
+        copy->Append(child_copy);
+      }
+      return copy;
+    }
+
+    case Value::TYPE_DICTIONARY: {
+      const base::DictionaryValue* dict =
+          static_cast<const base::DictionaryValue*>(node);
+      base::DictionaryValue* copy = new base::DictionaryValue;
+      for (base::DictionaryValue::key_iterator it = dict->begin_keys();
+           it != dict->end_keys(); ++it) {
+        const base::Value* child = NULL;
+        bool rv = dict->GetWithoutPathExpansion(*it, &child);

[Dan Beam, 2012/08/20 20:58:12]

DCHECK(dict->GetWithoutPathExpansion(*it, &child));

[achuithb, 2012/08/20 21:52:31]

Don't think you can do that - it would get compiled out in release.

[aruslan, 2012/08/20 22:38:17]

Yes, base/logging.h explicitly says that the |condition| will be "referenced"
(as in "false && condition;") to avoid warnings, but will NOT be executed (as in
"condition;").

On 2012/08/20 21:52:31, achuith.bhandarkar wrote:

[Dan Beam, 2012/08/21 03:23:25]

I guess we just use CHECK() everywhere in the NTP, which doesn't get compiled
out.  Ignore me I'm full of it.

+        DCHECK(rv);
+        base::Value* child_copy = DeepCopyAndResolveStrings(child, strings);
+        copy->SetWithoutPathExpansion(*it, child_copy);
+      }
+      return copy;
+    }
+
+    case Value::TYPE_STRING: {
+      const base::StringValue* str =
+          static_cast<const base::StringValue*>(node);
+      std::string value;
+      bool rv = str->GetAsString(&value);

[Dan Beam, 2012/08/20 20:58:12]

DCHECK(str->GetAsString(&value));

[achuithb, 2012/08/20 21:52:31]

same.

[aruslan, 2012/08/20 22:38:17]

Agreed.
On 2012/08/20 21:52:31, achuith.bhandarkar wrote:

+      DCHECK(rv);
+      std::string actual_value;
+      if (!strings || !strings->GetString(value, &actual_value))
+        actual_value = value;
+      return new base::StringValue(actual_value);
+    }
+
+    default:
+      // For everything else, just make a copy.
+      return node->DeepCopy();
+  }
+}
+
 }  // namespace
 
 NotificationPromo::NotificationPromo(Profile* profile)
     : profile_(profile),
       prefs_(profile_->GetPrefs()),
       promo_type_(NO_PROMO),
-#if defined(OS_ANDROID)
-      promo_action_args_(new base::ListValue),
-#endif
+      promo_payload_(new base::DictionaryValue()),
       start_(0.0),
       end_(0.0),
       num_groups_(kDefaultGroupSize),
@@ -245,16 +281,6 @@ void NotificationPromo::InitFromJson(const DictionaryValue& json,
   if (!promo_list->GetDictionary(0, &promo))
     return;
 
-  // Strings. Assume the first one is the promo text.
-  const DictionaryValue* strings = NULL;
-  if (promo->GetDictionary("strings", &strings)) {
-#if !defined(OS_ANDROID)
-    DictionaryValue::Iterator iter(*strings);
-    iter.value().GetAsString(&promo_text_);
-    DVLOG(1) << "promo_text_=" << promo_text_;
-#endif  // defined(OS_ANDROID)
-  }
-
   // Date.
   const ListValue* date_list = NULL;
   if (promo->GetList("date", &date_list)) {
@@ -293,66 +319,34 @@ void NotificationPromo::InitFromJson(const DictionaryValue& json,
              << ", max_group_ = " << max_group_;
   }
 
+  // Strings.
+  const DictionaryValue* strings = NULL;
+  promo->GetDictionary("strings", &strings);
+
   // Payload.
   const DictionaryValue* payload = NULL;
   if (promo->GetDictionary("payload", &payload)) {
     payload->GetBoolean("gplus_required", &gplus_required_);
-
     DVLOG(1) << "gplus_required_ = " << gplus_required_;
+
+    base::Value* ppcopy = DeepCopyAndResolveStrings(payload, strings);
+    DCHECK(ppcopy && ppcopy->IsType(base::Value::TYPE_DICTIONARY));

[Dan Beam, 2012/08/20 20:58:12]

you might want to split these into 2 statements so you see which part of this
fails, up to you

[aruslan, 2012/08/20 22:38:17]

It was 2 statements originally, but but either way we would know that
DeepCopyAndResolveStrings is wrong as the input isn't NULL and it's the
dictionary.
On 2012/08/20 20:58:12, Dan Beam wrote:

+    promo_payload_.reset(static_cast<base::DictionaryValue*>(ppcopy));
   }
 
+  if (!promo_payload_->GetString("promo_message_short", &promo_text_) &&
+      strings) {
+    // For compatibility with the legacy desktop version,
+    // if no |payload.promo_message_short| is specified,
+    // the first string in |strings| is used.
+    DictionaryValue::Iterator iter(*strings);
+    iter.value().GetAsString(&promo_text_);
+  }
+  DVLOG(1) << "promo_text_=" << promo_text_;
+
   promo->GetInteger("max_views", &max_views_);
   DVLOG(1) << "max_views_ " << max_views_;
 
-#if defined(OS_ANDROID)
-  int payload_version = 0;
-  if (!payload) {
-    LOG(ERROR) << "Malformed JSON: no payload";
-    return;
-  }
-  if (!strings) {
-    LOG(ERROR) << "Malformed JSON: no strings";
-    return;
-  }
-  if (!payload->GetInteger("payload_format_version", &payload_version) ||
-      payload_version != kCurrentMobilePayloadFormatVersion) {
-    LOG(ERROR) << "Unsupported promo payload_format_version " << payload_version
-               << "; expected " << kCurrentMobilePayloadFormatVersion;
-    return;
-  }
-  std::string promo_key_short;
-  std::string promo_key_long;
-  if (!payload->GetString("promo_message_short", &promo_key_short) ||
-      !payload->GetString("promo_message_long", &promo_key_long) ||
-      !strings->GetString(promo_key_short, &promo_text_) ||
-      !strings->GetString(promo_key_long, &promo_text_long_)) {
-    LOG(ERROR) << "Malformed JSON: no promo_message_short or _long";
-    return;
-  }
-  payload->GetString("promo_action_type", &promo_action_type_);
-  // We need to be idempotent as the tests call us more than once.
-  promo_action_args_.reset(new base::ListValue);
-  const ListValue* args;
-  if (payload->GetList("promo_action_args", &args)) {
-    // JSON format for args: "promo_action_args" : [ "<arg1>", "<arg2>"... ]
-    // Every value comes from "strings" dictionary, either directly or not.
-    // Every arg is either directly a key into "strings" dictionary,
-    // or a key into "payload" dictionary with the value that is a key into
-    // "strings" dictionary.
-    for (std::size_t i = 0; i < args->GetSize(); ++i) {
-      std::string name, key, value;
-      if (!args->GetString(i, &name) ||
-          !(strings->GetString(name, &value) ||
-          (payload->GetString(name, &key) &&
-              strings->GetString(key, &value)))) {
-        LOG(ERROR) << "Malformed JSON: failed to parse promo_action_args";
-        return;
-      }
-      promo_action_args_->Append(base::Value::CreateStringValue(value));
-    }
-  }
-#endif  // defined(OS_ANDROID)
-
   CheckForNewNotification();
 }
 
@@ -379,7 +373,7 @@ void NotificationPromo::OnNewNotification() {
 // static
 void NotificationPromo::RegisterUserPrefs(PrefService* prefs) {
   ClearDeprecatedPrefs(prefs);
-  prefs->RegisterDictionaryPref("promo",
+  prefs->RegisterDictionaryPref(kPrefPromoObject,
                                 new base::DictionaryValue,
                                 PrefService::UNSYNCABLE_PREF);
 }
@@ -388,12 +382,7 @@ void NotificationPromo::WritePrefs() {
   DVLOG(1) << "WritePrefs";
   base::DictionaryValue* ntp_promo = new base::DictionaryValue;
   ntp_promo->SetString(kPrefPromoText, promo_text_);
-#if defined(OS_ANDROID)
-  ntp_promo->SetString(kPrefPromoTextLong, promo_text_long_);
-  ntp_promo->SetString(kPrefPromoActionType, promo_action_type_);
-  DCHECK(promo_action_args_.get());
-  ntp_promo->Set(kPrefPromoActionArgs, promo_action_args_->DeepCopy());
-#endif  // defined(OS_ANDROID)
+  ntp_promo->Set(kPrefPromoPayload, promo_payload_->DeepCopy());
   ntp_promo->SetDouble(kPrefPromoStart, start_);
   ntp_promo->SetDouble(kPrefPromoEnd, end_);
 
@@ -426,25 +415,20 @@ void NotificationPromo::InitFromPrefs(PromoType promo_type) {
   if (!promo_dict)
     return;
 
-  const base::ListValue* promo_list(NULL);
+  const base::ListValue* promo_list = NULL;
   promo_dict->GetList(PromoTypeToString(promo_type_), &promo_list);
   if (!promo_list)
     return;
 
-  const base::DictionaryValue* ntp_promo(NULL);
+  const base::DictionaryValue* ntp_promo = NULL;
   promo_list->GetDictionary(0, &ntp_promo);
   if (!ntp_promo)
     return;
 
   ntp_promo->GetString(kPrefPromoText, &promo_text_);
-#if defined(OS_ANDROID)
-  ntp_promo->GetString(kPrefPromoTextLong, &promo_text_long_);
-  ntp_promo->GetString(kPrefPromoActionType, &promo_action_type_);
-  const base::ListValue* lv(NULL);
-  ntp_promo->GetList(kPrefPromoActionArgs, &lv);
-  DCHECK(lv != NULL);
-  promo_action_args_.reset(lv->DeepCopy());
-#endif  // defined(OS_ANDROID)
+  const base::DictionaryValue* dv = NULL;
+  if (ntp_promo->GetDictionary(kPrefPromoPayload, &dv))
+    promo_payload_.reset(dv->DeepCopy());
 
   ntp_promo->GetDouble(kPrefPromoStart, &start_);
   ntp_promo->GetDouble(kPrefPromoEnd, &end_);