promo_resource_service fixes/cleanup for promos.

chrome/browser/web_resource/promo_resource_service.cc

Index: chrome/browser/web_resource/promo_resource_service.cc
===================================================================
--- chrome/browser/web_resource/promo_resource_service.cc	(revision 97318)
+++ chrome/browser/web_resource/promo_resource_service.cc	(working copy)
@@ -48,10 +48,15 @@
 static const char kWebStoreLinkProperty[] = "inproduct";
 static const char kWebStoreExpireProperty[] = "tooltip";
 
+chrome::VersionInfo::Channel GetChannel() {
+  // GetChannel hits the registry on Windows. See http://crbug.com/70898.
+  base::ThreadRestrictions::ScopedAllowIO allow_io;
+  return chrome::VersionInfo::GetChannel();
+}
+
 }  // namespace
 
-// Server for dynamically loaded NTP HTML elements. TODO(mirandac): append
-// locale for future usage, when we're serving localizable strings.
+// Server for dynamically loaded NTP HTML elements.
 const char* PromoResourceService::kDefaultPromoResourceServer =
     "https://www.google.com/support/chrome/bin/topic/1142433/inproduct?hl=";
 
@@ -132,11 +137,8 @@
 }
 
 bool PromoResourceService::IsThisBuildTargeted(int builds_targeted) {
-  if (channel_ == chrome::VersionInfo::CHANNEL_UNKNOWN) {
-    // GetChannel hits the registry on Windows. See http://crbug.com/70898.
-    base::ThreadRestrictions::ScopedAllowIO allow_io;
-    channel_ = chrome::VersionInfo::GetChannel();
-  }
+  if (channel_ == chrome::VersionInfo::CHANNEL_UNKNOWN)
+    channel_ = GetChannel();
 
   return IsBuildTargeted(channel_, builds_targeted);
 }
