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..7e3556bbd823fef08cd21e4d49baab54c8f34f7f 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?
 const char kPrefPromoObject[] = "promo";
+
+// Keys into the kPrefPromoObject dictionary; used only here.
 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)

[achuithb, 2012/08/17 22:55:12]

Why remove this? We can probably keep it for another cycle before blowing it
away?

[aruslan, 2012/08/20 15:36:53]

I do not think removing mobile-specific prefs would cause issues in the
upgrade/downgrade scenarios -- am I missing something?  Otherwise, both iOS and
Android are branched out already, so we should be good to go.
On 2012/08/17 22:55:12, achuith.bhandarkar wrote:

-  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,70 @@ 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']}
+base::Value* deepCopyAndResolveStrings(

[achuithb, 2012/08/17 22:55:12]

DeepCopyAndResolveStrings.

Also, can we return scoped_ptr<base::Value> to make it clear that a new object
is being returned?

[aruslan, 2012/08/20 15:36:53]

Renamed to DeepCopyAndResolveStrings -- done.
I haven't changed the return type and added a comment instead.
(It's recursive, and handling scoped_ptr inside seems to be unnecessary
complicates the code.)
Let me know if you think otherwise.
On 2012/08/17 22:55:12, achuith.bhandarkar wrote:

+    const base::Value* node,
+    const base::DictionaryValue* strings) {
+  DCHECK(node);

[achuithb, 2012/08/17 22:55:12]

this DCHECK is pointless. Let's get rid of it.

[aruslan, 2012/08/20 15:36:53]

Done.

+  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();
+           it != list->end();
+           ++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);
+        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);
+      DCHECK(rv);
+      std::string actual_value;
+      if (!strings || !strings->GetString(value, &actual_value))
+        actual_value = value;
+      base::StringValue* copy = new base::StringValue(actual_value);

[achuithb, 2012/08/17 22:55:12]

This temporary seems unnecessary. Why not just
return new base::StringValue(actual_value);

[aruslan, 2012/08/20 15:36:53]

Done.

+      return copy;
+    }
+
+    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 +280,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 +318,35 @@ 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 != NULL);

[achuithb, 2012/08/17 22:55:12]

I think DCHECK(ppcopy) is sufficient.

[aruslan, 2012/08/20 15:36:53]

Done.

+    DCHECK(ppcopy->IsType(base::Value::TYPE_DICTIONARY));

[achuithb, 2012/08/17 22:55:12]

You might want to combine the DCHECKs like
DCHECK(ppcopy && ppcopy->IsType...)
otherwise this will crash anyway.

[aruslan, 2012/08/20 15:36:53]

Done.

+    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_);
 
@@ -437,14 +426,11 @@ void NotificationPromo::InitFromPrefs(PromoType promo_type) {
     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);

[achuithb, 2012/08/17 22:55:12]

let's do 
const base::DictionaryValue* dv = NULL;
as this seems to be the preferred construction syntax in chromium.

[aruslan, 2012/08/20 15:36:53]

Done.

+  if (ntp_promo->GetDictionary(kPrefPromoPayload, &dv)) {
+    DCHECK(dv != NULL);

[achuithb, 2012/08/17 22:55:12]

unnecessary

[aruslan, 2012/08/20 15:36:53]

Done.

+    promo_payload_.reset(dv->DeepCopy());
+  }
 
   ntp_promo->GetDouble(kPrefPromoStart, &start_);
   ntp_promo->GetDouble(kPrefPromoEnd, &end_);