Notificaitons refactor step 2

Notifications refactor step 2. 

Moved notification_types.* from ui/notifications -> ui/message_center (ui/notifications is removed)

Made message_center::Notification a class rather then a struct. Its lifetime is managed by NotificationList.

Moved UnpackOptionalFields from NotificationList to Notification, it is a private member used in ctor.

Removed 2-level collections (set<list>), now all notifications are stored in a std::list (the ScopedVector didn't work since it can't be sorted)

Use stable sort() to figure out the order of notifications in popups and NotificationCenter.

Changed order of notifications in NotificationListUnittest::MarkSinglePopupAsShown - need mukai@ to verify correctness.

NotificationList::RemoveNotification is now void since its bool result was not used.

In NotificationListUnittest, instead of /gtest.gyp:gtest_main include base.gyp:run_all_unittests to initialize ICU etc.

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

[Dmitry Titov, 2013-02-20 21:51:06]

Adding dewittj aand stevenjb to Cc - but please chime in if you feel like it!

There is a test failing which adds 2 notifications with same ID (not
replace_id). Will fix soon.

[Dmitry Titov, 2013-02-20 21:52:06]

Sorry for large CL. Most of the files are only namespace change for
NotificationTypes...

[Jun Mukai, 2013-02-20 22:52:15]

https://codereview.chromium.org/12277024/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/api/notification/notification_api.cc (right):

https://codereview.chromium.org/12277024/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/notification/notification_api.cc:139: // If older
notification runtime is used, MessageCenetr is not built.
nit: MessageCenter

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.cc:102:
NotificationList::~NotificationList() {
call STLDeleteContainerPointers() to notifications_, or
DCHECK(notifications_.empty())

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.cc:237:
notifications_.sort(CompareForPopupList);
sort would be O(n log n). Just scan all of the notifications_ linearly would be
better?

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
File ui/message_center/notification_list.h (right):

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.h:156: Notifications notifications_;
can we use std::priority_queue rather than std::list?  Then, you don't need to
sort at GetNotifications anymore.

[dewittj, 2013-02-20 23:47:53]

Do you think that a message center ought to have two NotificationLists?  This
might solve some of the sorting+resorting behavior that's been pointed out. 
Probably the sorting performance won't be so bad (since I imagine n < 10 in
normal usage anyway) but it is unusual for Chromium code to see sorting all over
the place.

comments + please fix 3 instances of misspelling (notificaitons ->
notifications) in issue description.

https://codereview.chromium.org/12277024/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/api/notification/notification_api.cc (right):

https://codereview.chromium.org/12277024/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/notification/notification_api.cc:151: // Ignore
options if running on the old notificaiton runtime.
nit: notificaiton->notification

https://codereview.chromium.org/12277024/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/notification/notification_api.cc:247: #endif //
!defined(ENABLE_MESSAGE_CENTER)
nit: 2 spaces before //

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
File ui/message_center/notification.cc (right):

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification.cc:11: unsigned g_serial_number = 0;
Should this be a message center instance variable?

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification.h:80: // Used to keep the order of notificaitons
with the same timestamp.
spelling notificaiton->notification

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification.h:86: // the notificaiton's data members.
notificaiton->notification

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.cc:21: : flags_(n->shown_as_popup() ? 1 :
0),
|flags_| is a pretty opaque name.  Looks like it only contains one bit of
information.  Do we need the full 32 unsigned bits at this moment?

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.cc:55: // Rank used to sort notificaitons by
(shown_as_popup, timestamp, serial_number)
notificaiton->notification

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.cc:59: // If equal, leave the order in which
notificaitons were added.
notificaiton->notification

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.cc:71: // If equal, leave the order in which
notificaitons were added.
notificaiton->notification

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.cc:250: // sorted, so stop when encounter
the first notificaiton that should not
notificaiton->notification

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
File ui/message_center/notification_list.h (right):

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.h:29: // A 'list' because we use sort() to
ensure display order by priority/time.
What if we use a std::set<Notification*> which is sorted, to prevent re-sorting
all the time?  I might think we want two sets, a set of those notifications that
are waiting to be shown as popups and a set of notifications that are only in
message center (since you seem to be sorting them in different ways).

[miket_OOO, 2013-02-22 21:07:09]

lgtm

https://codereview.chromium.org/12277024/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/api/notification/notification_api.cc (right):

