promo_resource_service fixes/cleanup for promos.

chrome/browser/web_resource/promo_resource_service.cc

 // 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.
 
 #include "chrome/browser/web_resource/promo_resource_service.h"
 
 #include "base/string_number_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
@@ skipping 30 matching lines @@
 // to versions with different types of promos).
 static const int kPromoServiceVersion = 2;
 
 // Properties used by the server.
 static const char kAnswerIdProperty[] = "answer_id";
 static const char kWebStoreHeaderProperty[] = "question";
 static const char kWebStoreButtonProperty[] = "inproduct_target";
 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=";
 
 // static
 void PromoResourceService::RegisterPrefs(PrefService* local_state) {
   local_state->RegisterIntegerPref(prefs::kNTPPromoVersion, 0);
   local_state->RegisterStringPref(prefs::kNTPPromoLocale, std::string());
 }
 
 // static
@@ skipping 60 matching lines @@
   Init();
 }
 
 PromoResourceService::~PromoResourceService() { }
 
 void PromoResourceService::Init() {
   ScheduleNotificationOnInit();
 }
 
 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);
 }
 
 void PromoResourceService::Unpack(const DictionaryValue& parsed_json) {
   UnpackLogoSignal(parsed_json);
   UnpackPromoSignal(parsed_json);
   UnpackWebStoreSignal(parsed_json);
 }
 
@@ skipping 45 matching lines @@
 }
 
 std::string PromoResourceService::GetPromoLocale() {
   PrefService* local_state = g_browser_process->local_state();
   return local_state->GetString(prefs::kNTPPromoLocale);
 }
 
 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))
           continue;
         DictionaryValue* a_dic =
             static_cast<DictionaryValue*>(*answer_iter);
         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) &&
                 base::StringToInt(promo_build.substr(split+1),
                                   &time_slice_hrs) &&
                 promo_build_type >= 0 &&
                 promo_build_type <= (DEV_BUILD | BETA_BUILD | STABLE_BUILD) &&
                 time_slice_hrs >= 0 &&
                 time_slice_hrs <= kMaxTimeSliceHours) {
               prefs_->SetInteger(prefs::kNTPPromoBuild, promo_build_type);
               prefs_->SetInteger(prefs::kNTPPromoGroupTimeSlice,
                                  time_slice_hrs);
             } else {
               // If no time data or bad time data are set, do not show promo.
               prefs_->SetInteger(prefs::kNTPPromoBuild, NO_BUILD);
               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);
           } else if (promo_signal == "promo_end") {
             a_dic->GetString("inproduct", &promo_end_string);
           }
         }
       }
-      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) &&
             base::Time::FromString(promo_end_string.c_str(), &end_time)) {
           // Add group time slice, adjusted from hours to seconds.
           promo_start = start_time.ToDoubleT() +
               (prefs_->FindPreference(prefs::kNTPPromoGroup) ?
                   prefs_->GetInteger(prefs::kNTPPromoGroup) *
                       time_slice_hrs * 60 * 60 : 0);
           promo_end = end_time.ToDoubleT();
         }
       }
     }
   }
 
+  // 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);
     ScheduleNotification(promo_start, promo_end);
   }
 }
 
 void PromoResourceService::UnpackWebStoreSignal(
     const DictionaryValue& parsed_json) {
   DictionaryValue* topic_dict;
@@ skipping 144 matching lines @@
       !(old_logo_end == logo_end)) {
     prefs_->SetDouble(prefs::kNTPCustomLogoStart, logo_start);
     prefs_->SetDouble(prefs::kNTPCustomLogoEnd, logo_end);
     NotificationService* service = NotificationService::current();
     service->Notify(chrome::NOTIFICATION_PROMO_RESOURCE_STATE_CHANGED,
                     Source<WebResourceService>(this),
                     NotificationService::NoDetails());
   }
 }
 
-namespace PromoResourceServiceUtil {
+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();
 
-bool CanShowPromo(Profile* profile) {
-  bool promo_closed = false;
-  PrefService* prefs = profile->GetPrefs();
-  if (prefs->HasPrefPath(prefs::kNTPPromoClosed))
-    promo_closed = prefs->GetBoolean(prefs::kNTPPromoClosed);
+  // Check if promo has been closed by the user.
+  if (prefs->HasPrefPath(prefs::kNTPPromoClosed) &&
+      prefs->GetBoolean(prefs::kNTPPromoClosed))
+    return false;
 
-  // Only show if not synced.
-  bool is_synced =
-      (profile->HasProfileSyncService() &&
-          sync_ui_util::GetStatus(
-              profile->GetProfileSyncService()) == sync_ui_util::SYNCED);
+  // 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.

 
-  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 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;
 
-  return !promo_closed && !is_synced && is_promo_build;
+  return prefs->HasPrefPath(prefs::kNTPPromoLine);
 }
-
-}  // namespace PromoResourceServiceUtil