Fixes the context menu for a notification.

Fixes the context menu for a notification.

- The contents of the menu is independent from views, so put it
  into message_center_tray.
- Then it can be unit-testable. wrote test case.
- Also MessageCenterTrayDelegate can control availability of the menu.
- views::ContextMenuController is not necessarily owned by views::View.
      Moving it to outside of MessageView will reduce a few lines of code
      from MessageViewController.

BUG=327828
R=dewittj@chromium.org, stevenjb@chromium.org
TBR=rsesek@chromium.org
TEST=message_center_unittests covers

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

[stevenjb, 2013-12-12 21:19:45]

https://codereview.chromium.org/114323002/diff/1/chrome/browser/ui/views/mess...
File chrome/browser/ui/views/message_center/web_notification_tray.cc (right):

https://codereview.chromium.org/114323002/diff/1/chrome/browser/ui/views/mess...
chrome/browser/ui/views/message_center/web_notification_tray.cc:196: // the
context menu shouldn't be enabled, such like lock screen.
nit: s/such like/such as in the/

https://codereview.chromium.org/114323002/diff/1/ui/message_center/message_ce...
File ui/message_center/message_center_tray.cc (right):

https://codereview.chromium.org/114323002/diff/1/ui/message_center/message_ce...
ui/message_center/message_center_tray.cc:142: }
This seems like it ought to be in MessageCenter, e.g.
context_menu_notification_ = message_center_->GetNotificationById(id);

https://codereview.chromium.org/114323002/diff/1/ui/message_center/views/noti...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/114323002/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:437: 
nit: Put in internal namespace, i.e. message_center::internal

https://codereview.chromium.org/114323002/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:441:
NotificationViewContextMenuController(MessageCenterTray* tray);
explicit

https://codereview.chromium.org/114323002/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:451: MessageCenterTray* tray_;
nit: WS

https://codereview.chromium.org/114323002/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:535: new
NotificationViewContextMenuController(tray)) {
context_menu_controller_ is a View member so it shouldn't get set here.

https://codereview.chromium.org/114323002/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:536:
set_context_menu_controller(context_menu_controller_.get());
This should be:
set_context_menu_controller(new NotificationViewContextMenuController(tray));

[dewittj, 2013-12-12 21:25:29]

I think that dmitry is working on a very similar patch in crrev.com/86083002 -
possibly want to coordinate

[Jun Mukai, 2013-12-14 01:10:08]

On 2013/12/12 21:25:29, dewittj wrote:
> I think that dmitry is working on a very similar patch in crrev.com/86083002 -
> possibly want to coordinate

PTAL.

Restructure the code a bit to support the new design of views
(MessageViewController and MessageCenterController).
The main difference from the previous patchset is:
- views::ContextMenuController is not necessarily owned by its views::View.
  So now it's owned by MessageCenterView or MessagePopupCollection.
- Removed no-more-used methods from MessageCenterController and
MessageViewController

thanks

[Jun Mukai, 2013-12-14 01:13:14]

forgotten to address the comments... sorry.

https://codereview.chromium.org/114323002/diff/1/chrome/browser/ui/views/mess...
File chrome/browser/ui/views/message_center/web_notification_tray.cc (right):

https://codereview.chromium.org/114323002/diff/1/chrome/browser/ui/views/mess...
chrome/browser/ui/views/message_center/web_notification_tray.cc:196: // the
context menu shouldn't be enabled, such like lock screen.
On 2013/12/12 21:19:46, stevenjb wrote:
> nit: s/such like/such as in the/

Done.

https://codereview.chromium.org/114323002/diff/1/ui/message_center/message_ce...
File ui/message_center/message_center_tray.cc (right):

https://codereview.chromium.org/114323002/diff/1/ui/message_center/message_ce...
ui/message_center/message_center_tray.cc:142: }
On 2013/12/12 21:19:46, stevenjb wrote:
> This seems like it ought to be in MessageCenter, e.g.
> context_menu_notification_ = message_center_->GetNotificationById(id);

This actually depends on the tray, for executing ShowNotifierSettingsBubble().

https://codereview.chromium.org/114323002/diff/1/ui/message_center/views/noti...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/114323002/diff/1/ui/message_center/views/noti...
ui/message_center/views/notification_view.cc:437: 
On 2013/12/12 21:19:46, stevenjb wrote:
> nit: Put in internal namespace, i.e. message_center::internal

this code has been removed.

[dewittj, 2013-12-16 19:08:29]

lgtm

[stevenjb, 2013-12-16 19:30:50]

lgtm

[Jun Mukai, 2013-12-16 20:45:17]

will submit with TBR=rsesek for a minor change on c/b/ui/cocoa/notifications

[Robert Sesek, 2013-12-16 20:45:49]

LGTM

[commit-bot: I haz the power, 2013-12-16 20:57:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/114323002/70001

[commit-bot: I haz the power, 2013-12-16 22:34:19]

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

[commit-bot: I haz the power, 2013-12-17 18:03:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/114323002/90001

[commit-bot: I haz the power, 2013-12-17 18:32:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/114323002/90001

[commit-bot: I haz the power, 2013-12-17 21:49:27]

Retried try job too often on win_x64_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[commit-bot: I haz the power, 2013-12-17 22:05:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/114323002/110001

[commit-bot: I haz the power, 2013-12-18 01:00:36]

Retried try job too often on win_x64_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[commit-bot: I haz the power, 2013-12-18 01:30:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/114323002/130001

[commit-bot: I haz the power, 2013-12-18 04:05:46]

Change committed as 241474