Start delegating notifications to MessageCenter on Windows.

Start delegating notifications to MessageCenter on Windows.

This is the next step.
MessageCeneterNotificationManager now creates MessageCenter and forwards
new notificaitons to it.

1. Upon chat with dewittj@, we decided to not trat MessageCenter as Singleton
since there are issues on process shutdown (MessageCenter should exist
while MessageCeneterNotificationManager does). Changes that.

2. MessageCeneterNotificationManager for now keeps a map of all displayed
Notification objects - to make sure NotificationDelegates are invoked and
Profiles are tracked.

3. Started to pass ExtensionId into as origin_id from Notification API, to
scope the notification updates to a specific app (bug 168924)

4. Additionally scoped update of notifications by profile, since in
multiprofile case the update was also not scoped by profile.

This does not yet render anything, since there is no rendering Observer
set for the MessageCenter. Will coordinate with Justin.

Next step would be to land a test for the add/update/remove logic,
and also move image download from Balloon_view_ash to NotificationUIManagerImpl,
where notifications wait until they are ready to be displayed.

BUG=168924, 168605
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178352

[Dmitry Titov, 2013-01-17 01:28:56]

Next step to MessageCeneter on Windows.
I run this code in the debugger but it needs real tests and this would be my
next patch.

Could you guys please look at:
Steven - overall Notifications flow.
Justin - Singleton, replace_id, MessageCenter specifics
Thanks!

[stevenjb, 2013-01-17 01:53:44]

Only looked at web_notification_tray so far, but wanted to give you a heads up
on a problem right away.

https://codereview.chromium.org/11958025/diff/1/ash/system/web_notification/w...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/11958025/diff/1/ash/system/web_notification/w...
ash/system/web_notification/web_notification_tray.cc:86:
message_center_.reset(new message_center::MessageCenter);
This is a problem. We may have more than one WebNotificationTray instance in
Ash.

Instead, instantiate a MessageCenter instance in ash::Shell and set this to
ash::Shell::GetInstance()->message_center();

[Dmitry Titov, 2013-01-17 03:04:29]

On 2013/01/17 01:53:44, stevenjb (chromium) wrote:

> ash/system/web_notification/web_notification_tray.cc:86:
> message_center_.reset(new message_center::MessageCenter);
> This is a problem. We may have more than one WebNotificationTray instance in
> Ash.
> 
> Instead, instantiate a MessageCenter instance in ash::Shell and set this to
> ash::Shell::GetInstance()->message_center();

Done.

[dewittj, 2013-01-17 17:41:21]

Looks good to me.

https://codereview.chromium.org/11958025/diff/4001/chrome/browser/notificatio...
File chrome/browser/notifications/message_center_notification_manager.h (right):

https://codereview.chromium.org/11958025/diff/4001/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.h:64: //
ProfileNotification and NotificaitonList::Notification) into a single class.
5x fix Notificaiton -> Notification above.

https://codereview.chromium.org/11958025/diff/4001/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.h:85: //
Helpers that add/remove the notification from local map and MessageCenter.
nit: indentation.

Perhaps mention that they take ownership of the ProfileNotification?

[Dmitry Titov, 2013-01-17 22:47:44]

Thinking a bit more about Steven's note, I realized it's better to make
MessageCenter to be created/owned by BrowserProcess in non-ash case. Then
MessageCenter is a shared instance between WebNotificationTrays on ash, future
UI impl on Windows, and NotificationUIManager. All these classes will pull the
MessageCenter instance pointer from ash::Shell or g_browser_process, depending
if it's an USE_ASH build or not.

Will update patch in a few min.

[Dmitry Titov, 2013-01-17 23:05:42]

https://codereview.chromium.org/11958025/diff/4001/chrome/browser/notificatio...
File chrome/browser/notifications/message_center_notification_manager.h (right):

https://codereview.chromium.org/11958025/diff/4001/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.h:64: //
ProfileNotification and NotificaitonList::Notification) into a single class.
On 2013/01/17 17:41:21, dewittj wrote:
> 5x fix Notificaiton -> Notification above.

Done.

