Remove promotion-type-specific JSON handling from NotificationPromo

chrome/browser/web_resource/notification_promo_mobile_ntp.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_mobile_ntp.h"
+
+#include "base/logging.h"

[Dan Beam, 2012/08/22 19:43:27]

probably don't need this any more

[aruslan, 2012/08/23 00:49:59]

Thanks! Done.

+#include "base/values.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/web_resource/notification_promo.h"
+
+
+NotificationPromoMobileNtp::NotificationPromoMobileNtp(Profile* profile)
+  : valid_(false),
+    action_args_(NULL),
+    payload_(NULL),
+    notification_promo_(profile) {
+}
+
+NotificationPromoMobileNtp::~NotificationPromoMobileNtp() {}
+
+bool NotificationPromoMobileNtp::InitFromPrefs() {
+  notification_promo_.InitFromPrefs(NotificationPromo::MOBILE_NTP_SYNC_PROMO);
+  return InitFromNotificationPromo();
+}
+
+bool NotificationPromoMobileNtp::InitFromJson(
+    const base::DictionaryValue& json) {
+  notification_promo_.InitFromJson(
+      json, NotificationPromo::MOBILE_NTP_SYNC_PROMO);
+  return InitFromNotificationPromo();
+}
+
+bool NotificationPromoMobileNtp::CanShow() const {
+  return valid() && notification_promo_.CanShow();
+}
+
+bool NotificationPromoMobileNtp::InitFromNotificationPromo() {
+  valid_ = false;
+  requires_mobile_only_sync_ = true;
+  requires_sync_ = true;
+  show_on_most_visited_ = false;
+  show_on_open_tabs_ = true;
+  show_as_virtual_computer_ = true;
+  action_args_ = NULL;
+
+  payload_ = notification_promo_.promo_payload();
+  if (!payload_ ||
+      !payload_->GetString("promo_message_short", &text_) ||
+      !payload_->GetString("promo_message_long", &text_long_) ||
+      !payload_->GetString("promo_action_type", &action_type_) ||
+      !payload_->GetList("promo_action_args", &action_args_) ||
+      !action_args_)

[Dan Beam, 2012/08/22 19:43:27]

nit: curlies (when conditional or body is over 1 line)

[aruslan, 2012/08/23 00:49:59]

Done.

+    return valid_;

[achuithb, 2012/08/22 19:49:20]

Let's return false here. No need to be indirect.

[aruslan, 2012/08/23 00:49:59]

Done.

+
+  // All the fields above are required.
+  // All the fields below are optional and used to override the defaults.

[achuithb, 2012/08/22 19:49:20]

Which comment is true?

[aruslan, 2012/08/23 00:49:59]

Both; I changed the comments to be more comprehensible.

+  valid_ = true;

[Dan Beam, 2012/08/22 19:43:27]

what happens if any of the next payload->Get*() calls fail? is this still valid?

[aruslan, 2012/08/23 00:49:59]

Yes; I changed the comments to make it more apparent.

+
+  payload_->GetBoolean("promo_requires_mobile_only_sync",
+      &requires_mobile_only_sync_);

[achuithb, 2012/08/22 19:49:20]

let's line this up with "

[aruslan, 2012/08/23 00:49:59]

Done.

+  payload_->GetBoolean("promo_requires_sync", &requires_sync_);
+  payload_->GetBoolean("promo_show_on_most_visited", &show_on_most_visited_);
+  payload_->GetBoolean("promo_show_on_open_tabs", &show_on_open_tabs_);
+  payload_->GetBoolean("promo_show_as_virtual_computer",
+      &show_as_virtual_computer_);

[achuithb, 2012/08/22 19:49:20]

let's line this up with the "

[aruslan, 2012/08/23 00:49:59]

Done.

+  payload_->GetString("promo_virtual_computer_title", &virtual_computer_title_);
+  payload_->GetString("promo_virtual_computer_lastsync",
+      &virtual_computer_lastsync_);

[achuithb, 2012/08/22 19:49:20]

let's line this up with "

[aruslan, 2012/08/23 00:49:59]

Done.

+
+  return valid_;
+}