promo_resource_service fixes/cleanup for promos.

promo_resource_service fixes/cleanup for promos.

Remove is_synced check in CanShowPromo.
Add support for additional question param max_group_number.
Fix a bug where group was reset everytime the promo resource was fetched.
Breakup UnpackPromoSignal to a number of smaller helper functions
    for clarity, and so they can be individually unit-tested.
Put all the logic of whether to display a promo into CanShowPromo.
Get rid of unnecessary namespace PromoResourceServiceUtil.
Make IsBuildTargeted private.
Make GetChannel a private function in anonymous scope 
    to consolidate duplicated comments/code.
Delete unused web_resource_cache_.

BUG=93201
TEST=Only relevant change is removal of is_synced check in CanShowPromo.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101979

[achuithb, 2011-08-18 18:24:05]

Please review. Thanks!

[Miranda Callahan, 2011-08-19 07:52:46]

On 2011/08/18 18:24:05, achuith.bhandarkar wrote:
> Please review. Thanks!

This elegant refactoring really delights me -- LGTM!

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

Thanks for the cleanup! LGTM

http://codereview.chromium.org/7655008/diff/9002/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/7655008/diff/9002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.cc:453: bool
PromoResourceService::CanShowPromo(Profile* profile) {
What do you think of renaming this something more specific, since it's only used
for one type of promo handled by PromoResourceService?

http://codereview.chromium.org/7655008/diff/9002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.cc:464: return false;
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]

Awaiting comments from Jon.

Thanks Miranda and Jon for your feedback and LGTMs.

http://codereview.chromium.org/7655008/diff/9002/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/7655008/diff/9002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.cc:453: bool
PromoResourceService::CanShowPromo(Profile* profile) {
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.

http://codereview.chromium.org/7655008/diff/9002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.cc:464: return false;
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]

LGTM still.

