Support for ntp promo bubble.

Support for ntp promo bubble.

BUG=123061
TEST=manual

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

[achuithb, 2012-09-11 03:32:58]

Please take a look, Dan.

[Dan Beam, 2012-09-11 03:44:57]

lgtm w/nits if it works

http://codereview.chromium.org/10911196/diff/14001/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/10911196/diff/14001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/new_tab.js:188: promoBubble.anchorNode =
$('logo-img');
can you use getRequiredElement() instead of $()?

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

http://codereview.chromium.org/10911196/diff/14001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:92:
NotificationPromo::NTP_BUBBLE_PROMO)) {
nit: needs moar spacez

http://codereview.chromium.org/10911196/diff/14001/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.h (right):

http://codereview.chromium.org/10911196/diff/14001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/new_tab_page_handler.h:36: // Callback for
"notificationPromoClosed".
can you put No arguments. on the callbacks that don't require arguments?

[achuithb, 2012-09-11 20:27:05]

Dan, I've incorporated your review feedback.

I've had to make some changes to PromoResourceService; please take a look at
promo_resource_service.{h|cc}

I've done the end to end test with --promo-server-url and it's looking good.

Thanks!

http://codereview.chromium.org/10911196/diff/14001/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/10911196/diff/14001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/new_tab.js:188: promoBubble.anchorNode =
$('logo-img');
On 2012/09/11 03:44:57, Dan Beam wrote:
> can you use getRequiredElement() instead of $()?

Done.

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

http://codereview.chromium.org/10911196/diff/14001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:92:
NotificationPromo::NTP_BUBBLE_PROMO)) {
On 2012/09/11 03:44:57, Dan Beam wrote:
> nit: needs moar spacez

Done.

http://codereview.chromium.org/10911196/diff/14001/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.h (right):

http://codereview.chromium.org/10911196/diff/14001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/new_tab_page_handler.h:36: // Callback for
"notificationPromoClosed".
On 2012/09/11 03:44:57, Dan Beam wrote:
> can you put No arguments. on the callbacks that don't require arguments?

Done.

[Dan Beam, 2012-09-11 20:32:20]

lgtm w/nit

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

http://codereview.chromium.org/10911196/diff/1018/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:79:
NotificationPromo::NTP_NOTIFICATION_PROMO)) {
I meant more spaces :)

[achuithb, 2012-09-11 20:37:05]

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

http://codereview.chromium.org/10911196/diff/1018/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:79:
NotificationPromo::NTP_NOTIFICATION_PROMO)) {
On 2012/09/11 20:32:21, Dan Beam wrote:
> I meant more spaces :)

Can't do it, doesn't fit if I line it up with the first arg :/ 

Moved both args to the second line, and that doesn't fit either. The only other
option is to have each arg on a separate line. It seems wasteful, but I can do
that if you prefer. The way I have it is acceptable, I believe.

[Dan Beam, 2012-09-11 20:51:22]

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

http://codereview.chromium.org/10911196/diff/1018/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:79:
NotificationPromo::NTP_NOTIFICATION_PROMO)) {
On 2012/09/11 20:37:05, achuith.bhandarkar wrote:
> On 2012/09/11 20:32:21, Dan Beam wrote:
> > I meant more spaces :)
> 
> Can't do it, doesn't fit if I line it up with the first arg :/ 
> 
> Moved both args to the second line, and that doesn't fit either. The only
other
> option is to have each arg on a separate line. It seems wasteful, but I can do
> that if you prefer. The way I have it is acceptable, I believe.

I meant +4 \s, let me just show you ;)

  if (NotificationPromo::HandleViewed(Profile::FromWebUI(web_ui()),
          NotificationPromo::NTP_NOTIFICATION_PROMO)) {

[achuithb, 2012-09-11 21:27:45]

I've added BR to parseHTMLSubset as agreed.

Thanks for the review feedback!

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

http://codereview.chromium.org/10911196/diff/1018/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:79:
NotificationPromo::NTP_NOTIFICATION_PROMO)) {
On 2012/09/11 20:51:22, Dan Beam wrote:
> On 2012/09/11 20:37:05, achuith.bhandarkar wrote:
> > On 2012/09/11 20:32:21, Dan Beam wrote:
> > > I meant more spaces :)
> > 
> > Can't do it, doesn't fit if I line it up with the first arg :/ 
> > 
> > Moved both args to the second line, and that doesn't fit either. The only
> other
> > option is to have each arg on a separate line. It seems wasteful, but I can
do
> > that if you prefer. The way I have it is acceptable, I believe.
> 
> I meant +4 \s, let me just show you ;)
> 
>   if (NotificationPromo::HandleViewed(Profile::FromWebUI(web_ui()),
>           NotificationPromo::NTP_NOTIFICATION_PROMO)) {

I've managed to do this now.

[commit-bot: I haz the power, 2012-09-11 21:31:28]

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

[commit-bot: I haz the power, 2012-09-11 23:49:01]

Change committed as 156178