Stop notifications from being clipped on resize.

Stop notifications from being clipped on resize.

If you hover over a notification, they are specially held in place so if
other notifications disappear or change size, you don't accidentally
click on a different one. But, if notifications at or below the
notification you are hovering over get bigger, then, in order to keep
the hovered notification in the same place, they would be pushed out the
bottom of the notification list view and be clipped. Instead, increase the
size of the list view and let the hovered notification move up.

BUG=

Review-Url: https://codereview.chromium.org/2696523002
Cr-Commit-Position: refs/heads/master@{#451547}
Committed: https://chromium.googlesource.com/chromium/src/+/9cfb02e626c5e82aa1f2ac8528c50778719bd8c3

[Eliot Courtney, 2017-02-13 08:49:13]

edcourtney@chromium.org changed reviewers:
+ xiyuan@chromium.org, yoshiki@chromium.org

[Eliot Courtney, 2017-02-13 09:29:41]

Does anyone know why in the call existing call to AnimateChild,
|animate_on_move| is only set to true for notifications above the target? For
now I've preserved the original behaviour.

Also, the other function, AnimateNotificationsBelowTarget, was for an experiment
added in 2014 (see https://codereview.chromium.org/493303002 ), to fix the
notification being obstructed problem. Maybe this can be removed now?

Thanks~!

[yoshiki, 2017-02-13 18:19:04]

On 2017/02/13 09:29:41, Eliot Courtney wrote:
> Does anyone know why in the call existing call to AnimateChild,
> |animate_on_move| is only set to true for notifications above the target? For
> now I've preserved the original behaviour.

IIRC, this is a method to keep the position of the items below the deleted item,
and move the items above with down-animation. We want to implement such an
animation.

> Also, the other function, AnimateNotificationsBelowTarget, was for an
experiment
> added in 2014 (see https://codereview.chromium.org/493303002 ), to fix the
> notification being obstructed problem. Maybe this can be removed now?

I think it's ok for me to remove. But Defer to Justin. 

> Thanks~!

[yoshiki, 2017-02-13 18:19:18]

yoshiki@chromium.org changed reviewers:
+ dewittj@chromium.org

[yoshiki, 2017-02-13 18:19:19]

+dewittj

[Eliot Courtney, 2017-02-14 02:29:06]

On 2017/02/13 18:19:04, yoshiki wrote:
> On 2017/02/13 09:29:41, Eliot Courtney wrote:
> > Does anyone know why in the call existing call to AnimateChild,
> > |animate_on_move| is only set to true for notifications above the target?
For
> > now I've preserved the original behaviour.
> 
> IIRC, this is a method to keep the position of the items below the deleted
item,
> and move the items above with down-animation. We want to implement such an
> animation.
I think the position of the notifications below the target can still move (e.g.
when a notification is deleted or shrinks), so maybe it should also be animated?

Thanks~~

[yoshiki, 2017-02-16 03:46:47]

On 2017/02/14 02:29:06, Eliot Courtney wrote:
> On 2017/02/13 18:19:04, yoshiki wrote:
> > On 2017/02/13 09:29:41, Eliot Courtney wrote:
> > > Does anyone know why in the call existing call to AnimateChild,
> > > |animate_on_move| is only set to true for notifications above the target?
> For
> > > now I've preserved the original behaviour.
> > 
> > IIRC, this is a method to keep the position of the items below the deleted
> item,
> > and move the items above with down-animation. We want to implement such an
> > animation.
> I think the position of the notifications below the target can still move
(e.g.
> when a notification is deleted or shrinks), so maybe it should also be
animated?

Yes, I think we can add animation now. When this code is written, any
notification doesn't shrink and only one notification is removed at the same
time..

[Eliot Courtney, 2017-02-16 04:30:23]

Okay, I made it animate all the time. Removing AnimateNotificationsBelowTarget
is for another CL anyway so I'll leave it for now.

PTAL - fyi ComputeRepositionOffsets has basically no changes, except for the set
fixed_height_ at the end.

[xiyuan, 2017-02-16 16:57:46]

lgtm but please wait for yoshiki@ or dewittj@.

https://codereview.chromium.org/2696523002/diff/40001/ui/message_center/views...
File ui/message_center/views/message_list_view.cc (right):

https://codereview.chromium.org/2696523002/diff/40001/ui/message_center/views...
ui/message_center/views/message_list_view.cc:371: std::vector<int> positions;
nit: pre-allocate space
  positions.reserve(heights.size());

https://codereview.chromium.org/2696523002/diff/40001/ui/message_center/views...
ui/message_center/views/message_list_view.cc:424: std::vector<bool> deleting;
nit: pre-allocate the space
  heights.reserve(child_count());
  deleting.reserve(child_count());

https://codereview.chromium.org/2696523002/diff/40001/ui/message_center/views...
File ui/message_center/views/message_list_view.h (right):

https://codereview.chromium.org/2696523002/diff/40001/ui/message_center/views...
ui/message_center/views/message_list_view.h:83: std::vector<int>
ComputeRepositionOffsets(const std::vector<int>& heights,
nit: #include <vector> ?

[dewittj, 2017-02-16 18:15:13]

this lg in principle so far

https://codereview.chromium.org/2696523002/diff/40001/ui/message_center/views...
File ui/message_center/views/message_list_view_unittest.cc (right):

https://codereview.chromium.org/2696523002/diff/40001/ui/message_center/views...
ui/message_center/views/message_list_view_unittest.cc:134: std::vector<int>
ComputeRepositionOffsets(const std::vector<int>& heights,
It's good that you can cover this function directly, but is there any chance you
could also test the end results in terms of the view hierarchy/dimensions
themselves too?

[yoshiki, 2017-02-17 02:03:37]

lgtm

[Eliot Courtney, 2017-02-17 07:47:39]

The CQ bit was checked by edcourtney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 07:47:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eliot Courtney, 2017-02-17 07:49:49]

Added additional tests. PTAL.

https://codereview.chromium.org/2696523002/diff/40001/ui/message_center/views...
File ui/message_center/views/message_list_view.cc (right):

https://codereview.chromium.org/2696523002/diff/40001/ui/message_center/views...
ui/message_center/views/message_list_view.cc:371: std::vector<int> positions;
On 2017/02/16 16:57:46, xiyuan wrote:
> nit: pre-allocate space
>   positions.reserve(heights.size());

Done.

https://codereview.chromium.org/2696523002/diff/40001/ui/message_center/views...
ui/message_center/views/message_list_view.cc:424: std::vector<bool> deleting;
On 2017/02/16 16:57:46, xiyuan wrote:
> nit: pre-allocate the space
>   heights.reserve(child_count());
>   deleting.reserve(child_count());

Done.

https://codereview.chromium.org/2696523002/diff/40001/ui/message_center/views...
File ui/message_center/views/message_list_view.h (right):

https://codereview.chromium.org/2696523002/diff/40001/ui/message_center/views...
ui/message_center/views/message_list_view.h:83: std::vector<int>
ComputeRepositionOffsets(const std::vector<int>& heights,
On 2017/02/16 16:57:46, xiyuan wrote:
> nit: #include <vector> ?

Done.

https://codereview.chromium.org/2696523002/diff/40001/ui/message_center/views...
File ui/message_center/views/message_list_view_unittest.cc (right):

https://codereview.chromium.org/2696523002/diff/40001/ui/message_center/views...
ui/message_center/views/message_list_view_unittest.cc:134: std::vector<int>
ComputeRepositionOffsets(const std::vector<int>& heights,
On 2017/02/16 18:15:13, dewittj wrote:
> It's good that you can cover this function directly, but is there any chance
you
> could also test the end results in terms of the view hierarchy/dimensions
> themselves too?

Looks like those type of tests live in MessageCenterViewTest. I added two tests
resizing the target notification, and one below it, confirming the sizes are
correct. I had to change MessageCenterView a bit, since it directly queried the
mouse state, which seemed hard to mock.

Done

[dewittj, 2017-02-17 16:16:54]

lgtm, thanks!

[Eliot Courtney, 2017-02-20 01:27:32]

The CQ bit was checked by edcourtney@chromium.org

[Eliot Courtney, 2017-02-20 01:27:33]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
yoshiki@chromium.org, dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/2696523002/#ps100001
(title: "Add additional comments.")

[commit-bot: I haz the power, 2017-02-20 01:27:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-20 02:36:31]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487554052394770,
"parent_rev": "c65ab79d3ddc7ffe1449b3ce25d079a3bcb6a181", "commit_rev":
"9cfb02e626c5e82aa1f2ac8528c50778719bd8c3"}

[commit-bot: I haz the power, 2017-02-20 02:37:06]

Description was changed from

==========
Stop notifications from being clipped on resize.

If you hover over a notification, they are specially held in place so if
other notifications disappear or change size, you don't accidentally
click on a different one. But, if notifications at or below the
notification you are hovering over get bigger, then, in order to keep
the hovered notification in the same place, they would be pushed out the
bottom of the notification list view and be clipped. Instead, increase the
size of the list view and let the hovered notification move up.

BUG=
==========

to

==========
Stop notifications from being clipped on resize.

If you hover over a notification, they are specially held in place so if
other notifications disappear or change size, you don't accidentally
click on a different one. But, if notifications at or below the
notification you are hovering over get bigger, then, in order to keep
the hovered notification in the same place, they would be pushed out the
bottom of the notification list view and be clipped. Instead, increase the
size of the list view and let the hovered notification move up.

BUG=

Review-Url: https://codereview.chromium.org/2696523002
Cr-Commit-Position: refs/heads/master@{#451547}
Committed:
https://chromium.googlesource.com/chromium/src/+/9cfb02e626c5e82aa1f2ac8528c5...
==========

[commit-bot: I haz the power, 2017-02-20 02:37:07]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9cfb02e626c5e82aa1f2ac8528c5...