Some fixups for app notifications:

Some fixups for app notifications:
- Generate STATE_CHANGED notification in the Add method instead of
  in other places which call Add.
- Generate STATE_CHANGED notification in ClearAll and Remove methods.
- Hook up the bubble close button action with code that calls ClearAll.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107063

[Munjal (Google), 2011-10-24 18:21:16]

Finnur, I know you have refactored some of the bubble JS code. But I think the
changes in this Cl to existing bubble JS code would be needed in any case. So
you will need to merge or hand pick these changes into your patch
(http://codereview.chromium.org/8208014/).

[asargent_no_longer_on_chrome, 2011-10-24 19:09:16]

http://codereview.chromium.org/8382010/diff/1/chrome/browser/extensions/app_n...
File chrome/browser/extensions/app_notification_manager.cc (left):

http://codereview.chromium.org/8382010/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notification_manager.cc:212:
chrome::NOTIFICATION_APP_NOTIFICATION_STATE_CHANGED,
I think removing this code block means that in the case of having the new tab
page open right at chrome start (before
AppNotificationManager::HandleLoadResults gets called), we'll miss showing
notifications.

I think we either need to leave this in, or make the AppLauncherHandler class
listen for NOTIFICATION_APP_NOTIFICATION_MANAGER_LOADED

[Munjal (Google), 2011-10-24 21:44:11]

http://codereview.chromium.org/8382010/diff/1/chrome/browser/extensions/app_n...
File chrome/browser/extensions/app_notification_manager.cc (left):

http://codereview.chromium.org/8382010/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notification_manager.cc:212:
chrome::NOTIFICATION_APP_NOTIFICATION_STATE_CHANGED,
On 2011/10/24 19:09:17, Antony Sargent wrote:
> I think removing this code block means that in the case of having the new tab
> page open right at chrome start (before
> AppNotificationManager::HandleLoadResults gets called), we'll miss showing
> notifications.
> 
> I think we either need to leave this in, or make the AppLauncherHandler class
> listen for NOTIFICATION_APP_NOTIFICATION_MANAGER_LOADED

I thought that since Add now generates STATE_CHANGED notifications, that will
not happen. Is that not right?

[Munjal (Google), 2011-10-24 21:46:25]

http://codereview.chromium.org/8382010/diff/1/chrome/browser/extensions/app_n...
File chrome/browser/extensions/app_notification_manager.cc (left):

http://codereview.chromium.org/8382010/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notification_manager.cc:212:
chrome::NOTIFICATION_APP_NOTIFICATION_STATE_CHANGED,
On 2011/10/24 21:44:11, munjal wrote:
> On 2011/10/24 19:09:17, Antony Sargent wrote:
> > I think removing this code block means that in the case of having the new
tab
> > page open right at chrome start (before
> > AppNotificationManager::HandleLoadResults gets called), we'll miss showing
> > notifications.
> > 
> > I think we either need to leave this in, or make the AppLauncherHandler
class
> > listen for NOTIFICATION_APP_NOTIFICATION_MANAGER_LOADED
> 
> I thought that since Add now generates STATE_CHANGED notifications, that will
> not happen. Is that not right?

Actually, you are right. We do not call Add in this code path. Let me revive
this code.

[Munjal (Google), 2011-10-24 21:55:22]

http://codereview.chromium.org/8382010/diff/1/chrome/browser/extensions/app_n...
File chrome/browser/extensions/app_notification_manager.cc (left):

http://codereview.chromium.org/8382010/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notification_manager.cc:212:
chrome::NOTIFICATION_APP_NOTIFICATION_STATE_CHANGED,
On 2011/10/24 19:09:17, Antony Sargent wrote:
> I think removing this code block means that in the case of having the new tab
> page open right at chrome start (before
> AppNotificationManager::HandleLoadResults gets called), we'll miss showing
> notifications.
> 
> I think we either need to leave this in, or make the AppLauncherHandler class
> listen for NOTIFICATION_APP_NOTIFICATION_MANAGER_LOADED

Done.

[asargent_no_longer_on_chrome, 2011-10-24 22:09:15]

LGTM

http://codereview.chromium.org/8382010/diff/5002/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification_manager.cc (right):

http://codereview.chromium.org/8382010/diff/5002/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.cc:225:
chrome::NOTIFICATION_APP_NOTIFICATION_STATE_CHANGED,
nit: this should be indented like the next 2 lines

[Munjal (Google), 2011-10-24 22:22:55]

http://codereview.chromium.org/8382010/diff/5002/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification_manager.cc (right):

http://codereview.chromium.org/8382010/diff/5002/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.cc:225:
chrome::NOTIFICATION_APP_NOTIFICATION_STATE_CHANGED,
On 2011/10/24 22:09:15, Antony Sargent wrote:
> nit: this should be indented like the next 2 lines

Done.

[Finnur, 2011-10-25 10:05:23]

LGTM