Updated multiple item notification view to match latest mockups

Updated multiple item notification view to match latest mockups

This also prepares this view to serve as a generic notification view
by simplifying the layout grid and by making the padding mechanism
more flexible. The resulting more generic view will be able to be used
to display image and basic notifications in addition to multiple item
ones. The visual changes made to match the mockups include removal of
the timestamp, separator line, message, and secondary icon.

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

[dharcourt, 2012-12-18 23:58:03]

This is the first of four change lists that will introduce the image
notification display and the new notification look:

1) This change list updates the multiple item notification view's look and
readies it to be used as an image notification view.

2) The second change list will rename ItemViewMultiple to NotificationView to
reflect its soon-to-be more generic nature.

3) The third change list will add support for image notifications to the API,
back-end, and to the new NotificationView.

4) The fourth change list will remove API and back-end support for the secondary
icon which is not used in the new notification style.

[Jun Mukai, 2012-12-19 00:33:53]

Several nitpicking comments.  They're not so relevant to your changes themselves
but would be better to be done at some point anyway.

https://codereview.chromium.org/11651002/diff/3001/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/balloon_view_ash.cc (right):

https://codereview.chromium.org/11651002/diff/3001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/balloon_view_ash.cc:33: const int
kPrimaryIconImageSize = 80;
It seems that this value is shared with values in message_view_multiple.cc
Then better to export the value in message_center side and use it here.

Also, you may want to create new files message_center_constants.(h|cc) and move
such shared constant variables there.  It's common among chromium (see
ash/system/tray/tray_constants.* for example)

https://codereview.chromium.org/11651002/diff/3001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/balloon_view_ash.cc:34: const int
kSecondaryIconImageSize = 15;
We don't use the secondary icon anymore right? then remove this.

