Remove promotion-type-specific JSON handling from NotificationPromo

chrome/browser/web_resource/notification_promo_mobile_ntp.h

+// 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.
+
+#ifndef CHROME_BROWSER_WEB_RESOURCE_NOTIFICATION_PROMO_MOBILE_NTP_H_
+#define CHROME_BROWSER_WEB_RESOURCE_NOTIFICATION_PROMO_MOBILE_NTP_H_
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "chrome/browser/web_resource/notification_promo.h"
+
+class Profile;
+
+namespace base {
+class DictionaryValue;
+class ListValue;
+}
+
+// Helper class for NotificationPromo that deals with mobile_ntp promos.
+class NotificationPromoMobileNtp {
+ public:
+  explicit NotificationPromoMobileNtp(Profile* profile_);
+  ~NotificationPromoMobileNtp();

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

nit: if this isn't virtual and just calling the default dtor, you can remove

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

Done.

+
+  // Initialize from prefs/JSON.
+  // Return true if the mobile NTP promotion is valid.
+  bool InitFromPrefs();
+  bool InitFromJson(const base::DictionaryValue& json);
+
+  // Return true if the promo is valid and can be shown.
+  bool CanShow() const;
+
+  bool valid() const { return valid_; }
+  const std::string& text() const { return text_; }
+  const std::string& text_long() const { return text_long_; }
+  const std::string& action_type() const { return action_type_; }
+  const base::ListValue* action_args() const { return action_args_; }
+  bool requires_mobile_only_sync() const { return requires_mobile_only_sync_; }
+  bool requires_sync() const { return requires_sync_; }
+  bool show_on_most_visited() const { return show_on_most_visited_; }
+  bool show_on_open_tabs() const { return show_on_open_tabs_; }
+  bool show_as_virtual_computer() const { return show_as_virtual_computer_; }
+  const std::string& virtual_computer_title() const {
+    return virtual_computer_title_;
+  }
+  const std::string& virtual_computer_lastsync() const {
+    return virtual_computer_lastsync_;
+  }
+  const base::DictionaryValue* payload() const { return payload_; }
+  const NotificationPromo& notification_promo() const {
+    return notification_promo_;
+  }
+
+ private:
+  bool InitFromNotificationPromo();
+

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

would it be worthwhile to make this a struct (so you could pass it around and/or
do promo->data()->field rather than having a bazillion getters)?  perhaps I just
hate how mixins work in C++ / how limited the language is wrt to literals.

[achuithb, 2012/08/22 19:51:50]

We could have a nested struct to hold all these variables - that's a good idea,
but I think we do need the outer class.

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

I like the idea; my attempt to implement it resulted in  not-so-pretty cruft
here, and the repetitive promo->data()->field instead of promo->field() on the
client side; neither looked pretty.  I'm not opposing the idea, though.

[Dan Beam, 2012/08/23 01:35:57]

OK, that's fine. C++ isn't a pretty language.

+  bool valid_;

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

it'd seem to be useful to document any of these (that are unique to this class)

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

Done.

+  std::string text_;
+  std::string text_long_;
+  std::string action_type_;
+  std::string virtual_computer_title_;
+  std::string virtual_computer_lastsync_;
+  bool requires_mobile_only_sync_;
+  bool requires_sync_;
+  bool show_on_most_visited_;
+  bool show_on_open_tabs_;
+  bool show_as_virtual_computer_;
+  const base::ListValue* action_args_;
+  const base::DictionaryValue* payload_;
+  NotificationPromo notification_promo_;
+
+  DISALLOW_COPY_AND_ASSIGN(NotificationPromoMobileNtp);
+};
+
+#endif  // CHROME_BROWSER_WEB_RESOURCE_NOTIFICATION_PROMO_MOBILE_NTP_H_