https://codereview.chromium.org/12277024/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/notification/notification_api.cc:247: #endif //
!defined(ENABLE_MESSAGE_CENTER)
On 2013/02/20 23:47:54, dewittj wrote:
> nit: 2 spaces before //

Nice!

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification.h:86: // the notificaiton's data members.
On 2013/02/20 23:47:54, dewittj wrote:
> notificaiton->notification

It might be easier to set up a nightly cron job over the entire codebase, or add
a presubmit check. Also for "persistant" and "independant." Exception for
third_party/hunspell.

[Dmitry Titov, 2013-02-23 00:58:51]

I've addressed feedback, Jun and Justin - PTAL?

I've replaced the need for periodic resort with 2 std::set<T, Comparer> so they
are auto-sorted.

Also updated the MessageCenterUnittest to reflect the new sort order.

Thanks!

https://codereview.chromium.org/12277024/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/api/notification/notification_api.cc (right):

https://codereview.chromium.org/12277024/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/notification/notification_api.cc:139: // If older
notification runtime is used, MessageCenetr is not built.
On 2013/02/20 22:52:16, Jun Mukai wrote:
> nit: MessageCenter

Done.

https://codereview.chromium.org/12277024/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/notification/notification_api.cc:151: // Ignore
options if running on the old notificaiton runtime.
On 2013/02/20 23:47:54, dewittj wrote:
> nit: notificaiton->notification

Done.

https://codereview.chromium.org/12277024/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/notification/notification_api.cc:247: #endif //
!defined(ENABLE_MESSAGE_CENTER)
On 2013/02/20 23:47:54, dewittj wrote:
> nit: 2 spaces before //

Done.

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
File ui/message_center/notification.cc (right):

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification.cc:11: unsigned g_serial_number = 0;
On 2013/02/20 23:47:54, dewittj wrote:
> Should this be a message center instance variable?

A notification can be created w/o having a MessageCenter pointer... I made it a
static of Notification instead.

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification.h:80: // Used to keep the order of notificaitons
with the same timestamp.
On 2013/02/20 23:47:54, dewittj wrote:
> spelling notificaiton->notification

Done.

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification.h:86: // the notificaiton's data members.
On 2013/02/20 23:47:54, dewittj wrote:
> notificaiton->notification

Done.

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification.h:86: // the notificaiton's data members.
On 2013/02/22 21:07:09, miket wrote:
> On 2013/02/20 23:47:54, dewittj wrote:
> > notificaiton->notification
> 
> It might be easier to set up a nightly cron job over the entire codebase, or
add
> a presubmit check. Also for "persistant" and "independant." Exception for
> third_party/hunspell.

Next time we build an IDE, we should built the spellchecker for comments in! :-)

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.cc:21: : flags_(n->shown_as_popup() ? 1 :
0),
On 2013/02/20 23:47:54, dewittj wrote:
> |flags_| is a pretty opaque name.  Looks like it only contains one bit of
> information.  Do we need the full 32 unsigned bits at this moment?

Done. Actually, removed this class.

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.cc:55: // Rank used to sort notificaitons by
(shown_as_popup, timestamp, serial_number)
On 2013/02/20 23:47:54, dewittj wrote:
> notificaiton->notification

Done.

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.cc:59: // If equal, leave the order in which
notificaitons were added.
On 2013/02/20 23:47:54, dewittj wrote:
> notificaiton->notification

Done.

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.cc:71: // If equal, leave the order in which
notificaitons were added.
On 2013/02/20 23:47:54, dewittj wrote:
> notificaiton->notification