http://codereview.chromium.org/7655008/diff/9002/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/7655008/diff/9002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.cc:453: bool
PromoResourceService::CanShowPromo(Profile* profile) {
On 2011/08/19 20:16:14, achuith.bhandarkar wrote:
> 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.

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

http://codereview.chromium.org/7655008/diff/9002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.cc:464: return false;
On 2011/08/19 20:16:14, achuith.bhandarkar wrote:
> 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.

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.

[achuithb, 2011-08-23 01:29:18]

In retrospect, I should've probably checked in at the previous LGTM point and
created a fresh CL.

Most of the changes are in promo_resource_service.cc.

I break out UnpackPromoSignal into a bunch of helper functions for purposes of
readability and testability. I'm planning to write more unit tests and having
these functions will help.

I'm convinced there was a bug where the testing group was reset every time a new
promo resource was downloaded. This effectively made it so a different set of
users saw the promo every two days. Please correct me if I'm wrong. 

I added another param to the 'question' field for the max promo group, which
also limits the users who get to see the promo. It's also easier to add more
params to 'question' in the future.

It looks like everything is working as expected in my testing, but this
definitely needs more unit tests. I'd like to do that in a separate CL.

Thanks!

[Miranda Callahan, 2011-08-23 07:59:52]

On 2011/08/23 01:29:18, achuith.bhandarkar wrote:
> In retrospect, I should've probably checked in at the previous LGTM point and
> created a fresh CL.
> 
> Most of the changes are in promo_resource_service.cc.
> 
> I break out UnpackPromoSignal into a bunch of helper functions for purposes of
> readability and testability. I'm planning to write more unit tests and having
> these functions will help.
> 
> I'm convinced there was a bug where the testing group was reset every time a
new
> promo resource was downloaded. This effectively made it so a different set of
> users saw the promo every two days. Please correct me if I'm wrong. 

You're correct -- very good catch!

> 
> I added another param to the 'question' field for the max promo group, which
> also limits the users who get to see the promo. It's also easier to add more
> params to 'question' in the future.

Excellent.

> 
> It looks like everything is working as expected in my testing, but this
> definitely needs more unit tests. I'd like to do that in a separate CL.
> 
> Thanks!

[Miranda Callahan, 2011-08-23 08:32:04]

On 2011/08/23 07:59:52, Miranda Callahan wrote:
> On 2011/08/23 01:29:18, achuith.bhandarkar wrote:
> > In retrospect, I should've probably checked in at the previous LGTM point
and
> > created a fresh CL.
> > 
> > Most of the changes are in promo_resource_service.cc.
> > 
> > I break out UnpackPromoSignal into a bunch of helper functions for purposes
of
> > readability and testability. I'm planning to write more unit tests and
having
> > these functions will help.
> > 
> > I'm convinced there was a bug where the testing group was reset every time a
> new
> > promo resource was downloaded. This effectively made it so a different set
of
> > users saw the promo every two days. Please correct me if I'm wrong. 
> 
> You're correct -- very good catch!
> 
> > 
> > I added another param to the 'question' field for the max promo group, which
> > also limits the users who get to see the promo. It's also easier to add more
> > params to 'question' in the future.
> 
> Excellent.
> 
> > 
> > It looks like everything is working as expected in my testing, but this
> > definitely needs more unit tests. I'd like to do that in a separate CL.
> > 
> > Thanks!

LGTM again -- thanks for the great simplifying refactorings. Yes please, several
more unit tests before this gets merged to 14, if that happens.

[Miranda Callahan, 2011-08-23 08:33:31]

Actually, a couple of nits -- the explanation of the fields in the .h file also
needs to be updated.

http://codereview.chromium.org/7655008/diff/19021/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service.h (right):

http://codereview.chromium.org/7655008/diff/19021/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:103: //         "question":
"1:24",
Could you add the new max group element to this example?

http://codereview.chromium.org/7655008/diff/19021/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:132: // and each group
should see it for 24 hours.
Here, too, a comment explaining the third field?

[achuithb, 2011-08-23 21:38:55]

I've made one more significant change, fixed a couple of nits, and added to the
unit test.

The nits are the changes to comments (thanks for pointing those out - I missed
them), and having the default promo group be 0 instead of -1. CanShowPromo can
get called before we've fetched the promo page, and group -1 was wreaking havoc
on the calculations and comparisons. 

The big change is that I store the promo_start without the group time slice
offset added in. This is still accurate for detecting if a promo has changed
based on new start/end times. Whenever we need the actual promo_start time (in
ScheduleNotification() and CanShowPromo()), we calculate it using
start/group/timeslice values with the new static function GetPromoStartTime().

The reason I need to do this is because we decide to create a new group if the
start time changes, but the start time was dependent on the group number,
creating a circular dependency and an ordering problem. There was also an
initialization problem because the group number would be invalid the first time
we did the start time comparison. I think it's cleaner this way. Patchset #11
had the same bug as the original code - where a new group would be generated
every time we downloaded the promo page.

The unit test now unpacks the same json a second time and checks to ensure that
a new group was not generated.

Only promo_resource_service.h, promo_resource_service.cc and
promo_resource_unittest.cc have changed. You can compare the latest patchset
with #11 to see the changes since your last LGTM.

Thanks again for taking the time to review this, and bearing with me through all
the iterations. I'm pretty happy with this CL now, and will be working on a
minimal patch for M14 next (this patch is entirely too big to be merged into
M14).

http://codereview.chromium.org/7655008/diff/19021/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service.h (right):

http://codereview.chromium.org/7655008/diff/19021/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:103: //         "question":
"1:24",
On 2011/08/23 08:33:31, Miranda Callahan wrote:
> Could you add the new max group element to this example?

Done.

http://codereview.chromium.org/7655008/diff/19021/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:132: // and each group
should see it for 24 hours.
On 2011/08/23 08:33:31, Miranda Callahan wrote:
> Here, too, a comment explaining the third field?

Done.

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

http://codereview.chromium.org/7655008/diff/25002/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/7655008/diff/25002/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:411: // the server, and this
promo string exists, set the localized string.
This comment isn't all that relevant anymore... maybe update the text to
something less specific?

http://codereview.chromium.org/7655008/diff/25002/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service.h (right):

http://codereview.chromium.org/7655008/diff/25002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:132: // For example,
"7:24:5" would indicate that for 24 hours, all builds and all
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

http://codereview.chromium.org/7655008/diff/25002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:197: std::string*
promo_start_string,
nit: indent

http://codereview.chromium.org/7655008/diff/25002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:229: void OnNewPromo(double
promo_start, double promo_end);
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.

