Added text line limits to collapsed and expanded notifications.

Added text line limits to collapsed and expanded notifications.

This is done through a new BoundedLabel views::Label subclass that
supports a max_lines value. This class comes with unit tests covering
its basic functionality.

Also, the Notifications Galore! test app was updated to include test
notifications with long titles, long messages, and long items which can
be used for manual tests of the line limits.

As with the rest of the collapse/expand functionality currently
committed, this only applies to the notification center. Notification
toasts are currently still always expanded and text line limits have not
been tested for them.

There are some known issues with incorrect notification card sizes in
the notification center after some sequence of create/destroy
operations, and these will be addressed in a separate change list.

BUG=161098, 168939, 168940
R=dewittj@chromium.org
TBR=mukai@chromium.org


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

[dharcourt, 2013-03-14 07:39:13]

At last, the limit-texts-to-2-or-7-lines functionality for notification cards in
the notification center!

PTAL when you get a chance.

Thanks!

    - Charles

[Jun Mukai, 2013-03-14 09:26:47]

https://codereview.chromium.org/12742005/diff/1/ui/message_center/message_cen...
File ui/message_center/message_center.gyp (right):

https://codereview.chromium.org/12742005/diff/1/ui/message_center/message_cen...
ui/message_center/message_center.gyp:85: '../views/run_all_unittests.cc',
I don't think that's a good idea.  Please fix views.gyp to extract this file as
a static library (like base.gyp:run_all_unittests), and add a dependency to the
new library.

https://codereview.chromium.org/12742005/diff/1/ui/message_center/views/bound...
File ui/message_center/views/bounded_label.cc (right):

https://codereview.chromium.org/12742005/diff/1/ui/message_center/views/bound...
ui/message_center/views/bounded_label.cc:139: string16 last = lines[max_lines -
1] + UTF8ToUTF16(ui::kEllipsis);
what will happen if lines[max_lines - 1] is shorter than the width, but
lines[max_lines - 1] + '...' exceeds the width?  Is that okay?

[dharcourt, 2013-03-14 20:21:57]

Jun, I've addressed your review comments, PTAL, and thanks for the review.

Scott, could you provide an OWNERS review for views.gyp? I've added a
run_all_unittests target to be a dependency of message_center.gyp's
message_center_unittests. It's similar to base.gyp's existing run_all_unittests
used by chromeos.gyp's chromeos_unittests and others. Let me know if another
approach would be preferable.

Thanks,

    - Charles

https://codereview.chromium.org/12742005/diff/1/ui/message_center/message_cen...
File ui/message_center/message_center.gyp (right):

https://codereview.chromium.org/12742005/diff/1/ui/message_center/message_cen...
ui/message_center/message_center.gyp:85: '../views/run_all_unittests.cc',
On 2013/03/14 09:26:47, Jun Mukai wrote:
> I don't think that's a good idea.  Please fix views.gyp to extract this file
as
> a static library (like base.gyp:run_all_unittests), and add a dependency to
the
> new library.

Ah, thanks for pointing that out. I didn't know how to do that and took this
shortcut. I've fixed that now by following your suggestion, thanks!

https://codereview.chromium.org/12742005/diff/1/ui/message_center/views/bound...
File ui/message_center/views/bounded_label.cc (right):

https://codereview.chromium.org/12742005/diff/1/ui/message_center/views/bound...
ui/message_center/views/bounded_label.cc:139: string16 last = lines[max_lines -
1] + UTF8ToUTF16(ui::kEllipsis);
On 2013/03/14 09:26:47, Jun Mukai wrote:
> what will happen if lines[max_lines - 1] is shorter than the width, but
> lines[max_lines - 1] + '...' exceeds the width?  Is that okay?

If lines[max_lines - 1] + '...' exceeds the width, it will get elided at line
142 below, so for example "ABC" will have become "ABC..." here and will become
"AB..." below to fit within the line width.

I've added a comment to that effect.

