Fix VK animation related issues

Fix VK animation related issues

This CL did the following things:
1. changes targeting opacity from 0 to 0.2 during hide animation
2. fix "regain focus before hide animation finish, VK doesn't show up" issue

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

[bshe, 2014-01-13 22:12:24]

Hi Kevin.

Do you mind to take a look at this CL?

Thanks!

https://codereview.chromium.org/134133003/diff/40001/ui/base/ime/input_method...
File ui/base/ime/input_method_factory.cc (right):

https://codereview.chromium.org/134133003/diff/40001/ui/base/ime/input_method...
ui/base/ime/input_method_factory.cc:60: void SetUpInputMethodFactoryForTesting()
{
My tests suffer the same problem as described in: crbug.com/331690. This fix is
borrowed from
(https://codereview.chromium.org/29943002/diff/980001/ui/base/ime/input_method...)
to keep my tests pass.

[kevers, 2014-01-14 19:08:43]

https://codereview.chromium.org/134133003/diff/40001/chrome/browser/ui/ash/as...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy_unittest.cc (right):

https://codereview.chromium.org/134133003/diff/40001/chrome/browser/ui/ash/as...
chrome/browser/ui/ash/ash_keyboard_controller_proxy_unittest.cc:137:
EXPECT_EQ(kAnimationStartOrAfterHideOpacity, layer->opacity());
Equality test on a floating point value seems susceptible to flakiness.

[bshe, 2014-01-15 19:28:13]

+jamescook for owners

https://codereview.chromium.org/134133003/diff/40001/chrome/browser/ui/ash/as...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy_unittest.cc (right):

https://codereview.chromium.org/134133003/diff/40001/chrome/browser/ui/ash/as...
chrome/browser/ui/ash/ash_keyboard_controller_proxy_unittest.cc:137:
EXPECT_EQ(kAnimationStartOrAfterHideOpacity, layer->opacity());
talked offline. Please see my latest patch.

On 2014/01/14 19:08:43, kevers wrote:
> Equality test on a floating point value seems susceptible to flakiness.

[James Cook, 2014-01-15 20:49:57]

https://codereview.chromium.org/134133003/diff/100001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/134133003/diff/100001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:229:
GetKeyboardWindow()->Hide();
I'm having a hard time following this and the Hide() call above. Is it really
necessary to have both the keyboard container and the keyboard window being
individually shown and hidden at different times?  Does it have to be this way,
or is there some way to have a single visibility switch?

[kevers, 2014-01-15 20:50:58]

lgtm

[bshe, 2014-01-16 17:47:58]

Thanks!

https://codereview.chromium.org/134133003/diff/100001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/134133003/diff/100001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:229:
GetKeyboardWindow()->Hide();
Looks like it is possible if I add a check condition in the keyboard_controller.
Please see the latest patch.

On 2014/01/15 20:49:57, James Cook wrote:
> I'm having a hard time following this and the Hide() call above. Is it really
> necessary to have both the keyboard container and the keyboard window being
> individually shown and hidden at different times?  Does it have to be this
way,
> or is there some way to have a single visibility switch?

[James Cook, 2014-01-16 17:55:03]

LGTM assuming we're not leaking observers.

https://codereview.chromium.org/134133003/diff/230001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/134133003/diff/230001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:221:
animator->RemoveObserver(this);
Is it possible to leak this observer?  I'm not clear on what happens if
OnLayerAnimationEnded() is called but this if() branch isn't taken.

https://codereview.chromium.org/134133003/diff/230001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy_unittest.cc (right):

https://codereview.chromium.org/134133003/diff/230001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_keyboard_controller_proxy_unittest.cc:161: // Test for
crbug.com/333284.
It's great that you wrote a test specifically for this bug.

[bshe, 2014-01-16 20:09:03]

Thanks!

https://codereview.chromium.org/134133003/diff/230001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/134133003/diff/230001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:221:
animator->RemoveObserver(this);
We have a few animations at the same time. So OnLayerAnimationEnded may called
multiple times. And we only want to remove the observer if the last animation
finished, hence the if() branch. If the remaining animation is somehow aborted,
the observer should be removed in the OnLayerAnimationAborted below. So it
should be safe.

On 2014/01/16 17:55:03, James Cook wrote:
> Is it possible to leak this observer?  I'm not clear on what happens if
> OnLayerAnimationEnded() is called but this if() branch isn't taken.

[James Cook, 2014-01-16 20:19:10]

lgtm

[commit-bot: I haz the power, 2014-01-16 20:21:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/134133003/290001

[commit-bot: I haz the power, 2014-01-16 21:48:55]

Change committed as 245340