[Miranda Callahan, 2011-08-25 08:23:29]

Ok, I went through and drew out a state diagram of this code, just as a sanity
check, and I think I see a weak spot (though nothing major). I think we're
setting the promo params and the promo line in the wrong place:

UnpackPromoSignal
  ParsePromo(Dictionary)
    SetPromoParams: kNTPPromoBuild, kNTPPromoGroupTimeSlice, kNTPPromoGroupMax
    SetPromoLine: kNTPPromoLine
  CheckForNewPromo
    CalculateNewPromoTimes
    if new times != old times:
      OnNewPromo
        calculate random promo group
        reset: kNTPPromoClosed, *Start, *End
        schedule notification

It seems to me that in SetPromoParams and SetPromoLine we should save the new
values in temporary slots, and only change the actual preferences in the
OnNewPromo method -- otherwise we're changing the preferences every time the
promo is read, which I think is unnecessary, and could potentially lead to odd
behavior. I propose instead something like this, which resets all prefs only in
the OnNewPromo method:

UnpackPromoSignal
  ParsePromo(Dictionary)
    SetPromoParams: save temp values for build, timeslice, group max 
    SetPromoLine: save temp value for promo line
  CheckForNewPromo
    CalculateNewPromoTimes
    if new times != old times:
      OnNewPromo
        calculate random promo group
        reset: kNTPPromoClosed, *Start, *End
                  kNTPPromoBuild, kNTPPromoGroupTimeSlice, kNTPPromoGroupMax
                  kNTPPromoLine
        schedule notification

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

http://codereview.chromium.org/7655008/diff/25002/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service.h (right):

http://codereview.chromium.org/7655008/diff/25002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:132: // For example,
"7:24:5" would indicate that for 24 hours, all builds and all
On 2011/08/24 20:13:37, jstritar wrote:
> 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

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.

[achuithb, 2011-08-25 22:30:56]

This is going to be on hold for a day or two as I pursue a M14 stable blocker.
I'll have another patch in a few days. Thanks for all the suggestions!

[Miranda Callahan, 2011-08-26 07:37:08]

On 2011/08/25 22:30:56, achuith.bhandarkar wrote:
> This is going to be on hold for a day or two as I pursue a M14 stable blocker.
> I'll have another patch in a few days. Thanks for all the suggestions!

No problem. One more suggestion, actually -- Min Li mentioned in her email
something that I think we've left out here; it used to be in the code, but was
dropped somewhere along the way -- that is the idea of capping the number of
times the promo is shown. We used to keep a user preference -- something like
kNTPPromoDisplays -- and incremented it every time the NTPResourceCache
populated the promo line. If that number exceeded a certain value (probably good
if we could set on the server, as usual), we would mark the promo closed. And
then at the point we reset the "closed" value, just reset that display number to
0.

[achuithb, 2011-09-12 18:43:52]

I'm finally finished with the crasher that has kept me occupied the last two
weeks.

I'm going to have to start working on some code yellow bugs next, before I can
come back to working on this some more.

I've filed a bug crbug.com/96290 to track 3 of the items that came up in your
comments:
1. Need to sync promo data to preferences only when a new promo is detected.
2. Support a cap in the number of times a promo is shown.
3. more unittests.

