New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 869278 link

Starred by 1 user
Status: Started
Owner:
█ yoshiki@chromium.org
Last visit > 30 days ago
Cc:
tetsui@chromium.org
peter@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Refactor message_center::UiController/UiDelegate

Project Member Reported by yoshiki@chromium.org, Jul 31 
This logic is somewhat complex because we share the logic between notification-with-message-center (CrOS) and notification-without-message-center (Win/Mac/Linux). I think we can split them and move these into chrome/browser/notification/ (for Win/Mac/Linux) and ash (for CrOS).

The first CL is https://chromium-review.googlesource.com/c/chromium/src/+/1154617

Comment 1 by yoshiki@chromium.org, Jul 31

Components: UI>Notifications
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 1 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/09f0bb3299abf11756f1a09b1b59ab201ef9b57b

commit 09f0bb3299abf11756f1a09b1b59ab201ef9b57b
Author: yoshiki iguchi <yoshiki@chromium.org>
Date: Wed Aug 01 06:52:08 2018

Extract the popup-only UI code from ui/message_center

message_center::UiController and UiDelegate were used by both the platform
with only popup and with the message center and popup. This CL is the first
CL of refactoring to split them into two for simplification.
The current dependency:
 CrOS: UiController --UiDelegate--> NotificationTray/UnifiedSystemTray
 Old Win/Linux: UiController --UiDelegate--> PopupsOnlyUiDelegate
 Old Mac: UiController --UiDelegate--> MessageCenterBridge

The new dependency:
 CrOS: UiController --UiDelegate--> NotificationTray/UnifiedSystemTray
 Old Win/Linux: PopupsOnlyUiController(new) --::Delegate(new)--> PopupsOnlyUiDelegate
 Old MacMac: PopupsOnlyUiController(new) --::Delegate(new)--> MessageCenterBridge

I will move the existing UiController and UiDelegate to ash/ and refactor
them in following CLs.

Bug: 869278
Test: Notification works on Linux

Change-Id: I8ab077f0339cb55cf25633aa481509f1775d43dd
Reviewed-on: https://chromium-review.googlesource.com/1154617
Commit-Queue: Yoshiki Iguchi <yoshiki@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Tetsui Ohkubo <tetsui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579715}
[modify] https://crrev.com/09f0bb3299abf11756f1a09b1b59ab201ef9b57b/chrome/browser/BUILD.gn
[modify] https://crrev.com/09f0bb3299abf11756f1a09b1b59ab201ef9b57b/chrome/browser/notifications/message_center_notification_manager.cc
[modify] https://crrev.com/09f0bb3299abf11756f1a09b1b59ab201ef9b57b/chrome/browser/notifications/message_center_notification_manager.h
[modify] https://crrev.com/09f0bb3299abf11756f1a09b1b59ab201ef9b57b/chrome/browser/notifications/message_center_notifications_unittest.cc
[add] https://crrev.com/09f0bb3299abf11756f1a09b1b59ab201ef9b57b/chrome/browser/notifications/popups_only_ui_controller.cc
[add] https://crrev.com/09f0bb3299abf11756f1a09b1b59ab201ef9b57b/chrome/browser/notifications/popups_only_ui_controller.h
[modify] https://crrev.com/09f0bb3299abf11756f1a09b1b59ab201ef9b57b/chrome/browser/ui/cocoa/notifications/message_center_bridge.h
[modify] https://crrev.com/09f0bb3299abf11756f1a09b1b59ab201ef9b57b/chrome/browser/ui/cocoa/notifications/message_center_bridge.mm
[modify] https://crrev.com/09f0bb3299abf11756f1a09b1b59ab201ef9b57b/chrome/browser/ui/views/message_center/popups_only_ui_delegate.cc
[modify] https://crrev.com/09f0bb3299abf11756f1a09b1b59ab201ef9b57b/chrome/browser/ui/views/message_center/popups_only_ui_delegate.h
[modify] https://crrev.com/09f0bb3299abf11756f1a09b1b59ab201ef9b57b/chrome/browser/ui/views/message_center/popups_only_ui_delegate_unittest.cc
[modify] https://crrev.com/09f0bb3299abf11756f1a09b1b59ab201ef9b57b/ui/message_center/BUILD.gn
[delete] https://crrev.com/c4138a471b04ad6f4cbd59c426cad4a7832e83c8/ui/message_center/fake_ui_delegate.cc
[delete] https://crrev.com/c4138a471b04ad6f4cbd59c426cad4a7832e83c8/ui/message_center/fake_ui_delegate.h
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 2 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6043d5cd74e065fde12c8a62fb5f3dfb878ddd9a

commit 6043d5cd74e065fde12c8a62fb5f3dfb878ddd9a
Author: yoshiki iguchi <yoshiki@chromium.org>
Date: Thu Aug 02 08:26:03 2018

Add tests to click a notification body to NotificationViewMDTest

- NotificationViewMDTest.TestClick
- NotificationViewMDTest.TestClickExpanded

I found the tests were missing during working around here.

Bug: 869278
Test: trybots pass

Change-Id: Ieb428fa8884755ed63ea2fe6d1c346629ab35ed8
Reviewed-on: https://chromium-review.googlesource.com/1158448
Reviewed-by: Tetsui Ohkubo <tetsui@chromium.org>
Commit-Queue: Yoshiki Iguchi <yoshiki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580109}
[modify] https://crrev.com/6043d5cd74e065fde12c8a62fb5f3dfb878ddd9a/ui/message_center/views/notification_view_md.h
[modify] https://crrev.com/6043d5cd74e065fde12c8a62fb5f3dfb878ddd9a/ui/message_center/views/notification_view_md_unittest.cc
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 2 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6791818d2df18ff4630a29e7d98ecb026a3d23e6

