Only show G+ promo for users logged into G+

chrome/browser/web_resource/notification_promo.cc

Index: chrome/browser/web_resource/notification_promo.cc
===================================================================
--- chrome/browser/web_resource/notification_promo.cc	(revision 108876)
+++ chrome/browser/web_resource/notification_promo.cc	(working copy)
@@ -4,14 +4,20 @@
 
 #include "chrome/browser/web_resource/notification_promo.h"
 
+#include "base/bind.h"
 #include "base/rand_util.h"
 #include "base/string_number_conversions.h"
+#include "base/string_split.h"
 #include "base/time.h"
 #include "base/values.h"
 #include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/profiles/profile_impl.h"
 #include "chrome/browser/web_resource/promo_resource_service.h"
 #include "chrome/common/pref_names.h"
+#include "googleurl/src/gurl.h"
+#include "net/base/cookie_store.h"
 
+
 namespace {
 
 // Maximum number of views.
@@ -33,6 +39,10 @@
 static const char kTimeProperty[] = "inproduct";
 static const char kParamsProperty[] = "question";
 
+static const char kGPlusDomainUrl[] = "http://plus.google.com/";
+static const char kGPlusDomainSecureCookieId[] = "SID=";
+static const char kSplitStringToken = ';';
+
 // Time getters.
 double GetTimeFromDict(const DictionaryValue* dict) {
   std::string time_str;
@@ -52,9 +62,9 @@
 
 }  // namespace
 
-NotificationPromo::NotificationPromo(PrefService* prefs, Delegate* delegate)
-    : prefs_(prefs),
-      delegate_(delegate),
+NotificationPromo::NotificationPromo(Profile* profile, Delegate* delegate)
+    : delegate_(delegate),
+      profile_(profile),

[achuithb, 2011/11/16 20:33:11]

Maybe keep this order consistent with the order of the params? It's cleaner.
profile_ should be first.

[Cait (Slow), 2011/11/16 23:05:24]

Done.

       start_(0.0),
       end_(0.0),
       build_(0),
@@ -64,10 +74,16 @@
       group_(0),
       views_(0),
       text_(),
-      closed_(false) {
-  DCHECK(prefs);
+      closed_(false),
+      gplus_(false),
+      feature_mask_(0) {
+  DCHECK(profile);
+  prefs_ = profile_->GetPrefs();

[achuithb, 2011/11/16 20:33:11]

Could you please put this in the initializer list with the others?
profile_(profile),
prefs_(profile->GetPrefs()),
delegate_(delegate),
etc.

[Cait (Slow), 2011/11/16 23:05:24]

Done.

+  DCHECK(prefs_);
 }
 
+NotificationPromo::~NotificationPromo() {}
+
 void NotificationPromo::InitFromJson(const DictionaryValue& json) {
   DictionaryValue* dict;
   if (json.GetDictionary(kHeaderProperty, &dict)) {
@@ -82,9 +98,42 @@
     }
   }
 
-  CheckForNewNotification();
+  GetCookies(profile_->GetRequestContext());

[achuithb, 2011/11/16 20:33:11]

Could you please call PostTask to BrowserThread::IO here instead?

[Cait (Slow), 2011/11/16 23:05:24]

Done.

 }
 
+void NotificationPromo::GetCookiesCallback(const std::string& cookies) {
+  std::vector<std::string> cookie_list;
+  bool found_cookie = false;
+  base::SplitString(cookies, kSplitStringToken, &cookie_list);
+  for (std::vector<std::string>::iterator current = cookie_list.begin();

[achuithb, 2011/11/16 20:33:11]

Please use const_iterator instead.

[Cait (Slow), 2011/11/16 23:05:24]

Done.

+       current != cookie_list.end();
+       ++current) {
+    size_t position = (*current).find(kGPlusDomainSecureCookieId);
+    if (position == 0) {

[achuithb, 2011/11/16 20:33:11]

I think the position var is unnecessary:
if ((*current).find(...) == 0)

[Cait (Slow), 2011/11/16 23:05:24]

Done.

+      found_cookie = true;
+      break;
+    }
+  }
+  gplus_ = found_cookie;

[achuithb, 2011/11/16 20:33:11]

Please remove this line. It's not safe to do this (modify state of
NotificationPromo) on the IO thread, and you're doing this in
CheckForNewNotification already.

[Cait (Slow), 2011/11/16 23:05:24]

Done.

+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&NotificationPromo::CheckForNewNotification,
+                 base::Unretained(this), found_cookie));

[achuithb, 2011/11/16 20:33:11]

Please do not use base::Unretained here. 
base::Bind(&NotificationPromo::CheckForNewNotification, this, found_cookie);

[Cait (Slow), 2011/11/16 23:05:24]

Done.

+}
+
+void NotificationPromo::GetCookies(net::URLRequestContextGetter* getter) {
+  if (content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)) {

[achuithb, 2011/11/16 20:33:11]

Please replace this if with a DCHECK like:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO);

[Cait (Slow), 2011/11/16 23:05:24]

Done.

+    getter->GetURLRequestContext()->cookie_store()->
+        GetCookiesWithOptionsAsync(
+            GURL(kGPlusDomainUrl), net::CookieOptions(),
+            base::Bind(&NotificationPromo::GetCookiesCallback,
+                       base::Unretained(this)));

[achuithb, 2011/11/16 20:33:11]

