Adding implementation for MessageCenter::Delegate on MessageCeneterNotificationManager.

Adding implementation for MessageCenter::Delegate on MessageCeneterNotificationManager.
Also adding a simple browsertest.

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

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

[Dmitry Titov, 2013-01-24 20:48:27]

Steven, could you please take a look?

[stevenjb, 2013-01-24 23:59:54]

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:146:
DCHECK(!extension_id.empty()); // or UI should not have enabled the command.
two spaces before  //

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:156:
DCHECK(source_url.is_valid()); // or UI should not have enabled the command.
;  //

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:159:
profile_notification->profile());
nit: Don't need local variable 'source'

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:166:
FindProfileNotification(notification_id);
nit: Don't need local variable

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:174:
Profile* profile = profile_notification->profile();
nit: Don't need local variables

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:187:
FindProfileNotification(notification_id);
nit: Don't need local variable

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:194:
FindProfileNotification(notification_id);
nit: Don't need local variable

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:252:
NOTREACHED(); // If the notification is shown in UI, it must be in the map.
;  //

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:254: }
This should be DCHECK(iter != profile_notifications_.end()) instead of using
NOTREACHED(). If we want to handle this case and return NULL here, use
LOG(ERROR) instead.

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
File chrome/browser/notifications/message_center_notifications_browsertest.cc
(right):

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notifications_browsertest.cc:60:
const std::string& log() { return log_; }
blank line

[Dmitry Titov, 2013-01-25 00:47:45]

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:146:
DCHECK(!extension_id.empty()); // or UI should not have enabled the command.
On 2013/01/24 23:59:54, stevenjb (chromium) wrote:
> two spaces before  //

Done.

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:156:
DCHECK(source_url.is_valid()); // or UI should not have enabled the command.
On 2013/01/24 23:59:54, stevenjb (chromium) wrote:
> ;  //

Done.

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:159:
profile_notification->profile());
On 2013/01/24 23:59:54, stevenjb (chromium) wrote:
> nit: Don't need local variable 'source'

Done.

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:166:
FindProfileNotification(notification_id);
On 2013/01/24 23:59:54, stevenjb (chromium) wrote:
> nit: Don't need local variable 

Done.

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:174:
Profile* profile = profile_notification->profile();
On 2013/01/24 23:59:54, stevenjb (chromium) wrote:
> nit: Don't need local variables

Removed 'profile', left those used twice...

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:187:
FindProfileNotification(notification_id);
On 2013/01/24 23:59:54, stevenjb (chromium) wrote:
> nit: Don't need local variable

Done.

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:194:
FindProfileNotification(notification_id);
On 2013/01/24 23:59:54, stevenjb (chromium) wrote:
> nit: Don't need local variable

Done.

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:252:
NOTREACHED(); // If the notification is shown in UI, it must be in the map.
On 2013/01/24 23:59:54, stevenjb (chromium) wrote:
> ;  //

Done.

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notification_manager.cc:254: }
On 2013/01/24 23:59:54, stevenjb (chromium) wrote:
> This should be DCHECK(iter != profile_notifications_.end()) instead of using
> NOTREACHED(). If we want to handle this case and return NULL here, use
> LOG(ERROR) instead.

Done.

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
File chrome/browser/notifications/message_center_notifications_browsertest.cc
(right):

https://codereview.chromium.org/11896085/diff/5004/chrome/browser/notificatio...
chrome/browser/notifications/message_center_notifications_browsertest.cc:60:
const std::string& log() { return log_; }
On 2013/01/24 23:59:54, stevenjb (chromium) wrote:
> blank line

Done.

[stevenjb, 2013-01-25 01:28:49]

lgtm

[commit-bot: I haz the power, 2013-01-25 10:18:55]

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

[commit-bot: I haz the power, 2013-01-25 10:18:57]

Failed to apply patch for chrome/chrome_tests.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/chrome_tests.gypi
  Hunk #1 FAILED at 1146.
  Hunk #2 succeeded at 1719 (offset 44 lines).
  1 out of 2 hunks FAILED -- saving rejects to file chrome/chrome_tests.gypi.rej

Patch:       chrome/chrome_tests.gypi
Index: chrome/chrome_tests.gypi
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index
12339794ead64afa970f27028dc6710e69627c06..a540ebb8a39bd3a2cba63e4463920abd601ee815
100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1146,6 +1146,7 @@
         'browser/net/load_timing_observer_browsertest.cc',
         'browser/net/predictor_browsertest.cc',
         'browser/net/proxy_browsertest.cc',
+        'browser/notifications/message_center_notifications_browsertest.cc',
         'browser/page_cycler/page_cycler_browsertest.cc',
         'browser/performance_monitor/performance_monitor_browsertest.cc',
         'browser/policy/cloud_policy_browsertest.cc',
@@ -1675,6 +1676,11 @@
             ['exclude', '^browser/ui/app_list/'],
           ],
         }],
+        ['enable_message_center==0', {
+          'sources!': [
+           
'browser/notifications/message_center_notifications_browsertest.cc',
+          ],
+        }],
       ],  # conditions
     },  # target browser_tests
     {

[commit-bot: I haz the power, 2013-01-25 19:34:37]

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

[commit-bot: I haz the power, 2013-01-25 21:40:15]

Change committed as 178898

[Dmitry Titov, 2013-01-25 22:35:14]

This was reverted due to chromeos clang issue on waterfall. The
linux_chromeos_clang is not a part of default try job/CQ set, and the new file
does not compile for linux/mac yet, so it missed the clang.
Fixing the trivial error and will re-land.

[commit-bot: I haz the power, 2013-01-25 22:51:45]

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

[commit-bot: I haz the power, 2013-01-26 02:40:38]

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

[commit-bot: I haz the power, 2013-01-26 05:30:55]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-01-26 07:51:45]

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

[commit-bot: I haz the power, 2013-01-26 09:33:03]

Change committed as 179059