Added padding below text in notifications.

Added padding below text in notifications.

This padding was in some previous versions but got lost somewhere along
the way. This change restores it so there are always at least 12 pixels
between the baseline of the last line of text and the bottom of the card
or the image below the text. This will be true for WebKit, simple,
basic, and image notifications but also for list ones.

Note that although there will be exactly 12 pixels of space on Windows,
there will be 13 pixels On Chrome OS because different fonts are used
there, and also that if the Chrome OS system font changes that number of
pixels may change slightly.

This change also makes the padding between the top of notification
text and the notification card be 12 pixels, as it was designed to be.
It had somehow become 11 pixels.

BUG=225871
TEST=Bring up a notification, take a screenshot, and count pixels.

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

[dharcourt, 2013-04-03 17:41:45]

https://codereview.chromium.org/13560002/diff/1/ui/message_center/views/notif...
File ui/message_center/views/notification_view.cc (left):

https://codereview.chromium.org/13560002/diff/1/ui/message_center/views/notif...
ui/message_center/views/notification_view.cc:36: const int kTextBottomPadding =
6;
This wasn't being used.

[msw, 2013-04-03 18:04:26]

https://codereview.chromium.org/13560002/diff/1/ui/message_center/views/notif...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/13560002/diff/1/ui/message_center/views/notif...
ui/message_center/views/notification_view.cc:412: views::View* padding_view =
new views::ImageView();
Adding a view for padding is rather heavy-weight. Instead, just adjust
GetPreferredSize and Layout to include the additional inset.

[dharcourt, 2013-04-04 18:33:52]

Jun, here's another way to achieve this without adding views. PTAL.

Mike, I didn't add you to the reviewers, but I see you're there. Your comments
are appreciated if you have some, and you can also feel free to leave the review
to Jun, whose LGTM I'll be waiting for.

Thanks!

   - Charles

https://codereview.chromium.org/13560002/diff/1/ui/message_center/views/notif...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/13560002/diff/1/ui/message_center/views/notif...
ui/message_center/views/notification_view.cc:412: views::View* padding_view =
new views::ImageView();
On 2013/04/03 18:04:26, msw wrote:
> Adding a view for padding is rather heavy-weight. Instead, just adjust
> GetPreferredSize and Layout to include the additional inset.

I was able to accomplish this using borders, which leaves GetPreferredSize() and
Layout() simple and unaware of these padding details.

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

https://codereview.chromium.org/13560002/diff/5001/ui/message_center/message_...
ui/message_center/message_center_constants.cc:18: const int kTextTopPadding =
12;
Changed this to be the actual desired padding between the top of the
notification and the top of text capitals. Code will adjusts this value to
account for white space in font metrics.

[msw, 2013-04-04 18:44:49]

I'm CC'ed on crbug.com/225871 and saw the CL mentioned, so I decided to do a
quick drive-by. You can remove me from reviewers if you like, and you certainly
don't need my approval here, but I'm glad you addressed my comment. Drive-by
reviews are a nice part of Chrome development :)

https://codereview.chromium.org/13560002/diff/1/ui/message_center/views/notif...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/13560002/diff/1/ui/message_center/views/notif...
ui/message_center/views/notification_view.cc:412: views::View* padding_view =
new views::ImageView();
On 2013/04/04 18:33:52, dharcourt wrote:
> On 2013/04/03 18:04:26, msw wrote:
> > Adding a view for padding is rather heavy-weight. Instead, just adjust
> > GetPreferredSize and Layout to include the additional inset.
> 
> I was able to accomplish this using borders, which leaves GetPreferredSize()
and
> Layout() simple and unaware of these padding details.

Awesome, that's even better.

https://codereview.chromium.org/13560002/diff/5001/ui/message_center/views/no...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/13560002/diff/5001/ui/message_center/views/no...
ui/message_center/views/notification_view.cc:383:
title_view_->set_border(MakeBorder(0, 0));
nit: this seems unnecessary now.

[dharcourt, 2013-04-06 00:13:41]

