Add NOTIMPLEMENTED checks to NotificationDelegate methods |
||
Issue descriptionAs suggested for the new method ButtonClickWithReply at https://codereview.chromium.org/2392343002/#msg39 , we should probably add NOTIMPLEMENTED for all the other methods in NotificationDelegate with default implementations - https://cs.chromium.org/chromium/src/ui/message_center/notification_delegate.cc (First insert a DCHECK and run the trybots in case any of the default implementations are actually getting hit)
,
Oct 19 2016
Added a NOTREACHED (essentially a DCHECK) to all the methods in question here https://codereview.chromium.org/2423413003/ and turns out some *are* reached. In particular, NotificationDelegate::Display is called from MessagePopupCollection::UpdateWidgets. Others are called from tests - e.g. NotificationDelegate::Close from some test tearDowns. Also the NotificationDelegate unit tests themselves call NotificationDelegate::ButtonClick. Discussing with peter@ we decided it's not a great use of time to fix them all up now. So closing as Won'tFix. (FYI dewittj@ since I said I would do this in response to your comment on a code review.) |
||
►
Sign in to add a comment |
||
Comment 1 by awdf@chromium.org
, Oct 18 2016