https://codereview.chromium.org/11651002/diff/3001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/balloon_view_ash.cc:203: // Set up a download for
the notification's secondary icon if appropriate.
is it necessary?

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
File ui/message_center/message_view_multiple.cc (right):

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:57: size_(0, 0) {
It's not this, but I noticed that ctor/dtor is declared inline but other methods
are not.  Can you keep the consistency?
Personally I think you can declare all inline in this class.

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:80: virtual void OnPaint(gfx::Canvas*
canvas) OVERRIDE;
merge line 71-76 and this line.  It doesn't make much sense some inherited
methods are public but some are protected.  I personally prefer everything in
protected.

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:122: void
ItemView::OnPaint(gfx::Canvas* canvas) {
Can you create views::Labels and put them to the children of this class (by
AddChildView()?)  You can align the title and message by horizontal BoxLayout. 
Then you don't need to override GetBaseline and OnPaint and all your class have
to care would be the width.

Also, after that I believe you don't need to override AccessibleState.  Label
class will take care instead of you.

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:150: class BoxView : public
views::View {
This seems a transparent view used for padding. Am I right? Then, you don't need
this, rather you can just set EmptyBorder of the size (see ui/views/border.h) to
MessageViewMultiple.

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
File ui/message_center/message_view_multiple.h (right):

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
ui/message_center/message_view_multiple.h:27: MessageViewMultiple();
this method is removed in cc, please remove from here too.

[dharcourt, 2012-12-19 01:47:44]

Thanks for the review. Lots of good fixes suggested. I implemented most of them
and also have questions below.

    - Charles

https://codereview.chromium.org/11651002/diff/3001/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/balloon_view_ash.cc (right):

https://codereview.chromium.org/11651002/diff/3001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/balloon_view_ash.cc:33: const int
kPrimaryIconImageSize = 80;
On 2012/12/19 00:33:53, Jun Mukai wrote:
> It seems that this value is shared with values in message_view_multiple.cc
> Then better to export the value in message_center side and use it here.
> 
> Also, you may want to create new files message_center_constants.(h|cc) and
move
> such shared constant variables there.  It's common among chromium (see
> ash/system/tray/tray_constants.* for example)

Done.

https://codereview.chromium.org/11651002/diff/3001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/balloon_view_ash.cc:34: const int
kSecondaryIconImageSize = 15;
On 2012/12/19 00:33:53, Jun Mukai wrote:
> We don't use the secondary icon anymore right? then remove this.

Done.

https://codereview.chromium.org/11651002/diff/3001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/balloon_view_ash.cc:203: // Set up a download for
the notification's secondary icon if appropriate.
On 2012/12/19 00:33:53, Jun Mukai wrote:
> is it necessary?

Not any more. Removed.

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
File ui/message_center/message_view_multiple.cc (right):

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:57: size_(0, 0) {
On 2012/12/19 00:33:53, Jun Mukai wrote:
> It's not this, but I noticed that ctor/dtor is declared inline but other
methods
> are not.  Can you keep the consistency?
> Personally I think you can declare all inline in this class.

I de-inlined the constructor & destructor to make things consistent. It turns
out http://goo.gl/S8DJC suggests I should never have inlined them.

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:80: virtual void OnPaint(gfx::Canvas*
canvas) OVERRIDE;
On 2012/12/19 00:33:53, Jun Mukai wrote:
> merge line 71-76 and this line.  It doesn't make much sense some inherited
> methods are public but some are protected.  I personally prefer everything in
> protected.

I didn't know this was possible in C++. Wouldn't turning superclass public
methods into private methods be confusing, though? http://goo.gl/ipVfR suggests
some problems it could cause. I'd rather avoid doing that if possible, either by
leaving things as is or by making OnPaint() public instead of making the public
methods protected. Of course, I'd be happy to make all the methods protected if
that's a common pattern here and if it helps readability..

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:122: void
ItemView::OnPaint(gfx::Canvas* canvas) {
On 2012/12/19 00:33:53, Jun Mukai wrote:
> Can you create views::Labels and put them to the children of this class (by
> AddChildView()?)  You can align the title and message by horizontal BoxLayout.

> Then you don't need to override GetBaseline and OnPaint and all your class
have
> to care would be the width.
> 
> Also, after that I believe you don't need to override AccessibleState.  Label
> class will take care instead of you.

I'm trying this out... I'll upload a patch and update this comment when I have
it working.

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:150: class BoxView : public
views::View {
On 2012/12/19 00:33:53, Jun Mukai wrote:
> This seems a transparent view used for padding. Am I right? Then, you don't
need
> this, rather you can just set EmptyBorder of the size (see ui/views/border.h)
to
> MessageViewMultiple.

This is indeed a transparent view used for padding, but I can't use an
EmptyBorder to do its job: The padding is needed by an ImageButton subview of
MessageViewMultiple, and ImageButton doesn't work well with borders (the image
gets placed in the wrong place and clipped). I end up having to create a row
with a BoxView above the ImageButton to have it padded correctly relative to the
icon in another column.

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
File ui/message_center/message_view_multiple.h (right):

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
ui/message_center/message_view_multiple.h:27: MessageViewMultiple();
On 2012/12/19 00:33:53, Jun Mukai wrote:
> this method is removed in cc, please remove from here too.

Done.

[Jun Mukai, 2012-12-19 22:28:41]

lgtm

with nitpicking comments

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
File ui/message_center/message_view_multiple.cc (right):

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:80: virtual void OnPaint(gfx::Canvas*
canvas) OVERRIDE;
On 2012/12/19 01:47:44, dharcourt wrote:
> On 2012/12/19 00:33:53, Jun Mukai wrote:
> > merge line 71-76 and this line.  It doesn't make much sense some inherited
> > methods are public but some are protected.  I personally prefer everything
in
> > protected.
> 
> I didn't know this was possible in C++. Wouldn't turning superclass public
> methods into private methods be confusing, though? http://goo.gl/ipVfR
suggests
> some problems it could cause. I'd rather avoid doing that if possible, either
by
> leaving things as is or by making OnPaint() public instead of making the
public
> methods protected. Of course, I'd be happy to make all the methods protected
if
> that's a common pattern here and if it helps readability..

It is quite common among chromium code base to put inherited methods to
protected or private. I don't agree with the discussion on the stack overflow...
but anyway, here isn't the place to discuss about that.
I'd say the consistency is more important.  Please move OnPaint to public then.

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:150: class BoxView : public
views::View {
On 2012/12/19 01:47:44, dharcourt wrote:
> On 2012/12/19 00:33:53, Jun Mukai wrote:
> > This seems a transparent view used for padding. Am I right? Then, you don't
> need
> > this, rather you can just set EmptyBorder of the size (see
ui/views/border.h)
> to
> > MessageViewMultiple.
> 
> This is indeed a transparent view used for padding, but I can't use an
> EmptyBorder to do its job: The padding is needed by an ImageButton subview of
> MessageViewMultiple, and ImageButton doesn't work well with borders (the image
> gets placed in the wrong place and clipped). I end up having to create a row
> with a BoxView above the ImageButton to have it padded correctly relative to
the
> icon in another column.

ah, I've misunderstood. thanks for description.

https://codereview.chromium.org/11651002/diff/7002/ui/message_center/message_...
File ui/message_center/message_view_multiple.cc (right):

https://codereview.chromium.org/11651002/diff/7002/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:158: color_(color) {
then, these method should be out-lined for the consistency?  sorry for the late
catch.

[dharcourt, 2012-12-20 03:27:35]

Hi Jun,

Following up on your review feedback, I was able to significantly reduce the
size of ItemView and get rid of BoxView entirely! The code in
message_view_multiple.cc is now 96 lines shorter (-34%) than in the first patch,
thanks!

Please take a look at the patch 3 changes if you get a chance. Thanks,

    - Charles

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
File ui/message_center/message_view_multiple.cc (right):

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:80: virtual void OnPaint(gfx::Canvas*
canvas) OVERRIDE;
On 2012/12/19 22:28:41, Jun Mukai wrote:
> On 2012/12/19 01:47:44, dharcourt wrote:
> > On 2012/12/19 00:33:53, Jun Mukai wrote:
> > > merge line 71-76 and this line.  It doesn't make much sense some inherited
> > > methods are public but some are protected.  I personally prefer everything
> in
> > > protected.
> > 
> > I didn't know this was possible in C++. Wouldn't turning superclass public
> > methods into private methods be confusing, though? http://goo.gl/ipVfR
> suggests
> > some problems it could cause. I'd rather avoid doing that if possible,
either
> by
> > leaving things as is or by making OnPaint() public instead of making the
> public
> > methods protected. Of course, I'd be happy to make all the methods protected
> if
> > that's a common pattern here and if it helps readability..
> 
> It is quite common among chromium code base to put inherited methods to
> protected or private. I don't agree with the discussion on the stack
overflow...
> but anyway, here isn't the place to discuss about that.
> I'd say the consistency is more important.  Please move OnPaint to public
then.

After implementing your suggestions for using Labels in ItemView, these methods
have all gone away, so this is moot :-). Otherwise I would have been happy to
make all the methods protected for consistency with the code base (in any case,
because ItemView is a private class, changing the access modifiers would have no
significant effect except improving code readability). I'd like to discuss this
general pattern with you offline to better understand it.

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:122: void
ItemView::OnPaint(gfx::Canvas* canvas) {
On 2012/12/19 01:47:44, dharcourt wrote:
> On 2012/12/19 00:33:53, Jun Mukai wrote:
> > Can you create views::Labels and put them to the children of this class (by
> > AddChildView()?)  You can align the title and message by horizontal
BoxLayout.
> 
> > Then you don't need to override GetBaseline and OnPaint and all your class
> have
> > to care would be the width.
> > 
> > Also, after that I believe you don't need to override AccessibleState. 
Label
> > class will take care instead of you.
> 
> I'm trying this out... I'll upload a patch and update this comment when I have
> it working.

Done, and it works very well and simplifies things.

https://codereview.chromium.org/11651002/diff/3001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:150: class BoxView : public
views::View {
On 2012/12/19 22:28:41, Jun Mukai wrote:
> On 2012/12/19 01:47:44, dharcourt wrote:
> > On 2012/12/19 00:33:53, Jun Mukai wrote:
> > > This seems a transparent view used for padding. Am I right? Then, you
don't
> > need
> > > this, rather you can just set EmptyBorder of the size (see
> ui/views/border.h)
> > to
> > > MessageViewMultiple.
> > 
> > This is indeed a transparent view used for padding, but I can't use an
> > EmptyBorder to do its job: The padding is needed by an ImageButton subview
of
> > MessageViewMultiple, and ImageButton doesn't work well with borders (the
image
> > gets placed in the wrong place and clipped). I end up having to create a row
> > with a BoxView above the ImageButton to have it padded correctly relative to
> the
> > icon in another column.
> 
> ah, I've misunderstood. thanks for description.

Inspired by the ItemView reduction in size, and prompted by your comment, I
tried using an empty ImageView in conjunction with borders to get the padding I
need, and it works (see code changes below).

Box view is now unnecessary and a bunch of code is avoided :-).

https://codereview.chromium.org/11651002/diff/7002/ui/message_center/message_...
File ui/message_center/message_view_multiple.cc (right):

https://codereview.chromium.org/11651002/diff/7002/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:158: color_(color) {
On 2012/12/19 22:28:41, Jun Mukai wrote:
> then, these method should be out-lined for the consistency?  sorry for the
late
> catch.

Correct, those should not be inline for consistency and according to the style
guide. Luckily BoxView has gone away so this is all moot :-).

[Jun Mukai, 2012-12-20 18:09:10]

lgtm++

[commit-bot: I haz the power, 2013-01-03 17:17:44]

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

[commit-bot: I haz the power, 2013-01-03 18:06:52]

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-01-03 18:20:35]

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

[commit-bot: I haz the power, 2013-01-03 19:17:52]

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

[commit-bot: I haz the power, 2013-01-03 19:27:30]

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

[commit-bot: I haz the power, 2013-01-03 19:46:52]

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-01-03 21:56:33]

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

[commit-bot: I haz the power, 2013-01-03 22:00:05]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-03 22:41:20]

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

[commit-bot: I haz the power, 2013-01-04 02:07:51]

Change committed as 175079