Make MessageCenter a singleton object and build on Windows.

Allow easier cross-platform access to MessageCenter by making it a singleton object, and add it to gyp files on Windows.

TBR=sky@chromium.org
R=stevenjb@chromium.org


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175081

[dewittj, 2012-12-27 19:01:31]

This change is to support Dmitry's work on refactoring NotificationUIManager by
enabling easy access to the message center on all platforms.

stevenjb: I added tray_bubble_view.{cc,h} back into views.gyp, which you
reviewed the removal of in https://codereview.chromium.org/11293124.  They are
required to include messsage_center.gyp, and will compile, though they're not in
use just yet.
 
sky: gyp changes

[stevenjb, 2012-12-27 19:49:34]

https://codereview.chromium.org/11684003/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/11684003/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/message_center_notification_manager.cc:15:
message_center_ = message_center::MessageCenter::GetInstance();
We don't need to seem to need this yet - maybe wait until we do to add it?

https://codereview.chromium.org/11684003/diff/1/ui/message_center/message_cen...
File ui/message_center/message_center.cc (right):

https://codereview.chromium.org/11684003/diff/1/ui/message_center/message_cen...
ui/message_center/message_center.cc:23: void MessageCenter::SetHost(Host* host)
{
DCHECK(!host_) since we don't support multiple hosts or host handoff.

https://codereview.chromium.org/11684003/diff/1/ui/message_center/message_cen...
File ui/message_center/message_center.h (right):

https://codereview.chromium.org/11684003/diff/1/ui/message_center/message_cen...
ui/message_center/message_center.h:80: // bubbles.  |host| can be NULL.
I would remove the '|host| can be NULL' part of the comment. The code won't
crash, but that doesn't seem like meaningful usage.

[dewittj, 2012-12-27 23:38:50]

https://codereview.chromium.org/11684003/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/11684003/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/message_center_notification_manager.cc:15:
message_center_ = message_center::MessageCenter::GetInstance();
On 2012/12/27 19:49:34, stevenjb (chromium) wrote:
> We don't need to seem to need this yet - maybe wait until we do to add it?

Done.

Good idea.

https://codereview.chromium.org/11684003/diff/1/ui/message_center/message_cen...
File ui/message_center/message_center.cc (right):

https://codereview.chromium.org/11684003/diff/1/ui/message_center/message_cen...
ui/message_center/message_center.cc:23: void MessageCenter::SetHost(Host* host)
{
On 2012/12/27 19:49:34, stevenjb (chromium) wrote:
> DCHECK(!host_) since we don't support multiple hosts or host handoff.

Since the MessageCenter is now not being destroyed until exit (and separately
from the host), I figured it'd be best to be able to SetHost(NULL) when I
destroy the host to prevent calling OnMessageCenterChanged on a destroyed
object.

Unfortunately, the DCHECK doesn't seem to work with the ash_unittests which
create multiple status trays without exiting the process.  Is there a reason we
don't support host handoff?  Since the only thing the host is doing is receiving
events it doesn't seem like handoff is a big deal.

If that DCHECK is important, I'll have to rethink the Singleton<> paradigm and
use something else.

https://codereview.chromium.org/11684003/diff/1/ui/message_center/message_cen...
File ui/message_center/message_center.h (right):

https://codereview.chromium.org/11684003/diff/1/ui/message_center/message_cen...
ui/message_center/message_center.h:80: // bubbles.  |host| can be NULL.
On 2012/12/27 19:49:34, stevenjb (chromium) wrote:
> I would remove the '|host| can be NULL' part of the comment. The code won't
> crash, but that doesn't seem like meaningful usage.

Done.

[stevenjb, 2013-01-02 16:35:02]

https://codereview.chromium.org/11684003/diff/1/ui/message_center/message_cen...
File ui/message_center/message_center.cc (right):

https://codereview.chromium.org/11684003/diff/1/ui/message_center/message_cen...
ui/message_center/message_center.cc:23: void MessageCenter::SetHost(Host* host)
{
On 2012/12/27 23:38:50, dewittj wrote:
> On 2012/12/27 19:49:34, stevenjb (chromium) wrote:
> > DCHECK(!host_) since we don't support multiple hosts or host handoff.
> 
> Since the MessageCenter is now not being destroyed until exit (and separately
> from the host), I figured it'd be best to be able to SetHost(NULL) when I
> destroy the host to prevent calling OnMessageCenterChanged on a destroyed
> object.
> 
> Unfortunately, the DCHECK doesn't seem to work with the ash_unittests which
> create multiple status trays without exiting the process.  Is there a reason
we
> don't support host handoff?  Since the only thing the host is doing is
receiving
> events it doesn't seem like handoff is a big deal.
> 
> If that DCHECK is important, I'll have to rethink the Singleton<> paradigm and
> use something else.

Thinking about this some more, it seems like we need to support multiple "hosts"
since we may have multiple status trays "hosting" the message center when there
are multiple displays. Looking at the code, the host is really an Observer, and
we should probably use ObserverList for this.

[dewittj, 2013-01-03 21:50:33]

ObserverList makes more sense, I agree.  As far as setting the delegate, I
imagine that the only time we'll be setting it more than once is in test.  Would
you recommend removing the DCHECK (as in the current patch) or adding a
SetDelegateForTest method that could clear the notifications and allows delegate
transfer?

[stevenjb, 2013-01-03 22:32:45]

Ah, I see, I was getting host_ and delegate_ confused. This is definitely better
with observer_list_ and delegate_.

I think this is fine as-is. Since there is no ownership relationship with the
delegate_, setting it to NULL or to a test delegate is fine.

LGTM

https://codereview.chromium.org/11684003/diff/30001/ui/message_center/message...
File ui/message_center/message_center.h (right):

https://codereview.chromium.org/11684003/diff/30001/ui/message_center/message...
ui/message_center/message_center.h:83: // NotificationList.
I would change this to something like:
// Called to set the delegate. Generally called only once, except in tests.
Changing the delegate does not affect notifications in its NotificationList.

[dewittj, 2013-01-04 00:18:14]

https://codereview.chromium.org/11684003/diff/30001/ui/message_center/message...
File ui/message_center/message_center.h (right):

https://codereview.chromium.org/11684003/diff/30001/ui/message_center/message...
ui/message_center/message_center.h:83: // NotificationList.
On 2013/01/03 22:32:45, stevenjb (chromium) wrote:
> I would change this to something like:
> // Called to set the delegate. Generally called only once, except in tests.
> Changing the delegate does not affect notifications in its NotificationList.

Done.

[commit-bot: I haz the power, 2013-01-04 00:53:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/11684003/24004

[commit-bot: I haz the power, 2013-01-04 02:08:34]

Change committed as 175081