commit 6791818d2df18ff4630a29e7d98ecb026a3d23e6
Author: yoshiki iguchi <yoshiki@chromium.org>
Date: Thu Aug 02 08:26:33 2018

Move UiController/UiDelegate to ash

- message_center::UiController -> ash::MessageCenterUiCiontroller
- message_center::UiDelegate -> ash::MessageCenterUiDelegate

These CL moves these classes into ash/, since they are used only from ash.

Bug: 869278
Test: trybot passes

Change-Id: I15390ba972f5c522c5a95bad8c2e1b6281ee8d24
Reviewed-on: https://chromium-review.googlesource.com/1152756
Reviewed-by: Tetsui Ohkubo <tetsui@chromium.org>
Commit-Queue: Yoshiki Iguchi <yoshiki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580110}
[modify] https://crrev.com/6791818d2df18ff4630a29e7d98ecb026a3d23e6/ash/BUILD.gn
[rename] https://crrev.com/6791818d2df18ff4630a29e7d98ecb026a3d23e6/ash/message_center/message_center_ui_controller.cc
[rename] https://crrev.com/6791818d2df18ff4630a29e7d98ecb026a3d23e6/ash/message_center/message_center_ui_controller.h
[rename] https://crrev.com/6791818d2df18ff4630a29e7d98ecb026a3d23e6/ash/message_center/message_center_ui_controller_unittest.cc
[rename] https://crrev.com/6791818d2df18ff4630a29e7d98ecb026a3d23e6/ash/message_center/message_center_ui_delegate.h
[modify] https://crrev.com/6791818d2df18ff4630a29e7d98ecb026a3d23e6/ash/system/message_center/notification_tray.cc
[modify] https://crrev.com/6791818d2df18ff4630a29e7d98ecb026a3d23e6/ash/system/message_center/notification_tray.h
[modify] https://crrev.com/6791818d2df18ff4630a29e7d98ecb026a3d23e6/ash/system/message_center/notification_tray_unittest.cc
[modify] https://crrev.com/6791818d2df18ff4630a29e7d98ecb026a3d23e6/ash/system/unified/unified_system_tray.cc
[modify] https://crrev.com/6791818d2df18ff4630a29e7d98ecb026a3d23e6/ash/system/unified/unified_system_tray.h
[modify] https://crrev.com/6791818d2df18ff4630a29e7d98ecb026a3d23e6/ui/message_center/BUILD.gn
[modify] https://crrev.com/6791818d2df18ff4630a29e7d98ecb026a3d23e6/ui/message_center/views/notification_menu_model_unittest.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 6 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d0d0ada04046f3271a2fb2ff339d6630902e48b6

commit d0d0ada04046f3271a2fb2ff339d6630902e48b6
Author: yoshiki iguchi <yoshiki@chromium.org>
Date: Mon Aug 06 19:51:00 2018

Rename MessageCenterNotificationManager -> NotificationUIManagerImpl

This CL renames these classes:
- MessageCenterNotificationManager -> NotificationUIManagerImpl
- MessageCenterNotificationManagerTest -> NotificationUIManagerTest
- MessageCenterNotificationsTest -> NotificationUIManagerBrowserTest
- MessageCenterNotificationManagerBrowserTest
    -> NotificationUIManagerInteractiveUITest

MessageCenterNotificationManager was confusing, because this class is
not for CrOS (which has message center) but for Win/Mac/Linux (popup only).

This CL renames this class into NotificationUIManagerImpl, because this is
an implementation of NotificationUIManager. And this also renames the
related test classes accordingly.

Bug: 869278
Test: trybots pass

Change-Id: I296f46d0b8634c95081167d6585f6f42798661f6
Reviewed-on: https://chromium-review.googlesource.com/1158669
Commit-Queue: Yoshiki Iguchi <yoshiki@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580955}
[modify] https://crrev.com/d0d0ada04046f3271a2fb2ff339d6630902e48b6/chrome/browser/BUILD.gn
[rename] https://crrev.com/d0d0ada04046f3271a2fb2ff339d6630902e48b6/chrome/browser/notifications/notification_ui_manager_browsertest.cc
[delete] https://crrev.com/2a9331ed168b3df959398c878b18923b074c5860/chrome/browser/notifications/notification_ui_manager_desktop.cc
[rename] https://crrev.com/d0d0ada04046f3271a2fb2ff339d6630902e48b6/chrome/browser/notifications/notification_ui_manager_impl.cc
[rename] https://crrev.com/d0d0ada04046f3271a2fb2ff339d6630902e48b6/chrome/browser/notifications/notification_ui_manager_impl.h
[rename] https://crrev.com/d0d0ada04046f3271a2fb2ff339d6630902e48b6/chrome/browser/notifications/notification_ui_manager_interactive_uitest.cc
[rename] https://crrev.com/d0d0ada04046f3271a2fb2ff339d6630902e48b6/chrome/browser/notifications/notification_ui_manager_unittest.cc
[modify] https://crrev.com/d0d0ada04046f3271a2fb2ff339d6630902e48b6/chrome/test/BUILD.gn
►

Sign in to add a comment