MacViews: Put wm window animation calls behind an interface.

MacViews: Put wm window animation calls behind an interface.

These calls are not available on MacViews without Aura, so put them behind an
interface through views::Widget.

BUG=425229
Committed: https://crrev.com/8c848c3f4b03bceab3e847f833afd0d6bd9dd4f5
Cr-Commit-Position: refs/heads/master@{#302368}

[Andre, 2014-10-30 21:05:45]

Patchset #2 (id:20001) has been deleted

[Andre, 2014-10-30 21:47:51]

Patchset #2 (id:40001) has been deleted

[Andre, 2014-10-30 21:48:15]

Patchset #2 (id:60001) has been deleted

[Andre, 2014-10-30 22:19:10]

Patchset #2 (id:80001) has been deleted

[Andre, 2014-10-30 22:19:26]

Patchset #1 (id:1) has been deleted

[Andre, 2014-10-30 22:30:49]

andresantoso@chromium.org changed reviewers:
+ sky@chromium.org

[Andre, 2014-10-30 22:30:50]

sky@ PTAL.
I think reads better compared to adding something like
SetVisibilityAnimationTransition and SetVisibilityAnimationDuration to Widget.
Plus I suspect we may need to add more visibility animation settings like these
in the future.

[sky, 2014-10-30 22:54:43]

https://codereview.chromium.org/693523003/diff/120001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/693523003/diff/120001/ui/views/widget/widget....
ui/views/widget/widget.h:447: VisibilityAnimation* GetVisibilityAnimation()
const;
I would rather have a single function that falls through to the nativewidget for
configuring this, eg SetVisibilityAnimationProperties(transition, duration).

[Andre, 2014-10-30 23:24:46]

On 2014/10/30 22:54:43, sky wrote:
> https://codereview.chromium.org/693523003/diff/120001/ui/views/widget/widget.h
> File ui/views/widget/widget.h (right):
> 
>
https://codereview.chromium.org/693523003/diff/120001/ui/views/widget/widget....
> ui/views/widget/widget.h:447: VisibilityAnimation* GetVisibilityAnimation()
> const;
> I would rather have a single function that falls through to the nativewidget
for
> configuring this, eg SetVisibilityAnimationProperties(transition, duration).

You don't think we will need to add more than duration and transition?
For example, I see extension_popup.cc calls SetWindowVisibilityAnimationType and
SetWindowVisibilityAnimationVerticalPosition.

[Andre, 2014-10-30 23:36:49]

On 2014/10/30 23:24:46, Andre wrote:
> On 2014/10/30 22:54:43, sky wrote:
> >
https://codereview.chromium.org/693523003/diff/120001/ui/views/widget/widget.h
> > File ui/views/widget/widget.h (right):
> > 
> >
>
https://codereview.chromium.org/693523003/diff/120001/ui/views/widget/widget....
> > ui/views/widget/widget.h:447: VisibilityAnimation* GetVisibilityAnimation()
> > const;
> > I would rather have a single function that falls through to the nativewidget
> for
> > configuring this, eg SetVisibilityAnimationProperties(transition, duration).
> 
> You don't think we will need to add more than duration and transition?
> For example, I see extension_popup.cc calls SetWindowVisibilityAnimationType
and
> SetWindowVisibilityAnimationVerticalPosition.

Also, most call sites only want to modify one property while leaving the other
property unchanged.

[sky, 2014-10-31 16:21:21]

On Thu, Oct 30, 2014 at 4:36 PM,  <andresantoso@chromium.org> wrote:
> On 2014/10/30 23:24:46, Andre wrote:
>>
>> On 2014/10/30 22:54:43, sky wrote:
>> >
>
>
https://codereview.chromium.org/693523003/diff/120001/ui/views/widget/widget.h
>>
>> > File ui/views/widget/widget.h (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/693523003/diff/120001/ui/views/widget/widget....
>>
>> > ui/views/widget/widget.h:447: VisibilityAnimation*
>> > GetVisibilityAnimation()
>> > const;
>> > I would rather have a single function that falls through to the
>> > nativewidget
>> for
>> > configuring this, eg SetVisibilityAnimationProperties(transition,
>> > duration).
>
>
>> You don't think we will need to add more than duration and transition?
>> For example, I see extension_popup.cc calls
>> SetWindowVisibilityAnimationType
>
> and
>>
>> SetWindowVisibilityAnimationVerticalPosition.
>
>
> Also, most call sites only want to modify one property while leaving the
> other
> property unchanged.

The interface feels heavyweight to me. All the consumers wants is to
configure a handful of properties. What about a struct with is a bunch
(well, two) of scoped_ptrs. A null value means use the default.

  -Scott
>
>
> https://codereview.chromium.org/693523003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Andre, 2014-10-31 21:15:26]

WDYT about making 2 separate functions (patch #3).
That way call-sites can remain as one-liners.

[sky, 2014-10-31 22:58:43]

My likey. LGTM

https://codereview.chromium.org/693523003/diff/140001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/693523003/diff/140001/ui/views/widget/widget....
ui/views/widget/widget.h:431: // Default behavior is to animate both show and
hide.
nit: move enum to top of section (like style guide says).

[Andre, 2014-10-31 23:05:10]

https://codereview.chromium.org/693523003/diff/140001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/693523003/diff/140001/ui/views/widget/widget....
ui/views/widget/widget.h:431: // Default behavior is to animate both show and
hide.
On 2014/10/31 22:58:43, sky wrote:
> nit: move enum to top of section (like style guide says).

Done.

[Andre, 2014-10-31 23:09:48]

Patchset #4 (id:160001) has been deleted

[Andre, 2014-10-31 23:11:19]

Fix for sky

[Andre, 2014-10-31 23:35:00]

Patchset #5 (id:200001) has been deleted

[Andre, 2014-11-01 00:07:17]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-11-01 00:08:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/693523003/220001

[commit-bot: I haz the power, 2014-11-01 00:50:33]

Committed patchset #5 (id:220001)

[commit-bot: I haz the power, 2014-11-01 00:51:47]

Patchset 5 (id:??) landed as
https://crrev.com/8c848c3f4b03bceab3e847f833afd0d6bd9dd4f5
Cr-Commit-Position: refs/heads/master@{#302368}