Added support for image notifications.

Added support for image notifications.

Main review by mukai, miket for OWNERS review of
ui/notifications/notification_types.{h,cc} changes.

TBR=derat@chromium.org for OWNERS review of balloon_view_ash.cc changes.
BUG=164292
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175247

[dharcourt, 2012-12-20 14:16:16]

Jun, thanks in advance for the review.

Mike, I'm adding you for an OWNERS review of the
ui/notifications/notification_types.*. Any suggestion you can make will be
welcome, but I'll count on Jun for the meat of the review and hopefully for an
eventual LGTM.

Daniel, I'm TBR'ing you for an OWNERS review of the
chrome/browser/ui/views/ash/balloon_view_ash.* changes. My understanding from
other change lists I've seen is that this works for you. If I misunderstood, let
me know and I'll add you in the Reviewers field instead.

After this, there'll be two more change lists concerning image notifications and
multiple item notifications: 1) Some tweaks to the pixel position of things; and
2) The removal of all traces of secondary notification icon code. Then it's on
the merging BaseFormatView with NotificationView and adding expand/collapse.

Thanks for the reviews!

    - Charles

[Jun Mukai, 2012-12-20 18:26:49]

it looks good basically, but can you write the comment to NotificationView class
in the header file?  It is now unclear what 'NotificationView' is.  It would be
simple to be like this:
// The class used to summarize multiple messages or to show an image.

Also I'm now wondering why these two features are merged into one class. 
Multiple messages and image view sound quite different, why don't you separate
another class for images?

https://codereview.chromium.org/11639041/diff/1/ui/message_center/notificatio...
File ui/message_center/notification_view.cc (right):

https://codereview.chromium.org/11639041/diff/1/ui/message_center/notificatio...
ui/message_center/notification_view.cc:175: }
remove braces for 1-line if.

[dharcourt, 2012-12-20 20:51:26]

On 2012/12/20 18:26:49, Jun Mukai wrote:
> it looks good basically, but can you write the comment to NotificationView
class
> in the header file?  It is now unclear what 'NotificationView' is.  It would
be
> simple to be like this:
> // The class used to summarize multiple messages or to show an image.
> 
> Also I'm now wondering why these two features are merged into one class. 
> Multiple messages and image view sound quite different, why don't you separate
> another class for images?
> 
>
https://codereview.chromium.org/11639041/diff/1/ui/message_center/notificatio...
> File ui/message_center/notification_view.cc (right):
> 
>
https://codereview.chromium.org/11639041/diff/1/ui/message_center/notificatio...
> ui/message_center/notification_view.cc:175: }
> remove braces for 1-line if.

It sounds like I misunderstood something we talked about a couple of days. I
thought you were actually suggesting merging all the notification view
functionality (for web, basic, image, and multiple-item notifications)  into a
single class. Because so much is shared, this actually saves a lot of code over
having multiple classes share code through an inheritance hierarchy. It might
even be possible in the future to define the differences between different types
of notification views declaratively, so adding a new type would simply be a
matter of adding a value or set of values to a constant. Let's talk about this
offline...

[dharcourt, 2012-12-20 20:51:32]

https://codereview.chromium.org/11639041/diff/1/ui/message_center/notificatio...
File ui/message_center/notification_view.cc (right):

https://codereview.chromium.org/11639041/diff/1/ui/message_center/notificatio...
ui/message_center/notification_view.cc:175: }
On 2012/12/20 18:26:49, Jun Mukai wrote:
> remove braces for 1-line if.

Done.

[miket_OOO, 2012-12-20 21:35:26]

ui/notifications/notification_types.{h,cc} LGTM

[Jun Mukai, 2012-12-20 22:28:21]

On 2012/12/20 20:51:26, dharcourt wrote:
> On 2012/12/20 18:26:49, Jun Mukai wrote:
> > it looks good basically, but can you write the comment to NotificationView
> class
> > in the header file?  It is now unclear what 'NotificationView' is.  It would
> be
> > simple to be like this:
> > // The class used to summarize multiple messages or to show an image.
> > 
> > Also I'm now wondering why these two features are merged into one class. 
> > Multiple messages and image view sound quite different, why don't you
separate
> > another class for images?
> > 
> >
>
https://codereview.chromium.org/11639041/diff/1/ui/message_center/notificatio...
> > File ui/message_center/notification_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/11639041/diff/1/ui/message_center/notificatio...
> > ui/message_center/notification_view.cc:175: }
> > remove braces for 1-line if.
> 
> It sounds like I misunderstood something we talked about a couple of days. I
> thought you were actually suggesting merging all the notification view
> functionality (for web, basic, image, and multiple-item notifications)  into a
> single class. Because so much is shared, this actually saves a lot of code
over
> having multiple classes share code through an inheritance hierarchy. It might
> even be possible in the future to define the differences between different
types
> of notification views declaratively, so adding a new type would simply be a
> matter of adding a value or set of values to a constant. Let's talk about this
> offline...

Ah, what I said would be to introduce delegation over the view.
- class NotificationView will have NotificationViewDelegate
- the delegate class builds the contents, such like list-up of multiple items
and/or images

so the code, in my mind, would look like:
    :
  delegate_->CreateView(notification);
    :
  // for the contents, like the summary lines of multiple items or message of
single item
  if (delegate_->HasContentsView())
    layout->AddView(delegate_->GetContentsView());
  // for the view filled at the end of the card, like image
  if (delegate_->HasTrailingView())
    layout->AddView(delegate_->GetTrailingView())

ui/views/window/dialog_* code would be doing similar approach --
dialog_client_view provides the OK/Cancel buttons and the user inherits
dialog_delegate and builds its contents view in the dialog.
I am not 100% sure this approach will work well, or simplify the code. Sorry for
the confusion.

Putting LGTM in advance, you can proceed with the current code. Please let me
know if you redesign the code.

[commit-bot: I haz the power, 2013-01-04 16:42:01]

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

[commit-bot: I haz the power, 2013-01-04 17:00:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
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-01-04 18:07:49]

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

[commit-bot: I haz the power, 2013-01-04 19:02:47]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2013-01-04 19:21:37]

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

[commit-bot: I haz the power, 2013-01-04 19:55:25]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2013-01-04 20:59:56]

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

[commit-bot: I haz the power, 2013-01-05 01:15:32]

Change committed as 175247