On 2013/04/04 18:44:49, msw wrote:
> I'm CC'ed on crbug.com/225871 and saw the CL mentioned, so I decided to do a
> quick drive-by. You can remove me from reviewers if you like, and you
certainly
> don't need my approval here, but I'm glad you addressed my comment. Drive-by
> reviews are a nice part of Chrome development :)
> 
>
https://codereview.chromium.org/13560002/diff/1/ui/message_center/views/notif...
> File ui/message_center/views/notification_view.cc (right):
> 
>
https://codereview.chromium.org/13560002/diff/1/ui/message_center/views/notif...
> ui/message_center/views/notification_view.cc:412: views::View* padding_view =
> new views::ImageView();
> On 2013/04/04 18:33:52, dharcourt wrote:
> > On 2013/04/03 18:04:26, msw wrote:
> > > Adding a view for padding is rather heavy-weight. Instead, just adjust
> > > GetPreferredSize and Layout to include the additional inset.
> > 
> > I was able to accomplish this using borders, which leaves GetPreferredSize()
> and
> > Layout() simple and unaware of these padding details.
> 
> Awesome, that's even better.
> 
>
https://codereview.chromium.org/13560002/diff/5001/ui/message_center/views/no...
> File ui/message_center/views/notification_view.cc (right):
> 
>
https://codereview.chromium.org/13560002/diff/5001/ui/message_center/views/no...
> ui/message_center/views/notification_view.cc:383:
> title_view_->set_border(MakeBorder(0, 0));
> nit: this seems unnecessary now.

Mike, I apologize, I had spaced out and thought the original comment was from
Jun, so I didn't see where your name came from on the reviewers line, and I was
afraid I might have fat-fingered it there. I didn't want you to feel obligated
to review, hence my last comment, but I actually appreciate Drive-by reviews
very much, thanks!

Anyway this last patch should address your set_border(MakeBorder(0, 0)) comment.
It also tweaks some values to fix some padding errors found in testing (padding
had to be correct at the top and bottom of notifications, but also between
notifications' title and message, list items, etc...).

MakeBorder() had some magical default argument values that meant MakeBorder(0,
0) was not making a zero-width border. This was far from clear so I've split
MakeBorder() into MakeEmptyBorder(), MakeTextBorder(), and
MakeSeparatorBorder(). This removes the default arguments and hopefully makes
things clearer.

Jun or Mike, can I get a LGTM when this has become acceptable?

Thanks!

    - Charles

[dharcourt, 2013-04-06 00:21:08]

Robert, could you take a look at notification_controller.mm?

I changed message_center_constants.cc's kTextTopPadding value to be the number
of blank pixels in the padding (it previously was an implementation-dependent
magic number that would _lead_ to the required number of blank pixels). This
caused some changes to notification_controller.mm, and I made those changes
minimally. Let me know if you'd like me to make those changes differently (for
example by introducing a derived local constant or a local delta constant), or
if I should commit like this.

Thanks!

    - Charles

[Robert Sesek, 2013-04-08 13:32:12]

Can you pull out 6 into a namespace {} constant? I think it'd be fine to either
derive this from another value or to make it just 6.

[dharcourt, 2013-04-08 20:35:04]

Jun, can I get a review & comments or LGTM? Thanks!

Robert, I modified the constant as you suggested, thanks for the review.

    - Charles

[Jun Mukai, 2013-04-08 20:42:25]

lgtm

[commit-bot: I haz the power, 2013-04-08 20:44:42]

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

[commit-bot: I haz the power, 2013-04-08 21:08:31]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
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.

[Robert Sesek, 2013-04-08 21:15:19]

LGTM

https://chromiumcodereview.appspot.com/13560002/diff/29001/ui/message_center/...
File ui/message_center/cocoa/notification_controller.mm (right):

https://chromiumcodereview.appspot.com/13560002/diff/29001/ui/message_center/...
ui/message_center/cocoa/notification_controller.mm:20: // Compensates for
padding already provided by UI elements involved.
Should go above the constant without any indentation.

https://chromiumcodereview.appspot.com/13560002/diff/29001/ui/message_center/...
ui/message_center/cocoa/notification_controller.mm:89:
message_center::kTextTopPadding + kTextTopPaddingAdjustment) {
nit: indent 4 spaces since this is a continuation

[dharcourt, 2013-04-08 21:30:26]

Thanks for the review!

    - C=

https://codereview.chromium.org/13560002/diff/29001/ui/message_center/cocoa/n...
File ui/message_center/cocoa/notification_controller.mm (right):

https://codereview.chromium.org/13560002/diff/29001/ui/message_center/cocoa/n...
ui/message_center/cocoa/notification_controller.mm:20: // Compensates for
padding already provided by UI elements involved.
On 2013/04/08 21:15:19, rsesek wrote:
> Should go above the constant without any indentation.

Done.

https://codereview.chromium.org/13560002/diff/29001/ui/message_center/cocoa/n...
ui/message_center/cocoa/notification_controller.mm:89:
message_center::kTextTopPadding + kTextTopPaddingAdjustment) {
On 2013/04/08 21:15:19, rsesek wrote:
> nit: indent 4 spaces since this is a continuation

Done.

[commit-bot: I haz the power, 2013-04-08 21:30:45]

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

[commit-bot: I haz the power, 2013-04-09 00:08:56]

Change committed as 192957