Remove native VK window height logic and wait for resizeTo to setup VK window height

Currently, we try to set virtual keyboard window height both
in native code and in VK's javascript code through window.resizeTo
It sometimes creates conflict and cause strange animation issues.

In this CL, the window height logic in native side is removed.
The default window height is set to 0 and we expect javascript
will set the window to correct height.
If window is resized from a non zero height to another non
zeor height, we current dont do any animation.

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

[bshe, 2014-04-22 16:20:02]

Hi Kevin and Shu.

Per discussion earlier today, this is the CL that I came up with. I will update
the keyboard tests accordingly. But in the same time, could you guys take a look
to see if this is a reasonable first attempt.

Thanks!

[kevers, 2014-04-22 17:45:23]

https://codereview.chromium.org/240443006/diff/40001/ui/keyboard/keyboard_uti...
File ui/keyboard/keyboard_util.cc (right):

https://codereview.chromium.org/240443006/diff/40001/ui/keyboard/keyboard_uti...
ui/keyboard/keyboard_util.cc:49: const float kInitialHeightRatio = 0.0f;
Please add a comment.  With the value now being 0 or 1, depending on the
usability experiment flag, we can simply update
DefaultKeyboardBoundsFromWindowBounds, eliminating the need for
GetKeyboardHeightRatio.

[Shu Chen, 2014-04-22 17:54:57]

https://codereview.chromium.org/240443006/diff/40001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/240443006/diff/40001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:280: OnShowImeIfNeeded();
From the current call paths, I think it would be better to merge most of the
code from OnShowImeIfNeeded() to ShowKeyboardInternal() and the call paths would
be:
  ShowKeyboard() -> ShowKeyboardInternal()
  OnShowImeIfNeeded() -> ShowKeyboardInternal()

https://codereview.chromium.org/240443006/diff/40001/ui/keyboard/keyboard_lay...
File ui/keyboard/keyboard_layout_manager.cc (right):

https://codereview.chromium.org/240443006/diff/40001/ui/keyboard/keyboard_lay...
ui/keyboard/keyboard_layout_manager.cc:50: bool forceShow =
child->bounds().height() == 0;
The naming "forceShow" causes confusion.

If forceShow == true (a.k.a height == 0), controller_->ShowKeyboard(..) won't
actually make keyboard visible.

[bshe, 2014-04-23 01:08:52]

Thanks for review. I have addressed all the comments. The new patch also include
some fix for tests as well as fix for system keyboard.

PTAL! Thanks

https://codereview.chromium.org/240443006/diff/40001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/240443006/diff/40001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:280: OnShowImeIfNeeded();
On 2014/04/22 17:54:57, Shu Chen wrote:
> From the current call paths, I think it would be better to merge most of the
> code from OnShowImeIfNeeded() to ShowKeyboardInternal() and the call paths
would
> be:
>   ShowKeyboard() -> ShowKeyboardInternal()
>   OnShowImeIfNeeded() -> ShowKeyboardInternal()

Done.

https://codereview.chromium.org/240443006/diff/40001/ui/keyboard/keyboard_lay...
File ui/keyboard/keyboard_layout_manager.cc (right):

https://codereview.chromium.org/240443006/diff/40001/ui/keyboard/keyboard_lay...
ui/keyboard/keyboard_layout_manager.cc:50: bool forceShow =
child->bounds().height() == 0;
I have changed the code to make it more readable. The bounds that we get here is
actually the old bounds. When ShowKeyboard is called, the new bounds should
already set by SetChildBoundsDirect.

On 2014/04/22 17:54:57, Shu Chen wrote:
> The naming "forceShow" causes confusion.
> 
> If forceShow == true (a.k.a height == 0), controller_->ShowKeyboard(..) won't
> actually make keyboard visible.

https://codereview.chromium.org/240443006/diff/40001/ui/keyboard/keyboard_uti...
File ui/keyboard/keyboard_util.cc (right):

https://codereview.chromium.org/240443006/diff/40001/ui/keyboard/keyboard_uti...
ui/keyboard/keyboard_util.cc:49: const float kInitialHeightRatio = 0.0f;
On 2014/04/22 17:45:23, kevers wrote:
> Please add a comment.  With the value now being 0 or 1, depending on the
> usability experiment flag, we can simply update
> DefaultKeyboardBoundsFromWindowBounds, eliminating the need for
> GetKeyboardHeightRatio.