https://codereview.chromium.org/12742005/diff/9001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/12742005/diff/9001/ui/views/views.gyp#newcode642
ui/views/views.gyp:642: },
This target is used as a dependency of ui/message_center/message_center.gyp's
message_center_unittests target.

[sky, 2013-03-14 20:39:05]

The gyp dependency seems bizarre to me. In particular it seems like the
run_all_unittests should be in message_center, not reusing the views one. But
I'm not a sophisticated gyp guy. +mentovai

[Mark Mentovai, 2013-03-14 20:46:17]

https://codereview.chromium.org/12742005/diff/11003/ui/message_center/message...
File ui/message_center/message_center.gyp (right):

https://codereview.chromium.org/12742005/diff/11003/ui/message_center/message...
ui/message_center/message_center.gyp:88: '../views/views.gyp:run_all_unittests',
No.

This says “you need to RUN the views unit tests to BUILD the message center unit
tests.”

[Mark Mentovai, 2013-03-14 20:48:29]

https://codereview.chromium.org/12742005/diff/11003/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/12742005/diff/11003/ui/views/views.gyp#newcod...
ui/views/views.gyp:631: 'target_name': 'run_all_unittests',
Oh, this is a target here? OK, so the first thing you need to do is pick a
better name.

https://codereview.chromium.org/12742005/diff/11003/ui/views/views.gyp#newcod...
ui/views/views.gyp:632: 'type': 'static_library',
You can’t put main() into a static library.

[dharcourt, 2013-03-14 21:09:18]

On 2013/03/14 20:48:29, Mark Mentovai wrote:
> https://codereview.chromium.org/12742005/diff/11003/ui/views/views.gyp
> File ui/views/views.gyp (right):
> 
>
https://codereview.chromium.org/12742005/diff/11003/ui/views/views.gyp#newcod...
> ui/views/views.gyp:631: 'target_name': 'run_all_unittests',
> Oh, this is a target here? OK, so the first thing you need to do is pick a
> better name.
> 
>
https://codereview.chromium.org/12742005/diff/11003/ui/views/views.gyp#newcod...
> ui/views/views.gyp:632: 'type': 'static_library',
> You can’t put main() into a static library.

OK, I admit it, trying to make view's run_all_unittests into a library was not
the best idea I've ever had :-). I've reverted that and created a
run_all_unittests.cc in ui/message_center. Patch sets 2 and 3 can now safely be
ignored, and I don't need ui/views/OWNERS review any more... Thanks for nudging
me towards a better solution.

[dharcourt, 2013-03-15 14:59:46]

Hi Jun,

Could you take a look again? I think I addressed the issues you raised. I'm not
modifying views/views.gyp any more so Scott and Mark are not reviewing this any
more, it's all you!

Thanks,

    - Charles

[dharcourt, 2013-03-15 15:19:35]

Actually, I need to land this before I can update the look of old-style
notifications (the updated look needs the multi-line text elisio support which
this brings). We're trying to get that look update in ASAP, so I'm switching
reviewers to Justin who's in the same time zone as me and more likely to be able
to review this in time.

Justin, can you take a look when you get a chance?

Thanks,

    - Charles

[dewittj, 2013-03-15 17:53:30]

On 2013/03/15 15:19:35, dharcourt wrote:
> Actually, I need to land this before I can update the look of old-style
> notifications (the updated look needs the multi-line text elisio support which
> this brings). We're trying to get that look update in ASAP, so I'm switching
> reviewers to Justin who's in the same time zone as me and more likely to be
able
> to review this in time.
> 
> Justin, can you take a look when you get a chance?
> 
> Thanks,
> 
>     - Charles

What are the compositor test utils in there for?

[dharcourt, 2013-03-15 18:16:01]