@@ -202,28 +204,14 @@
 void PromoResourceService::UnpackPromoSignal(
     const DictionaryValue& parsed_json) {
   DictionaryValue* topic_dict;
-  ListValue* answer_list;
-  double old_promo_start = 0;
-  double old_promo_end = 0;
-  double promo_start = 0;
-  double promo_end = 0;
 
-  // Check for preexisting start and end values.
-  if (prefs_->HasPrefPath(prefs::kNTPPromoStart) &&
-      prefs_->HasPrefPath(prefs::kNTPPromoEnd)) {
-    old_promo_start = prefs_->GetDouble(prefs::kNTPPromoStart);
-    old_promo_end = prefs_->GetDouble(prefs::kNTPPromoEnd);
-  }
-
   // Check for newly received start and end values.
+  double promo_start = 0.0, promo_end = 0.0;
+  int time_slice_hrs = 0;
   if (parsed_json.GetDictionary("topic", &topic_dict)) {
+    ListValue* answer_list;
     if (topic_dict->GetList("answers", &answer_list)) {
-      std::string promo_start_string = "";
-      std::string promo_end_string = "";
-      std::string promo_string = "";
-      std::string promo_build = "";
-      int promo_build_type = 0;
-      int time_slice_hrs = 0;
+      std::string promo_start_string, promo_end_string;
       for (ListValue::const_iterator answer_iter = answer_list->begin();
            answer_iter != answer_list->end(); ++answer_iter) {
         if (!(*answer_iter)->IsType(Value::TYPE_DICTIONARY))
@@ -233,8 +221,10 @@
         std::string promo_signal;
         if (a_dic->GetString("name", &promo_signal)) {
           if (promo_signal == "promo_start") {
+            std::string promo_build;
             a_dic->GetString("question", &promo_build);
             size_t split = promo_build.find(":");
+            int promo_build_type = 0;
             if (split != std::string::npos &&
                 base::StringToInt(promo_build.substr(0, split),
                                   &promo_build_type) &&
@@ -253,8 +243,9 @@
               prefs_->SetInteger(prefs::kNTPPromoGroupTimeSlice, 0);
             }
             a_dic->GetString("inproduct", &promo_start_string);
-            a_dic->GetString("tooltip", &promo_string);
-            prefs_->SetString(prefs::kNTPPromoLine, promo_string);
+            std::string promo_line;
+            a_dic->GetString("tooltip", &promo_line);
+            prefs_->SetString(prefs::kNTPPromoLine, promo_line);
             srand(static_cast<uint32>(time(NULL)));
             prefs_->SetInteger(prefs::kNTPPromoGroup,
                                rand() % kNTPPromoGroupSize);
@@ -263,10 +254,8 @@
           }
         }
       }
-      if (!promo_start_string.empty() &&
-          promo_start_string.length() > 0 &&
-          !promo_end_string.empty() &&
-          promo_end_string.length() > 0) {
+
+      if (!promo_start_string.empty() && !promo_end_string.empty()) {
         base::Time start_time;
         base::Time end_time;
         if (base::Time::FromString(promo_start_string.c_str(), &start_time) &&
@@ -282,13 +271,21 @@
     }
   }
 
+  // Check for pre-existing start and end values.
+  double old_promo_start = 0.0, old_promo_end = 0.0;
+  if (prefs_->HasPrefPath(prefs::kNTPPromoStart) &&
+      prefs_->HasPrefPath(prefs::kNTPPromoEnd)) {
+    old_promo_start = prefs_->GetDouble(prefs::kNTPPromoStart);
+    old_promo_end = prefs_->GetDouble(prefs::kNTPPromoEnd);
+  }
+
   // If start or end times have changed, trigger a new web resource
   // notification, so that the logo on the NTP is updated. This check is
   // outside the reading of the web resource data, because the absence of
   // dates counts as a triggering change if there were dates before.
   // Also reset the promo closed preference, to signal a new promo.
-  if (!(old_promo_start == promo_start) ||
-      !(old_promo_end == promo_end)) {
+  if (old_promo_start != promo_start ||
+      old_promo_end != promo_end) {
     prefs_->SetDouble(prefs::kNTPPromoStart, promo_start);
     prefs_->SetDouble(prefs::kNTPPromoEnd, promo_end);
     prefs_->SetBoolean(prefs::kNTPPromoClosed, false);
@@ -453,30 +450,27 @@
   }
 }
 
-namespace PromoResourceServiceUtil {
-
-bool CanShowPromo(Profile* profile) {
-  bool promo_closed = false;
+bool PromoResourceService::CanShowPromo(Profile* profile) {

[jstritar, 2011/08/19 14:56:13]

What do you think of renaming this something more specific, since it's only used
for one type of promo handled by PromoResourceService?

[achuithb, 2011/08/19 20:16:14]

Do you have any suggestions? It looks like this class has a bunch of *Promo*
methods, *Logo* methods, and *WebStore* methods. CanShowPromo is specific to
Promo, so it seems consistent. I'm happy to change this if you can think of
something better.

[jstritar, 2011/08/22 18:06:39]

Hmmm... not sure. I guess this type of promo is called "Promo" so its fine for
now :).

   PrefService* prefs = profile->GetPrefs();
-  if (prefs->HasPrefPath(prefs::kNTPPromoClosed))
-    promo_closed = prefs->GetBoolean(prefs::kNTPPromoClosed);
 
-  // Only show if not synced.
-  bool is_synced =
-      (profile->HasProfileSyncService() &&
-          sync_ui_util::GetStatus(
-              profile->GetProfileSyncService()) == sync_ui_util::SYNCED);
+  // Check if promo has been closed by the user.
+  if (prefs->HasPrefPath(prefs::kNTPPromoClosed) &&
+      prefs->GetBoolean(prefs::kNTPPromoClosed))
+    return false;
 
-  bool is_promo_build = false;
-  if (prefs->HasPrefPath(prefs::kNTPPromoBuild)) {
-    // GetChannel hits the registry on Windows. See http://crbug.com/70898.
-    base::ThreadRestrictions::ScopedAllowIO allow_io;
-    chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel();
-    is_promo_build = PromoResourceService::IsBuildTargeted(
-        channel, prefs->GetInteger(prefs::kNTPPromoBuild));
-  }
+  // Check if our build is appropriate for this promo.
+  if (!prefs->HasPrefPath(prefs::kNTPPromoBuild) ||
+      !IsBuildTargeted(GetChannel(), prefs->GetInteger(prefs::kNTPPromoBuild)))
+    return false;

[jstritar, 2011/08/19 14:56:13]

If we want to avoid calling GetChannel() on the UI thread, you could probably
check it from within UnpackPromoSignal since that's run in the utility process,
and only set the prefs if the build is targeted.

[achuithb, 2011/08/19 20:16:14]

So I looked into doing this. CanShowPromo is a static, so either channel_ needs
to be static, or we need to store the boolean result of IsThisBuildTargeted in
Preferences.

I think making channel_ static makes a lot of sense, and then
IsThisBuildTargeted/set_channel also become statics. Unfortunately it looks like
channel_ is accessed from multiple threads, and even though channel_ would be
set only once, I didn't want to risk introducing any races since I'm not very
familiar with this code.

I don't know if you had a simpler solution in mind that I'm missing. 

While examing this, I noticed that data member web_resource_cache_ is unused, so
I deleted it.

[jstritar, 2011/08/22 18:06:39]

I was thinking something like what we do in UnpackWebstorePromo:

if (IsThisBuildTargeted(...)) {
  set promo prefs
}

UnpackPromoSignal sets prefs from a bunch of places, so it won't be that small
of a change. We can do this another time since you want to merge to M14.

 
-  return !promo_closed && !is_synced && is_promo_build;
-}
+  // Check if we are in the right time window for this promo.
+  if (!prefs->FindPreference(prefs::kNTPPromoStart) ||
+      !prefs->FindPreference(prefs::kNTPPromoEnd) ||
+      base::Time::FromDoubleT(prefs->GetDouble(prefs::kNTPPromoStart)) >
+          base::Time::Now() ||
+      base::Time::FromDoubleT(prefs->GetDouble(prefs::kNTPPromoEnd)) <
+          base::Time::Now())
+    return false;
 
-}  // namespace PromoResourceServiceUtil
+  return prefs->HasPrefPath(prefs::kNTPPromoLine);
+}