NotificationPromo

NotificationPromo.

* Split out NotificationPromo helper class for PromoResourceService to handle promo notification.
* Support for views/max_views.
* NotificationPromo has data members for all the prefs fields (start, end, build, time_slice, 
    max_group, max_views, group, views, text and closed).
* Move notification parsing methods from PromoResourceService to NotificationPromo.
* NotificationPromo can be initialized from json when the promo resource is parsed, or from prefs, 
    when CanShowNotificationPromo is called.
* NotificationPromo now only writes out prefs upon detecting a new notification.
* NotificationPromo has a Delegate class, useful for testing.
* Static helper methods introduced for extracting time from DictionaryValue, string and prefs. 
    These may be easily unit-tested in the future.
* Number of additional tests to more thoroughly test parsing, CanShow logic, and static helper 
    functions like GetNextQuestionValue and NewGroup.
* NewGroup now uses RandInt instead of rand(), so this CL passes lint with no complaints.
* Some additional cleanup of PromoResourceService, esp GetChannel and IsBuildTargeted.

BUG=96290
TEST=Unit tests pass. Use 
    --promo-server-url='http://achuithz600.mtv.corp.google.com/www/files/promoresource2?hl='
    to see the promo. If you exceed 5 views, you have to reset the views count (ntp.promo_views)
    in your Preferences.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=103646

[achuithb, 2011-09-26 23:46:30]

Please review. I apologize for the size of the CL.

[Dan Beam, 2011-09-27 03:27:55]