https://codereview.chromium.org/11958025/diff/4001/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.h:85: //
Helpers that add/remove the notification from local map and MessageCenter.
On 2013/01/17 17:41:21, dewittj wrote:
> nit: indentation.
> 
> Perhaps mention that they take ownership of the ProfileNotification?

Done.

[Dmitry Titov, 2013-01-17 23:07:02]

PTAL. Now pulling shared instance of MessageCenter from ash::Shell or
g_browser_process, depending on USE_ASH.

[dewittj, 2013-01-18 00:45:44]

lgtm.

https://codereview.chromium.org/11958025/diff/15002/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notification_manager.h (right):

https://codereview.chromium.org/11958025/diff/15002/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.h:64: //
ProfileNotification and NotificaitonList::Notification) into a single class.
nit: There are still 4 more misspellings of Notification in the above
paragraphs.

https://codereview.chromium.org/11958025/diff/15002/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.h:86: // They
take ownership of profile_notificaiton object.
nit: profile_notification

[Dmitry Titov, 2013-01-18 01:43:24]

Fixed typos, thanks Justin!
Also fixed couple more build errors on systems where ENABLE_MESSAGE_CENTER is
not defined.

[stevenjb, 2013-01-18 19:56:18]

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:451: #ifdef ENABLE_MESSAGE_CENTER
Shouldn't we add && !USE_ASH here, otherwise we'll get two message center
instances?

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.h:212: #if defined(ENABLE_MESSAGE_CENTER)
Shouldn't we add && !USE_ASH here?

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.cc:28: #endif
Would it make sense for MessageCenter* to be passed to this class in the
constructor, to avoid dependencies here on browser_process / shall?

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notification_manager.h (right):

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.h:41: //
MessageCenter::Delegate.
nit: no .

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.h:74:
std::string GetExtensionId();
nit: WS

[Dmitry Titov, 2013-01-18 21:23:00]

Thanks Steven!

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:451: #ifdef ENABLE_MESSAGE_CENTER
On 2013/01/18 19:56:19, stevenjb (chromium) wrote:
> Shouldn't we add && !USE_ASH here, otherwise we'll get two message center
> instances?

Done.

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.h:212: #if defined(ENABLE_MESSAGE_CENTER)
On 2013/01/18 19:56:19, stevenjb (chromium) wrote:
> Shouldn't we add && !USE_ASH here?

Done.
While this instance is created on demand and it should not be demanded in case
of USE_ASH, it's indeed a good point to safeguard here.

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.cc:28: #endif
On 2013/01/18 19:56:19, stevenjb (chromium) wrote:
> Would it make sense for MessageCenter* to be passed to this class in the
> constructor, to avoid dependencies here on browser_process / shall?

I just tried that, but found that it causes BrowserProcessImpl to know about
ash::Shell which it currently doesn't - because to pass the MessageCenter, the
BrowserProcessImpl that creates NotificationUIManager on all platforms, will
need to pull MessageCenter from ash::Shell.

It seems we want MessageCenter to live on ash::Shell in case of USE_ASH, that
makes WebNotificationTray testable w/o a browser. So there is going to be some
place that knows that MessageCenter can be pulled from Shell or some other
place... I'm very open to suggestions here.

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notification_manager.h (right):

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.h:41: //
MessageCenter::Delegate.
On 2013/01/18 19:56:19, stevenjb (chromium) wrote:
> nit: no . 

Done.

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.h:74:
std::string GetExtensionId();
On 2013/01/18 19:56:19, stevenjb (chromium) wrote:
> nit: WS

Done.

[stevenjb, 2013-01-18 22:50:04]

lgtm

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/11958025/diff/28002/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.cc:28: #endif
I see. So MessageCenter is owned by either ash::Shell or BrowserProcess, but
MessageCenterNotificationManager is always owned by BrowserProcess. Hmm.

I think it might be a little clearer to me to have this logic in
NotificationUIManager::Create(), since that already determines whether or not to
construct MessageCenterNotificationManager at all. I'll leave that to your
judgement however.

[Dmitry Titov, 2013-01-18 23:15:31]

> I think it might be a little clearer to me to have this logic in
> NotificationUIManager::Create(), since that already determines whether or not
to
> construct MessageCenterNotificationManager at all. 

