Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8520009:
Only show G+ promo for users logged into G+ (Closed)
|
Created:
9 years, 1 month ago by Cait (Slow) Modified:
9 years ago Reviewers:
achuithb CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionOnly show G+ promo for users logged into G+
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112225
Patch Set 1 : '' #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 34
Patch Set 4 : '' #
Total comments: 23
Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 45
Patch Set 7 : '' #
Total comments: 18
Patch Set 8 : '' #
Total comments: 2
Patch Set 9 : '' #Messages
Total messages: 12 (0 generated)
Please let me know if you have any questions. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.cc (right): http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:67: profile_(profile), Maybe keep this order consistent with the order of the params? It's cleaner. profile_ should be first. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:81: prefs_ = profile_->GetPrefs(); Could you please put this in the initializer list with the others? profile_(profile), prefs_(profile->GetPrefs()), delegate_(delegate), etc. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:101: GetCookies(profile_->GetRequestContext()); Could you please call PostTask to BrowserThread::IO here instead? http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:108: for (std::vector<std::string>::iterator current = cookie_list.begin(); Please use const_iterator instead. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:112: if (position == 0) { I think the position var is unnecessary: if ((*current).find(...) == 0) http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:117: gplus_ = found_cookie; 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. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:120: base::Unretained(this), found_cookie)); Please do not use base::Unretained here. base::Bind(&NotificationPromo::CheckForNewNotification, this, found_cookie); http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:124: if (content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)) { Please replace this if with a DCHECK like: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO); http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:129: base::Unretained(this))); Please do not use base::Unretained here; you should just do: base::Bind(&NotificationPromo::GetCookiesCallback, this); http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:131: content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, Please PostTask to the IO thread at the calling site. You don't need this code. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:162: if (index < question.length()) You don't need this. GetNextQuestionValue handles this case. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:384: gplus_ == other.gplus_; feature_mask_ is needed here too. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.h (right): http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:61: NotificationPromo(Profile* profile, Delegate* delegate); please make these private (the ctor, friend decl and dtor. I think friend declaration comes before ctor). http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:76: Please add an enum here like: enum FEATURE { NO_FEATURE = 0, FEATURE_GPLUS = 1, }; Please get rid of the gplus_ variable and preference. You can add an accessor like: bool gplus() const { return feature_mask_ & FEATURE_GPLUS; } http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:116: Profile* profile_; I think profile_ should be the first data member http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... File chrome/browser/web_resource/promo_resource_service.cc (right): http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service.cc:219: NotificationPromo* notification_promo = You are leaking NotificationPromo here. Please do: scoped_refptr<NotificationPromo> notification_promo = NotificationPromo::Factory(profile_, this); http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service.cc:225: NotificationPromo* notification_promo = You are leaking NotificationPromo here. Please do: scoped_refptr<NotificationPromo> notification_promo = NotificationPromo::Factory(profile_, this);
I'm a little unclear on how we will know whether or not a cookie was found if we get rid of the gplus_ variable (see inline). http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.cc (right): http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:67: profile_(profile), On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > Maybe keep this order consistent with the order of the params? It's cleaner. > profile_ should be first. Done. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:81: prefs_ = profile_->GetPrefs(); On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > Could you please put this in the initializer list with the others? > profile_(profile), > prefs_(profile->GetPrefs()), > delegate_(delegate), > etc. Done. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:101: GetCookies(profile_->GetRequestContext()); On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > Could you please call PostTask to BrowserThread::IO here instead? Done. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:108: for (std::vector<std::string>::iterator current = cookie_list.begin(); On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > Please use const_iterator instead. Done. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:112: if (position == 0) { On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > I think the position var is unnecessary: > if ((*current).find(...) == 0) Done. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:117: gplus_ = found_cookie; On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > 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. Done. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:120: base::Unretained(this), found_cookie)); On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > Please do not use base::Unretained here. > base::Bind(&NotificationPromo::CheckForNewNotification, this, found_cookie); Done. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:124: if (content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)) { On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > Please replace this if with a DCHECK like: > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO); Done. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:129: base::Unretained(this))); On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > Please do not use base::Unretained here; you should just do: > base::Bind(&NotificationPromo::GetCookiesCallback, this); Done. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:131: content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > Please PostTask to the IO thread at the calling site. You don't need this code. Done. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:162: if (index < question.length()) On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > You don't need this. GetNextQuestionValue handles this case. Done. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:384: gplus_ == other.gplus_; On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > feature_mask_ is needed here too. Done. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.h (right): http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:61: NotificationPromo(Profile* profile, Delegate* delegate); On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > please make these private (the ctor, friend decl and dtor. I think friend > declaration comes before ctor). Done. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:76: On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > Please add an enum here like: > enum FEATURE { > NO_FEATURE = 0, > FEATURE_GPLUS = 1, > }; > > Please get rid of the gplus_ variable and preference. You can add an accessor > like: > bool gplus() const { return feature_mask_ & FEATURE_GPLUS; } I agree that this is a cleaner way to go, but it seems like the gplus_ variable is still needed to record whether or not we found a G+ cookie? http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:116: Profile* profile_; On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > I think profile_ should be the first data member Done. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... File chrome/browser/web_resource/promo_resource_service.cc (right): http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service.cc:219: NotificationPromo* notification_promo = On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > You are leaking NotificationPromo here. Please do: > scoped_refptr<NotificationPromo> notification_promo = > NotificationPromo::Factory(profile_, this); > Done. http://codereview.chromium.org/8520009/diff/21001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service.cc:225: NotificationPromo* notification_promo = On 2011/11/16 20:33:11, achuith.bhandarkar wrote: > You are leaking NotificationPromo here. Please do: > scoped_refptr<NotificationPromo> notification_promo = > NotificationPromo::Factory(profile_, this); Done.
http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.cc (right): http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:177: gplus_ = found_cookie; Instead of this, you would do: if (found_cookie) feature_mask_ |= FEATURE_GPLUS; http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:180: const bool old_gplus = prefs_->GetBoolean(prefs::kNTPPromoIsLoggedInToPlus); don't need this. http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:181: const bool has_views = prefs_->HasPrefPath(prefs::kNTPPromoViewsMax); don't need this line either. http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:184: // we previously never wrote out a max_views preference. please change this comment: s/max_views/feature_mask
http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.cc (right): http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:177: gplus_ = found_cookie; Ok, I see the problem. I'm sorry, I'm re-using feature_mask_. gplus_ = found_cookie; is ok. http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:186: || old_gplus != gplus_ || old_feature_mask != feature_mask_) Just old_feature_mask should be sufficient. http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:314: ((feature_mask_ & !gplus_) == 0); I think this condition should be: (feature_mask_ & FEATURE_GPLUS) == !gplus_ http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.h (right): http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:13: #include "net/url_request/url_request_context.h" Shouldn't this be in the cc file? You're forward declaring net::URLRequestContextGetter http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:25: // Helper class for PromoResourceService that parses promo notification info newline above this line please. http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:38: // Static factory for creating new promo objects. Perhaps change this to 'Static factory for creating new notification promos'. We have a number of different promo objects. http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:60: protected: Remove these lines http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:93: void GetCookiesCallback(const std::string& cookies); Switch the order of GetCookies and GetCookiesCallback and add comments please.
http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.cc (right): http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:180: const bool old_gplus = prefs_->GetBoolean(prefs::kNTPPromoIsLoggedInToPlus); On 2011/11/16 23:17:33, achuith.bhandarkar wrote: > don't need this. I had this in here to force a refresh of the promo if the users gplus status changed (i.e. they weren't logged in before, but now they are, or vice versa). Otherwise, when we update cookies, the new gplus value will get set, but the promo won't get updated until the cache is invalidated for a another reason. http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:181: const bool has_views = prefs_->HasPrefPath(prefs::kNTPPromoViewsMax); On 2011/11/16 23:17:33, achuith.bhandarkar wrote: > don't need this line either. Done. http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:184: // we previously never wrote out a max_views preference. On 2011/11/16 23:17:33, achuith.bhandarkar wrote: > please change this comment: > s/max_views/feature_mask Done. http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:186: || old_gplus != gplus_ || old_feature_mask != feature_mask_) On 2011/11/16 23:27:03, achuith.bhandarkar wrote: > Just old_feature_mask should be sufficient. Done. http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:314: ((feature_mask_ & !gplus_) == 0); On 2011/11/16 23:27:03, achuith.bhandarkar wrote: > I think this condition should be: > (feature_mask_ & FEATURE_GPLUS) == !gplus_ Done. http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.h (right): http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:13: #include "net/url_request/url_request_context.h" On 2011/11/16 23:27:03, achuith.bhandarkar wrote: > Shouldn't this be in the cc file? You're forward declaring > net::URLRequestContextGetter Done. http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:25: // Helper class for PromoResourceService that parses promo notification info On 2011/11/16 23:27:03, achuith.bhandarkar wrote: > newline above this line please. Done. http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:38: // Static factory for creating new promo objects. On 2011/11/16 23:27:03, achuith.bhandarkar wrote: > Perhaps change this to 'Static factory for creating new notification promos'. We > have a number of different promo objects. Done. http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:60: protected: On 2011/11/16 23:27:03, achuith.bhandarkar wrote: > Remove these lines Done. http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:93: void GetCookiesCallback(const std::string& cookies); On 2011/11/16 23:27:03, achuith.bhandarkar wrote: > Switch the order of GetCookies and GetCookiesCallback and add comments please. Done.
http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.cc (right): http://codereview.chromium.org/8520009/diff/24001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:180: const bool old_gplus = prefs_->GetBoolean(prefs::kNTPPromoIsLoggedInToPlus); On 2011/11/17 00:05:59, caitkp wrote: > On 2011/11/16 23:17:33, achuith.bhandarkar wrote: > > don't need this. > > I had this in here to force a refresh of the promo if the users gplus status > changed (i.e. they weren't logged in before, but now they are, or vice versa). > Otherwise, when we update cookies, the new gplus value will get set, but the > promo won't get updated until the cache is invalidated for a another reason. Ah, ok. That makes sense. Could you add a comment noting this?
Achuith, Please have a look when you get a chance. The existing unit tests are working again and I added a couple new ones for the cookies. I was having difficulty posting tasks from the IO thread to the UI thread (and vice versa) in the unit tests, so I refactored a bit so that cookies could be fed in from the unit test (similar to what is done with the json). My only concern with this approach is that it does not test the end to end async sequence (InitFromJson -> GetCookies ->GetCookiesCallback -> CheckForNewNotification). If you know of a way to do this, I'm open to suggestions. I tried an IN_PROC_BROWSER_TEST, but that didn't seem to work either.
Thanks! Let me know if you have any questions! http://codereview.chromium.org/8520009/diff/31001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right): http://codereview.chromium.org/8520009/diff/31001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:69: NotificationPromo* notification_promo = NotificationPromo::Factory( This is a leak. Could you please use scoped_refptr<NotificationPromo> here? http://codereview.chromium.org/8520009/diff/31001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:76: NotificationPromo* notification_promo = NotificationPromo::Factory( scoped_refptr http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.cc (right): http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:112: std::vector<std::string> cookie_list; Could you please add a DCHECK for the IO thread here? http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:125: bool found_cookie = CheckForGPlusCookie(cookies); Could you please add a DCHECK for the IO thread here? http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:194: const double old_end = GetTimeFromPrefs(prefs_, prefs::kNTPPromoEnd); Add 2 more variables here: double start = 0.0, end = 0.0; Replace lines 205-206 with start = StartTimeWithOffset(); end = end_; Get rid of lines 207-211 and replace with if (delegate_) delegate_->OnNotificationParsed(start, end); I think this is a bit cleaner? http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:195: const bool has_platform = prefs_->HasPrefPath(prefs::kNTPPromoPlatform); Don't think has_platform is necessary any longer... http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:322: (!(feature_mask_ & NotificationPromo::FEATURE_GPLUS) || gplus_); I think it's better if the feature_mask has the inverse effect, ie, if the feature_mask is set to GPLUS, only show the promo if gplus_ is false. So this logic should become: !((feature_mask_ & NotificationPromo::FEATURE_GPLUS) && gplus_); ie, do not show the promo if feature mask has FEATURE_GPLUs, and the cookie is set. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.h (right): http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:42: static NotificationPromo* Factory(Profile* profile, Delegate* delegate); I think Create is more common than Factory in the chromium codebase. Sorry if I suggested the name Factory before. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:43: delete unnecessary newline please. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:92: remove unnecessary newline please. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:99: // <build_type>:<time_slice>:<max_group>:<max_views> Please fix this comment. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:117: bool CheckForGPlusCookie(const std::string& cookies); Please make this function static. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... File chrome/browser/web_resource/promo_resource_service.h (right): http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service.h:149: // additional group sees the promo every 24 hours. Fix this comment please. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... File chrome/browser/web_resource/promo_resource_service_unittest.cc (right): http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:201: received_notification_(false), please initialize should_receive_notification_ here. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:290: void ParseAndCallCookies(const DictionaryValue& json, You don't need this function at all. You're mainly testing CheckForGplusCookie, so write some tests that pass in different cookie strings, and check the return value. Eg: EXPECT_TRUE(NotificationPromo::CheckForGPlusCookie("SID=12346")); EXPECT_FALSE(NotificationPromo::CheckForGPlusCookie("WARNING=123456"); Also, there's a lot of copy/paste from NotificationPromo::InitFromJson, which should be avoided. What am I missing? http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:583: notification_promo_->feature_mask_ = 1; Please use the value NotificationPromo::FEATURE_GPLUS instead. It's clearer and less fragile. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:584: EXPECT_FALSE(notification_promo_->CanShow()); I would EXPECT_TRUE here, based on the inversion of logic for this feature. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:644: " \"inproduct\": \"31/01/12 01:00 GMT\"" Could you please update this date? This test is going to start failing in 2+ months. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:717: TEST_F(PromoResourceServiceTest, NotificationPromoCookieTest) { Please delete this test - it has a lot of copy/paste. Replace it with a test that just tests CheckForGPlusCookie. If you want to exercise the ability for the cookie to not trigger a promo, try to integrate it with NotificationPromoTest instead. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:768: } Delete this test too. It's a bit difficult to maintain all the copy/paste code here.
http://codereview.chromium.org/8520009/diff/31001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right): http://codereview.chromium.org/8520009/diff/31001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:69: NotificationPromo* notification_promo = NotificationPromo::Factory( On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > This is a leak. Could you please use scoped_refptr<NotificationPromo> here? Done. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:76: NotificationPromo* notification_promo = NotificationPromo::Factory( On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > scoped_refptr Done. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.cc (right): http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:112: std::vector<std::string> cookie_list; On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > Could you please add a DCHECK for the IO thread here? Right now I'm calling this method directly in my unit tests--if I add this check, I'll have to post to the IO thread when I call it in the unit tests...Is it sufficient to do the DCHECK in GetCookies and GetCookiesCallback since this method doesn't actually deal with reading or writing cookies (only processing a string of cookies)? http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:125: bool found_cookie = CheckForGPlusCookie(cookies); On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > Could you please add a DCHECK for the IO thread here? Done. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:194: const double old_end = GetTimeFromPrefs(prefs_, prefs::kNTPPromoEnd); On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > Add 2 more variables here: > double start = 0.0, end = 0.0; > Replace lines 205-206 with > start = StartTimeWithOffset(); > end = end_; > Get rid of lines 207-211 and replace with > if (delegate_) > delegate_->OnNotificationParsed(start, end); > > I think this is a bit cleaner? Done. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:195: const bool has_platform = prefs_->HasPrefPath(prefs::kNTPPromoPlatform); On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > Don't think has_platform is necessary any longer... Really? All the platform-related code showed up when I synced last week... http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:322: (!(feature_mask_ & NotificationPromo::FEATURE_GPLUS) || gplus_); On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > I think it's better if the feature_mask has the inverse effect, ie, if the > feature_mask is set to GPLUS, only show the promo if gplus_ is false. So this > logic should become: > !((feature_mask_ & NotificationPromo::FEATURE_GPLUS) && gplus_); > ie, do not show the promo if feature mask has FEATURE_GPLUs, and the cookie is > set. If feature_mask_ is set to GPLUS, this means we are targeting gplus users, and only want to show the promo if the cookie is set (meaning they are logged in to gplus, so gplus_ = true) http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.h (right): http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:42: static NotificationPromo* Factory(Profile* profile, Delegate* delegate); On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > I think Create is more common than Factory in the chromium codebase. Sorry if I > suggested the name Factory before. Done. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:43: On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > delete unnecessary newline please. Done. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:92: On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > remove unnecessary newline please. Done. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:99: // <build_type>:<time_slice>:<max_group>:<max_views> On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > Please fix this comment. Done. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:117: bool CheckForGPlusCookie(const std::string& cookies); On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > Please make this function static. Done. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... File chrome/browser/web_resource/promo_resource_service.h (right): http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service.h:149: // additional group sees the promo every 24 hours. On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > Fix this comment please. Done. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... File chrome/browser/web_resource/promo_resource_service_unittest.cc (right): http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:201: received_notification_(false), On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > please initialize should_receive_notification_ here. Done. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:290: void ParseAndCallCookies(const DictionaryValue& json, The purpose of this code is to set the promo values from json w/o triggering an async cookie fetch or a call directly to CheckForNewNotification (as gplus_ has to be set first). Your idea of integrating with NotificationPromoTest is much cleaner and avoids this, though. So I'll do that. On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > You don't need this function at all. You're mainly testing CheckForGplusCookie, > so write some tests that pass in different cookie strings, and check the return > value. Eg: > EXPECT_TRUE(NotificationPromo::CheckForGPlusCookie("SID=12346")); > EXPECT_FALSE(NotificationPromo::CheckForGPlusCookie("WARNING=123456"); > > Also, there's a lot of copy/paste from NotificationPromo::InitFromJson, which > should be avoided. What am I missing? http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:583: notification_promo_->feature_mask_ = 1; On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > Please use the value NotificationPromo::FEATURE_GPLUS instead. It's clearer and > less fragile. Done. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:584: EXPECT_FALSE(notification_promo_->CanShow()); We want to show the promo if the user is already logged in to gplus, so it should not show if no cookie is found. On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > I would EXPECT_TRUE here, based on the inversion of logic for this feature.
I apologize for all the rounds of changes. Please bear with me. I think it's pretty close. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.cc (right): http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:112: std::vector<std::string> cookie_list; On 2011/11/28 19:06:25, caitkp wrote: > On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > > Could you please add a DCHECK for the IO thread here? > > Right now I'm calling this method directly in my unit tests--if I add this > check, I'll have to post to the IO thread when I call it in the unit tests...Is > it sufficient to do the DCHECK in GetCookies and GetCookiesCallback since this > method doesn't actually deal with reading or writing cookies (only processing a > string of cookies)? Ok, that's fine. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:195: const bool has_platform = prefs_->HasPrefPath(prefs::kNTPPromoPlatform); The platform code is still there. We only need to write out the preferences when we notice that the preference related to the most recently added client feature (feature_mask in this case) hasn't been written out. So you should delete this line, and add a test for feature_mask instead (you don't need to test that old_feature_mask doesn't match). http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:198: const int old_feature_mask = prefs_->GetInteger(prefs::kNTPPromoFeatureMask); This should be: const bool has_feature_mask = prefs_->HasPrefPath(prefs::kNTPPromoFeatureMask); http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:203: old_feature_mask != feature_mask_ || !has_platform) { This line should be !has_feature_mask) { http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:322: (!(feature_mask_ & NotificationPromo::FEATURE_GPLUS) || gplus_); On 2011/11/28 19:06:25, caitkp wrote: > On 2011/11/27 07:48:26, achuith.bhandarkar wrote: > > I think it's better if the feature_mask has the inverse effect, ie, if the > > feature_mask is set to GPLUS, only show the promo if gplus_ is false. So this > > logic should become: > > !((feature_mask_ & NotificationPromo::FEATURE_GPLUS) && gplus_); > > ie, do not show the promo if feature mask has FEATURE_GPLUs, and the cookie > is > > set. > If feature_mask_ is set to GPLUS, this means we are targeting gplus users, and > only want to show the promo if the cookie is set (meaning they are logged in to > gplus, so gplus_ = true) Ok, I misunderstood the use of this feature. I assumed we were targeting non-gplus users to get them to sign up. Could you please double check to make sure we only care about signed-in users for this promo, and that we'll never need to target non-signed-in users? http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.h (right): http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:88: enum FEATURE { I believe enums should appear before static data members, so could you please move this to right after PlatformType? http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:115: // Parse cookies in search of a SID= value. nit: extra space before Parse. http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... File chrome/browser/web_resource/promo_resource_service_unittest.cc (right): http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:258: // Test if notification received Period at the end of the comment (not sure if this comment is necessary). http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:290: void SetFeatureMaskToGPlus() { I don't think this function is necessary if you create a separate TestGPlus function as I suggest below. http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:295: void RunCookieTests(const DictionaryValue& json, Could you rename this to TestCookie or TestGPlusCookie for consistency? Also, you're not using the first argument json at all, so please just remove it. http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:313: EXPECT_EQ(prefs_->GetInteger(prefs::kNTPPromoFeatureMask), feature_mask_); Is there a way to also test CanShow() here? shouldn't it be equal to should_find_cookie? http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:564: // If no feature mask, gplus_ value is ignored. Could you move this sub-test (no feature mask, gplus_ false) to be the final case in TestFeatureMask? That way you leave notification_promo_ the way you found it. In this current implementation, you have a side effect of setting gplus_ to true and feature_mask_ to FEATURE_GPLUS. If this test was commented out, some succeeding tests could fail. http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:664: delegate.SetFeatureMaskToGPlus(); could you move lines 664-675 into a separate function? Call it TestGplus perhaps? At the end of that function, please reset the feature_mask_ to 0 and set gplus_ to false to reset notification_promo_ to the same state as you found it. http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:675: EXPECT_FALSE(notification_promo->CanShow()); Could you reverse the order so you test failing cookies, then passing cookies? That way, when somebody adds more tests, notification_promo->CanShow() will continue to be true (as it is at the end of all the previous tests).
http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.cc (right): http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:195: const bool has_platform = prefs_->HasPrefPath(prefs::kNTPPromoPlatform); On 2011/11/28 21:21:22, achuith.bhandarkar wrote: > The platform code is still there. We only need to write out the preferences when > we notice that the preference related to the most recently added client feature > (feature_mask in this case) hasn't been written out. > > So you should delete this line, and add a test for feature_mask instead (you > don't need to test that old_feature_mask doesn't match). Done. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:198: const int old_feature_mask = prefs_->GetInteger(prefs::kNTPPromoFeatureMask); On 2011/11/28 21:21:22, achuith.bhandarkar wrote: > This should be: > const bool has_feature_mask = prefs_->HasPrefPath(prefs::kNTPPromoFeatureMask); Done. http://codereview.chromium.org/8520009/diff/31001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:203: old_feature_mask != feature_mask_ || !has_platform) { On 2011/11/28 21:21:22, achuith.bhandarkar wrote: > This line should be > !has_feature_mask) { Done. http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.h (right): http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:88: enum FEATURE { On 2011/11/28 21:21:22, achuith.bhandarkar wrote: > I believe enums should appear before static data members, so could you please > move this to right after PlatformType? Done. http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.h:115: // Parse cookies in search of a SID= value. On 2011/11/28 21:21:22, achuith.bhandarkar wrote: > nit: extra space before Parse. Done. http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... File chrome/browser/web_resource/promo_resource_service_unittest.cc (right): http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:258: // Test if notification received On 2011/11/28 21:21:22, achuith.bhandarkar wrote: > Period at the end of the comment (not sure if this comment is necessary). Done. http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:290: void SetFeatureMaskToGPlus() { On 2011/11/28 21:21:22, achuith.bhandarkar wrote: > I don't think this function is necessary if you create a separate TestGPlus > function as I suggest below. Done. http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:295: void RunCookieTests(const DictionaryValue& json, On 2011/11/28 21:21:22, achuith.bhandarkar wrote: > Could you rename this to TestCookie or TestGPlusCookie for consistency? Also, > you're not using the first argument json at all, so please just remove it. Done. http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:313: EXPECT_EQ(prefs_->GetInteger(prefs::kNTPPromoFeatureMask), feature_mask_); On 2011/11/28 21:21:22, achuith.bhandarkar wrote: > Is there a way to also test CanShow() here? shouldn't it be equal to > should_find_cookie? Done. http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:564: // If no feature mask, gplus_ value is ignored. On 2011/11/28 21:21:22, achuith.bhandarkar wrote: > Could you move this sub-test (no feature mask, gplus_ false) to be the final > case in TestFeatureMask? That way you leave notification_promo_ the way you > found it. In this current implementation, you have a side effect of setting > gplus_ to true and feature_mask_ to FEATURE_GPLUS. If this test was commented > out, some succeeding tests could fail. Done. http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:664: delegate.SetFeatureMaskToGPlus(); On 2011/11/28 21:21:22, achuith.bhandarkar wrote: > could you move lines 664-675 into a separate function? Call it TestGplus > perhaps? At the end of that function, please reset the feature_mask_ to 0 and > set gplus_ to false to reset notification_promo_ to the same state as you found > it. Done. http://codereview.chromium.org/8520009/diff/39001/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:675: EXPECT_FALSE(notification_promo->CanShow()); On 2011/11/28 21:21:22, achuith.bhandarkar wrote: > Could you reverse the order so you test failing cookies, then passing cookies? > That way, when somebody adds more tests, notification_promo->CanShow() will > continue to be true (as it is at the end of all the previous tests). Done.
LGTM with nits. Thanks for your patience! http://codereview.chromium.org/8520009/diff/40010/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.cc (right): http://codereview.chromium.org/8520009/diff/40010/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:205: // we previously never wrote out a feature_mask or platform preference, or nit: also fix this comment. http://codereview.chromium.org/8520009/diff/40010/chrome/browser/web_resource... File chrome/browser/web_resource/promo_resource_service_unittest.cc (right): http://codereview.chromium.org/8520009/diff/40010/chrome/browser/web_resource... chrome/browser/web_resource/promo_resource_service_unittest.cc:257: nit: don't think this newline is necessary. |