By the way, you might consider changing the hostname to the production one
before committing / while reviewing (to not put internal URLs in a commit). 
Just passing the flag with the same URL should trigger the debug version as
you're just using ->HasSwitch() I think.  In the mean time (in this CL), for
those not in MTV offices you should add the MTV domain to the hostname or search
the right domain in your /etc/resolv.conf (on *nix systems) to make this URL
respond correctly.

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:57: }
You might consider making this a different notification or sending details to be
able to differentiate this from a re-download of the JSON from the promo service
(and something being changed).  Up to you.

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_notification.cc (right):

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_notification.cc:123: void
PromoNotification::RegisterUserPrefs(PrefService* prefs) {
I still think that all of the below would be best to simply have a Promo class
and ask that promo class for the names of these fields, i.e.
promo->pref_names()->start instead of hard-coding all these prefs specific to
the NTP Promo.

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_notification.cc:213:
base::Time::FromDoubleT(end_) > base::Time::Now();
Not to be a pedant, but should these be <= / >= anywhere (for any of the above 4
comparisons)?

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_notification.cc:229: 
Should the following 3 static methods be const if we're just getting things from
them?

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_notification.cc:253: 
Why are we getting these as doubles but converting back to Times for comparison?

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_notification.h (right):

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_notification.h:7: #pragma once
Why are you using this ^, exactly?  Shouldn't a header guard be enough?  I've
heard there can be cross-platform issues with #pragma directives.  On further
research (http://goo.gl/4GMwE), I guess it's OK if you leave it here, but it
doesn't really do anything (and I don't see it in any other files) and in one
case GCC recommends not using it (but this was a really long time ago...,
http://goo.gl/wmXgo).

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_notification.h:86: double
GetTimeFromPrefs(const char* pref) const;
Should these be returning Times or doubles?  I guess it doesn't matter that
much, but I just remember reading in the time class that's it's better to do
calculation on Time objects instead of on doubles.  Eh, I just re-looked at the
note in the code (http://goo.gl/jVmdg) and I guess we don't really care, so
whatevs...

[achuithb, 2011-09-27 08:02:17]

On 2011/09/27 03:27:55, Dan Beam wrote:
> By the way, you might consider changing the hostname to the production one
> before committing / while reviewing (to not put internal URLs in a commit). 
> Just passing the flag with the same URL should trigger the debug version as
> you're just using ->HasSwitch() I think.  In the mean time (in this CL), for
> those not in MTV offices you should add the MTV domain to the hostname or
search
> the right domain in your /etc/resolv.conf (on *nix systems) to make this URL
> respond correctly.
> 

Are you talking about my commit message? I'll put in the fully qualified url
(the one I've specified only works in mtv - you're right). But it's ok to
provide the internal url for testers. There should not be an internal url
anywhere in the code.

[achuithb, 2011-09-27 08:03:19]

Thanks for the notes, Dan.

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:57: }
The same notification is sent by the promo_resource_service (which you refer
to), and also when the notification is closed - basically it tells the
ntp_resource_cache to delete the cache. I believe it's correct the way it is.

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_notification.cc (right):

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_notification.cc:123: void
PromoNotification::RegisterUserPrefs(PrefService* prefs) {
I agree. We should probably have the promo class register the common prefs, and
have subclasses register the prefs specific to them, but it's outside the scope
of this CL. There is an argument to be made for having these stay in
PromoResourceService instead of having them here. Let's see what Miranda and Jon
have to say.

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_notification.cc:213:
base::Time::FromDoubleT(end_) > base::Time::Now();
Nope, I think the comparisons are correct as they are. There's a case to be made
for the start time comparison being <=, but being off by one second won't kill
us.

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_notification.cc:229: 
All static functions are const by definition, since static methods cannot modify
(or read) class variables. static const doesn't make sense.

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_notification.cc:253: 
They are saved as doubles in the preferences file. I don't know the reason for
that decision, but I'd like this CL to be backwards compatible so I haven't
changed that. These should've probably just been saved as ints or int64s.

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_notification.h (right):

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_notification.h:7: #pragma once
You're supposed to use it:
http://dev.chromium.org/developers/coding-style#TOC-File-headers

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_notification.h:86: double
GetTimeFromPrefs(const char* pref) const;
start_ and end_ are stored as doubles (because the underlying prefs are
doubles), so we convert everything to double. This is unchanged from the
PromoResourceService code.

[Miranda Callahan, 2011-09-27 09:18:40]

Thanks, Achuith!

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_notification.h (right):

http://codereview.chromium.org/8045012/diff/8002/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_notification.h:57: static const int kMaxViews
= 1000;
Can you add a comment for kMaxViews as well?

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_notification.cc (right):

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.cc:64: bool
PromoNotification::OutOfBounds(int var, int min, int max) {
This should probably just be a helper function in an anonymous namespace for
this file.

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.cc:68: void
PromoNotification::GetParams(const DictionaryValue* dict) {
Perhaps this should be called "ParseParams", since it's essentially the same
method as Parse (same argument type, same effect), but just for a subset of the
parameters.

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.cc:250:
dict->GetString("inproduct", &str);
It's unfortunate that we still have to use strings like "inproduct" and
"tooltip". Perhaps we should gather these strings together at the top of the
file, and add a comment explaining why we're using these bizarre names, for
future reference?

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.cc:255: double
PromoNotification::GetTimeFromString(const std::string& time_str) {
I would not have this as a separate method, but just include it in
GetTimeFromDict -- it's not called anywhere else, and the two methods are both
quite short. Having three separate "GetTimeFrom..." methods could be confusing
to the reader.

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_notification.h (right):

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.h:59: // Parse the answers array
element.
Can you add to the comment to indicate that this function has the side effect of
adding/changing data in the notification's member fields?

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.h:76: // Parses question string.
Could you indicate in the comment that this method has the side effect of
increasing the value of index, and that it will may the value of |err|?

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

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service_unittest.cc:14: #include
"chrome/browser/web_resource/promo_notification.h"
whoops, swap for alphabetical order.

[achuithb, 2011-09-28 01:40:13]

Thanks for the feedback, Miranda! PTAL.

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_notification.cc (right):

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.cc:64: bool
PromoNotification::OutOfBounds(int var, int min, int max) {
On 2011/09/27 09:18:40, Miranda Callahan wrote:
> This should probably just be a helper function in an anonymous namespace for
> this file.

Done.

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.cc:68: void
PromoNotification::GetParams(const DictionaryValue* dict) {
On 2011/09/27 09:18:40, Miranda Callahan wrote:
> Perhaps this should be called "ParseParams", since it's essentially the same
> method as Parse (same argument type, same effect), but just for a subset of
the
> parameters.

Done.

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.cc:250:
dict->GetString("inproduct", &str);
On 2011/09/27 09:18:40, Miranda Callahan wrote:
> It's unfortunate that we still have to use strings like "inproduct" and
> "tooltip". Perhaps we should gather these strings together at the top of the
> file, and add a comment explaining why we're using these bizarre names, for
> future reference?

Done.

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.cc:255: double
PromoNotification::GetTimeFromString(const std::string& time_str) {
On 2011/09/27 09:18:40, Miranda Callahan wrote:
> I would not have this as a separate method, but just include it in
> GetTimeFromDict -- it's not called anywhere else, and the two methods are both
> quite short. Having three separate "GetTimeFrom..." methods could be confusing
> to the reader.

Done.

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_notification.h (right):

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.h:59: // Parse the answers array
element.
On 2011/09/27 09:18:40, Miranda Callahan wrote:
> Can you add to the comment to indicate that this function has the side effect
of
> adding/changing data in the notification's member fields?

Done.

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.h:76: // Parses question string.
On 2011/09/27 09:18:40, Miranda Callahan wrote:
> Could you indicate in the comment that this method has the side effect of
> increasing the value of index, and that it will may the value of |err|?

Done.

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

http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service_unittest.cc:14: #include
"chrome/browser/web_resource/promo_notification.h"
On 2011/09/27 09:18:40, Miranda Callahan wrote:
> whoops, swap for alphabetical order.

Done.

[Miranda Callahan, 2011-09-28 07:07:04]

thanks -- LGTM!

On 2011/09/28 01:40:13, achuith.bhandarkar wrote:
> Thanks for the feedback, Miranda! PTAL.
> 
>
http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
> File chrome/browser/web_resource/promo_notification.cc (right):
> 
>
http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
> chrome/browser/web_resource/promo_notification.cc:64: bool
> PromoNotification::OutOfBounds(int var, int min, int max) {
> On 2011/09/27 09:18:40, Miranda Callahan wrote:
> > This should probably just be a helper function in an anonymous namespace for
> > this file.
> 
> Done.
> 
>
http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
> chrome/browser/web_resource/promo_notification.cc:68: void
> PromoNotification::GetParams(const DictionaryValue* dict) {
> On 2011/09/27 09:18:40, Miranda Callahan wrote:
> > Perhaps this should be called "ParseParams", since it's essentially the same
> > method as Parse (same argument type, same effect), but just for a subset of
> the
> > parameters.
> 
> Done.
> 
>
http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
> chrome/browser/web_resource/promo_notification.cc:250:
> dict->GetString("inproduct", &str);
> On 2011/09/27 09:18:40, Miranda Callahan wrote:
> > It's unfortunate that we still have to use strings like "inproduct" and
> > "tooltip". Perhaps we should gather these strings together at the top of the
> > file, and add a comment explaining why we're using these bizarre names, for
> > future reference?
> 
> Done.
> 
>
http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
> chrome/browser/web_resource/promo_notification.cc:255: double
> PromoNotification::GetTimeFromString(const std::string& time_str) {
> On 2011/09/27 09:18:40, Miranda Callahan wrote:
> > I would not have this as a separate method, but just include it in
> > GetTimeFromDict -- it's not called anywhere else, and the two methods are
both
> > quite short. Having three separate "GetTimeFrom..." methods could be
confusing
> > to the reader.
> 
> Done.
> 
>
http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
> File chrome/browser/web_resource/promo_notification.h (right):
> 
>
http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
> chrome/browser/web_resource/promo_notification.h:59: // Parse the answers
array
> element.
> On 2011/09/27 09:18:40, Miranda Callahan wrote:
> > Can you add to the comment to indicate that this function has the side
effect
> of
> > adding/changing data in the notification's member fields?
> 
> Done.
> 
>
http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
> chrome/browser/web_resource/promo_notification.h:76: // Parses question
string.
> On 2011/09/27 09:18:40, Miranda Callahan wrote:
> > Could you indicate in the comment that this method has the side effect of
> > increasing the value of index, and that it will may the value of |err|?
> 
> Done.
> 
>
http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
> File chrome/browser/web_resource/promo_resource_service_unittest.cc (right):
> 
>
http://codereview.chromium.org/8045012/diff/13001/chrome/browser/web_resource...
> chrome/browser/web_resource/promo_resource_service_unittest.cc:14: #include
> "chrome/browser/web_resource/promo_notification.h"
> On 2011/09/27 09:18:40, Miranda Callahan wrote:
> > whoops, swap for alphabetical order.
> 
> Done.

[achuithb, 2011-09-28 07:38:13]

Thanks Miranda!

Jon - ping?

[jstritar, 2011-09-28 15:28:20]

http://codereview.chromium.org/8045012/diff/13007/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/8045012/diff/13007/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:50: const int max_views =
prefs->GetInteger(prefs::kNTPPromoViewsMax);
You're accessing PromoNotification's encapsulated data by going to the prefs
directly here. Is there a way you can go through PromoNotification? Not sure if
there's an easy way right now...

http://codereview.chromium.org/8045012/diff/13007/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_notification.h (right):

http://codereview.chromium.org/8045012/diff/13007/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.h:22: class PromoNotification {
naming nit: Do you think NotificationPromo would make more sense?
PromoNotification seems like a notification for the promo system, rather than an
actual promo. Up to you...

http://codereview.chromium.org/8045012/diff/13007/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.h:41: static void
RegisterUserPrefs(PrefService* prefs);
nit: static methods should be defined above regular methods (here and in
private:)

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/8045012/diff/13007/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.h:56: static const int
kMaxGroupSize = 99;
Depending on what's needed for testing, you can probably put many of these
private static members & methods in the anonymous namespace. It's probably
better to only keep what's needed in the class definition.

http://codereview.chromium.org/8045012/diff/13007/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.h:91: double
GetTimeFromPrefs(const char* pref) const;
These could also be in the anonymous namespace. Something along these lines:

double GetTimeFromDict(const base::DictionaryValue* dict)
double GetTimeFromPrefs(PrefService* prefs, cont char* pref)

[achuithb, 2011-09-29 02:00:50]

Thanks for your feedback Jon!

Patchset 12 contains the bulk of the fixes from Jon's feedback. 

I've introduced a NotifyPromoResourceChanged method to NewTabPageHandler.

Patchset 13 is just a bunch of renames from PromoNotification to
NotificationPromo.

Patchset 14 is a file move. 

Unfortunately the changes in patchset 12 I made to promo_notification are lost,
so you have to look at the entire file :/

PTAL, Jon.

http://codereview.chromium.org/8045012/diff/13007/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/8045012/diff/13007/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:50: const int max_views =
prefs->GetInteger(prefs::kNTPPromoViewsMax);
Done.

I've added methods HandleClosed() and HandleViewed() to PromoNotification which
moves this logic to the PromoNotification class.

http://codereview.chromium.org/8045012/diff/13007/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_notification.h (right):

http://codereview.chromium.org/8045012/diff/13007/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.h:41: static void
RegisterUserPrefs(PrefService* prefs);
I don't see the rule in the Declaration Order link you posted?

http://codereview.chromium.org/8045012/diff/13007/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.h:56: static const int
kMaxGroupSize = 99;
Yup, I'm using this variable in the unit tests so I have it here. I'll move the
other 2 to the .cc in an anonymous namespace.

http://codereview.chromium.org/8045012/diff/13007/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.h:91: double
GetTimeFromPrefs(const char* pref) const;
Done. 

I was in half a mind whether these needed unit tests, but since I haven't
written any, I'll just put them in the anonymous namespace for now.

[jstritar, 2011-09-30 19:57:57]

LGTM

http://codereview.chromium.org/8045012/diff/13007/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_notification.h (right):

http://codereview.chromium.org/8045012/diff/13007/chrome/browser/web_resource...
chrome/browser/web_resource/promo_notification.h:41: static void
RegisterUserPrefs(PrefService* prefs);
On 2011/09/29 02:00:50, achuith.bhandarkar wrote:
> I don't see the rule in the Declaration Order link you posted? 

Ah, interesting. Maybe this is just something we do in extensions code.

[Dan Beam, 2011-09-30 22:49:04]

rubber stamp, btw, don't let me hold you back from landing this

[achuithb, 2011-10-01 21:09:06]

Thanks for the reviews Miranda, Jon and Dan. This has landed in T-O-T