combobox: don't animate state changes in TransparentButton

combobox: don't animate state changes in TransparentButton

TransparentButton has state-change animations enabled despite being invisible,
which causes a bunch of spurious repaints of the combobox on mouse enter/exit.

BUG=645266
Committed: https://crrev.com/9469d57224252e71f21394f17a30929ec3fd102e
Cr-Commit-Position: refs/heads/master@{#418824}

[Elly Fong-Jones, 2016-09-09 14:42:02]

Description was changed from

==========
combobox: don't animate state changes in TransparentButton

TransparentButton has state-change animations enabled despite being invisible,
which causes a bunch of spurious repaints of the combobox on mouse enter/exit.

BUG=645266
==========

to

==========
combobox: don't animate state changes in TransparentButton

TransparentButton has state-change animations enabled despite being invisible,
which causes a bunch of spurious repaints of the combobox on mouse enter/exit.

BUG=645266
==========

[Elly Fong-Jones, 2016-09-09 14:42:02]

ellyjones@chromium.org changed reviewers:
+ estade@chromium.org

[Elly Fong-Jones, 2016-09-09 14:42:08]

estade: ptal? :)

[Evan Stade, 2016-09-09 18:37:13]

good find. I wonder if this is why the ripples that were added recently for
harmony can look janky when clicking on the combobox?

https://codereview.chromium.org/2327673003/diff/1/ui/views/controls/combobox/...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/2327673003/diff/1/ui/views/controls/combobox/...
ui/views/controls/combobox/combobox.cc:119:
SetAnimationDuration(LabelButton::kHoverAnimationDurationMs);
why is this here?

[Elly Fong-Jones, 2016-09-12 11:46:40]

https://codereview.chromium.org/2327673003/diff/1/ui/views/controls/combobox/...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/2327673003/diff/1/ui/views/controls/combobox/...
ui/views/controls/combobox/combobox.cc:119:
SetAnimationDuration(LabelButton::kHoverAnimationDurationMs);
On 2016/09/09 18:37:13, Evan Stade wrote:
> why is this here?

Your guess is as good as mine. It was added in the implementation of "action
style" comboboxes in https://codereview.chromium.org/59383003 and it seems like
this specific line was copied verbatim from LabelButton's constructor. I would
rather like to remove this entire class.

[Evan Stade, 2016-09-12 14:51:17]

https://codereview.chromium.org/2327673003/diff/1/ui/views/controls/combobox/...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/2327673003/diff/1/ui/views/controls/combobox/...
ui/views/controls/combobox/combobox.cc:119:
SetAnimationDuration(LabelButton::kHoverAnimationDurationMs);
On 2016/09/12 11:46:40, Elly Jones wrote:
> On 2016/09/09 18:37:13, Evan Stade wrote:
> > why is this here?
> 
> Your guess is as good as mine. It was added in the implementation of "action
> style" comboboxes in https://codereview.chromium.org/59383003 and it seems
like
> this specific line was copied verbatim from LabelButton's constructor. I would
> rather like to remove this entire class.

It seems the line you're adding defeats the purpose of SetAnimationDuration. I
think you're breaking PaintButtons below (only used for action style comboboxes,
but one of those still exists for now). You should probably either set the
animation duration or set animate false depending on the style of combobox.

[Elly Fong-Jones, 2016-09-12 17:41:27]

On 2016/09/12 14:51:17, Evan Stade wrote:
>
https://codereview.chromium.org/2327673003/diff/1/ui/views/controls/combobox/...
> File ui/views/controls/combobox/combobox.cc (right):
> 
>
https://codereview.chromium.org/2327673003/diff/1/ui/views/controls/combobox/...
> ui/views/controls/combobox/combobox.cc:119:
> SetAnimationDuration(LabelButton::kHoverAnimationDurationMs);
> On 2016/09/12 11:46:40, Elly Jones wrote:
> > On 2016/09/09 18:37:13, Evan Stade wrote:
> > > why is this here?
> > 
> > Your guess is as good as mine. It was added in the implementation of "action
> > style" comboboxes in https://codereview.chromium.org/59383003 and it seems
> like
> > this specific line was copied verbatim from LabelButton's constructor. I
would
> > rather like to remove this entire class.
> 
> It seems the line you're adding defeats the purpose of SetAnimationDuration. I
> think you're breaking PaintButtons below (only used for action style
comboboxes,
> but one of those still exists for now). You should probably either set the
> animation duration or set animate false depending on the style of combobox.

Hm, that is a good point. There, I added a constructor argument for whether to
do animations or not.

I can't wait to get rid of STYLE_ACTION, heh...

[Evan Stade, 2016-09-13 22:03:17]

lgtm

https://codereview.chromium.org/2327673003/diff/20001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/2327673003/diff/20001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:119:
SetAnimationDuration(LabelButton::kHoverAnimationDurationMs);
could you make this conditional on the new parameter as well, if only to make it
harder to miss the fact that we can remove it when style_action is no more?

[Elly Fong-Jones, 2016-09-14 11:00:51]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2016-09-14 11:00:52]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2327673003/#ps40001
(title: "conditional SetAnimationDuration")

[commit-bot: I haz the power, 2016-09-14 11:01:18]

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, 2016-09-14 11:08:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 11:08:47]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Elly Fong-Jones, 2016-09-14 14:50:51]

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

[Elly Fong-Jones, 2016-09-14 14:50:59]

sky: ptal? :)

[sky, 2016-09-14 17:47:20]

LGTM

[Elly Fong-Jones, 2016-09-15 10:15:56]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2016-09-15 10:16:16]

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, 2016-09-15 10:20:51]

Description was changed from

==========
combobox: don't animate state changes in TransparentButton

TransparentButton has state-change animations enabled despite being invisible,
which causes a bunch of spurious repaints of the combobox on mouse enter/exit.

BUG=645266
==========

to

==========
combobox: don't animate state changes in TransparentButton

TransparentButton has state-change animations enabled despite being invisible,
which causes a bunch of spurious repaints of the combobox on mouse enter/exit.

BUG=645266
==========

[commit-bot: I haz the power, 2016-09-15 10:20:52]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-15 10:22:50]

Description was changed from

==========
combobox: don't animate state changes in TransparentButton

TransparentButton has state-change animations enabled despite being invisible,
which causes a bunch of spurious repaints of the combobox on mouse enter/exit.

BUG=645266
==========

to

==========
combobox: don't animate state changes in TransparentButton

TransparentButton has state-change animations enabled despite being invisible,
which causes a bunch of spurious repaints of the combobox on mouse enter/exit.

BUG=645266

Committed: https://crrev.com/9469d57224252e71f21394f17a30929ec3fd102e
Cr-Commit-Position: refs/heads/master@{#418824}
==========

[commit-bot: I haz the power, 2016-09-15 10:22:51]

Patchset 3 (id:??) landed as
https://crrev.com/9469d57224252e71f21394f17a30929ec3fd102e
Cr-Commit-Position: refs/heads/master@{#418824}