NotificationPromo

chrome/browser/web_resource/promo_notification.h

+// Copyright (c) 2011 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_PROMO_NOTIFICATION_H_
+#define CHROME_BROWSER_WEB_RESOURCE_PROMO_NOTIFICATION_H_
+#pragma once
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
+
+namespace base {
+  class DictionaryValue;
+}
+
+class PrefService;
+
+// Helper class for PromoResourceService that parses promo notification info
+// from json or prefs.
+class PromoNotification {

[jstritar, 2011/09/28 15:28:21]

naming nit: Do you think NotificationPromo would make more sense?
PromoNotification seems like a notification for the promo system, rather than an
actual promo. Up to you...

+ public:
+  class Delegate {
+   public:
+     virtual ~Delegate() {}
+     virtual void OnNewNotification(double start, double end) = 0;
+     // For testing.
+     virtual bool IsBuildAllowed(int builds_targeted) const { return false; }
+  };
+
+  explicit PromoNotification(PrefService* prefs, Delegate* delegate);
+
+  // Initialize from json/prefs.
+  void InitFromJson(const base::DictionaryValue& json);
+  void InitFromPrefs();
+
+  // Can this promo be shown?
+  bool CanShow() const;
+
+  static void RegisterUserPrefs(PrefService* prefs);

[jstritar, 2011/09/28 15:28:21]

nit: static methods should be defined above regular methods (here and in
private:)

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

[achuithb, 2011/09/29 02:00:50]

I don't see the rule in the Declaration Order link you posted? 

[jstritar, 2011/09/30 19:57:57]

Ah, interesting. Maybe this is just something we do in extensions code.

+
+  // Calculates promo notification start time with group-based time slice
+  // offset.
+  double StartTimeWithOffset() const;
+
+ private:
+  // For testing.
+  friend class PromoNotificationTestDelegate;
+  FRIEND_TEST_ALL_PREFIXES(PromoResourceServiceTest, GetNextQuestionValueTest);
+  FRIEND_TEST_ALL_PREFIXES(PromoResourceServiceTest, NewGroupTest);
+
+  // Users are randomly assigned to one of kMaxGroupSize + 1 buckets, in order
+  // to be able to roll out promos slowly, or display different promos to
+  // different groups.
+  static const int kMaxGroupSize = 99;

[jstritar, 2011/09/28 15:28:21]

Depending on what's needed for testing, you can probably put many of these
private static members & methods in the anonymous namespace. It's probably
better to only keep what's needed in the class definition.

[achuithb, 2011/09/29 02:00:50]

Yup, I'm using this variable in the unit tests so I have it here. I'll move the
other 2 to the .cc in an anonymous namespace.

+
+  // Maximum number of views.
+  static const int kMaxViews = 1000;
+
+  // Maximum number of hours for each time slice (4 weeks).
+  static const int kMaxTimeSliceHours = 24 * 7 * 4;
+
+  // Parse the answers array element. Set the data members of this instance
+  // and trigger OnNewNotification callback if necessary.
+  void Parse(const base::DictionaryValue* dict);
+
+  // Set promo notification params from a question string, which is of the form
+  // <build_type>:<time_slice>:<max_group>:<max_views>
+  void ParseParams(const base::DictionaryValue* dict);
+
+  // Check if this promo notification is new based on start/end times,
+  // and trigger events accordingly.
+  void CheckForNewNotification();
+
+  // Actions on receiving a new promo notification.
+  void OnNewNotification();
+
+  // Create a new promo notification group.
+  static int NewGroup();
+
+  // Returns an int converted from the question substring starting at index
+  // till the next colon. Sets index to the location right after the colon.
+  // Returns 0 if *err is true, and sets *err to true upon error.
+  static int GetNextQuestionValue(const std::string& question,
+                                  size_t* index,
+                                  bool* err);
+
+  // Time getters.
+  static double GetTimeFromDict(const base::DictionaryValue* dict);
+  double GetTimeFromPrefs(const char* pref) const;

[jstritar, 2011/09/28 15:28:21]

These could also be in the anonymous namespace. Something along these lines:

double GetTimeFromDict(const base::DictionaryValue* dict)
double GetTimeFromPrefs(PrefService* prefs, cont char* pref)

[achuithb, 2011/09/29 02:00:50]

Done. 

I was in half a mind whether these needed unit tests, but since I haven't
written any, I'll just put them in the anonymous namespace for now.

+
+  // Flush data members to prefs for storage.
+  void WritePrefs();
+
+  // Match our channel with specified build type.
+  bool IsBuildAllowed(int builds_allowed) const;
+
+  // For testing.
+  bool operator==(const PromoNotification& other) const;
+
+  PrefService* prefs_;
+  Delegate* delegate_;
+
+  double start_;
+  double end_;
+
+  int build_;
+  int time_slice_;
+  int max_group_;
+  int max_views_;
+
+  int group_;
+  int views_;
+  std::string text_;
+  bool closed_;
+
+  DISALLOW_COPY_AND_ASSIGN(PromoNotification);
+};
+
+#endif  // CHROME_BROWSER_WEB_RESOURCE_PROMO_NOTIFICATION_H_
+