Done.

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.cc:102:
NotificationList::~NotificationList() {
On 2013/02/20 22:52:16, Jun Mukai wrote:
> call STLDeleteContainerPointers() to notifications_, or
> DCHECK(notifications_.empty())

Great point, done!

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.cc:237:
notifications_.sort(CompareForPopupList);
On 2013/02/20 22:52:16, Jun Mukai wrote:
> sort would be O(n log n). Just scan all of the notifications_ linearly would
be
> better?

Replaced with 2 sorted std::sets - no need to re-sort things anymore.

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.cc:250: // sorted, so stop when encounter
the first notificaiton that should not
On 2013/02/20 23:47:54, dewittj wrote:
> notificaiton->notification

Done.

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
File ui/message_center/notification_list.h (right):

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.h:29: // A 'list' because we use sort() to
ensure display order by priority/time.
On 2013/02/20 23:47:54, dewittj wrote:
> What if we use a std::set<Notification*> which is sorted, to prevent
re-sorting
> all the time?  I might think we want two sets, a set of those notifications
that
> are waiting to be shown as popups and a set of notifications that are only in
> message center (since you seem to be sorting them in different ways).

Done.

https://codereview.chromium.org/12277024/diff/18001/ui/message_center/notific...
ui/message_center/notification_list.h:156: Notifications notifications_;
On 2013/02/20 22:52:16, Jun Mukai wrote:
> can we use std::priority_queue rather than std::list?  Then, you don't need to
> sort at GetNotifications anymore.

I've replaced it with a std::set<T*, Comparer>. It can keep the order while
inserting/removing.

Also, since we need 2 different sort orders - for popups and for Center - I have
2 sorted sets.

[Jun Mukai, 2013-02-23 01:19:09]

lgtm with nits

https://codereview.chromium.org/12277024/diff/20016/ui/message_center/notific...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/12277024/diff/20016/ui/message_center/notific...
ui/message_center/notification_list.cc:180: return (dummy !=
notifications_.end());
variable name 'dummy' doesn't make much sense on the new code.
You can just fix this as 'return GetNotification(id) != notifications_.end()'

https://codereview.chromium.org/12277024/diff/20016/ui/message_center/notific...
File ui/message_center/notification_list.h (right):

https://codereview.chromium.org/12277024/diff/20016/ui/message_center/notific...
ui/message_center/notification_list.h:28: bool operator()(Notification* n1,
Notification* n2);
nit: 2 space indent?

https://codereview.chromium.org/12277024/diff/20016/ui/message_center/notific...
ui/message_center/notification_list.h:31: bool operator()(Notification* n1,
Notification* n2);
ditto

[dewittj, 2013-02-23 01:22:49]

lgtm

[Dmitry Titov, 2013-02-23 01:35:48]

Sky, could you please do OWNERS look? 
Most files touched because of a file move from ui/notificaitons to
ui/message_center...

Dirs causing you to be suggested by presubmit:
    chrome\browser\ui\views
    ash
    ui
    ash\system
    chrome\browser\ui\views\ash

Thanks!

https://codereview.chromium.org/12277024/diff/20016/ui/message_center/notific...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/12277024/diff/20016/ui/message_center/notific...
ui/message_center/notification_list.cc:180: return (dummy !=
notifications_.end());
On 2013/02/23 01:19:10, Jun Mukai wrote:
> variable name 'dummy' doesn't make much sense on the new code.
> You can just fix this as 'return GetNotification(id) != notifications_.end()'

Done.

https://codereview.chromium.org/12277024/diff/20016/ui/message_center/notific...
File ui/message_center/notification_list.h (right):

https://codereview.chromium.org/12277024/diff/20016/ui/message_center/notific...
ui/message_center/notification_list.h:28: bool operator()(Notification* n1,
Notification* n2);
On 2013/02/23 01:19:10, Jun Mukai wrote:
> nit: 2 space indent?

Done.

https://codereview.chromium.org/12277024/diff/20016/ui/message_center/notific...
ui/message_center/notification_list.h:31: bool operator()(Notification* n1,
Notification* n2);
On 2013/02/23 01:19:10, Jun Mukai wrote:
> ditto

Done.

[sky, 2013-02-25 16:14:30]

LGTM - feel free to TBR this sort of stuff

[stevenjb, 2013-02-25 18:11:08]

Nice refactor, thanks.
Just a couple of questions/concerns.

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification.h:83: // very unlikely to have 4 Billion of
notifications shown between reboots.
nit: last two lines should be well understood and are unnecessary. Also, is this
really necessary at all? Unless we're using time as a unique key, this seems
like we're adding complexity for an extremely unlikely edge case that we don't
really care about...?

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification.h:109: static unsigned s_next_serial_number_;
nit: make this file local instead of exposing it here

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification_list.cc:30: return false;
Instead of adding a serial number, maybe just compare pointers for this edge
case?

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification_list.cc:184: iter != notifications_.end();
++iter) {
Aren't these stored in priority order, so couldn't we just check the first one
(if any)?

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification_list.cc:204: continue;
Can we break here?

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification_list.cc:210: default_priority_popup_count ++ >=
kMaxVisiblePopupNotifications) {
no ' ' before ++

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
File ui/message_center/notification_view.cc (right):

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification_view.cc:235: Notification* notification)
Can we use const* here (and elsewhere)? We really should only pass non-const*
where we are actually modifying the notification.
Also, please make sure there is no chance that the View could outlive the
Notification. Views are not always destroyed immediately. Otherwise we may need
to add a unique Id to notifications (which could be the serial index).

