Keep virtual keyboard visibility the same after enable an IME in a different extension

Keep virtual keyboard visibility the same after enable an IME in a different extension

BUG=404340
Committed: https://crrev.com/f1e33461737736db4ccd9a79ff768cd54b4418a1
Cr-Commit-Position: refs/heads/master@{#291678}

[bshe, 2014-08-19 18:50:41]

Hi Kevin.

Do you mind to take a look at this CL?

Thanks!

[kevers, 2014-08-20 14:02:29]

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

https://codereview.chromium.org/487253003/diff/40001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:297: if (window->GetRootWindow() ==
root_window) {
Not clear to me why we need a bounds changed observer on the keyboard window
itself since it should never have insets for overscrolling.

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

https://codereview.chromium.org/487253003/diff/40001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.h:158: bool wait_for_resize_to_show_;
This variable name seems to imply a decision to show immediately or on resize,
whereas I think the intended behavior is whether or not to show the VK after
receiving a resize.  Perhaps it should be called show_on_resize_.

[bshe, 2014-08-20 14:59:59]

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

https://codereview.chromium.org/487253003/diff/40001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:297: if (window->GetRootWindow() ==
root_window) {
On 2014/08/20 14:02:29, kevers wrote:
> Not clear to me why we need a bounds changed observer on the keyboard window
> itself since it should never have insets for overscrolling.

Sorry. It shouldn't be removed. I thought I could just do the check in
AddBoundsChangedObserver. But it is not exactly the same.

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

https://codereview.chromium.org/487253003/diff/40001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.h:158: bool wait_for_resize_to_show_;
On 2014/08/20 14:02:29, kevers wrote:
> This variable name seems to imply a decision to show immediately or on resize,
> whereas I think the intended behavior is whether or not to show the VK after
> receiving a resize.  Perhaps it should be called show_on_resize_.

Done.

[kevers, 2014-08-20 15:16:04]

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

https://codereview.chromium.org/487253003/diff/60001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:301: GetFrameWindow(window) !=
keyboard_window &&
Think you only need second check.

[bshe, 2014-08-20 15:29:23]

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

https://codereview.chromium.org/487253003/diff/60001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:301: GetFrameWindow(window) !=
keyboard_window &&
On 2014/08/20 15:16:04, kevers wrote:
> Think you only need second check.

Done.

[kevers, 2014-08-20 15:51:11]

lgtm

[bshe, 2014-08-20 15:52:32]

Thanks Kevin.

+oshima for the browser test file owner.

[oshima, 2014-08-20 16:03:33]

lgtm

https://codereview.chromium.org/487253003/diff/80001/chrome/browser/ui/ash/ke...
File chrome/browser/ui/ash/keyboard_controller_browsertest.cc (right):

https://codereview.chromium.org/487253003/diff/80001/chrome/browser/ui/ash/ke...
chrome/browser/ui/ash/keyboard_controller_browsertest.cc:62: bool
KeyboardVisible() const {
nit: IsKeyboardVisible()

[bshe, 2014-08-25 13:00:11]

The CQ bit was checked by bshe@chromium.org

[bshe, 2014-08-25 13:00:20]

The CQ bit was unchecked by bshe@chromium.org

[bshe, 2014-08-25 13:00:49]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2014-08-25 13:01:19]

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

[commit-bot: I haz the power, 2014-08-25 13:52:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-25 14:31:08]

Committed patchset #6 (100001) as 1d1ac761a3b84652541e8b6e06f3f46450c0f39d

[Jeffrey Yasskin, 2014-08-26 01:54:24]

A revert of this CL (patchset #6) has been created in
https://codereview.chromium.org/501293002/ by jyasskin@chromium.org.

The reason for reverting is:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v...
reports:

UninitCondition
Conditional jump or move depends on uninitialised value(s)
  keyboard::KeyboardLayoutManager::SetChildBounds(aura::Window*, gfx::Rect
const&) (ui/keyboard/keyboard_layout_manager.cc:53)
  aura::Window::SetBounds(gfx::Rect const&) (ui/aura/window.cc:437)
 
ash::test::VirtualKeyboardRootWindowControllerTest_RestoreWorkspaceAfterLogin_Test::TestBody()
(ash/root_window_controller_unittest.cc:738)


UninitCondition
Conditional jump or move depends on uninitialised value(s)
  keyboard::KeyboardLayoutManager::SetChildBounds(aura::Window*, gfx::Rect
const&) (ui/keyboard/keyboard_layout_manager.cc:53)
  aura::Window::SetBounds(gfx::Rect const&) (ui/aura/window.cc:437)
 
ash::test::VirtualKeyboardRootWindowControllerTest_ClickWithActiveModalDialog_Test::TestBody()
(ash/root_window_controller_unittest.cc:771)


UninitCondition
Conditional jump or move depends on uninitialised value(s)
  keyboard::KeyboardLayoutManager::SetChildBounds(aura::Window*, gfx::Rect
const&) (ui/keyboard/keyboard_layout_manager.cc:53)
  aura::Window::SetBounds(gfx::Rect const&) (ui/aura/window.cc:437)
 
ash::test::VirtualKeyboardRootWindowControllerTest_EnsureCaretInWorkArea_Test::TestBody()
(ash/root_window_controller_unittest.cc:827)
.

[commit-bot: I haz the power, 2014-09-10 02:34:23]

Patchset 6 (id:??) landed as
https://crrev.com/f1e33461737736db4ccd9a79ff768cd54b4418a1
Cr-Commit-Position: refs/heads/master@{#291678}