Update notification style: allow 2-line titles if the message is blank.

Update notification style: allow 2-line titles if the message is blank.

This is to support longer text in simple notifications.

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

[Jun Mukai, 2014-04-26 00:08:48]

lgtm

[dewittj, 2014-04-28 20:45:55]

rsesek: ping

[Robert Sesek, 2014-04-28 20:49:28]

Sorry, please explicitly call out reviewers, otherwise it looks like just a CC.

https://codereview.chromium.org/256883002/diff/40001/ui/message_center/cocoa/...
File ui/message_center/cocoa/notification_controller.mm (right):

https://codereview.chromium.org/256883002/diff/40001/ui/message_center/cocoa/...
ui/message_center/cocoa/notification_controller.mm:350:
setString:base::SysUTF16ToNSString([self wrapText:notification_->title()
You probably clang-formatted this, but it looks wrong. It should break like it
used to.

In fact, this is just churn for the sake of churn. You're changing the value of
one param, the diff includes 3 extra lines.

https://codereview.chromium.org/256883002/diff/40001/ui/message_center/cocoa/...
ui/message_center/cocoa/notification_controller.mm:369: // is empty, mark it as
hidden and set 0 hight, so it doesn't take up any
nit: height

[dewittj, 2014-04-28 21:05:31]

Will call you out in the future.  Thanks for letting me know why it didn't stand
out to you.

https://codereview.chromium.org/256883002/diff/40001/ui/message_center/cocoa/...
File ui/message_center/cocoa/notification_controller.mm (right):

https://codereview.chromium.org/256883002/diff/40001/ui/message_center/cocoa/...
ui/message_center/cocoa/notification_controller.mm:350:
setString:base::SysUTF16ToNSString([self wrapText:notification_->title()
On 2014/04/28 20:49:28, rsesek wrote:
> You probably clang-formatted this, but it looks wrong. It should break like it
> used to.
> 
> In fact, this is just churn for the sake of churn. You're changing the value
of
> one param, the diff includes 3 extra lines.

Done.

This is not actually churn just for the sake of churn.  Once changed to match
clang-format, no more churn will happen and formatting becomes completely a
non-issue.  I think the objective-c formatting could probably use some tweaking.

https://codereview.chromium.org/256883002/diff/40001/ui/message_center/cocoa/...
ui/message_center/cocoa/notification_controller.mm:369: // is empty, mark it as
hidden and set 0 hight, so it doesn't take up any
On 2014/04/28 20:49:28, rsesek wrote:
> nit: height

Done.

[Robert Sesek, 2014-04-28 21:07:28]

lgtm

https://codereview.chromium.org/256883002/diff/40001/ui/message_center/cocoa/...
File ui/message_center/cocoa/notification_controller.mm (right):

https://codereview.chromium.org/256883002/diff/40001/ui/message_center/cocoa/...
ui/message_center/cocoa/notification_controller.mm:350:
setString:base::SysUTF16ToNSString([self wrapText:notification_->title()
On 2014/04/28 21:05:32, dewittj wrote:
> On 2014/04/28 20:49:28, rsesek wrote:
> > You probably clang-formatted this, but it looks wrong. It should break like
it
> > used to.
> > 
> > In fact, this is just churn for the sake of churn. You're changing the value
> of
> > one param, the diff includes 3 extra lines.
> 
> Done.
> 
> This is not actually churn just for the sake of churn.  Once changed to match
> clang-format, no more churn will happen and formatting becomes completely a
> non-issue.  I think the objective-c formatting could probably use some
tweaking.

Yeah, I think the ObjC formatter is less-tested/used than the C++ one.

[dewittj, 2014-04-28 21:08:10]

The CQ bit was checked by dewittj@chromium.org

[commit-bot: I haz the power, 2014-04-28 21:09:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/256883002/60001

[commit-bot: I haz the power, 2014-04-29 07:31:17]

Change committed as 266767