PromoResourceService fixes for 835

PromoResourceService fixes for 835.

Increase number of groups to 100.
Introduce ntp.promo_group_max.
Add max_group param to question.
Remove is_sync code from CanShowPromo.
Add is_valid_group code to CanShowPromo.
New promo trigger is based on promo_end only (and not 
    also on promo_start).
Reset group number on new promo, and not every time 
    the promo resource is downloaded.
Remove sync action from showPromoNotification.
Fix unit test.

BUG=93201
TEST=Should be able to see promo without sync text.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98150

[achuithb, 2011-08-24 00:47:54]

Here is the minimal set of changes for M14, based off of branch 835.

Let me know if you see something here that you think is risky or that i should
leave out.

This code still has the bug where the group is reset on every load of the promo
page, ie, every 2 days. So this works fine for promos that roll out quickly.

I also didn't update comments in the promo_resource_service.h file to reflect
the new scheme for max_group to minimize the number of files touched.

Let me know what you think.

[Miranda Callahan, 2011-08-24 05:30:57]

On 2011/08/24 00:47:54, achuith.bhandarkar wrote:
> Here is the minimal set of changes for M14, based off of branch 835.
> 
> Let me know if you see something here that you think is risky or that i should
> leave out.
> 
> This code still has the bug where the group is reset on every load of the
promo
> page, ie, every 2 days. So this works fine for promos that roll out quickly.
> 
> I also didn't update comments in the promo_resource_service.h file to reflect
> the new scheme for max_group to minimize the number of files touched.
> 
> Let me know what you think.

What if we fix the update bug by checking the difference between promo_end, and
not promo_start? That is, just remove line 305 from promo_resource_service.cc. 
I hate having this bug in there, and the restriction is too much, I think. From
my experience with promos, the end date is more of an afterthought than anything
else -- it would be easy to force an update by changing the end time by 1
minute, if necessary. Also, removing this comparison is, I believe, safe.

[achuithb, 2011-08-24 08:45:41]

Ok, I've updated the code so a new promo_group is created whenever the promo
changes (which is defined by the promo_end changing). The promo_start
calculation has to be moved to after the promo_group is set since it depends on
it. 

PTAL. Thanks!

[Miranda Callahan, 2011-08-24 19:07:11]

On 2011/08/24 08:45:41, achuith.bhandarkar wrote:
> Ok, I've updated the code so a new promo_group is created whenever the promo
> changes (which is defined by the promo_end changing). The promo_start
> calculation has to be moved to after the promo_group is set since it depends
on
> it. 
> 
> PTAL. Thanks!

LGTM! I'd like Jon to also take an eagle-eyed look, as this is going to be
merged into M14. Jon?

[jstritar, 2011-08-24 19:41:48]

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

http://codereview.chromium.org/7719013/diff/5010/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.cc:493: return !promo_closed
&& is_valid_group && is_promo_build;
It might be good to test that CanShowPromo is false when the current user's
group is not in range.

[achuithb, 2011-08-24 22:05:45]

Jon, at the end of PromoResourceServiceTest.UnpackPromoSignal, I added the
following test:

EXPECT_EQ(PromoResourceServiceUtil::CanShowPromo(&profile_), 
    promo_group < promo_group_max);

I set the promo_group_max value to 50 so there's an even chance that the promo
succeeds.

The is_valid_group check succeeds in both cases (when we are part of the
experiment or not, and also in the edge case where promo_group and
promo_group_max are both 50), but is_promo_build fails because my local build is
CHANNEL_UNKNOWN. 

We have a couple of options:
I can add another line to the switch in IsBuildTargeted like:
case chrome::VersionInfo::CHANNEL_UNKNOWN:
    return true;

Alternatively, I can change the default to return true instead of returning
false.

I'm concerned that either of these changes could be dangerous and am not
comfortable making them for this CL. 

I don't think it's appropriate to check in a unit test that fails on local
builds either.

Any suggestions?

[jstritar, 2011-08-24 22:36:14]

LGTM per follow up discussion