Support for promo_type.

Support for promo_type.

* Create an enum PromoType and a data member promo_type_ of NotificationPromo.
* InitFromJson, InitFromPrefs, HandleClosed and HandleViewed all take a PromoType now.
* HandleClosed and HandleViewed are static.
* Add an anon function PromoTypeToString for conversion from PromoType to string.
* Improve unit tests TestViewed() and TestClosed().

BUG=123061
TEST=unit tests. Manual.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150235

[achuithb, 2012-08-04 09:27:00]

Ruslan: Does the ntp promo on android use new_tab_page_handler.cc or
ntp_resource_cache.cc? I'm guessing not?

Please take a look at this CL. Thanks!

[aruslan, 2012-08-06 16:29:10]

LGTM. This will break Android and iOS downstream, I'll fix Android; please
include noyau@ so that he could fix iOS.  ntp_resource_cache.cc is not used on
Android.

http://codereview.chromium.org/10836099/diff/4003/chrome/browser/web_resource...
File chrome/browser/web_resource/notification_promo.cc (right):

http://codereview.chromium.org/10836099/diff/4003/chrome/browser/web_resource...
chrome/browser/web_resource/notification_promo.cc:221: LOG(ERROR) << "Malfromed
JSON: not " << promo_type_;
nit: I believe it should be "Malformed JSON: no " << promo_type_;, or may be
this LOG could be removed completely as long as we are fine with receiving an
empty set of promos etc.

[aruslan, 2012-08-06 16:29:42]

(I'm not a committer.)

[achuithb, 2012-08-06 21:05:44]

Ruslan, I changed the string to an enum and add a convertor function. Please
take another look.

Eric: Added you for fyi.

Rob: Please take a look!

[aruslan, 2012-08-06 21:11:12]

Looks good to me with 2 nits (indentation and "Malfromed").

http://codereview.chromium.org/10836099/diff/12002/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/10836099/diff/12002/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:73:
NotificationPromo::NTP_NOTIFICATION_PROMO)) {
nit: indentation cf 6 lines above.

[Robert Sesek, 2012-08-06 21:21:44]

LGTM

http://codereview.chromium.org/10836099/diff/12002/chrome/browser/web_resourc...
File chrome/browser/web_resource/notification_promo.cc (right):

http://codereview.chromium.org/10836099/diff/12002/chrome/browser/web_resourc...
chrome/browser/web_resource/notification_promo.cc:493: promo.views_++;
Use ++pre-increment.

http://codereview.chromium.org/10836099/diff/12002/chrome/browser/web_resourc...
File chrome/browser/web_resource/notification_promo.h (right):

http://codereview.chromium.org/10836099/diff/12002/chrome/browser/web_resourc...
chrome/browser/web_resource/notification_promo.h:53: static void
HandleClosed(Profile* profile, PromoType promo_type);
Should document what this function does.

[achuithb, 2012-08-06 22:16:19]

Thanks for the reviews!

Dan: Need owner lgtm for ntp stuff.

http://codereview.chromium.org/10836099/diff/12002/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/10836099/diff/12002/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:73:
NotificationPromo::NTP_NOTIFICATION_PROMO)) {
On 2012/08/06 21:11:12, aruslan wrote:
> nit: indentation cf 6 lines above.

Done.

http://codereview.chromium.org/10836099/diff/12002/chrome/browser/web_resourc...
File chrome/browser/web_resource/notification_promo.cc (right):

http://codereview.chromium.org/10836099/diff/12002/chrome/browser/web_resourc...
chrome/browser/web_resource/notification_promo.cc:493: promo.views_++;
On 2012/08/06 21:21:44, rsesek wrote:
> Use ++pre-increment.

Done.

http://codereview.chromium.org/10836099/diff/12002/chrome/browser/web_resourc...
File chrome/browser/web_resource/notification_promo.h (right):

http://codereview.chromium.org/10836099/diff/12002/chrome/browser/web_resourc...
chrome/browser/web_resource/notification_promo.h:53: static void
HandleClosed(Profile* profile, PromoType promo_type);
On 2012/08/06 21:21:44, rsesek wrote:
> Should document what this function does.

Done.

[Evan Stade, 2012-08-07 00:21:55]

ntp lgtm

[achuithb, 2012-08-07 00:23:09]

On 2012/08/07 00:21:55, Evan Stade wrote:
> ntp lgtm

Thanks Evan!

[commit-bot: I haz the power, 2012-08-07 00:24:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/10836099/9002

[commit-bot: I haz the power, 2012-08-07 02:16:18]

Change committed as 150235