Remove promotion-type-specific JSON handling from NotificationPromo

chrome/browser/web_resource/notification_promo.cc

 // 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 "chrome/browser/web_resource/notification_promo.h"
 
 #include <cmath>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 17 matching lines @@
 #endif  // defined(OS_ANDROID)
 
 using content::UserMetricsAction;
 
 namespace {
 
 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";
 const char kPrefPromoSegment[] = "segment";
 const char kPrefPromoIncrement[] = "increment";
 const char kPrefPromoIncrementFrequency[] = "increment_frequency";
 const char kPrefPromoIncrementMax[] = "increment_max";
 const char kPrefPromoMaxViews[] = "max_views";
 const char kPrefPromoGroup[] = "group";
 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)
   return "win";
 #elif defined(OS_IOS)
   // TODO(noyau): add iOS-specific implementation
   const bool isTablet = false;
   return std::string("ios-") + (isTablet ? "tablet" : "phone");
 #elif defined(OS_MACOSX)
   return "mac";
@@ skipping 48 matching lines @@
 // Convert PromoType to appropriate string.
 const char* PromoTypeToString(NotificationPromo::PromoType promo_type) {
   for (size_t i = 0; i < arraysize(kPromoMap); ++i) {
     if (kPromoMap[i].promo_type == promo_type)
       return kPromoMap[i].promo_type_str;
   }
   NOTREACHED();
   return "";
 }
 
 // TODO(achuith): remove this in m23.

[noyau (Ping after 24h), 2012/08/20 15:53:16]

It is probably time to remove this now. M22 has been split.

[achuithb, 2012/08/20 19:17:48]

My only concern is that people who skip m22 and go from m21 -> m23 will be left
with a whole bunch of garbage prefs on their disk. This code is doing no harm,
let's update the comment to m24 and remove it after one more cycle?

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

Done; bug http://crbug.com/143773 added.
On 2012/08/20 19:17:48, achuith.bhandarkar wrote:

 void ClearDeprecatedPrefs(PrefService* prefs) {
   prefs->RegisterStringPref(prefs::kNtpPromoLine,
                             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);
   prefs->RegisterDoublePref(prefs::kNtpPromoEnd,
                             0,
                             PrefService::UNSYNCABLE_PREF);
 
   prefs->RegisterIntegerPref(prefs::kNtpPromoNumGroups,
                              0,
                              PrefService::UNSYNCABLE_PREF);
@@ skipping 35 matching lines @@
   prefs->ClearPref(prefs::kNtpPromoIncrement);
   prefs->ClearPref(prefs::kNtpPromoGroupTimeSlice);
   prefs->ClearPref(prefs::kNtpPromoGroupMax);
   prefs->ClearPref(prefs::kNtpPromoViewsMax);
   prefs->ClearPref(prefs::kNtpPromoGroup);
   prefs->ClearPref(prefs::kNtpPromoViews);
   prefs->ClearPref(prefs::kNtpPromoClosed);
   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),
       initial_segment_(0),
       increment_(1),
       time_slice_(0),
       max_group_(0),
       max_views_(0),
       group_(0),
       views_(0),
@@ skipping 13 matching lines @@
   if (!json.GetList(PromoTypeToString(promo_type_), &promo_list)) {
     LOG(ERROR) << "Malformed JSON: not " << PromoTypeToString(promo_type_);
     return;
   }
 
   // No support for multiple promos yet. Only consider the first one.
   const DictionaryValue* promo = NULL;
   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)) {
     const DictionaryValue* date;
     if (date_list->GetDictionary(0, &date)) {
       std::string time_str;
       base::Time time;
       if (date->GetString("start", &time_str) &&
           base::Time::FromString(time_str.c_str(), &time)) {
         start_ = time.ToDoubleT();
@@ skipping 18 matching lines @@
     grouping->GetInteger("increment_frequency", &time_slice_);
     grouping->GetInteger("increment_max", &max_group_);
 
     DVLOG(1) << "num_groups_ = " << num_groups_
              << ", initial_segment_ = " << initial_segment_
              << ", increment_ = " << increment_
              << ", time_slice_ = " << time_slice_
              << ", 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_;
 
-    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();
 }
 
 void NotificationPromo::CheckForNewNotification() {
   NotificationPromo old_promo(profile_);
   old_promo.InitFromPrefs(promo_type_);
   const double old_start = old_promo.start_;
   const double old_end = old_promo.end_;
   const std::string old_promo_text = old_promo.promo_text_;
 
   new_notification_ =
       old_start != start_ || old_end != end_ || old_promo_text != promo_text_;
   if (new_notification_)
     OnNewNotification();
 }
 
 void NotificationPromo::OnNewNotification() {
   DVLOG(1) << "OnNewNotification";
   // Create a new promo group.
   group_ = base::RandInt(0, num_groups_ - 1);
   WritePrefs();
 }
 
 // static
 void NotificationPromo::RegisterUserPrefs(PrefService* prefs) {
   ClearDeprecatedPrefs(prefs);
-  prefs->RegisterDictionaryPref("promo",
+  prefs->RegisterDictionaryPref(kPrefPromoObject,
                                 new base::DictionaryValue,
                                 PrefService::UNSYNCABLE_PREF);
 }
 
 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_);
 
   ntp_promo->SetInteger(kPrefPromoNumGroups, num_groups_);
   ntp_promo->SetInteger(kPrefPromoSegment, initial_segment_);
   ntp_promo->SetInteger(kPrefPromoIncrement, increment_);
   ntp_promo->SetInteger(kPrefPromoIncrementFrequency, time_slice_);
   ntp_promo->SetInteger(kPrefPromoIncrementMax, max_group_);
 
   ntp_promo->SetInteger(kPrefPromoMaxViews, max_views_);
@@ skipping 12 matching lines @@
   prefs_->Set(kPrefPromoObject, promo_dict);
 }
 
 void NotificationPromo::InitFromPrefs(PromoType promo_type) {
   promo_type_ = promo_type;
   const base::DictionaryValue* promo_dict =
       prefs_->GetDictionary(kPrefPromoObject);
   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_);
 
   ntp_promo->GetInteger(kPrefPromoNumGroups, &num_groups_);
   ntp_promo->GetInteger(kPrefPromoSegment, &initial_segment_);
   ntp_promo->GetInteger(kPrefPromoIncrement, &increment_);
   ntp_promo->GetInteger(kPrefPromoIncrementFrequency, &time_slice_);
   ntp_promo->GetInteger(kPrefPromoIncrementMax, &max_group_);
 
@@ skipping 67 matching lines @@
   if (group_ < initial_segment_)
     return start_;
   return start_ +
       std::ceil(static_cast<float>(group_ - initial_segment_ + 1) / increment_)
       * time_slice_;
 }
 
 double NotificationPromo::EndTime() const {
   return end_;
 }