I agree. It's better to keep all the ifdefs and conditions in one place. I've
moved the MessageCenter-retrieving code to NotificationUIManager::Create(). It
is not very readable file, but I hope we can simplify it soon.

[commit-bot: I haz the power, 2013-01-18 23:18:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dimich@chromium.org/11958025/20018

[commit-bot: I haz the power, 2013-01-18 23:18:48]

Presubmit check for 11958025-20018 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/test
    ash
    chrome/browser/extensions

Presubmit checks took 1.9s to calculate.

[Dmitry Titov, 2013-01-18 23:28:21]

Could you guys please take a look for owners bit?
chrome/browser, ash : sky
chrome/test : phajdan.jr

Thanks!

[Paweł Hajdan Jr., 2013-01-18 23:44:00]

chrome/test LGTM

[sky, 2013-01-22 19:29:00]

https://codereview.chromium.org/11958025/diff/22044/chrome/browser/browser_pr...
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/11958025/diff/22044/chrome/browser/browser_pr...
chrome/browser/browser_process.h:120: virtual message_center::MessageCenter*
message_center() = 0;
Should this be in a defined(ENABLE_MESSAGE_CENTER) ? Same with 58-61?

https://codereview.chromium.org/11958025/diff/22044/chrome/browser/notificati...
File chrome/browser/notifications/balloon_notification_ui_manager.cc (right):

https://codereview.chromium.org/11958025/diff/22044/chrome/browser/notificati...
chrome/browser/notifications/balloon_notification_ui_manager.cc:94: const
Notification& notification, Profile* profile) {
When you wrap each arg on its own line.

https://codereview.chromium.org/11958025/diff/22044/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notification_manager.h (right):

https://codereview.chromium.org/11958025/diff/22044/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.h:26:
MessageCenterNotificationManager(
explicit

[Dmitry Titov, 2013-01-22 21:01:07]

https://codereview.chromium.org/11958025/diff/22044/chrome/browser/browser_pr...
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/11958025/diff/22044/chrome/browser/browser_pr...
chrome/browser/browser_process.h:120: virtual message_center::MessageCenter*
message_center() = 0;
On 2013/01/22 19:29:00, sky wrote:
> Should this be in a defined(ENABLE_MESSAGE_CENTER) ? Same with 58-61?

Done. Also !(USE_ASH)

https://codereview.chromium.org/11958025/diff/22044/chrome/browser/notificati...
File chrome/browser/notifications/balloon_notification_ui_manager.cc (right):

https://codereview.chromium.org/11958025/diff/22044/chrome/browser/notificati...
chrome/browser/notifications/balloon_notification_ui_manager.cc:94: const
Notification& notification, Profile* profile) {
On 2013/01/22 19:29:00, sky wrote:
> When you wrap each arg on its own line.

Done.

https://codereview.chromium.org/11958025/diff/22044/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notification_manager.h (right):

https://codereview.chromium.org/11958025/diff/22044/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.h:26:
MessageCenterNotificationManager(
On 2013/01/22 19:29:00, sky wrote:
> explicit

Done.

[sky, 2013-01-22 21:46:39]

https://codereview.chromium.org/11958025/diff/39003/chrome/browser/browser_pr...
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/11958025/diff/39003/chrome/browser/browser_pr...
chrome/browser/browser_process.h:58: #if defined(ENABLE_MESSAGE_CENTER) &&
!defined(USE_ASH)
Why do we need the !ASH here? Can we instead set ENABLE_MESSAGE_CENTER to false
for ash?

[Dmitry Titov, 2013-01-22 22:09:24]

ENABLE_MESSAGE_CENTER is enabled on ash as well. This is a new implementation of
notification_ui_manager which directly delegate to MessageCenter and it's
enabled now on cromeos (ash) and win.

The separate angle is when we are USE_ASH, then MessageCenter instance lives on
ash::Shell, while if !USE_ASH, it lives on BrowserProcess.

So those are independent, and both should be checked.

[Dmitry Titov, 2013-01-22 23:09:41]

To summarize previous post:

ENABLE_MESSAGE_CENTER    USE_ASH
       0                    0       Current Mac and Linux - do not compile
MessageCenter in at all
       1                    0       Current Win - compiles MC in, but hosts MC
on BrowserProcess
       0                    1       Is not used
       1                    1       Current Ash - compiles MC in, but hosts MC
on ash::Shell

[sky, 2013-01-22 23:59:14]

What is the long term goal? Can't we have it live in BrowserProcess so
we don't need these ifdefs?

  -Scott

On Tue, Jan 22, 2013 at 2:09 PM,  <dimich@chromium.org> wrote:
> ENABLE_MESSAGE_CENTER is enabled on ash as well. This is a new
> implementation of
> notification_ui_manager which directly delegate to MessageCenter and it's
> enabled now on cromeos (ash) and win.
>
> The separate angle is when we are USE_ASH, then MessageCenter instance lives
> on
> ash::Shell, while if !USE_ASH, it lives on BrowserProcess.
>
> So those are independent, and both should be checked.
>
> https://codereview.chromium.org/11958025/

[Dmitry Titov, 2013-01-23 00:15:11]

The long-term goal is to remove ENABLE_MESSAGE_CENTER once we get
MessageCenter-based notifications implemented across platforms (likely when
views are supported on Linux and Mac but maybe before if that will prove long
haul)

However, USE_ASH is permanent because the WebNotificationTray which is a part of
Ash shell which is buildable w/o browser and it needs access to MessageCenter.
So there may be no BrowserProcess there. The MessageCenter is a collection of
'currently displayed' notifications and as such it has to be used by UI
components and by browser code, so it has to be accessible from both sides.
Keeping it on well-known globals solves this and normally it'd be fine on
BrowserProcess. However, since NotificationCenter UI (WebNotificationTray and
co) are the ash components, I don't think we can access g_browser_process from
there.
Totally open to suggestions here...

[sky, 2013-01-23 01:19:31]

Can't the BrowserProcess implementation call through to the shell?
That way no ifdef in browser_process.h and you get the right thing no
matter which route you go through.

  -Scott

On Tue, Jan 22, 2013 at 4:15 PM,  <dimich@chromium.org> wrote:
> The long-term goal is to remove ENABLE_MESSAGE_CENTER once we get
> MessageCenter-based notifications implemented across platforms (likely when
> views are supported on Linux and Mac but maybe before if that will prove
> long
> haul)
>
> However, USE_ASH is permanent because the WebNotificationTray which is a
> part of
> Ash shell which is buildable w/o browser and it needs access to
> MessageCenter.
> So there may be no BrowserProcess there. The MessageCenter is a collection
> of
> 'currently displayed' notifications and as such it has to be used by UI
> components and by browser code, so it has to be accessible from both sides.
> Keeping it on well-known globals solves this and normally it'd be fine on
> BrowserProcess. However, since NotificationCenter UI (WebNotificationTray
> and
> co) are the ash components, I don't think we can access g_browser_process
> from
> there.
> Totally open to suggestions here...
>
> https://codereview.chromium.org/11958025/

[Dmitry Titov, 2013-01-23 02:09:58]

On 2013/01/23 01:19:31, sky wrote:
> Can't the BrowserProcess implementation call through to the shell?
> That way no ifdef in browser_process.h and you get the right thing no
> matter which route you go through.

That sounds good, I tried this - see the last patch.
Although, it still requires ENABLE_MESSAGE_CENTER ifdefs (in browser_process.h
as well) for the case of Mac/Linux where MessageCenter is not used at all
(instead, a BalloonCollection is), and it needs a USE_ASH ifdef in
browser_process_impl.* to do a call through to ash::Shell in one case or have a
scoped_ptr with its own in another.
On a positive side, ENABLE_MESSAGE_CENTER will go away, and it simplified code
in notification_ui_manager.cc quite a bit.

[sky, 2013-01-23 16:12:08]

LGTM - thanks

[commit-bot: I haz the power, 2013-01-23 16:35:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dimich@chromium.org/11958025/42004

[commit-bot: I haz the power, 2013-01-23 19:40:15]

Change committed as 178352