promo_resource_service fixes/cleanup for promos.

chrome/browser/web_resource/promo_resource_service.h

Index: chrome/browser/web_resource/promo_resource_service.h
===================================================================
--- chrome/browser/web_resource/promo_resource_service.h	(revision 97899)
+++ chrome/browser/web_resource/promo_resource_service.h	(working copy)
@@ -11,16 +11,11 @@
 #include "chrome/browser/web_resource/web_resource_service.h"
 #include "chrome/common/chrome_version_info.h"
 
+namespace base {
+  class DictionaryValue;
+}
+
 class Profile;
-
-namespace PromoResourceServiceUtil {
-
-// Certain promotions should only be shown to certain classes of users. This
-// function will change to reflect each kind of promotion.
-bool CanShowPromo(Profile* profile);
-
-}  // namespace PromoResourceServiceUtil
-
 class PrefService;
 
 // A PromoResourceService fetches data from a web resource server to be used to
@@ -34,8 +29,8 @@
 class PromoResourceService
     : public WebResourceService {
  public:
-  static bool IsBuildTargeted(chrome::VersionInfo::Channel channel,
-                              int builds_targeted);
+  // Checks for conditions to show promo: start/end times, channel, etc.
+  static bool CanShowPromo(Profile* profile);
 
   static void RegisterPrefs(PrefService* local_state);
 
@@ -47,6 +42,7 @@
   static const char* kDefaultPromoResourceServer;
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(PromoResourceServiceTest, IsBuildTargeted);
   FRIEND_TEST_ALL_PREFIXES(PromoResourceServiceTest, UnpackLogoSignal);
   FRIEND_TEST_ALL_PREFIXES(PromoResourceServiceTest, UnpackPromoSignal);
   FRIEND_TEST_ALL_PREFIXES(PromoResourceServiceTest, UnpackWebStoreSignal);
@@ -72,6 +68,9 @@
 
   void Init();
 
+  static bool IsBuildTargeted(chrome::VersionInfo::Channel channel,
+                              int builds_targeted);
+
   // Returns true if |builds_targeted| includes the release channel Chrome
   // belongs to. For testing purposes, you can override the current channel
   // with set_channel.
@@ -101,7 +100,7 @@
   //       {
   //         "answer_id": "1067976",
   //         "name": "promo_start",
-  //         "question": "1:24",
+  //         "question": "1:24:10",
   //         "tooltip":
   //       "Click \u003ca href=http://www.google.com\u003ehere\u003c/a\u003e!",
   //         "inproduct": "10/8/09 12:00",
@@ -127,10 +126,11 @@
   // promotional line is given by the "inproduct" line.
   // For "promo_start", the promotional line itself is given in the "tooltip"
   // field. The "question" field gives the type of builds that should be shown
-  // this promo (see the BuildType enum in web_resource_service.cc) and the
-  // number of hours that each promo group should see it, separated by ":".
-  // For example, "7:24" would indicate that all builds should see the promo,
-  // and each group should see it for 24 hours.
+  // this promo (see the BuildType enum in web_resource_service.cc), the
+  // number of hours that each promo group should see it, and the maximum promo
+  // group that should see it, separated by ":".
+  // For example, "7:24:5" would indicate that for 24 hours, all builds and all

[jstritar, 2011/08/24 20:13:37]

Are you sure this is how it works? I thought we did a ramp up, like this:

Day 1: group 1
Day 2: groups 1, 2
Day 3: groups 1, 2, 3
Day 4: groups 1, 2, 3, 4
Day 5: groups 1, 2, 3, 4, 5

[Miranda Callahan, 2011/08/25 08:23:38]

Jon's comment reflects the way the code works, which I think is correct.
Obviously, a very small group time-slice means we could effectively make the
code behave in the way that Achuith describes.

+  // groups with ids less than 5 should see the promo.
   //
   void UnpackPromoSignal(const base::DictionaryValue& parsed_json);
 
@@ -192,10 +192,42 @@
   //   answer_id: the promo's id
   void UnpackWebStoreSignal(const base::DictionaryValue& parsed_json);
 
-  // Gets mutable dictionary attached to user's preferences, so that we
-  // can write resource data back to user's pref file.
-  base::DictionaryValue* web_resource_cache_;
+  // Parse the answers array element.
+  void ParsePromo(base::DictionaryValue* a_dic,
+                   std::string* promo_start_string,

[jstritar, 2011/08/24 20:13:37]

nit: indent

+                   std::string* promo_end_string);
 
+  // Set promo params from a question string, which is of the form
+  // <build_type>:<time_slice>:<max_group>.
+  void SetPromoParams(base::DictionaryValue* a_dic);
+
+  // Extract the promo text from the tooltip string.
+  void SetPromoLine(base::DictionaryValue* a_dic);
+
+  // Check if this promo is new based on start/end times,
+  // and trigger events accordingly.
+  void CheckForNewPromo(const std::string& promo_start_string,
+                        const std::string& promo_end_string);
+
+  // Calculate the promo times, taking into account our group, and the
+  // group time slice.
+  void CalculateNewPromoTimes(const std::string& promo_start_string,
+                              const std::string& promo_end_string,
+                              double* promo_start,
+                              double* promo_end);
+
+  // Calculates promo start time with group-based time slice offset.
+  static double GetPromoStartTime(PrefService* prefs);
+
+  // Create a new promo group.
+  int ResetPromoGroup();
+
+  // Get saved promo times.
+  void GetCurrentPromoTimes(double* old_promo_start, double* old_promo_end);
+
+  // Actions on receiving a new promo.
+  void OnNewPromo(double promo_start, double promo_end);

[jstritar, 2011/08/24 20:13:37]

This makes the parsing easier to read but I'm not sure this is the right place
for these methods. This class handles multiple types of promos, but these
methods are specific to the one type, and they're named very generically. If
this is used more in the future, maybe we need some sort of delegate class--
when you add a new type of promo you write a small delegate that implements
Unpack, and register it with this service? Maybe there's something simple you
could do now to keep it somewhat grouped? If these methods applied to any promo
(e.g., the user group got assigned per Chrome client and other types of promos
could use it too) then it would make more sense for me.

+
   // Overrides the current Chrome release channel for testing purposes.
   chrome::VersionInfo::Channel channel_;