[Dmitry Titov, 2013-02-25 23:21:14]

Thanks Steven, I've updated patch once more.

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification.h:83: // very unlikely to have 4 Billion of
notifications shown between reboots.
On 2013/02/25 18:11:08, stevenjb (chromium) wrote:
> nit: last two lines should be well understood and are unnecessary. Also, is
this
> really necessary at all? Unless we're using time as a unique key, this seems
> like we're adding complexity for an extremely unlikely edge case that we don't
> really care about...?

Updated comment.

We discussed sorting last week with the team and decided to sort on timestamp
(exposed as eventTime in API) in order to render. This is to address the
scenarios like a Chrombook waking up and potentially several apps getting a
bunch of push messages/updates about events happened in the last hour s - so
they can use eventTime reasonably to keep the notifications in the same order as
they are on the computer that was not sleeping.
At the same time, an app that posts 2 notifications in a row will naturally
expect the same order all the time, even if the timestamp is the same. Serial
number keeps the order automatically.
One of those apps are our own tests - they fire a lot of notifications (so the
timestamps often happen to be the same) and they rely on visual order that
corresponds the order of creating those notifications.

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification.h:109: static unsigned s_next_serial_number_;
On 2013/02/25 18:11:08, stevenjb (chromium) wrote:
> nit: make this file local instead of exposing it here

Done.

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification_list.cc:30: return false;
On 2013/02/25 18:11:08, stevenjb (chromium) wrote:
> Instead of adding a serial number, maybe just compare pointers for this edge
> case?

See the previous comment. I think we might want to enforce the order matching
the order of API calls by an app...

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification_list.cc:184: iter != notifications_.end();
++iter) {
On 2013/02/25 18:11:08, stevenjb (chromium) wrote:
> Aren't these stored in priority order, so couldn't we just check the first one
> (if any)?

The notifications are stored in priority/timestamp order indeed (notification
center order), however I assume the ones that were not yet 'shown_as_popup' can
happen later in this list. For example, if we had all notifications shown
already, and then a notification of DEFAULT_PRIORITY comes in, it is inserted
behind hi-pri ones, but it still has to be shown.

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification_list.cc:204: continue;
On 2013/02/25 18:11:08, stevenjb (chromium) wrote:
> Can we break here?

Good catch, done.

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification_list.cc:210: default_priority_popup_count ++ >=
kMaxVisiblePopupNotifications) {
On 2013/02/25 18:11:08, stevenjb (chromium) wrote:
> no ' ' before ++

Done.

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
File ui/message_center/notification_view.cc (right):

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification_view.cc:235: Notification* notification)
On 2013/02/25 18:11:08, stevenjb (chromium) wrote:
> Can we use const* here (and elsewhere)? We really should only pass non-const*
> where we are actually modifying the notification.
> Also, please make sure there is no chance that the View could outlive the
> Notification. Views are not always destroyed immediately. Otherwise we may
need
> to add a unique Id to notifications (which could be the serial index).

This is a very cool observation, thanks! I am pretty sure a vew can outlive
notification in some edge cases (under a context menu for example).

Here is a new patch that does not have this issue, because it does not store the
reference to Notification in Views beyond initialization.