Please do not use base::Unretained here; you should just do:
base::Bind(&NotificationPromo::GetCookiesCallback, this);

[Cait (Slow), 2011/11/16 23:05:24]

Done.

+  } else {
+    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,

[achuithb, 2011/11/16 20:33:11]

Please PostTask to the IO thread at the calling site. You don't need this code.

[Cait (Slow), 2011/11/16 23:05:24]

Done.

+        base::Bind(&NotificationPromo::GetCookies, base::Unretained(this),
+                   base::Unretained(getter)));
+  }
+}
+
 void NotificationPromo::Parse(const DictionaryValue* dict) {
   std::string key;
   if (dict->GetString(kIdentifierProperty, &key)) {
@@ -110,6 +159,8 @@
   time_slice_ = GetNextQuestionValue(question, &index, &err);
   max_group_ = GetNextQuestionValue(question, &index, &err);
   max_views_ = GetNextQuestionValue(question, &index, &err);
+  if (index < question.length())

[achuithb, 2011/11/16 20:33:11]

You don't need this. GetNextQuestionValue handles this case.

[Cait (Slow), 2011/11/16 23:05:24]

Done.

+    feature_mask_ = GetNextQuestionValue(question, &index, &err);
 
   if (err ||
       OutOfBounds(build_, PromoResourceService::NO_BUILD,
@@ -130,14 +181,17 @@
   }
 }
 
-void NotificationPromo::CheckForNewNotification() {
+void NotificationPromo::CheckForNewNotification(bool found_cookie) {
+  gplus_ = found_cookie;
   const double old_start = GetTimeFromPrefs(prefs_, prefs::kNTPPromoStart);
   const double old_end = GetTimeFromPrefs(prefs_, prefs::kNTPPromoEnd);
+  const bool old_gplus = prefs_->GetBoolean(prefs::kNTPPromoIsLoggedInToPlus);
   const bool has_views = prefs_->HasPrefPath(prefs::kNTPPromoViewsMax);
-
+  const int old_feature_mask = prefs_->GetInteger(prefs::kNTPPromoFeatureMask);
   // Trigger a new notification if the times have changed, or if
   // we previously never wrote out a max_views preference.
-  if (old_start != start_ || old_end != end_ || !has_views)
+  if (old_start != start_ || old_end != end_ || !has_views
+      || old_gplus != gplus_ || old_feature_mask != feature_mask_)
     OnNewNotification();
 }
 
@@ -187,9 +241,21 @@
   prefs->RegisterBooleanPref(prefs::kNTPPromoClosed,
                              false,
                              PrefService::UNSYNCABLE_PREF);
+  prefs->RegisterBooleanPref(prefs::kNTPPromoIsLoggedInToPlus,
+                             false,
+                             PrefService::UNSYNCABLE_PREF);
+  prefs->RegisterIntegerPref(prefs::kNTPPromoFeatureMask,
+                             0,
+                             PrefService::UNSYNCABLE_PREF);
 }
 
+// static
+NotificationPromo* NotificationPromo::Factory(Profile *profile,
+    NotificationPromo::Delegate * delegate) {
+  return new NotificationPromo(profile, delegate);
+}
 
+
 void NotificationPromo::WritePrefs() {
   prefs_->SetDouble(prefs::kNTPPromoStart, start_);
   prefs_->SetDouble(prefs::kNTPPromoEnd, end_);
@@ -203,6 +269,8 @@
   prefs_->SetInteger(prefs::kNTPPromoGroup, group_);
   prefs_->SetInteger(prefs::kNTPPromoViews, views_);
   prefs_->SetBoolean(prefs::kNTPPromoClosed, closed_);
+  prefs_->SetBoolean(prefs::kNTPPromoIsLoggedInToPlus, gplus_);
+  prefs_->SetInteger(prefs::kNTPPromoFeatureMask, feature_mask_);
 }
 
 void NotificationPromo::InitFromPrefs() {
@@ -235,6 +303,12 @@
 
   if (prefs_->HasPrefPath(prefs::kNTPPromoClosed))
     closed_ = prefs_->GetBoolean(prefs::kNTPPromoClosed);
+
+  if (prefs_->HasPrefPath(prefs::kNTPPromoIsLoggedInToPlus))
+    gplus_ = prefs_->GetBoolean(prefs::kNTPPromoIsLoggedInToPlus);
+
+  if (prefs_->HasPrefPath(prefs::kNTPPromoFeatureMask))
+    feature_mask_ = prefs_->GetInteger(prefs::kNTPPromoFeatureMask);
 }
 
 bool NotificationPromo::CanShow() const {
@@ -244,7 +318,8 @@
       views_ < max_views_ &&
       IsBuildAllowed(build_) &&
       base::Time::FromDoubleT(StartTimeWithOffset()) < base::Time::Now() &&
-      base::Time::FromDoubleT(end_) > base::Time::Now();
+      base::Time::FromDoubleT(end_) > base::Time::Now() &&
+      ((feature_mask_ & !gplus_) == 0));
 }
 
 void NotificationPromo::HandleClosed() {
@@ -305,5 +380,6 @@
          group_ == other.group_ &&
          views_ == other.views_ &&
          text_ == other.text_ &&
-         closed_ == other.closed_;
+         closed_ == other.closed_ &&
+         gplus_ == other.gplus_;

[achuithb, 2011/11/16 20:33:11]

feature_mask_ is needed here too.

[Cait (Slow), 2011/11/16 23:05:24]

Done.

 }