Made --enable-rich-notifications flag available to message_center.

Made --enable-rich-notifications flag available to message_center.

Also added an --enable-new-simple-notifications flag to be used to control the appearance of simple notifications (notifications created using the HTML5 webkitNotifications API instead of the new chrome.notification APIs).

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

[dharcourt, 2013-01-26 02:14:13]

@mukai for overall review.
@stevenjb for OWNERS review of
chrome/browser/notifications/notification_ui_manager.cc changes.

Thanks for the review!

    - Charles

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

https://codereview.chromium.org/12096004/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/notification_ui_manager.cc:19:
message_center::switches::kEnableRichNotifications);
This constant was moved from chrome/common/chrome_switches.h to
ui/message_center/message_center_switches.h so it could be available to
ui/message_center/message_simple_view.h as well as notification_ui_manager.cc.

https://codereview.chromium.org/12096004/diff/1/ui/message_center/message_sim...
File ui/message_center/message_simple_view.h (right):

https://codereview.chromium.org/12096004/diff/1/ui/message_center/message_sim...
ui/message_center/message_simple_view.h:36: #endif //
UI_MESSAGE_CENTER_MESSAGE_SIMPLE_VIEW_H_
This was restored from subversion 178195, with the only change being the
addition of the old_style_close_button_ instance variable and ButtonPressed()
handling so MessageSimpleView can create its own style of close button now that
MessageView creates a different style.

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

https://codereview.chromium.org/12096004/diff/1/ui/message_center/message_vie...
ui/message_center/message_view.h:69: scoped_ptr<views::ImageButton>
close_button_;
Using a scoped_ptr allows close_button to be ignored by subclasses like
MessageSimpleView. Previously subclasses had to insert close_button in the view
hierarchy to make sure it was deleted when the view was deleted, or they had to
delete it manually.

[Jun Mukai, 2013-01-26 02:18:46]

Probably better to ping dimich too

Overall looks good, but can you split this into two CLs? One is for
adding/moving flags, and the other is the actual roll back.

[dharcourt, 2013-01-26 02:44:59]

The restore-simple-notification-look functionality was split into
http://crrev.com/12090011 so these changes only concern the moving of the
--enable-rich-notifications flag into ui/message_center and the addition of an
as-yet-unused --enable-new-simple-notifications flag.

Jun and Dmitry, please review, I'll wait for lgtms from _both_ of you before
committing.

Steven, I'll also wait for an OWNERS lgtm from you for
notification_ui_manager.cc before committing.

Thanks for your help,

    - Charles

[Jun Mukai, 2013-01-26 02:52:52]

https://codereview.chromium.org/12096004/diff/3002/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/12096004/diff/3002/chrome/browser/about_flags...
chrome/browser/about_flags.cc:33: #include
"ui/message_center/message_center_switches.h"
#if defined(ENABLE_MESSAGE_CENTER) is needed here

https://codereview.chromium.org/12096004/diff/3002/chrome/browser/about_flags...
chrome/browser/about_flags.cc:1200: {
also ENABLE_MESSAGE_CENTER here

[Dmitry Titov, 2013-01-26 02:53:06]

I think we currently do not compile the message_center dependency in Mac and
Linux at least - perhaps using a flag from there will not compile on those?

[Dmitry Titov, 2013-01-26 02:54:50]

https://codereview.chromium.org/12096004/diff/3002/chrome/browser/notificatio...
File chrome/browser/notifications/notification_ui_manager.cc (right):

https://codereview.chromium.org/12096004/diff/3002/chrome/browser/notificatio...
chrome/browser/notifications/notification_ui_manager.cc:9: #include
"ui/message_center/message_center_switches.h"
#if defined(ENABLE_MESSAGE_CENTER) needed.

https://codereview.chromium.org/12096004/diff/3002/chrome/browser/notificatio...
chrome/browser/notifications/notification_ui_manager.cc:19:
message_center::switches::kEnableRichNotifications);
Ditto here.

[Dmitry Titov, 2013-01-26 02:58:28]

Also, if https://codereview.chromium.org/11896085/ lands before this one, you'll
need to combine flags in notification_ui_manager, like this:

#if defined(ENABLE_MESSAGE_CENTER) && !defined(USE_ASH)
  return CommandLine::ForCurrentProcess()->HasSwitch(
      message_center::switches::kEnableRichNotifications);
#endif

[dharcourt, 2013-01-26 04:07:04]

https://codereview.chromium.org/12096004/diff/3002/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/12096004/diff/3002/chrome/browser/about_flags...
chrome/browser/about_flags.cc:33: #include
"ui/message_center/message_center_switches.h"
On 2013/01/26 02:52:52, Jun Mukai wrote:
> #if defined(ENABLE_MESSAGE_CENTER) is needed here

Done.

https://codereview.chromium.org/12096004/diff/3002/chrome/browser/about_flags...
chrome/browser/about_flags.cc:1200: {
On 2013/01/26 02:52:52, Jun Mukai wrote:
> also ENABLE_MESSAGE_CENTER here

Done.

https://codereview.chromium.org/12096004/diff/3002/chrome/browser/notificatio...
File chrome/browser/notifications/notification_ui_manager.cc (right):

https://codereview.chromium.org/12096004/diff/3002/chrome/browser/notificatio...
chrome/browser/notifications/notification_ui_manager.cc:9: #include
"ui/message_center/message_center_switches.h"
On 2013/01/26 02:54:50, Dmitry Titov wrote:
> #if defined(ENABLE_MESSAGE_CENTER) needed.

Done.

https://codereview.chromium.org/12096004/diff/3002/chrome/browser/notificatio...
chrome/browser/notifications/notification_ui_manager.cc:19:
message_center::switches::kEnableRichNotifications);
On 2013/01/26 02:54:50, Dmitry Titov wrote:
> Ditto here.

Done.

[Dmitry Titov, 2013-01-26 04:44:06]

Lgtm

[stevenjb, 2013-01-28 21:46:32]

lgtm

[dharcourt, 2013-01-28 22:54:18]

Hi Jun,

Let me know if you're planning on reviewing this further or I should go with
Dmitri's LGTM.

Thanks,

    - Charles

[Jun Mukai, 2013-01-28 23:13:26]

lgtm

[commit-bot: I haz the power, 2013-01-29 06:33:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/12096004/5011

[commit-bot: I haz the power, 2013-01-29 06:49:53]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-29 07:26:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/12096004/5011

[commit-bot: I haz the power, 2013-01-29 07:41:32]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-29 15:42:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/12096004/5011

[commit-bot: I haz the power, 2013-01-29 16:13:02]

Change committed as 179341