Done.

[kevers, 2014-04-23 01:27:13]

lgtm

[Shu Chen, 2014-04-23 01:40:14]

lgtm

[bshe, 2014-04-23 14:09:45]

+missing owners

oshima@ for: ash/root_window_controller_unittest.cc

sadrul@ for : ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc

pkotwicz@ for:	ash/wm/system_modal_container_layout_manager_unittest.cc

[pkotwicz, 2014-04-23 15:18:41]

LGTM with nits

https://codereview.chromium.org/240443006/diff/80002/ash/wm/system_modal_cont...
File ash/wm/system_modal_container_layout_manager_unittest.cc (right):

https://codereview.chromium.org/240443006/diff/80002/ash/wm/system_modal_cont...
ash/wm/system_modal_container_layout_manager_unittest.cc:190:
ui::ScopedAnimationDurationScaleMode::NORMAL_DURATION);
[Optional Nit]: You may be able to remove the need for enabling animations here.
Moving proxy_->ShowKeyboardContainer() in
KeyboardController::ShowKeyboardInternal() to the end of the function seems to
do the trick.

https://codereview.chromium.org/240443006/diff/80002/ash/wm/system_modal_cont...
ash/wm/system_modal_container_layout_manager_unittest.cc:197: window_bounds.y()
+ window_bounds.height() - 100,
Nit: You can use window_bounds.bottom() here

[bshe, 2014-04-23 16:07:12]

Done. Thanks for review.

https://codereview.chromium.org/240443006/diff/80002/ash/wm/system_modal_cont...
File ash/wm/system_modal_container_layout_manager_unittest.cc (right):

https://codereview.chromium.org/240443006/diff/80002/ash/wm/system_modal_cont...
ash/wm/system_modal_container_layout_manager_unittest.cc:190:
ui::ScopedAnimationDurationScaleMode::NORMAL_DURATION);
On 2014/04/23 15:18:42, pkotwicz wrote:
> [Optional Nit]: You may be able to remove the need for enabling animations
here.
> Moving proxy_->ShowKeyboardContainer() in
> KeyboardController::ShowKeyboardInternal() to the end of the function seems to
> do the trick.

Done.

https://codereview.chromium.org/240443006/diff/80002/ash/wm/system_modal_cont...
ash/wm/system_modal_container_layout_manager_unittest.cc:197: window_bounds.y()
+ window_bounds.height() - 100,
On 2014/04/23 15:18:42, pkotwicz wrote:
> Nit: You can use window_bounds.bottom() here

Done.

[oshima, 2014-04-23 16:14:07]

https://codereview.chromium.org/240443006/diff/110001/ui/keyboard/keyboard_co...
File ui/keyboard/keyboard_controller_unittest.cc (right):

https://codereview.chromium.org/240443006/diff/110001/ui/keyboard/keyboard_co...
ui/keyboard/keyboard_controller_unittest.cc:200: gfx::Rect window_bounds =
controller()->GetContainerWindow()->bounds();
you're doing this in muliple places. Can you add a utility function for this?

[bshe, 2014-04-23 16:39:07]

Add a function in keyboard_util. Thanks

https://codereview.chromium.org/240443006/diff/110001/ui/keyboard/keyboard_co...
File ui/keyboard/keyboard_controller_unittest.cc (right):

https://codereview.chromium.org/240443006/diff/110001/ui/keyboard/keyboard_co...
ui/keyboard/keyboard_controller_unittest.cc:200: gfx::Rect window_bounds =
controller()->GetContainerWindow()->bounds();
On 2014/04/23 16:14:08, oshima wrote:
> you're doing this in muliple places. Can you add a utility function for this?
> 
> 

Done.

[oshima, 2014-04-23 16:40:02]

thanks, lgtm

[sadrul, 2014-04-23 16:51:25]

[removing myself; you don't need review from me]

[bshe, 2014-04-23 17:00:35]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2014-04-23 17:01:31]

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

[commit-bot: I haz the power, 2014-04-23 20:00:04]

Change committed as 265712