Overscroll for keyboard disabled while keyboard is hiding.

Overscroll for keyboard disabled while keyboard is hiding.

While the keyboard is in the process of hiding, the proxy's visibility
flag may not be updated yet. Use the private keyboard_visible_ flag
instead as it stores the future visibility of the keyboard window after
animation is completed.

BUG=401670
TEST=Updated:
        KeyboardControllerTest.CheckOverscrollInsetDuringVisibilityChange
     On device:
         As the inset update must be initiated after the animation is
         started and before it is finished. Tested on clapper with the
         same method as in the bug, i.e. switch tab after hiding
         keyboard to trigger fast inset update.

Committed: https://crrev.com/7975dd824d83890224e342a8e401a7566e68a3eb
Cr-Commit-Position: refs/heads/master@{#295530}

[Peter Wen, 2014-09-16 21:51:57]

wnwen@chromium.org changed reviewers:
+ kevers@chromium.org

[kevers, 2014-09-16 21:56:40]

lgtm

[Peter Wen, 2014-09-17 11:42:50]

wnwen@chromium.org changed reviewers:
+ bshe@chromium.org

[Peter Wen, 2014-09-17 11:42:50]

Hi Biao, please take a look at this fix for overscroll.

This occurs when:
1. Keyboard is scheduled to be hidden.
2. HideKeyboard is called and resets all insets to 0.
3. HideKeyboard completes and schedules animation.
4. A new tab is opened, so UpdateInsets is called, but since animation isn't
finished yet, proxy's IsVisible is true, and insets on the new page are created
for the VK.
5. Animation completes and proxy hides the keyboard, thus we are left with the
white inset and no keyboard.

This fix makes sure that insets are only updated if keyboard is not going to
immediately hide.

Thanks,
Peter

[bshe, 2014-09-17 13:49:19]

On 2014/09/17 11:42:50, Peter Wen wrote:
> Hi Biao, please take a look at this fix for overscroll.
> 
> This occurs when:
> 1. Keyboard is scheduled to be hidden.
> 2. HideKeyboard is called and resets all insets to 0.
> 3. HideKeyboard completes and schedules animation.
> 4. A new tab is opened, so UpdateInsets is called, but since animation isn't
> finished yet, proxy's IsVisible is true, and insets on the new page are
created
> for the VK.
> 5. Animation completes and proxy hides the keyboard, thus we are left with the
> white inset and no keyboard.
> 
> This fix makes sure that insets are only updated if keyboard is not going to
> immediately hide.
> 
> Thanks,
> Peter

lgtm do you mind to add a unit test for this case?

[Peter Wen, 2014-09-17 18:41:40]

Added unittest.

[Peter Wen, 2014-09-18 13:08:32]

Hi Biao, would you mind checking that the test and refactor is okay?

Thanks,
Peter

[bshe, 2014-09-18 13:59:07]

On 2014/09/18 13:08:32, Peter Wen wrote:
> Hi Biao, would you mind checking that the test and refactor is okay?
> 
> Thanks,
> Peter

Sorry for the late reply. The test looks good. The only suggestion that I have
is to move the
test code to a new test. As you are testing overscroll behavior not visibility
change behavior.
It would be cleaner.

[Peter Wen, 2014-09-18 16:51:18]

PTAL. :)

[bshe, 2014-09-18 18:26:07]

lgtm

thanks for adding the test!

https://codereview.chromium.org/578653004/diff/60001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller_unittest.cc (right):

https://codereview.chromium.org/578653004/diff/60001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller_unittest.cc:427: 
nit: Do you mind to add a link to the bug that you are testing? It would be
easier to understand the test in the future.

[Peter Wen, 2014-09-18 18:41:47]

https://codereview.chromium.org/578653004/diff/60001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller_unittest.cc (right):

https://codereview.chromium.org/578653004/diff/60001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller_unittest.cc:427: 
On 2014/09/18 18:26:07, bshe wrote:
> nit: Do you mind to add a link to the bug that you are testing? It would be
> easier to understand the test in the future.

Done.

[Peter Wen, 2014-09-18 18:42:03]

The CQ bit was checked by wnwen@chromium.org

[commit-bot: I haz the power, 2014-09-18 18:43:30]

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

[commit-bot: I haz the power, 2014-09-18 19:40:59]

Committed patchset #5 (id:80001) as 2d33dd73ec7ecb55b602c7397cfc4930f5a30bcb

[commit-bot: I haz the power, 2014-09-18 19:41:36]

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