If I'm understanding Jon correctly, he objects to the use of the term 'promo' as
I'm using it because it's too general. I should probably use something more
specific like perhaps 'ad'? I've updated the functions that I added as a part of
this CL to use ad in place of promo. Please correct me if I did not understand
you correctly, or if you'd prefer a different term, Jon. The term ad could be
politically undesirable. There's still a LOT of references to 'promo', in param
names, preferences, etc. Some, like preferences, cannot be changed. The others,
I can look to change in the next round of refactoring.

I think it's somewhat important to get this CL  landed, and merged into M15
since we need promo support in M15.

I'm happy to fix nits and any shortcomings that you believe are not acceptable
for landing this CL. I feel we should defer big ticket items for a subsequent CL
and make a note of it in 96290. What do you both think?

[Miranda Callahan, 2011-09-13 07:35:52]

On 2011/09/12 18:43:52, achuith.bhandarkar wrote:
> I'm finally finished with the crasher that has kept me occupied the last two
> weeks.
> 
> I'm going to have to start working on some code yellow bugs next, before I can
> come back to working on this some more.
> 
> I've filed a bug crbug.com/96290 to track 3 of the items that came up in your
> comments:
> 1. Need to sync promo data to preferences only when a new promo is detected.
> 2. Support a cap in the number of times a promo is shown.
> 3. more unittests.
> 
> If I'm understanding Jon correctly, he objects to the use of the term 'promo'
as
> I'm using it because it's too general. I should probably use something more
> specific like perhaps 'ad'? I've updated the functions that I added as a part
of
> this CL to use ad in place of promo. Please correct me if I did not understand
> you correctly, or if you'd prefer a different term, Jon. The term ad could be
> politically undesirable. There's still a LOT of references to 'promo', in
param
> names, preferences, etc. Some, like preferences, cannot be changed. The
others,
> I can look to change in the next round of refactoring.
> 
> I think it's somewhat important to get this CL  landed, and merged into M15
> since we need promo support in M15.
> 
> I'm happy to fix nits and any shortcomings that you believe are not acceptable
> for landing this CL. I feel we should defer big ticket items for a subsequent
CL
> and make a note of it in 96290. What do you both think?

Completely agree with deferring big-ticket items for a subsequent CL; though I
think that capping shows should be in M15 (even if in a separate CL), as it's
not difficult and will probably be needed for the promo to the stable channel. 

As far as the naming goes, yeah -- I had the feeling Jon wanted a separation of
terminology for the three possible types of promos; so instead of "CanShowPromo"
it should be "CanShowNotificationPromo", or something like that.  A helpful
categorization might be:

WebStorePromo
LogoPromo
NotificationPromo

