Made notification center notifications collapsed and expandable.

Made notification center notifications collapsible and expandable.

Implemented collapse/expand for notifications in the notification
center. Notifications are collapsed by default and can be expanded by
pressing their expand button. Once expanded, they can't be collapsed
again.

Limitations of this version are:
1) Notifications in alerts/toasts are not affected (they're always expanded).
2) Notifications get expand buttons even if they have no content to expand.
3) Collapsed text is always collapsed to 1 lines (not 2 or 3 as required) and expanded text
is always expanded to its full height (not limited to 7 lines as
required).
4) Notification expansion is lost when notifications are
added to the notification center or when the notification center is
closed and opened again. These issues will be addressed in separate
change lists.

BUG=161098
TBR=stevenjb@chromium.org

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

[dharcourt, 2013-02-23 04:32:00]

Jun, PTAL.

Mike, any suggestions from you would also be very welcome.

Thanks!

    - Charles

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
File ui/message_center/message_center_bubble.cc (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
ui/message_center/message_center_bubble.cc:390: button_view_ = new
WebNotificationButtonView(list_delegate);
Button creation had to be moved up in this method to allow button heights to be
used to calculate the scroll view's minimum and maximum heights.

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
ui/message_center/message_center_bubble.cc:421: view->set_scroller(scroller_);
SetUpView() is now avoided as MessageView and all of its subclasses now set
themselves up either in their constructor or in their Layout() method.

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
File ui/message_center/message_center_bubble.h (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
ui/message_center/message_center_bubble.h:14: class MessageCenterView;
I renamed "MessageCenterContentsView" to "MessageCenterView" and
"ScrollContentView" to "MessagesView" so their names reflect what they are
rather than how they're used (for example MessagesView displays a list of
messages whether it's used in a scroll view or not, so its name doesn't need to
include "ScrollContent").

This increases encapsulation and should simplify things for people reading this
code for the first time (for example they can read the MessagesView code without
having that class' name make them look for its scroll-related code).

MessageCenterView is still not as encapsulated as I'd like and it remains
coupled to MessageCenterBubble, but if we replace that with delegation we could
move MessageCenterView and MessagesView to a separate message_center_view.{h,cc}
and have that be message center UI code that's totally independent of where it's
used.

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
File ui/message_center/message_popup_bubble.cc (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
ui/message_center/message_popup_bubble.cc:69: view->SetExpanded(true);
NotificationViews are expanded by default in the popup/alerts/toasts because
MessagePopupBubble isn't set up yet to deal with the changes in view size which
could come if the views were initially collapsed and allowed to be expanded by
the user.

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
File ui/message_center/message_view.cc (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
ui/message_center/message_view.cc:237: close->set_owned_by_client();
This isn't strictly needed because close_button_ will be deleted by the
scoped_ptr's destructor before the parent views::View's destructor is called,
and this deletion will remove it from the children_ vector, so the parent's
destructor will never even check for owner_by_client_, but it doesn't hurt to
have this just in case RemoveAllChildViews() gets called from somewhere else in
the future.

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
File ui/message_center/message_view.h (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
ui/message_center/message_view.h:50: virtual void SetExpanded(bool expanded);
These are not is_expanded() and set_expanded() because they need to be virtual
methods: Subclasses probably will want to override them to do more than
MessageView does on expansion (NotificationView does that).

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/notifica...
File ui/message_center/notification_view.cc (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/notifica...
ui/message_center/notification_view.cc:147: }
SetImageSize() needed to be removed from GetImageSizeForWidth() because it set
up a GetImageSizeForWidth() -> SetImageSize() -> PreferredSizeChanged() ->
ChildPreferredSizeChanged() -> Layout() -> GetImageSizeForWith() infinite loop.
Breaking this loop required the image sizing logic to be moved to the OnPaint()
method below.

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/notifica...
ui/message_center/notification_view.cc:336: return size;
Having a return gfx::Size() in the middle of this method made logging a pain
during debugging, so I changed it to have a single return statement. It also
made the method 18 characters shorter :-).

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/notifica...
ui/message_center/notification_view.cc:400: }
Item display had to be moved up here so the displayed-item-count could be used
to determine below whether to display the message (the message should be
displayed even when the notification is expanded if it has no items).

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/notifica...
File ui/message_center/notification_view.h (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/notifica...
ui/message_center/notification_view.h:8: #include <vector>
Better to have this here explicitly since we use a std::Vector below rather than
depend on one of the .h's included below happening to have its own include of
<vector>.

[Jun Mukai, 2013-02-26 02:23:30]

lgtm

lgtm with nits.

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
File ui/message_center/message_center_bubble.cc (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
ui/message_center/message_center_bubble.cc:296: class MessagesView : public
views::View {
I think it's too hard to distinguish this from message_center::MessageView
How about MessagesContainerView ?

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
File ui/message_center/message_center_constants.cc (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
ui/message_center/message_center_constants.cc:15: const int
kNotificationMaximumItems = 8;
I prefer keeping size_t for this type of data (see my comments on
notification_view.cc too)

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
File ui/message_center/message_popup_bubble.cc (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
ui/message_center/message_popup_bubble.cc:69: view->SetExpanded(true);
On 2013/02/23 04:32:00, dharcourt wrote:
> NotificationViews are expanded by default in the popup/alerts/toasts because
> MessagePopupBubble isn't set up yet to deal with the changes in view size
which
> could come if the views were initially collapsed and allowed to be expanded by
> the user.

Can you leave this as comment?

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
File ui/message_center/message_view.h (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
ui/message_center/message_view.h:50: virtual void SetExpanded(bool expanded);
nit: I am not so sure, but probably our style of declaration order would put
these virtual methods before overriding methods.

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/notifica...
File ui/message_center/notification_view.cc (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/notifica...
ui/message_center/notification_view.cc:395: items = std::min(items, IsExpanded()
? kNotificationMaximumItems : 0);
as I wrote in _constants.h, I prefer to keep size_t here.
size_t items = ...size();
items = std::min(items, IsExpanded() ? ... : 0ul);

would work well.

[dharcourt, 2013-02-26 03:23:30]

I implemented the review suggestions except that I tried MessageListView instead
of MessageContainerView. Let me know how that looks to you and I'll commit or
change to MessageContainerView.

Thanks for the review,

    - Charles

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
File ui/message_center/message_center_bubble.cc (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
ui/message_center/message_center_bubble.cc:296: class MessagesView : public
views::View {
On 2013/02/26 02:23:30, Jun Mukai wrote:
> I think it's too hard to distinguish this from message_center::MessageView
> How about MessagesContainerView ?

Good point. What would you think of MessageListView?

If you don't like that name, I'll implement MessagesContainerView, but I think
list would achieve the required distinction while being a bit less generic than
container.

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
File ui/message_center/message_center_constants.cc (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
ui/message_center/message_center_constants.cc:15: const int
kNotificationMaximumItems = 8;
On 2013/02/26 02:23:30, Jun Mukai wrote:
> I prefer keeping size_t for this type of data (see my comments on
> notification_view.cc too)

Done.

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
File ui/message_center/message_popup_bubble.cc (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
ui/message_center/message_popup_bubble.cc:69: view->SetExpanded(true);
On 2013/02/26 02:23:30, Jun Mukai wrote:
> On 2013/02/23 04:32:00, dharcourt wrote:
> > NotificationViews are expanded by default in the popup/alerts/toasts because
> > MessagePopupBubble isn't set up yet to deal with the changes in view size
> which
> > could come if the views were initially collapsed and allowed to be expanded
by
> > the user.
> 
> Can you leave this as comment?

Done.

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
File ui/message_center/message_view.h (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/message_...
ui/message_center/message_view.h:50: virtual void SetExpanded(bool expanded);
On 2013/02/26 02:23:30, Jun Mukai wrote:
> nit: I am not so sure, but probably our style of declaration order would put
> these virtual methods before overriding methods.

SGTM. Done.

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/notifica...
File ui/message_center/notification_view.cc (right):

https://codereview.chromium.org/12326091/diff/2002/ui/message_center/notifica...
ui/message_center/notification_view.cc:395: items = std::min(items, IsExpanded()
? kNotificationMaximumItems : 0);
On 2013/02/26 02:23:30, Jun Mukai wrote:
> as I wrote in _constants.h, I prefer to keep size_t here.
> size_t items = ...size();
> items = std::min(items, IsExpanded() ? ... : 0ul);
> 
> would work well.

Done.

[Jun Mukai, 2013-02-26 04:06:55]

lgtm++

Please make sure that this doesn't break UI of non --enable-rich-notifications,
before the submit (it looks working but just in case)

[dharcourt, 2013-03-01 10:17:31]

Jun, PTAL and comment/LGTM as appropriate. The last patch has significant
changes from the earlier version you reviewed and LGTMed. These changes were
required because MessageViews changed in the trunk to not have a direct
connection to their Notification.

Dmitry, any comments and suggestions you have for improvements would be welcome.
You may be particularly interested in the delegate mechanism used to allow
MessageViews to be updated when they are expanded (message_view.cc and
message_center_bubble.cc).

Mike, as usual any comments and suggestions you have would be very welcome.

General implementation note: I tweaked NotificationList::Delegate a bit so it
can better be used as a general delegate-for-things-that-modify-notifications
(and then I used it in this role with MessageView/MessageCenterBubble).
Ultimately I'd like to take NotificationList::Delegate out of
notification_list.h and rename it because it is much more of a general
delegate-for-things-that-modify-notifications than a
delegate-of-notification-lists. In fact NotificationList doesn't really need a
delegate and could talk to its owning MessageCenter directly instead of treating
it as a delegate: NotificationList and MessageCenter are really just
implementations of two aspects of the same object so there's little point in
trying to abstract out one direction of the communication between them,
especially when the abstraction used is the same one used for the very different
purpose of having things-that-modify-notifications report changes to
things-that-manage-notifications.

Thanks for the review,

    - Charles

https://codereview.chromium.org/12326091/diff/26001/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/12326091/diff/26001/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.cc:144:
DCHECK(message_center_->notification_list()->HasNotification(old_id));
GetNotificationList() was a functional duplicate of notification() and has been
removed. See the comments in notification_list.h for details.

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
File ui/message_center/message_bubble_base.h (right):

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
ui/message_center/message_bubble_base.h:18: explicit
MessageBubbleBase(MessageCenter* message_center);
Because NotificationList::Delegate doesn't provide access to the MessageCenter's
NotificationList any more (see notification_list.h comments for details),
MessageBubbleBase and its children need direct access to the MessageCenter so
they can ask it directly for its NotificationList. They're actually getting
exactly the same object, only the type of their pointer to it has changed so
they're allowed to call its notification_list().

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
File ui/message_center/message_center.h (right):

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
ui/message_center/message_center.h:154: virtual void OnQuietModeChanged(bool
quiet_mode) OVERRIDE;
GetNotificationList() was removed from MessageCenter because it was removed from
NotificationList::Delegate (see notification_list.h comments for details). It is
not otherwise needed in MessageCenter because notification_list() can be used to
get the MessageCenter's NotificationList. OnQuietModeChanged() was moved to
group the On*Clicked() methods together.

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

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
ui/message_center/message_center_bubble.cc:139: // Overridden from views::View:
I took this opportunity to make the override comments consistent in the
src/ui/message_view code. 75% of the /src/ui code uses the "// Overridden from
X" format and only 25% use the "// X overrides" one, so I shifted the dozen or
so uses of that later format to the former one. Now 80% of /src/ui use "//
Overridden from X:".

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
File ui/message_center/message_center_bubble.h (right):

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
ui/message_center/message_center_bubble.h:19: public NotificationList::Delegate
{
MessageCenterBubble now implements NotificationList::Delegate so it can
intercept its OnExpand() method and perform appropriate view updates on
notification expansion.

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

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/notific...
ui/message_center/notification.h:103: gfx::ImageSkia icon_;
Cleanup: Since notifications don't have secondary icon any more, the one icon
they do have doesn't need to be described as "primary" :-).

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

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/notific...
ui/message_center/notification_list.h:78: virtual void OnQuietModeChanged(bool
quiet_mode) = 0;
GetNotificationList() was removed because it caused too much coupling between
MessageCenter's NotificationList and objects that had a
NotificationList::Delegate.

As it turns out all objects calling GetNotificationList() were directly coupled
to the MessageCenter so it was easy to modify them to call MessageCenter's
notification_list() accessor directly instead of calling GetNotificationList().
GetNotificationList() could therefor be removed.

[Jun Mukai, 2013-03-01 21:44:42]

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
File ui/message_center/message_bubble_base.h (right):

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
ui/message_center/message_bubble_base.h:11: #include
"ui/message_center/notification_list.h"
safe to remove this inclusion?

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
File ui/message_center/message_center_bubble.h (right):

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
ui/message_center/message_center_bubble.h:19: public NotificationList::Delegate
{
On 2013/03/01 10:17:31, dharcourt wrote:
> MessageCenterBubble now implements NotificationList::Delegate so it can
> intercept its OnExpand() method and perform appropriate view updates on
> notification expansion.

Inheriting Delegate is not a good idea here. Usually a delegate has 1-1
relationship with the target instance (NotificationList here).
Also this class seems delegating the message center instead of the notification
list.

I think you should declare your own methods with another name. I don't have a
good name in my mind (sorry!), but this should not be a
NotificationList::Delegate.  Probably MessageView::Delegate or
Notification::Delegate?  not sure.

[dharcourt, 2013-03-02 02:08:02]

Hi Jun,

I implemented your review suggestions. I think this is much cleaner.

Please take a look and let me know what you think.

Thanks!

     - Charles

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
File ui/message_center/message_bubble_base.h (right):

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
ui/message_center/message_bubble_base.h:11: #include
"ui/message_center/notification_list.h"
On 2013/03/01 21:44:42, Jun Mukai wrote:
> safe to remove this inclusion?

Yes, thanks for catching this. Done.

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

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
ui/message_center/message_center_bubble.cc:548: 
This isn't now only in NotificationList::Delegate and not needed for
NotificationChangeDelegate so it doesn't need to be implemented here any more.

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
File ui/message_center/message_center_bubble.h (right):

https://codereview.chromium.org/12326091/diff/26001/ui/message_center/message...
ui/message_center/message_center_bubble.h:19: public NotificationList::Delegate
{
On 2013/03/01 21:44:42, Jun Mukai wrote:
> On 2013/03/01 10:17:31, dharcourt wrote:
> > MessageCenterBubble now implements NotificationList::Delegate so it can
> > intercept its OnExpand() method and perform appropriate view updates on
> > notification expansion.
> 
> Inheriting Delegate is not a good idea here. Usually a delegate has 1-1
> relationship with the target instance (NotificationList here).
> Also this class seems delegating the message center instead of the
notification
> list.
> 
> I think you should declare your own methods with another name. I don't have a
> good name in my mind (sorry!), but this should not be a
> NotificationList::Delegate.  Probably MessageView::Delegate or
> Notification::Delegate?  not sure.

Sounds good. I've implemented this by taking the
delegate-of-things-that-change-notifications functionality out of
NotificationList::Delegate and into a new NotificationChangeDelegate. This
leaves NotificationList::Delegate much smaller, and used only by MessageCenter,
NotificationList's only delegate. NotificationChangeDelegate replaces
NotificationList::Delegate in MessageCenterBubble, MessagePopupBubble, and
MessageView. I think this is much cleaner than having a single delegate class
handling notification change delegation and notification list delegation.

https://codereview.chromium.org/12326091/diff/30030/ui/message_center/message...
File ui/message_center/message_center.cc (right):

https://codereview.chromium.org/12326091/diff/30030/ui/message_center/message...
ui/message_center/message_center.cc:16: }
To avoid an "undefined reference to vtable..." link error, at least one
non-inline non-fully-virtual method must be defined. In this and other
delegates, the destructor fulfills that role.

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

https://codereview.chromium.org/12326091/diff/30030/ui/message_center/notific...
ui/message_center/notification_list.h:53: virtual void
AllNotificationsWereRemoved(bool by_user) = 0;
Much simpler because most methods have moved to NotificationChangeDelegate.

[Jun Mukai, 2013-03-02 02:23:58]

lgtm

https://codereview.chromium.org/12326091/diff/30030/ui/message_center/notific...
File ui/message_center/notification_change_delegate.h (right):

https://codereview.chromium.org/12326091/diff/30030/ui/message_center/notific...
ui/message_center/notification_change_delegate.h:15: class MESSAGE_CENTER_EXPORT
NotificationChangeDelegate {
Sorry I didn't saying this, but I still don't like the name 'delegate'... I
prefer NotificationChangeController here.  Does that sound better?
I'm not certain that my naming sense is better (you know, I'm not a native
English speaker), so I believe you. Please ignore my comment if you think I'm
wrong. Thanks!

[dharcourt, 2013-03-04 21:17:52]

https://codereview.chromium.org/12326091/diff/30030/ui/message_center/message...
File ui/message_center/message_center.h (right):

https://codereview.chromium.org/12326091/diff/30030/ui/message_center/message...
ui/message_center/message_center.h:158: virtual void
AllNotificationsWereRemoved(bool by_user) OVERRIDE;
It turned out no one was using this so I pruned it.

https://codereview.chromium.org/12326091/diff/30030/ui/message_center/notific...
File ui/message_center/notification_change_delegate.h (right):

https://codereview.chromium.org/12326091/diff/30030/ui/message_center/notific...
ui/message_center/notification_change_delegate.h:15: class MESSAGE_CENTER_EXPORT
NotificationChangeDelegate {
On 2013/03/02 02:23:58, Jun Mukai wrote:
> Sorry I didn't saying this, but I still don't like the name 'delegate'... I
> prefer NotificationChangeController here.  Does that sound better?
> I'm not certain that my naming sense is better (you know, I'm not a native
> English speaker), so I believe you. Please ignore my comment if you think I'm
> wrong. Thanks!

Yes, you're right, and "delegate" is wrong for this. Apart from anything else,
delegates typically modify the behavior of objects delegating to them, which is
not the case here.

I think "Controller" may be wrong also, as controller has a strong meaning in
user interface code associated with the MVC pattern, and NotificationChangeX
classes don't need to be MVC controllers.

How about "NotificationChangeObserver"? There are other examples of that name
being used in the code base for this kind of thing:
ui/views/widget/widget_observer.h, ui/aura/window_observer.h,
base/prefs/pref_observer.h.

In keeping with those other examples, I've change the method names to indicate
more clearly that they are event notifications with an "On" prefix.

Let me know what you think. If "NotificationChangeObserver" doesn't work for
you, alternatives may be "NotificationChangeWatcher" or
"NotificationChangeMonitor".

Thanks!

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

https://codereview.chromium.org/12326091/diff/30030/ui/message_center/notific...
ui/message_center/notification_list.h:53: virtual void
AllNotificationsWereRemoved(bool by_user) = 0;
AllNotificationsWereRemoved was never called so I removed it from the protocol.

[dharcourt, 2013-03-05 03:24:37]

Hi Mike,

I forgot that Jun is travelling to tokyo and offline for a couple of days. Any
chance you could review the latest changes (patch set 4) in this change list
while he's out?

Thanks,

    - Charles

[miket_OOO, 2013-03-05 18:41:32]

LGTM on deltas post-4.

https://codereview.chromium.org/12326091/diff/33001/ui/message_center/notific...
File ui/message_center/notification_change_observer.h (right):

https://codereview.chromium.org/12326091/diff/33001/ui/message_center/notific...
ui/message_center/notification_change_observer.h:15: class MESSAGE_CENTER_EXPORT
NotificationChangeObserver {
If you anticipate both that (1) this observer class will be adding more methods
in the future, and (2) the folks doing concrete implementations will be spread
across many different OWNER areas of the codebase, then you might want to
consider default implementations of these methods, so that adding new methods
won't cause a hellish N-reviewer mess across OWNERS.

[commit-bot: I haz the power, 2013-03-05 21:03:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/12326091/52001

[commit-bot: I haz the power, 2013-03-05 21:03:54]

Failed to apply patch for ui/message_center/views/message_popup_collection.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ui/message_center/views/message_popup_collection.h
  Hunk #1 FAILED at 8.
  Hunk #2 succeeded at 25 (offset 5 lines).
  Hunk #3 succeeded at 39 (offset 5 lines).
  Hunk #4 succeeded at 54 (offset 7 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
ui/message_center/views/message_popup_collection.h.rej

Patch:       ui/message_center/views/message_popup_collection.h
Index: ui/message_center/views/message_popup_collection.h
diff --git a/ui/message_center/views/message_popup_collection.h
b/ui/message_center/views/message_popup_collection.h
index
3bf65681033527c37802f2fede9f7827db9cb8ec..ff63789796e51491d930d4df9eb3a6f7974d37e6
100644
--- a/ui/message_center/views/message_popup_collection.h
+++ b/ui/message_center/views/message_popup_collection.h
@@ -8,10 +8,9 @@
 #include <list>
 #include <map>
 
-#include "base/timer.h"
+#include "base/compiler_specific.h"
+#include "ui/gfx/native_widget_types.h"
 #include "ui/message_center/message_center_export.h"
-#include "ui/message_center/notification_list.h"
-#include "ui/message_center/views/message_bubble_base.h"
 #include "ui/views/widget/widget_observer.h"
 
 namespace views {
@@ -20,6 +19,7 @@ class Widget;
 
 namespace message_center {
 
+class MessageCenter;
 class ToastContentsView;
 
 // Container for popup toasts. Because each toast is a frameless window rather
@@ -33,7 +33,7 @@ class MESSAGE_CENTER_EXPORT MessagePopupCollection
   // |context| specifies the context to create toast windows. It can be NULL
   // for non-aura environment. See comments in ui/views/widget/widget.h.
   MessagePopupCollection(gfx::NativeView context,
-                         NotificationList::Delegate* list_delegate);
+                         MessageCenter* message_center);
   virtual ~MessagePopupCollection();
 
   void UpdatePopups();
@@ -46,11 +46,11 @@ class MESSAGE_CENTER_EXPORT MessagePopupCollection
 
   void CloseAllWidgets();
 
-  // views::WidgetObserver overrides:
+  // Overridden from views::WidgetObserver:
   virtual void OnWidgetDestroying(views::Widget* widget) OVERRIDE;
 
   gfx::NativeView context_;
-  NotificationList::Delegate* list_delegate_;
+  MessageCenter* message_center_;
   ToastContainer toasts_;
 
   DISALLOW_COPY_AND_ASSIGN(MessagePopupCollection);

[commit-bot: I haz the power, 2013-03-05 21:40:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/12326091/56001

[commit-bot: I haz the power, 2013-03-05 22:12:30]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-05 23:10:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/12326091/61005

[commit-bot: I haz the power, 2013-03-05 23:49:10]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-06 16:41:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/12326091/85001

[commit-bot: I haz the power, 2013-03-06 16:41:22]

Presubmit check for 12326091-85001 failed and returned exit status 1.

INFO:root:Found 31 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ash/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/views/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    ash/system/web_notification/web_notification_tray_unittest.cc

Presubmit checks took 2.8s to calculate.

[dharcourt, 2013-03-06 16:52:00]

Hi Steven,

Could you do an OWNERS review of
ash/system/web_notification/web_notification_tray_unittest.cc? The changes
introduced yesterday with http://crrev.com/12391041 were incompatible with some
refactoring that was in-flight with this change list.

Thanks,

    - Charles

[commit-bot: I haz the power, 2013-03-07 18:02:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/12326091/85001

[commit-bot: I haz the power, 2013-03-07 18:03:08]

Failed to apply patch for ui/message_center/notification.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ui/message_center/notification.h
  Hunk #1 FAILED at 57.
  Hunk #2 succeeded at 67 with fuzz 2.
  Hunk #3 FAILED at 100.
  2 out of 3 hunks FAILED -- saving rejects to file
ui/message_center/notification.h.rej

Patch:       ui/message_center/notification.h
Index: ui/message_center/notification.h
diff --git a/ui/message_center/notification.h b/ui/message_center/notification.h
index
36e4504b3a37db34e2f8b8bed258162ebf014295..ede2a60342edab4e652e4c3199dd721d1c6de4c3
100644
--- a/ui/message_center/notification.h
+++ b/ui/message_center/notification.h
@@ -57,8 +57,8 @@ class MESSAGE_CENTER_EXPORT Notification {
   // End unpacked values.
 
   // Images fetched asynchronously.
-  const gfx::ImageSkia& primary_icon() const { return primary_icon_; }
-  void set_primary_icon(const gfx::ImageSkia& icon) { primary_icon_ = icon; }
+  const gfx::ImageSkia& icon() const { return icon_; }
+  void set_icon(const gfx::ImageSkia& icon) { icon_ = icon; }
 
   const gfx::ImageSkia& image() const { return image_; }
   void set_image(const gfx::ImageSkia& image) { image_ = image; }
@@ -67,15 +67,19 @@ class MESSAGE_CENTER_EXPORT Notification {
   const std::vector<ButtonInfo>& buttons() const { return buttons_; }
   bool SetButtonIcon(size_t index, const gfx::ImageSkia& icon);
 
-  // Status in MessageCenter.
-  bool is_read() const { return is_read_; }
-  void set_is_read(bool is_read) { is_read_ = is_read; }
-
   bool shown_as_popup() const { return shown_as_popup_; }
   void set_shown_as_popup(bool shown_as_popup) {
     shown_as_popup_ = shown_as_popup;
   }
 
+  // Read status in the message center.
+  bool is_read() const { return is_read_; }
+  void set_is_read(bool read) { is_read_ = read; }
+
+  // Expanded status in the message center (not the popups).
+  bool is_expanded() const { return is_expanded_; }
+  void set_is_expanded(bool expanded) { is_expanded_ = expanded; }
+
   // Used to keep the order of notifications with the same timestamp.
   // The notification with lesser serial_number is considered 'older'.
   unsigned serial_number() { return serial_number_; }
@@ -96,11 +100,12 @@ class MESSAGE_CENTER_EXPORT Notification {
   unsigned serial_number_;
   string16 expanded_message_;
   std::vector<NotificationItem> items_;
-  gfx::ImageSkia primary_icon_;
+  gfx::ImageSkia icon_;
   gfx::ImageSkia image_;
   std::vector<ButtonInfo> buttons_;
+  bool shown_as_popup_;  // True if this has been shown as a popup.
   bool is_read_;  // True if this has been seen in the message center.
-  bool shown_as_popup_;  // True if this has been shown as a popup
notification.
+  bool is_expanded_;  // True if this has been expanded in the message center.
 
   DISALLOW_COPY_AND_ASSIGN(Notification);
 };

[commit-bot: I haz the power, 2013-03-07 18:10:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/12326091/99001

[commit-bot: I haz the power, 2013-03-07 18:42:05]

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

[commit-bot: I haz the power, 2013-03-07 19:10:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/12326091/94003

[commit-bot: I haz the power, 2013-03-07 20:16:36]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-07 22:35:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/12326091/123001

[commit-bot: I haz the power, 2013-03-08 01:32:42]

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

[commit-bot: I haz the power, 2013-03-08 01:59:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/12326091/123001

[commit-bot: I haz the power, 2013-03-08 03:55:41]

Change committed as 186873