On 2013/03/15 17:53:30, dewittj wrote:
> On 2013/03/15 15:19:35, dharcourt wrote:
> > Actually, I need to land this before I can update the look of old-style
> > notifications (the updated look needs the multi-line text elisio support
which
> > this brings). We're trying to get that look update in ASAP, so I'm switching
> > reviewers to Justin who's in the same time zone as me and more likely to be
> able
> > to review this in time.
> > 
> > Justin, can you take a look when you get a chance?
> > 
> > Thanks,
> > 
> >     - Charles
> 
> What are the compositor test utils in there for?

Without those the tests can't find the font they need (undefined reference to
gfx::Font::* at link time).

[dewittj, 2013-03-15 19:35:16]

lgtm

I'd love to see a comment about the font/compositortest relationship, too.

https://codereview.chromium.org/12742005/diff/21001/ui/message_center/views/b...
File ui/message_center/views/bounded_label.h (right):

https://codereview.chromium.org/12742005/diff/21001/ui/message_center/views/b...
ui/message_center/views/bounded_label.h:49: friend class BoundedLabelTest;
do you want FRIEND_TEST_ALL_PREFIXES here?

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

On 2013/03/15 19:35:16, dewittj wrote:
> lgtm
> 
> I'd love to see a comment about the font/compositortest relationship, too.
> 
>
https://codereview.chromium.org/12742005/diff/21001/ui/message_center/views/b...
> File ui/message_center/views/bounded_label.h (right):
> 
>
https://codereview.chromium.org/12742005/diff/21001/ui/message_center/views/b...
> ui/message_center/views/bounded_label.h:49: friend class BoundedLabelTest;
> do you want FRIEND_TEST_ALL_PREFIXES here?

Justin, I added the comment you suggested.

Jun, setting you as TBR for the ui/message_center OWNERS review, please take a
look when you get a chance.

[dharcourt, 2013-03-15 21:18:05]

https://codereview.chromium.org/12742005/diff/21001/ui/message_center/views/b...
File ui/message_center/views/bounded_label.h (right):

https://codereview.chromium.org/12742005/diff/21001/ui/message_center/views/b...
ui/message_center/views/bounded_label.h:49: friend class BoundedLabelTest;
On 2013/03/15 19:35:16, dewittj wrote:
> do you want FRIEND_TEST_ALL_PREFIXES here?

No, the BoundedLabelTest fixture needs friend access, not its
BoundedLabelTest##_##*##_Test subclasses. FRIEND_TEST_ALL_PREFIXES can be added
when tests are added that need friend access, but in any case the friend class
BoundedLabelTest will need to stay there for the fixture to have the access it
needs.

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

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

[commit-bot: I haz the power, 2013-03-15 21:29:44]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, chromedriver2_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, nacl_integration, net_unittests, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-03-15 23:13:55]

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

[commit-bot: I haz the power, 2013-03-16 02:56:25]

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

[commit-bot: I haz the power, 2013-03-17 11:25:56]

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

[commit-bot: I haz the power, 2013-03-17 12:56:34]

Change committed as 188637

[tfarina, 2013-03-17 14:19:12]

https://chromiumcodereview.appspot.com/12742005/diff/103001/ui/message_center...
File ui/message_center/run_all_unittests.cc (right):

https://chromiumcodereview.appspot.com/12742005/diff/103001/ui/message_center...
ui/message_center/run_all_unittests.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
no (c)

https://chromiumcodereview.appspot.com/12742005/diff/103001/ui/message_center...
ui/message_center/run_all_unittests.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
would have been better to put this into a test directory:
ui/message_center/test/run_all_unittests.cc

This is the pattern followed by some other components already.

[dharcourt, 2013-03-18 07:01:51]

https://chromiumcodereview.appspot.com/12742005/diff/103001/ui/message_center...
File ui/message_center/run_all_unittests.cc (right):

https://chromiumcodereview.appspot.com/12742005/diff/103001/ui/message_center...
ui/message_center/run_all_unittests.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/03/17 14:19:12, tfarina wrote:
> no (c)

Thanks for the suggestions. Done & done, but in a separate change list
(http://crrev.com/12880004), because this one has sailed :-)... and been
reverted :-(...