Change the icon on the desktop notifications infobar.

Change the icon on the desktop notifications infobar.


BUG=94112
TEST=Trigger the infobar asking the user whether they would like to allow desktop notifications from a site and verify that it uses the new icon instead of the old (Chrome product) icon.


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

[gideonwald, 2011-09-07 23:01:19]

Whoo, Windows trybot finished - compile errors begone! :)

[msw, 2011-09-07 23:36:12]

Wait for approval from pkasting before landing.

LGTM with one nit, see my in-line comment.
A nit is a recommended inconsequential fix that doesn't require my re-review.

Run all automatically detected trybots for a cross-platform change (like your
first patch set), not just Windows.

http://codereview.chromium.org/7742002/diff/2002/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/7742002/diff/2002/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:126:
IDR_INFOBAR_DESKTOP_NOTIFICATIONS);
nit: I think this line should be indented one more space
(IDR_INFOBAR_DESKTOP_NOTIFICATIONS should be indented 4 spaces from return). The
indentation was wrong before you changed this, but but it's nice if you can fix
it while you're touching it.

[gideonwald, 2011-09-07 23:51:50]

On 2011/09/07 23:36:12, msw wrote:
> Wait for approval from pkasting before landing.
> 
> LGTM with one nit, see my in-line comment.
> A nit is a recommended inconsequential fix that doesn't require my re-review.
> 
> Run all automatically detected trybots for a cross-platform change (like your
> first patch set), not just Windows.
> 
>
http://codereview.chromium.org/7742002/diff/2002/chrome/browser/notifications...
> File chrome/browser/notifications/desktop_notification_service.cc (right):
> 
>
http://codereview.chromium.org/7742002/diff/2002/chrome/browser/notifications...
> chrome/browser/notifications/desktop_notification_service.cc:126:
> IDR_INFOBAR_DESKTOP_NOTIFICATIONS);
> nit: I think this line should be indented one more space
> (IDR_INFOBAR_DESKTOP_NOTIFICATIONS should be indented 4 spaces from return).
The
> indentation was wrong before you changed this, but but it's nice if you can
fix
> it while you're touching it.

Done. Trybots rerunning (on the patch set before the extra space was added -
hope that's okay).

pkasting, let me know what you think!

[Peter Kasting, 2011-09-14 19:00:30]

http://codereview.chromium.org/7742002/diff/10002/chrome/app/theme/theme_reso...
File chrome/app/theme/theme_resources_large.grd (right):

http://codereview.chromium.org/7742002/diff/10002/chrome/app/theme/theme_reso...
chrome/app/theme/theme_resources_large.grd:73: <include
name="IDR_INFOBAR_DESKTOP_NOTIFICATIONS"
file="infobar_desktop_notifications.png" type="BINDATA" />
There needs to be a large version of this icon that is used here.

http://codereview.chromium.org/7742002/diff/10002/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/7742002/diff/10002/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service.cc:40: #include
"grit/theme_resources.h"
Is this still needed?

[gideonwald, 2011-10-04 20:23:11]

On 2011/09/14 19:00:30, Peter Kasting wrote:
>
http://codereview.chromium.org/7742002/diff/10002/chrome/app/theme/theme_reso...
> File chrome/app/theme/theme_resources_large.grd (right):
> 
>
http://codereview.chromium.org/7742002/diff/10002/chrome/app/theme/theme_reso...
> chrome/app/theme/theme_resources_large.grd:73: <include
> name="IDR_INFOBAR_DESKTOP_NOTIFICATIONS"
> file="infobar_desktop_notifications.png" type="BINDATA" />
> There needs to be a large version of this icon that is used here.

I filed http://code.google.com/p/chromium/issues/detail?id=95533 back when I
made this CL originally. Should I wait to submit this until that one is
resolved? Or should I get this in and update the large file when it's created?

> 
>
http://codereview.chromium.org/7742002/diff/10002/chrome/browser/notification...
> File chrome/browser/notifications/desktop_notification_service.cc (right):
> 
>
http://codereview.chromium.org/7742002/diff/10002/chrome/browser/notification...
> chrome/browser/notifications/desktop_notification_service.cc:40: #include
> "grit/theme_resources.h"
> Is this still needed?

Looks like not. I've removed it. I was fooled by the fact that the other IDR
constants were in browser_resources - didn't realize they weren't in
theme_resources. Thanks for catching it!

[Peter Kasting, 2011-10-04 20:25:59]

On 2011/10/04 20:23:11, gideonwald wrote:
> On 2011/09/14 19:00:30, Peter Kasting wrote:
> > chrome/app/theme/theme_resources_large.grd:73: <include
> > name="IDR_INFOBAR_DESKTOP_NOTIFICATIONS"
> > file="infobar_desktop_notifications.png" type="BINDATA" />
> > There needs to be a large version of this icon that is used here.
> 
> I filed http://code.google.com/p/chromium/issues/detail?id=95533 back when I
> made this CL originally. Should I wait to submit this until that one is
> resolved? Or should I get this in and update the large file when it's created?

Sounds like you should harass Glen and Nicholas and tell them you need the icon
immediately :)

[gideonwald, 2011-10-04 20:28:41]

On 2011/10/04 20:25:59, Peter Kasting wrote:
> On 2011/10/04 20:23:11, gideonwald wrote:
> > On 2011/09/14 19:00:30, Peter Kasting wrote:
> > > chrome/app/theme/theme_resources_large.grd:73: <include
> > > name="IDR_INFOBAR_DESKTOP_NOTIFICATIONS"
> > > file="infobar_desktop_notifications.png" type="BINDATA" />
> > > There needs to be a large version of this icon that is used here.
> > 
> > I filed http://code.google.com/p/chromium/issues/detail?id=95533 back when I
> > made this CL originally. Should I wait to submit this until that one is
> > resolved? Or should I get this in and update the large file when it's
created?
> 
> Sounds like you should harass Glen and Nicholas and tell them you need the
icon
> immediately :)

Righto, done :). Will update when I get the icon.

[gideonwald, 2011-11-30 23:39:08]

On 2011/10/04 20:28:41, gideonwald wrote:
> On 2011/10/04 20:25:59, Peter Kasting wrote:
> > On 2011/10/04 20:23:11, gideonwald wrote:
> > > On 2011/09/14 19:00:30, Peter Kasting wrote:
> > > > chrome/app/theme/theme_resources_large.grd:73: <include
> > > > name="IDR_INFOBAR_DESKTOP_NOTIFICATIONS"
> > > > file="infobar_desktop_notifications.png" type="BINDATA" />
> > > > There needs to be a large version of this icon that is used here.
> > > 
> > > I filed http://code.google.com/p/chromium/issues/detail?id=95533 back when
I
> > > made this CL originally. Should I wait to submit this until that one is
> > > resolved? Or should I get this in and update the large file when it's
> created?
> > 
> > Sounds like you should harass Glen and Nicholas and tell them you need the
> icon
> > immediately :)
> 
> Righto, done :). Will update when I get the icon.

And finally, two months later (my fault for the delay)...

Sailesh submitted http://code.google.com/p/chromium/issues/detail?id=95533 with
the large icon and this CL now uses it. Peter, if you have a chance to take a
look and let us know if it's good from your perspective, I'll submit then.

Thanks everyone.

[Peter Kasting, 2011-12-01 19:11:58]

LGTM

[commit-bot: I haz the power, 2011-12-01 21:35:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gideonwald@chromium.org/7742002/28001

[commit-bot: I haz the power, 2011-12-01 22:51:43]

Change committed as 112563