The 1st patch to disambiguate message center notifications

The 1st patch to disambiguate message center notifications

This is the first half of the work to disambiguate the message center
notifications originated from different profiles.
It clarifies the semantics of message center as the backend of
notification by:

1) Adding comments on the usage of id in the MessageCenter interface.
2) Adding a FindVisibleNotificationById(id) method to message center
interface so users can directly find backend notifications without
going to the notification ui manager interface.
3) Removing the redundant HasNotification() method since the
FindVisibleNotificationById() can server the same purpose. 

Reviewers:
* davemoore: please review chrome/browser/chromeos/*
* jamescook: please review ash/*

TBR=davemoore@chromium.org
BUG=297867
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276756

[juyik, 2014-06-07 18:06:43]

Hi Justin, I will add owners of other dependent code as reviewers after you have
approved this change. Thanks.

[dewittj, 2014-06-09 18:38:19]

Please update your commit description to be better formatted for various tools. 
The "Subject:" field should be less than 72 chars long.  The first line of
"Description:" should be the same as the "Subject:" field, and followed by a
blank line.  Subsequent description lines should be <72 chars long as well. 
Thanks!

[juyik, 2014-06-09 18:51:58]

On 2014/06/09 18:38:19, dewittj wrote:
> Please update your commit description to be better formatted for various
tools. 
> The "Subject:" field should be less than 72 chars long.  The first line of
> "Description:" should be the same as the "Subject:" field, and followed by a
> blank line.  Subsequent description lines should be <72 chars long as well. 
> Thanks!

Done.

[dewittj, 2014-06-09 19:59:10]

lgtm

https://codereview.chromium.org/324583002/diff/1/chrome/browser/chromeos/net/...
File
chrome/browser/chromeos/net/network_portal_notification_controller_unittest.cc
(right):

https://codereview.chromium.org/324583002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller_unittest.cc:23:
return MessageCenter::Get()->FindVisibleNotificationById(kNotificationId) !=
prefer to split this into two statements

https://codereview.chromium.org/324583002/diff/1/ui/message_center/notificati...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/324583002/diff/1/ui/message_center/notificati...
ui/message_center/notification_list.cc:274: else
nit: no else clause, just return null

[dewittj, 2014-06-09 19:59:10]

lgtm

https://codereview.chromium.org/324583002/diff/1/chrome/browser/chromeos/net/...
File
chrome/browser/chromeos/net/network_portal_notification_controller_unittest.cc
(right):

https://codereview.chromium.org/324583002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller_unittest.cc:23:
return MessageCenter::Get()->FindVisibleNotificationById(kNotificationId) !=
prefer to split this into two statements

https://codereview.chromium.org/324583002/diff/1/ui/message_center/notificati...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/324583002/diff/1/ui/message_center/notificati...
ui/message_center/notification_list.cc:274: else
nit: no else clause, just return null

[dewittj, 2014-06-09 20:00:30]

https://codereview.chromium.org/324583002/diff/1/ui/message_center/notificati...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/324583002/diff/1/ui/message_center/notificati...
ui/message_center/notification.h:67: Notification(const std::string& id, const
Notification& other);
by the way, are you using this new constructor in this patch?  If not, save it
for the next patch.

[juyik, 2014-06-09 21:28:08]

https://codereview.chromium.org/324583002/diff/1/chrome/browser/chromeos/net/...
File
chrome/browser/chromeos/net/network_portal_notification_controller_unittest.cc
(right):

https://codereview.chromium.org/324583002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller_unittest.cc:23:
return MessageCenter::Get()->FindVisibleNotificationById(kNotificationId) !=
On 2014/06/09 19:59:10, dewittj wrote:
> prefer to split this into two statements

Done.

https://codereview.chromium.org/324583002/diff/1/ui/message_center/notificati...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/324583002/diff/1/ui/message_center/notificati...
ui/message_center/notification.h:67: Notification(const std::string& id, const
Notification& other);
On 2014/06/09 20:00:30, dewittj wrote:
> by the way, are you using this new constructor in this patch?  If not, save it
> for the next patch.

Done.

https://codereview.chromium.org/324583002/diff/1/ui/message_center/notificati...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/324583002/diff/1/ui/message_center/notificati...
ui/message_center/notification_list.cc:274: else
On 2014/06/09 19:59:10, dewittj wrote:
> nit: no else clause, just return null

Done.

[James Cook, 2014-06-09 22:10:23]

Also, please change the subject to say "message center notifications" or
something similar. "Notifications" is an overloaded term in the Chrome codebase
and just reading the subject line I thought you were changing
chrome::NOTIFICATION_FOO code.

https://codereview.chromium.org/324583002/diff/20001/ash/display/resolution_n...
File ash/display/resolution_notification_controller_unittest.cc (right):

https://codereview.chromium.org/324583002/diff/20001/ash/display/resolution_n...
ash/display/resolution_notification_controller_unittest.cc:127:
ResolutionNotificationController::kNotificationId) != NULL;
No need for comparison to NULL here or anywhere else.

https://codereview.chromium.org/324583002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/display/display_preferences_unittest.cc (right):

https://codereview.chromium.org/324583002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/display/display_preferences_unittest.cc:441:
EXPECT_TRUE(message_center::MessageCenter::Get()->FindVisibleNotificationById(
EXPECT_FALSE(message_center::MessageCenter::Get()->FindVisibleNotificationById());

[juyik, 2014-06-09 22:46:41]

Done.

https://codereview.chromium.org/324583002/diff/20001/ash/display/resolution_n...
File ash/display/resolution_notification_controller_unittest.cc (right):

https://codereview.chromium.org/324583002/diff/20001/ash/display/resolution_n...
ash/display/resolution_notification_controller_unittest.cc:127:
ResolutionNotificationController::kNotificationId) != NULL;
On 2014/06/09 22:10:23, James Cook wrote:
> No need for comparison to NULL here or anywhere else.

Done.

https://codereview.chromium.org/324583002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/display/display_preferences_unittest.cc (right):

https://codereview.chromium.org/324583002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/display/display_preferences_unittest.cc:441:
EXPECT_TRUE(message_center::MessageCenter::Get()->FindVisibleNotificationById(
On 2014/06/09 22:10:23, James Cook wrote:
>
EXPECT_FALSE(message_center::MessageCenter::Get()->FindVisibleNotificationById());
> 

Done.

[James Cook, 2014-06-09 23:27:21]

ash/* LGTM

[juyik, 2014-06-10 20:26:08]

Hi Dave, can you do a owner review of files under chrome/browser/chromeos/*?
Thanks.

[dewittj, 2014-06-10 20:26:56]

On 2014/06/10 20:26:08, juyik wrote:
> Hi Dave, can you do a owner review of files under chrome/browser/chromeos/*?
> Thanks.

I think tbr is appropriate in this case if you like.

[juyik, 2014-06-11 17:09:38]

The CQ bit was checked by juyik@chromium.org

[commit-bot: I haz the power, 2014-06-11 17:11:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/juyik@chromium.org/324583002/40001

[commit-bot: I haz the power, 2014-06-11 20:31:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 20:35:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-11 20:35:23]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[juyik, 2014-06-12 15:12:21]

The CQ bit was checked by juyik@chromium.org

[commit-bot: I haz the power, 2014-06-12 15:13:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/juyik@chromium.org/324583002/100001

[commit-bot: I haz the power, 2014-06-12 18:37:48]

Change committed as 276756