Remove ENABLE_MESSAGE_CENTER

Remove ENABLE_MESSAGE_CENTER

Next step on re-factoring Notifications. This will bring the MessageCenter classes, most important message_center::Notification, into build on all systems, including those where the MessageCenter is not yet appearign in UI. This will allow to start using this Notification class in client code and remove the old Notification class defined in chrome/browser/notificaitons/notification.h

That will allow the clients that already use Rich Notifications to use richer data type support, for example supply an image bits for a Notification (as in Snapshot notifications).

This also removes a lot of compile-time @ifdefs and replaces them with checking a runtime flag which we already have anyways.

On Android and iOS, the MessageCenter is not compiled in, for the size concerns and uncertain story for notifications in general - the existing ENABLE_NOTIFICATIONS define is used for that.

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

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

[Dmitry Titov, 2013-05-08 00:04:02]

Reviewers: 

dewittj@ - a general review
sky@ - OWNERS look for a lot of scattered dirs.

Thanks!

[dewittj, 2013-05-08 18:53:01]

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/notifications/notifications_api.cc:149: if
(message_center::IsRichNotificationEnabled()) {
need a "!" in the conditional?

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/notifications/notifications_api.cc:170:
g_browser_process->notification_ui_manager()->Add(notification, profile());
put a return here to buy one level of indentation for the remaining code in the
function.

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
File chrome/browser/extensions/extension_crash_recovery_browsertest.cc (right):

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/extension_crash_recovery_browsertest.cc:193:
LOG(ERROR) << "Basic 1";
These log statements should probably be removed.

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
File chrome/browser/extensions/notifications_apitest.cc (right):

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/notifications_apitest.cc:63: return;
This is fine, but I wonder if this is a good place for a NOTIMPLEMENTED()?

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/notifications_apitest.cc:86: // Test is looking for
HTML Notificaitons.
nit: spelling

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:21:
#include "ui/message_center/message_center_util.h"
Why add these lines?

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/synced_notification.cc:342: } else {
While we're in here we should remove this else statement and just put the
following block in the main flow of the function.

https://codereview.chromium.org/15025002/diff/50001/ui/message_center/dummy_m...
File ui/message_center/dummy_message_center.cc (right):

https://codereview.chromium.org/15025002/diff/50001/ui/message_center/dummy_m...
ui/message_center/dummy_message_center.cc:14: //#error This file should only be
used in Android or iOS builds.
should this be commented out?

https://codereview.chromium.org/15025002/diff/50001/ui/message_center/dummy_m...
ui/message_center/dummy_message_center.cc:24: const char
kDisableRichNotifications[] = "disable-rich-notifications";
Can we just include messag_center_switches.h/cc in the android and ios builds? 
I imagine this will cause a lot of try failures.

[Dmitry Titov, 2013-05-08 23:03:26]

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/notifications/notifications_api.cc:149: if
(message_center::IsRichNotificationEnabled()) {
On 2013/05/08 18:53:01, dewittj wrote:
> need a "!" in the conditional?

oops. Done. Tested that Galore works :-)

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/notifications/notifications_api.cc:170:
g_browser_process->notification_ui_manager()->Add(notification, profile());
On 2013/05/08 18:53:01, dewittj wrote:
> put a return here to buy one level of indentation for the remaining code in
the
> function.

Done.

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
File chrome/browser/extensions/extension_crash_recovery_browsertest.cc (right):

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/extension_crash_recovery_browsertest.cc:193:
LOG(ERROR) << "Basic 1";
On 2013/05/08 18:53:01, dewittj wrote:
> These log statements should probably be removed.

Done.

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
File chrome/browser/extensions/notifications_apitest.cc (right):

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/notifications_apitest.cc:63: return;
On 2013/05/08 18:53:01, dewittj wrote:
> This is fine, but I wonder if this is a good place for a NOTIMPLEMENTED()?

Done, added a special macro for that.

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/notifications_apitest.cc:86: // Test is looking for
HTML Notificaitons.
On 2013/05/08 18:53:01, dewittj wrote:
> nit: spelling

Removed the whole test. It's giving exactly the coverage of the previous one,
running the same test under same condition (no rich notifications). Renamed the
previous one to make it clear.

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:21:
#include "ui/message_center/message_center_util.h"
On 2013/05/08 18:53:01, dewittj wrote:
> Why add these lines?

Indeed, not needed after merge :-) Removed.

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/15025002/diff/50001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/synced_notification.cc:342: } else {
On 2013/05/08 18:53:01, dewittj wrote:
> While we're in here we should remove this else statement and just put the
> following block in the main flow of the function.

I've started to do that but then I realized I don't understand this code enough,
and I'd convert it into a switch() with a defualt label... Maybe it's an ask for
Peter?

https://codereview.chromium.org/15025002/diff/50001/ui/message_center/dummy_m...
File ui/message_center/dummy_message_center.cc (right):

https://codereview.chromium.org/15025002/diff/50001/ui/message_center/dummy_m...
ui/message_center/dummy_message_center.cc:14: //#error This file should only be
used in Android or iOS builds.
On 2013/05/08 18:53:01, dewittj wrote:
> should this be commented out?

Yes! Thanks, done.

https://codereview.chromium.org/15025002/diff/50001/ui/message_center/dummy_m...
ui/message_center/dummy_message_center.cc:24: const char
kDisableRichNotifications[] = "disable-rich-notifications";
On 2013/05/08 18:53:01, dewittj wrote:
> Can we just include messag_center_switches.h/cc in the android and ios builds?

> I imagine this will cause a lot of try failures.

Done.

[dewittj, 2013-05-08 23:52:01]

awesome. lgtm

[Dmitry Titov, 2013-05-09 01:22:27]

Scott, it's ready for OWNERS look in:
ash/
chrome/test
chrome/browser

[sky, 2013-05-09 15:50:38]

https://codereview.chromium.org/15025002/diff/72002/chrome/browser/extensions...
File chrome/browser/extensions/extension_crash_recovery_browsertest.cc (right):

https://codereview.chromium.org/15025002/diff/72002/chrome/browser/extensions...
chrome/browser/extensions/extension_crash_recovery_browsertest.cc:162: } else {
nit: no else (see style guide)

https://codereview.chromium.org/15025002/diff/72002/chrome/browser/extensions...
File chrome/browser/extensions/notifications_apitest.cc (right):

https://codereview.chromium.org/15025002/diff/72002/chrome/browser/extensions...
chrome/browser/extensions/notifications_apitest.cc:63:
SKIP_THIS_TEST_IF_MESSAGE_CENTER_DISABLED();
Having this compile switch still results in spinning up the browser (which takes
quite a while). Can we instead DISABLE the tests were applicable?

[Dmitry Titov, 2013-05-10 00:16:45]

https://codereview.chromium.org/15025002/diff/72002/chrome/browser/extensions...
File chrome/browser/extensions/extension_crash_recovery_browsertest.cc (right):

https://codereview.chromium.org/15025002/diff/72002/chrome/browser/extensions...
chrome/browser/extensions/extension_crash_recovery_browsertest.cc:162: } else {
On 2013/05/09 15:50:38, sky wrote:
> nit: no else (see style guide)

Done.

https://codereview.chromium.org/15025002/diff/72002/chrome/browser/extensions...
File chrome/browser/extensions/notifications_apitest.cc (right):

https://codereview.chromium.org/15025002/diff/72002/chrome/browser/extensions...
chrome/browser/extensions/notifications_apitest.cc:63:
SKIP_THIS_TEST_IF_MESSAGE_CENTER_DISABLED();
On 2013/05/09 15:50:38, sky wrote:
> Having this compile switch still results in spinning up the browser (which
takes
> quite a while). Can we instead DISABLE the tests were applicable?

Done.

[sky, 2013-05-10 05:05:14]

LGTM

[commit-bot: I haz the power, 2013-05-10 21:38:38]

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

[commit-bot: I haz the power, 2013-05-11 19:23:23]

Change committed as 199625

[commit-bot: I haz the power, 2013-05-14 01:15:36]

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

[commit-bot: I haz the power, 2013-05-14 05:30:25]

Change committed as 199920