for the three types of Promos that can be shown by the PromoResourceService. I
think the only real problem now is that the "NotificationPromo" (I'm calling it
that because it's a promo that is displayed in the NTP notification line) isn't
referred to in the code as anything other than a "promo", when really it's a
specific category of promo. Does that make sense?

[achuithb, 2011-09-15 04:39:41]

> Completely agree with deferring big-ticket items for a subsequent CL; though I
> think that capping shows should be in M15 (even if in a separate CL), as it's
> not difficult and will probably be needed for the promo to the stable channel.

> 
> As far as the naming goes, yeah -- I had the feeling Jon wanted a separation
of
> terminology for the three possible types of promos; so instead of
"CanShowPromo"
> it should be "CanShowNotificationPromo", or something like that.  A helpful
> categorization might be:
> 
> WebStorePromo
> LogoPromo
> NotificationPromo
> 
> for the three types of Promos that can be shown by the PromoResourceService. I
> think the only real problem now is that the "NotificationPromo" (I'm calling
it
> that because it's a promo that is displayed in the NTP notification line)
isn't
> referred to in the code as anything other than a "promo", when really it's a
> specific category of promo. Does that make sense?

Miranda - totally makes sense, thanks for the explanation. I like Notification.
I've renamed the methods to use Notification instead of Ad or Promo.

I don't want to add the capping feature to this CL because it's not completely
trivial. I have to create a new pref and pass the value back from javascript. I
think a separate CL (to be merged into M15) would be more appropriate.

If you feel comfortable with this CL, please LGTM it so I can land and get
started on the next CL. I'm almost done with my code yellow responsibilities and
would like to start on the remaining work items for promo soon...

[Miranda Callahan, 2011-09-15 08:18:41]

On 2011/09/15 04:39:41, achuith.bhandarkar wrote:
> > Completely agree with deferring big-ticket items for a subsequent CL; though
I
> > think that capping shows should be in M15 (even if in a separate CL), as
it's
> > not difficult and will probably be needed for the promo to the stable
channel.
> 
> > 
> > As far as the naming goes, yeah -- I had the feeling Jon wanted a separation
> of
> > terminology for the three possible types of promos; so instead of
> "CanShowPromo"
> > it should be "CanShowNotificationPromo", or something like that.  A helpful
> > categorization might be:
> > 
> > WebStorePromo
> > LogoPromo
> > NotificationPromo
> > 
> > for the three types of Promos that can be shown by the PromoResourceService.
I
> > think the only real problem now is that the "NotificationPromo" (I'm calling
> it
> > that because it's a promo that is displayed in the NTP notification line)
> isn't
> > referred to in the code as anything other than a "promo", when really it's a
> > specific category of promo. Does that make sense?
> 
> Miranda - totally makes sense, thanks for the explanation. I like
Notification.
> I've renamed the methods to use Notification instead of Ad or Promo.
> 
> I don't want to add the capping feature to this CL because it's not completely
> trivial. I have to create a new pref and pass the value back from javascript.
I
> think a separate CL (to be merged into M15) would be more appropriate.
> 
> If you feel comfortable with this CL, please LGTM it so I can land and get
> started on the next CL. I'm almost done with my code yellow responsibilities
and
> would like to start on the remaining work items for promo soon...

Thanks for the thorough answer, and that's fine -- LGTM.

[Miranda Callahan, 2011-09-15 08:21:30]

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:235: void
PromoResourceService::UnpackAdSignal(
nit:UnpackNotificationSignal

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service.h (right):

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:142: void
UnpackAdSignal(const base::DictionaryValue& parsed_json);
nit: could you change this to UnpackNotificationSignal?

[jstritar, 2011-09-15 15:42:27]

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:421: if
(PromoResourceService::CanShowPromo(profile_)) {
nit: CanShowNotificationPromo

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:258: void
PromoResourceService::ParseNotification(DictionaryValue* a_dic,
nit: DictionaryValue* a_dic should be on the next line, indented with the
others. or they could all line up under DictionaryValue*

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:286: promo_build >
(DEV_BUILD | BETA_BUILD | STABLE_BUILD) ||
There's also a CANARY_BUILD now. How about we define a MAX_BUILD or ALL_BUILDS
equal to (1 << 4) - 1 that we can use here so we don't forget to update this in
the future?

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:330:
OnNewNotification(promo_start, promo_end);
nit: don't need the { }

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:333: void
PromoResourceService::CalculateNewNotificationTimes(
Would ParseNewNotificationTimes be a better name, since it's not doing many
calculations (like adding group offsets)?

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:353: double*
old_promo_end) {
nit: indent

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:383: return promo_start +
promo_group * time_slice * 60 * 60;
nit: maybe put 60*60 in a constant?

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service.h (right):

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:140: // groups with ids
less than 5 should see the promo.
I don't think this comment is accurate. I think 7 targets dev, beta, and stable
but not canary. Also, the groups ramp up with 1 additional group every being
targeted every 24 hours.

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:225: double* promo_start,
nit: indentation

[achuithb, 2011-09-15 18:58:13]

Fixed nits, PTAL.

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:421: if
(PromoResourceService::CanShowPromo(profile_)) {
On 2011/09/15 15:42:27, jstritar wrote:
> nit: CanShowNotificationPromo

Done.

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:235: void
PromoResourceService::UnpackAdSignal(
On 2011/09/15 08:21:30, Miranda Callahan wrote:
> nit:UnpackNotificationSignal

Done.

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:258: void
PromoResourceService::ParseNotification(DictionaryValue* a_dic,
On 2011/09/15 15:42:27, jstritar wrote:
> nit: DictionaryValue* a_dic should be on the next line, indented with the
> others. or they could all line up under DictionaryValue*

Done.

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:286: promo_build >
(DEV_BUILD | BETA_BUILD | STABLE_BUILD) ||
On 2011/09/15 15:42:27, jstritar wrote:
> There's also a CANARY_BUILD now. How about we define a MAX_BUILD or ALL_BUILDS
> equal to (1 << 4) - 1 that we can use here so we don't forget to update this
in
> the future?

Done.

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:330:
OnNewNotification(promo_start, promo_end);
On 2011/09/15 15:42:27, jstritar wrote:
> nit: don't need the { }

Done.

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:333: void
PromoResourceService::CalculateNewNotificationTimes(
On 2011/09/15 15:42:27, jstritar wrote:
> Would ParseNewNotificationTimes be a better name, since it's not doing many
> calculations (like adding group offsets)?

Done.

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:353: double*
old_promo_end) {
On 2011/09/15 15:42:27, jstritar wrote:
> nit: indent

Done.

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:383: return promo_start +
promo_group * time_slice * 60 * 60;
On 2011/09/15 15:42:27, jstritar wrote:
> nit: maybe put 60*60 in a constant?

Done.

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service.h (right):

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:140: // groups with ids
less than 5 should see the promo.
On 2011/09/15 15:42:27, jstritar wrote:
> I don't think this comment is accurate. I think 7 targets dev, beta, and
stable
> but not canary. Also, the groups ramp up with 1 additional group every being
> targeted every 24 hours.

Done.

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:142: void
UnpackAdSignal(const base::DictionaryValue& parsed_json);
On 2011/09/15 08:21:30, Miranda Callahan wrote:
> nit: could you change this to UnpackNotificationSignal?

Done.

http://codereview.chromium.org/7655008/diff/43002/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.h:225: double* promo_start,
On 2011/09/15 15:42:27, jstritar wrote:
> nit: indentation

Done.

[achuithb, 2011-09-19 22:15:05]

jstritar: ping?

[jstritar, 2011-09-19 22:20:54]

LGTM

http://codereview.chromium.org/7655008/diff/48003/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/7655008/diff/48003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:332: }
nit: new line

http://codereview.chromium.org/7655008/diff/48003/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service_unittest.cc (right):

http://codereview.chromium.org/7655008/diff/48003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service_unittest.cc:185:
EXPECT_EQ(promo_end, prefs->GetDouble(prefs::kNTPPromoEnd));
Maybe test CanShowNotificationPromo? You could try with an in-range and
out-of-range group number?

[achuithb, 2011-09-19 22:57:56]

I'm going to land this with your LGTM. I've added an AI to test
CanShowNotificationPromo in 96290 and will address this with the next CL.

http://codereview.chromium.org/7655008/diff/48003/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/7655008/diff/48003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:332: }
On 2011/09/19 22:20:54, jstritar wrote:
> nit: new line

Done.

http://codereview.chromium.org/7655008/diff/48003/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service_unittest.cc (right):

http://codereview.chromium.org/7655008/diff/48003/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service_unittest.cc:185:
EXPECT_EQ(promo_end, prefs->GetDouble(prefs::kNTPPromoEnd));
On 2011/09/19 22:20:54, jstritar wrote:
> Maybe test CanShowNotificationPromo? You could try with an in-range and
> out-of-range group number?

I can't because of the build check in CanShowNotificationPromo.

I need to either have a way to separate out the build check of
CanShowNotificationPromo, or even better, as you once suggested, do the build
check during the parse stage and not in CanShowNotificationPromo. I've added an
action item to crbug.com/96290 to test CanShowNotificationPromo.