New issue
Advanced search Search tips

Issue 795331 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

remove MessageCenterObserver

Project Member Reported by est...@chromium.org, Dec 15 2017

Issue description

MessageCenterObserver should go away. It only has one use in production code (MessageCenterStatsCollector). That class needs to go away anyway because it's no longer a reliable way to collect UMA about notifications --- we'll need to replace it with something that cares about NotificationDisplayService instead.

There are some uses in tests which I have already begun to remove.
 

Comment 1 by est...@chromium.org, Dec 19 2017

actually not sure if we can remove this since message_center::UiController also uses it.
Project Member

Comment 2 by bugdroid1@chromium.org, Feb 16 2018

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

commit 309d7b01c2314ffafa7115ff86f9ba0cf79343ce
Author: Evan Stade <estade@chromium.org>
Date: Fri Feb 16 04:26:56 2018

Remove MessageCenter's NotificationChangeQueue.

This was added due to TetherNotificationPresenter being a
MessageCenterObserver that modifies the message center. That was only
the case due to a misunderstanding (it should have used
NotificationDelegate) and is now fixed. No MessageCenterObserver instance
should modify the MC during iteration. The change queue made that
possible, but that just allowed abuse of MessageCenterObserver.

Bug:  795331 
Change-Id: I25f94853e0c8b29f3929d7459ed3cd3bfd2ed780
Reviewed-on: https://chromium-review.googlesource.com/829983
Commit-Queue: Yoshiki Iguchi <yoshiki@chromium.org>
Reviewed-by: Mitsuru Oshima (In Tokyo) <oshima@chromium.org>
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537196}
[modify] https://crrev.com/309d7b01c2314ffafa7115ff86f9ba0cf79343ce/ui/message_center/BUILD.gn
[delete] https://crrev.com/c54ab7cbb7e628b3c48452b6c4c8c931f7d02871/ui/message_center/change_queue.cc
[delete] https://crrev.com/c54ab7cbb7e628b3c48452b6c4c8c931f7d02871/ui/message_center/change_queue.h
[delete] https://crrev.com/c54ab7cbb7e628b3c48452b6c4c8c931f7d02871/ui/message_center/change_queue_unittest.cc
[modify] https://crrev.com/309d7b01c2314ffafa7115ff86f9ba0cf79343ce/ui/message_center/message_center_impl.cc
[modify] https://crrev.com/309d7b01c2314ffafa7115ff86f9ba0cf79343ce/ui/message_center/message_center_impl.h
[modify] https://crrev.com/309d7b01c2314ffafa7115ff86f9ba0cf79343ce/ui/message_center/message_center_impl_unittest.cc
[modify] https://crrev.com/309d7b01c2314ffafa7115ff86f9ba0cf79343ce/ui/message_center/message_center_observer.h
[modify] https://crrev.com/309d7b01c2314ffafa7115ff86f9ba0cf79343ce/ui/message_center/ui_controller.cc
[modify] https://crrev.com/309d7b01c2314ffafa7115ff86f9ba0cf79343ce/ui/message_center/ui_controller.h

Project Member

Comment 3 by bugdroid1@chromium.org, Feb 20 2018

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

commit 4fa63f81f2eba01941d8e23fe0afc6d8be4ac362
Author: Evan Stade <estade@chromium.org>
Date: Tue Feb 20 17:01:33 2018

Add early return in message_center::UiController.

This is a follow up to 309d7b01c2314ffafa7115

Bug:  795331 
Change-Id: I2765f13bf66e2fd0f1a0cce94c7a4c84fd237c3c
Reviewed-on: https://chromium-review.googlesource.com/923356
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537801}
[modify] https://crrev.com/4fa63f81f2eba01941d8e23fe0afc6d8be4ac362/ui/message_center/ui_controller.cc

Status: WontFix (was: Assigned)

Sign in to add a comment