[stevenjb, 2013-02-25 23:55:56]

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification_list.cc:184: iter != notifications_.end();
++iter) {
On 2013/02/25 23:21:15, Dmitry Titov wrote:
> On 2013/02/25 18:11:08, stevenjb (chromium) wrote:
> > Aren't these stored in priority order, so couldn't we just check the first
one
> > (if any)?
> 
> The notifications are stored in priority/timestamp order indeed (notification
> center order), however I assume the ones that were not yet 'shown_as_popup'
can
> happen later in this list. For example, if we had all notifications shown
> already, and then a notification of DEFAULT_PRIORITY comes in, it is inserted
> behind hi-pri ones, but it still has to be shown.
Ah, yes, I didn't think that through. Might be clearer (and slightly faster)
like:
  if ((*iter)->priority() < DEFAULT_PRIORITY)
    break;
  if (!(*iter)->shown_as_popup())
    return true;

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/message...
File ui/message_center/message_center_bubble.cc (right):

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/message...
ui/message_center/message_center_bubble.cc:411:
NotificationView::ViewForNotification(*(*iter), list_delegate_);
I know this is unrelated to this CL, but could view ever be NULL here?

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/message...
File ui/message_center/message_popup_bubble.cc (right):

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/message...
ui/message_center/message_popup_bubble.cc:68:
NotificationView::ViewForNotification(*(*iter), list_delegate_);
NULL view?

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/message...
File ui/message_center/message_view.h (right):

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/message...
ui/message_center/message_view.h:41: virtual void SetUpView(const Notification&
notification);
message_view.cc does not appear to have uploaded correctly.

I'm not a huge fan of the pattern where a derived function has to call the base
class implementation. I would either copy the shared code if it's simple, or use
this pattern:

void SetupView(...) {
  <do something>
  PostSetup(...)
}

virtual void PostSetup() = 0;  (Or make the default a no-op)

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/notific...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/notific...
ui/message_center/notification_list.cc:318: // Take ownership. The notification
can only be removed form the list
s/form/from

[Dmitry Titov, 2013-02-26 01:01:51]

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/12277024/diff/11042/ui/message_center/notific...
ui/message_center/notification_list.cc:184: iter != notifications_.end();
++iter) {
On 2013/02/25 23:55:56, stevenjb (chromium) wrote:
> On 2013/02/25 23:21:15, Dmitry Titov wrote:
> > On 2013/02/25 18:11:08, stevenjb (chromium) wrote:
> > > Aren't these stored in priority order, so couldn't we just check the first
> one
> > > (if any)?
> > 
> > The notifications are stored in priority/timestamp order indeed
(notification
> > center order), however I assume the ones that were not yet 'shown_as_popup'
> can
> > happen later in this list. For example, if we had all notifications shown
> > already, and then a notification of DEFAULT_PRIORITY comes in, it is
inserted
> > behind hi-pri ones, but it still has to be shown.
> Ah, yes, I didn't think that through. Might be clearer (and slightly faster)
> like:
>   if ((*iter)->priority() < DEFAULT_PRIORITY)
>     break;
>   if (!(*iter)->shown_as_popup())
>     return true;

Done.

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/message...
File ui/message_center/message_center_bubble.cc (right):

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/message...
ui/message_center/message_center_bubble.cc:411:
NotificationView::ViewForNotification(*(*iter), list_delegate_);
On 2013/02/25 23:55:56, stevenjb (chromium) wrote:
> I know this is unrelated to this CL, but could view ever be NULL here?

I think you are asking if the ViewForNotification() can ever return NULL so the
subsequent code crashes. It can't because ViewForNotificaiton is essentially a
Create(), it calls new() inside. Will rename in subsequent CL.

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/message...
File ui/message_center/message_popup_bubble.cc (right):

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/message...
ui/message_center/message_popup_bubble.cc:68:
NotificationView::ViewForNotification(*(*iter), list_delegate_);
On 2013/02/25 23:55:56, stevenjb (chromium) wrote:
> NULL view?

Never returns NULL, always creates. Will rename in sep. CL.

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/message...
File ui/message_center/message_view.h (right):

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/message...
ui/message_center/message_view.h:41: virtual void SetUpView(const Notification&
notification);
On 2013/02/25 23:55:56, stevenjb (chromium) wrote:
> message_view.cc does not appear to have uploaded correctly.
> 
> I'm not a huge fan of the pattern where a derived function has to call the
base
> class implementation. I would either copy the shared code if it's simple, or
use
> this pattern:
> 
> void SetupView(...) {
>   <do something>
>   PostSetup(...)
> }
> 
> virtual void PostSetup() = 0;  (Or make the default a no-op)
> 

Done. Removed this virtual completely, moved some initialization to ctors.

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/notific...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/12277024/diff/23037/ui/message_center/notific...
ui/message_center/notification_list.cc:318: // Take ownership. The notification
can only be removed form the list
On 2013/02/25 23:55:56, stevenjb (chromium) wrote:
> s/form/from

Done.

[stevenjb, 2013-02-26 01:11:57]

lgtm

[commit-bot: I haz the power, 2013-02-26 01:15:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dimich@chromium.org/12277024/23042

[commit-bot: I haz the power, 2013-02-26 04:42:45]

Change committed as 184581