New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 845780 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 731537



Sign in to add a comment

Simplify KeyboardController

Project Member Reported by shend@chromium.org, May 23 2018

Issue description

KeyboardController is quite a complex class with several responsibilities. We should probably split it up into smaller classes and reduce the size of the interface.
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5c6ed9e6941517deed7f545ee73ddb196b77d2e3

commit 5c6ed9e6941517deed7f545ee73ddb196b77d2e3
Author: Darren Shen <shend@chromium.org>
Date: Wed May 23 07:41:16 2018

[VK] Simplify KeyboardController::SetDraggableArea.

|SetDraggableArea| returns checks if the container behavior is null,
but KeyboardController always has a container behavior (initialised in
the constructor).

This patch removes the container behavior null check.

Bug: 845780
Change-Id: Iaa21bd8179a7fb88a0dd277f4888ae778c624d99
Reviewed-on: https://chromium-review.googlesource.com/1069950
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560995}
[modify] https://crrev.com/5c6ed9e6941517deed7f545ee73ddb196b77d2e3/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/5c6ed9e6941517deed7f545ee73ddb196b77d2e3/ui/keyboard/keyboard_controller.h

Project Member

Comment 2 by bugdroid1@chromium.org, May 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/95e84eba8fda7f5eaf48e84045e51bb30077e895

commit 95e84eba8fda7f5eaf48e84045e51bb30077e895
Author: Darren Shen <shend@chromium.org>
Date: Tue May 29 03:25:03 2018

[VK] Remove unnecessary KeyboardController::enabled_.

We currently store a separate member |enabled| to indicate whether the
KeyboardController is enabled or not. The original plan was that this
member could be replaced by a keyboard state. However, we can get rid
of it right now by checking whether the keyboard UI is initialized or
not.

We could've also chosen to check the keyboard container, but the
container might undergo changes in the future.

Bug: 845780
Change-Id: I52218e7312f89b05be09fd09d38df6d89e724b9a
Reviewed-on: https://chromium-review.googlesource.com/1074951
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562326}
[modify] https://crrev.com/95e84eba8fda7f5eaf48e84045e51bb30077e895/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/95e84eba8fda7f5eaf48e84045e51bb30077e895/ui/keyboard/keyboard_controller.h

Project Member

Comment 3 by bugdroid1@chromium.org, May 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b0c1d4c7d25443c4fc2c2fe96b45c34c4d8bfcda

commit b0c1d4c7d25443c4fc2c2fe96b45c34c4d8bfcda
Author: Darren Shen <shend@chromium.org>
Date: Tue May 29 03:48:17 2018

[VK] Move keyboard container creation to constructor.

Currently, KeyboardController lazily initializes its container window
by creating it when |GetContentWindow| is called. However, it is unclear
what the exact lifetime of the container window is.

My hypothesis:
   existence of KeyboardController <=> existence of container window

1) KeyboardController => Container:

   Apart from tests, KeyboardController is only created by ash::Shell::
   CreateKeyboard. In CreateKeyboard, if we call the KeyboardController
   constructor, we also call RootWindowController::ActivateKeyboard. In
   ActivateKeyboard, we always call |GetContainerWindow| (both branches
   of the if statement call it), which would ensure the existence of
   the container.

2) Container => KeyboardController

   The container is a member of KeyboardController and is created/
   destroyed with it.

So unless there's some weird case where we have to create the container
in RootWindowController::ActivateKeyboard, I think it's okay to
create the container in the KeyboardController constructor.

Bug: 845780
Change-Id: I2028f8adbf6a5f09877041a4b85d9032b7ebd88f
Reviewed-on: https://chromium-review.googlesource.com/1070048
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562331}
[modify] https://crrev.com/b0c1d4c7d25443c4fc2c2fe96b45c34c4d8bfcda/ash/root_window_controller.cc
[modify] https://crrev.com/b0c1d4c7d25443c4fc2c2fe96b45c34c4d8bfcda/ash/root_window_controller_unittest.cc
[modify] https://crrev.com/b0c1d4c7d25443c4fc2c2fe96b45c34c4d8bfcda/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/b0c1d4c7d25443c4fc2c2fe96b45c34c4d8bfcda/ui/keyboard/keyboard_controller.h
[modify] https://crrev.com/b0c1d4c7d25443c4fc2c2fe96b45c34c4d8bfcda/ui/keyboard/keyboard_test_util.cc

Project Member

Comment 4 by bugdroid1@chromium.org, May 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fbf8c384706721b01c8003af26c18572ce0ba105

commit fbf8c384706721b01c8003af26c18572ce0ba105
Author: Darren Shen <shend@chromium.org>
Date: Wed May 30 03:53:26 2018

[VK] Separate content loading from bounds changing.

When KeyboardController is told to show the keyboard, it first has to
wait for the keyboard contents to load. When contents load, it resizes
itself, so KeyboardController just listens to changes in content size.
If the size changed from 0 to some nonzero value, it means the contents
must've loaded.

However, this is indirect and not very intuitive. The more direct way
is for KeyboardUI to listen for when the contents load and to tell
KeyboardController.

Change-Id: I1a1e8518374b67e92aa2c24920216d4f9207cbb2
Bug: 845780
Reviewed-on: https://chromium-review.googlesource.com/1065551
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562724}
[modify] https://crrev.com/fbf8c384706721b01c8003af26c18572ce0ba105/ash/login/ui/login_keyboard_test_base.cc
[modify] https://crrev.com/fbf8c384706721b01c8003af26c18572ce0ba105/ash/wm/lock_action_handler_layout_manager_unittest.cc
[modify] https://crrev.com/fbf8c384706721b01c8003af26c18572ce0ba105/ash/wm/lock_layout_manager_unittest.cc
[modify] https://crrev.com/fbf8c384706721b01c8003af26c18572ce0ba105/ash/wm/system_modal_container_layout_manager_unittest.cc
[modify] https://crrev.com/fbf8c384706721b01c8003af26c18572ce0ba105/ash/wm/workspace/workspace_layout_manager_unittest.cc
[modify] https://crrev.com/fbf8c384706721b01c8003af26c18572ce0ba105/chrome/browser/ui/ash/chrome_keyboard_ui.cc
[modify] https://crrev.com/fbf8c384706721b01c8003af26c18572ce0ba105/chrome/browser/ui/ash/chrome_keyboard_ui.h
[modify] https://crrev.com/fbf8c384706721b01c8003af26c18572ce0ba105/chrome/browser/ui/ash/keyboard_controller_browsertest.cc
[modify] https://crrev.com/fbf8c384706721b01c8003af26c18572ce0ba105/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/fbf8c384706721b01c8003af26c18572ce0ba105/ui/keyboard/keyboard_controller.h
[modify] https://crrev.com/fbf8c384706721b01c8003af26c18572ce0ba105/ui/keyboard/keyboard_controller_unittest.cc
[modify] https://crrev.com/fbf8c384706721b01c8003af26c18572ce0ba105/ui/keyboard/keyboard_layout_manager.cc

Project Member

Comment 5 by bugdroid1@chromium.org, May 31 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8cb7ca2b1d508e42fdf612433b633a13db269b64

commit 8cb7ca2b1d508e42fdf612433b633a13db269b64
Author: Darren Shen <shend@chromium.org>
Date: Thu May 31 06:04:10 2018

[VK] Fix TODO by adding DCHECK.

Change-Id: I86106fe1d67f9365633d3bfdc8da3bdb19f60816
Bug: 845780
Reviewed-on: https://chromium-review.googlesource.com/1077871
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563161}
[modify] https://crrev.com/8cb7ca2b1d508e42fdf612433b633a13db269b64/ui/keyboard/keyboard_controller.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/45105a292d437ac7e10782b9d982bb42b092cda3

commit 45105a292d437ac7e10782b9d982bb42b092cda3
Author: Darren Shen <shend@chromium.org>
Date: Wed Jun 13 01:43:39 2018

[VK] Remove KeyboardObserverRegister.

Whenever KeyboardController is created/destroyed, observers need to be
re-registered. KeyboardObserverRegister was introduced to simplify this
process. However, with the new KeyboardController lifetimes, we only
create/destroy the KeyboardController once, so we don't need to use
KeyboardObserverRegisters anymore; we just need to register once at the
beginning and unregister once at the end.

We don't need to check whether KeyboardController is enabled when
adding an observer, as none of the observer events are raised when
the KeyboardController is disabled.

Bug: 845780
Change-Id: I2bba7919ad7330649209fe3b6f44f74d26b650e3
Reviewed-on: https://chromium-review.googlesource.com/1075841
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566679}
[modify] https://crrev.com/45105a292d437ac7e10782b9d982bb42b092cda3/ash/BUILD.gn
[modify] https://crrev.com/45105a292d437ac7e10782b9d982bb42b092cda3/ash/accessibility/touch_exploration_manager.cc
[modify] https://crrev.com/45105a292d437ac7e10782b9d982bb42b092cda3/ash/accessibility/touch_exploration_manager.h
[delete] https://crrev.com/5f734e3228cd68c306c7870cfd4e8ae9a56e41cb/ash/keyboard/keyboard_observer_register.cc
[delete] https://crrev.com/5f734e3228cd68c306c7870cfd4e8ae9a56e41cb/ash/keyboard/keyboard_observer_register.h
[modify] https://crrev.com/45105a292d437ac7e10782b9d982bb42b092cda3/ash/login/ui/lock_contents_view.cc
[modify] https://crrev.com/45105a292d437ac7e10782b9d982bb42b092cda3/ash/login/ui/lock_contents_view.h
[modify] https://crrev.com/45105a292d437ac7e10782b9d982bb42b092cda3/ash/shelf/shelf_layout_manager.cc
[modify] https://crrev.com/45105a292d437ac7e10782b9d982bb42b092cda3/ash/shelf/shelf_layout_manager.h
[modify] https://crrev.com/45105a292d437ac7e10782b9d982bb42b092cda3/ash/wm/lock_layout_manager.cc
[modify] https://crrev.com/45105a292d437ac7e10782b9d982bb42b092cda3/ash/wm/lock_layout_manager.h
[modify] https://crrev.com/45105a292d437ac7e10782b9d982bb42b092cda3/ash/wm/panels/panel_layout_manager.cc
[modify] https://crrev.com/45105a292d437ac7e10782b9d982bb42b092cda3/ash/wm/panels/panel_layout_manager.h
[modify] https://crrev.com/45105a292d437ac7e10782b9d982bb42b092cda3/ash/wm/workspace/workspace_layout_manager.cc
[modify] https://crrev.com/45105a292d437ac7e10782b9d982bb42b092cda3/ash/wm/workspace/workspace_layout_manager.h

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dc839a42d993d5627a3c901a268940ad7826d35d

commit dc839a42d993d5627a3c901a268940ad7826d35d
Author: Darren Shen <shend@chromium.org>
Date: Tue Jun 26 06:32:00 2018

[VK] Clarify terminology in KeyboardControllerObserver.

We rename some KeyboardControllerObserver events to avoid confusion:
- OnKeyboardClosed -> OnKeyboardDisabled ("closed" could mean hidden
  too).
- OnKeyboardAvailabilityChanged -> OnKeyboardVisibilityChanged
  ("available" could mean that VK is enabled).

Bug: 845780
Change-Id: I8a0f1532b064377ce4441e2e407ba136778cd506
Reviewed-on: https://chromium-review.googlesource.com/1111489
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Blake O'Hare <blakeo@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570339}
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ash/accessibility/touch_exploration_manager.cc
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ash/accessibility/touch_exploration_manager.h
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ash/app_list/app_list_controller_impl.cc
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ash/app_list/app_list_controller_impl.h
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ash/keyboard/virtual_keyboard_controller.cc
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ash/keyboard/virtual_keyboard_controller.h
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ash/shelf/shelf.cc
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ash/shelf/shelf_layout_manager.cc
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ash/shelf/shelf_layout_manager.h
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ash/shelf/shelf_layout_manager_unittest.cc
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ash/system/virtual_keyboard/virtual_keyboard_tray.cc
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ash/system/virtual_keyboard/virtual_keyboard_tray.h
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/chrome/browser/chromeos/login/ui/webui_login_view.cc
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/chrome/browser/chromeos/login/ui/webui_login_view.h
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/chrome/browser/ui/ash/chrome_keyboard_ui.cc
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/components/arc/ime/arc_ime_service.cc
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ui/keyboard/keyboard_controller_observer.h
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ui/keyboard/keyboard_controller_unittest.cc
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ui/keyboard/notification_manager.cc
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ui/keyboard/notification_manager.h
[modify] https://crrev.com/dc839a42d993d5627a3c901a268940ad7826d35d/ui/keyboard/notification_manager_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/862313762ae0b2fa2db73bdc61f759525e5c1fee

commit 862313762ae0b2fa2db73bdc61f759525e5c1fee
Author: Darren Shen <shend@chromium.org>
Date: Thu Jun 28 00:22:25 2018

[VK] Remove unused WindowObserver events.

KeyboardController listens to WindowObserver events on the parent
container window. It updates the observer whenever the keyboard moves
to a different display by removing the observer on the old root window
and adding the observer on the new root window. See original patch
that added the code:
https://crrev.com/1008453002

We don't really need to do this as we already do that in Activate/
Deactivate keyboard.

Bug: 845780
Change-Id: Idc25d5beadbe1f2233e184fd1c189a3fde05aa50
Reviewed-on: https://chromium-review.googlesource.com/1113714
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570970}
[modify] https://crrev.com/862313762ae0b2fa2db73bdc61f759525e5c1fee/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/862313762ae0b2fa2db73bdc61f759525e5c1fee/ui/keyboard/keyboard_controller.h

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/463aac3b51d6490816019124a8677a517cb3d83a

commit 463aac3b51d6490816019124a8677a517cb3d83a
Author: Darren Shen <shend@chromium.org>
Date: Wed Jul 04 04:27:52 2018

[VK] Remove unused IsKeyboardWindowCreated function.

Not used. Can use |GetKeyboardWindow| to do the same thing (check for
null).

TBR=yhanada@chromium.org

Bug: 845780
Change-Id: I4f104099e92032e25c1aec15c3b67850253ad3e8
Reviewed-on: https://chromium-review.googlesource.com/1124737
Reviewed-by: Darren Shen <shend@chromium.org>
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572472}
[modify] https://crrev.com/463aac3b51d6490816019124a8677a517cb3d83a/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/463aac3b51d6490816019124a8677a517cb3d83a/ui/keyboard/keyboard_controller.h

Project Member

Comment 10 by bugdroid1@chromium.org, Jul 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/333e69dd214b121ac48a4ece1c1e5cb57f553b5f

commit 333e69dd214b121ac48a4ece1c1e5cb57f553b5f
Author: Darren Shen <shend@chromium.org>
Date: Wed Jul 04 22:58:06 2018

[VK] Remove unnecessary keyboard visibility check.

By definition, |visual_bounds_in_screen_| is the empty rectange when
the keyboard is not visible. Hence, when returning the visual bounds,
we don't need to check if the keyboard is visible.

Bug: 845780
Change-Id: Ib598b3b3361d9a342ee94a5ac7557b3036e864ca
Reviewed-on: https://chromium-review.googlesource.com/1125556
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572671}
[modify] https://crrev.com/333e69dd214b121ac48a4ece1c1e5cb57f553b5f/ui/keyboard/keyboard_controller.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/398b69d8d1f92204eb41552c9843debf09906f50

commit 398b69d8d1f92204eb41552c9843debf09906f50
Author: Darren Shen <shend@chromium.org>
Date: Wed Jul 04 23:03:23 2018

[VK] Remove duplicate visiblity getter in KeyboardController.

We currently have |keyboard_visible| and |IsKeyboardVisible|. Both do the
same thing. Since |IsKeyboardVisible| is required by the
InputMethodVirtualKeyboardController interface, we'll use
|IsKeyboardVisible| and delete |keyboard_visible|.

TBR=sky@chromium.org

Bug: 845780
Change-Id: I02205fc9032a299eb9a102b6ab2b7e143d747e54
Reviewed-on: https://chromium-review.googlesource.com/1124735
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572672}
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/ash/accelerators/accelerator_controller.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/ash/app_list/app_list_presenter_delegate_unittest.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/ash/app_list/views/app_list_view.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/ash/login/ui/lock_contents_view.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/ash/login/ui/login_keyboard_test_base.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/ash/magnifier/magnification_controller.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/ash/system/ime_menu/ime_menu_tray.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/ash/wm/lock_action_handler_layout_manager_unittest.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/ash/wm/lock_layout_manager_unittest.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/ash/wm/system_modal_container_layout_manager_unittest.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/chrome/browser/ui/ash/chrome_keyboard_ui.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/chrome/browser/ui/ash/keyboard_controller_browsertest.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/ui/keyboard/keyboard_controller.h
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/ui/keyboard/keyboard_controller_unittest.cc
[modify] https://crrev.com/398b69d8d1f92204eb41552c9843debf09906f50/ui/keyboard/keyboard_util.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Jul 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18bc6df3c2a99db3193c01850566f5914f00a3b1

commit 18bc6df3c2a99db3193c01850566f5914f00a3b1
Author: Darren Shen <shend@chromium.org>
Date: Mon Jul 09 04:29:45 2018

Fix search and replace error in lock_contents_view.

In 1124735 we accidentally replaced a variable name called
keyboard_visible to IsKeyboardVisible, which is the wrong style. We
change it back to snake case.

TBR=sky@chromium.org

Bug: 845780
Change-Id: I0b30e1d32623e4f7bbc531d141eb1e13a55fd6a6
Reviewed-on: https://chromium-review.googlesource.com/1127917
Reviewed-by: Darren Shen <shend@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573215}
[modify] https://crrev.com/18bc6df3c2a99db3193c01850566f5914f00a3b1/ash/login/ui/lock_contents_view.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Jul 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/afc4e1a8124f283175da9e881e43e2589d28345e

commit afc4e1a8124f283175da9e881e43e2589d28345e
Author: Darren Shen <shend@chromium.org>
Date: Mon Jul 09 22:31:53 2018

[VK] Remove extraneous comment.

Follow up on r1124735.

TBR=blakeo@chromium.org

Bug: 845780
Change-Id: If2e40e0884fe46f5cf874f5eb5d1aeb4e479c722
Reviewed-on: https://chromium-review.googlesource.com/1130199
Reviewed-by: Darren Shen <shend@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573488}
[modify] https://crrev.com/afc4e1a8124f283175da9e881e43e2589d28345e/ui/keyboard/keyboard_controller.h

Project Member

Comment 14 by bugdroid1@chromium.org, Jul 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4c597e35ea365cc192bfad7ef1effc618e427004

commit 4c597e35ea365cc192bfad7ef1effc618e427004
Author: Darren Shen <shend@chromium.org>
Date: Wed Jul 11 23:52:47 2018

[VK] Remove OnWindowHierarchyChanged event handler.

In https://crrev.com/14200002, we handled OnWindowHierarchyChanged in
KeyboardController, which calls OnTextInputStateChanged. Originally,
the author of the CL wanted to call OnTextInputStateChanged when the
keyboard is enabled (so that if there's a focused textfield, we would
show the keyboard [*]). However, OnTextInputChanged requires that the
keyboard window is attached to a parent, which does not happen until
later in the code. Hence, the author moved the OnTextInputStateChanged
call to OnWindowHierarchyChanged, where we know for sure that the
keyboard window has been attached.

However, with recent changes to KeyboardController, we explicitly
attach in ActivateKeyboardInContainer, so we can just move the code
there without issues.

[*] This behaviour is no longer true. OnTextInputChanged can't show
the keyboard anymore under those circumstances. It may be possible
to remove the OnTextInputChanged call completely.

Bug: 845780
Change-Id: I8fb028f3b03435e43c902b40e8ffe48844be1ea3
Reviewed-on: https://chromium-review.googlesource.com/1132259
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574413}
[modify] https://crrev.com/4c597e35ea365cc192bfad7ef1effc618e427004/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/4c597e35ea365cc192bfad7ef1effc618e427004/ui/keyboard/keyboard_controller.h

Project Member

Comment 15 by bugdroid1@chromium.org, Jul 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0cb32b12d33d62e3e56c9b8c9922aa9d5ca2b68f

commit 0cb32b12d33d62e3e56c9b8c9922aa9d5ca2b68f
Author: Darren Shen <shend@chromium.org>
Date: Thu Jul 12 01:42:39 2018

[VK] Tidy up KeyboardController::PopulateKeyboardContent.

Just moved some code around. Looks more consistent IMO when every switch
case early returns.

Bug: 845780
Change-Id: If828b05cb088395d1e44d83ec038b57e8bffe9bd
Reviewed-on: https://chromium-review.googlesource.com/1132908
Reviewed-by: Blake O'Hare <blakeo@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574452}
[modify] https://crrev.com/0cb32b12d33d62e3e56c9b8c9922aa9d5ca2b68f/ui/keyboard/keyboard_controller.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Jul 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3099becad3e1f35c2602ba01686e818d2f546fc8

commit 3099becad3e1f35c2602ba01686e818d2f546fc8
Author: Darren Shen <shend@chromium.org>
Date: Mon Jul 23 00:14:56 2018

[VK] Observe changes to keyboard window in KeyboardController.

There are two ways for the keyboard window bounds to change. Either
through KeyboardController or through JavaScript / the IME extension.
Notably, we always receive a JavaScript bound change when initializing
the keyboard UI.

To handle bounds changes from JavaScript, we use a LayoutManager
to intercept those changes and forward them to KeyboardController.
In production, the LayoutManager is installed by ash::Shell.
In unit tests however, we do not always have this setup because the
test may not be able to depend on ash. Unit tests that load the
keyboard will simulate the initial JavaScript bound change, but because
there's no layout manager, KeyboardController does not receive the
update and gets into a bad state.

The current workaround is to manually add a LayoutManager to the
root window in tests that need it. This is not ideal, as the correctness
of KeyboardController is silently dependent on the construction of
a LayoutManager [*].

To fix this, we observe bounds changes to the keyboard window in
KeyboardController. Then, KeyboardController will no longer need the
LayoutManager to notify bounds changes; it can observe them directly.

[*] We didn't have this problem before because we could install the
layout manager on the keyboard container, which was part of the
KeyboardController. Since we got rid of the keyboard container, we can
only install the layout manager on an ash container, which is
external to KeyboardController.

Bug: 845780
Change-Id: I053778f04ab3ccf4d13fcd79d05832f9238b21b5
Reviewed-on: https://chromium-review.googlesource.com/1133607
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577099}
[modify] https://crrev.com/3099becad3e1f35c2602ba01686e818d2f546fc8/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/3099becad3e1f35c2602ba01686e818d2f546fc8/ui/keyboard/keyboard_controller_unittest.cc
[modify] https://crrev.com/3099becad3e1f35c2602ba01686e818d2f546fc8/ui/keyboard/keyboard_layout_manager.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Jul 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ffcb250e07f285e5f23455e45bb635c6efcbc687

commit ffcb250e07f285e5f23455e45bb635c6efcbc687
Author: Darren Shen <shend@chromium.org>
Date: Mon Jul 23 01:45:20 2018

[VK] Remove unused friend declaration in KeyboardController.

In a previous patch, we removed KeyboardLayoutManager's dependency
on KeyboardController's SetContainerType. This means that the friend
declaration in KeyboardController is no longer needed.

TBR=blakeo@chromium.org

Bug: 845780
Change-Id: I56ea22d989dcefc3bc70d4ca054678fd52c5bef7
Reviewed-on: https://chromium-review.googlesource.com/1146139
Reviewed-by: Darren Shen <shend@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577101}
[modify] https://crrev.com/ffcb250e07f285e5f23455e45bb635c6efcbc687/ui/keyboard/keyboard_controller.h

Project Member

Comment 18 by bugdroid1@chromium.org, Aug 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7696adb0cd2f6aa848a0f6113576d6ed9dfcdcdc

commit 7696adb0cd2f6aa848a0f6113576d6ed9dfcdcdc
Author: Darren Shen <shend@chromium.org>
Date: Wed Aug 01 23:36:40 2018

[VK] Make shelf use displaced/occluded bounds instead of is_locked.

KeyboardController has the concept of "locked". When the keyboard is
locked (e.g. when it is launched from the accessibility menu), the
work area of the screen is shrunk, so that only the part above the
keyboard is considered 'useable'.

Currently, this is implemented in the shelf by observing changes to the
keyboard bounds. If the bounds change and the keyboard is locked, we
update the work area with the given bounds.

While this is correct, whether the keyboard is locked or not should be
internal to the keyboard. Instead, shelf should be listening for the
*displaced* bounds, which essentially tell observers how much of the
screen is now unuseable.

Thus, we replace the keyboard bounds in shelf to either the displaced
bounds or the occluded bounds, depending on what the shelf actually
wants. If the bounds are used for the work area, then it's the displaced
bounds. Otherwise, it's the occluded bounds.

Change-Id: I7c53d0b2fe8b3b6769ce7113b924def34db2785e
Bug: 845780
Reviewed-on: https://chromium-review.googlesource.com/1146530
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Blake O'Hare <blakeo@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579992}
[modify] https://crrev.com/7696adb0cd2f6aa848a0f6113576d6ed9dfcdcdc/ash/shelf/shelf.cc
[modify] https://crrev.com/7696adb0cd2f6aa848a0f6113576d6ed9dfcdcdc/ash/shelf/shelf_layout_manager.cc
[modify] https://crrev.com/7696adb0cd2f6aa848a0f6113576d6ed9dfcdcdc/ash/shelf/shelf_layout_manager.h
[modify] https://crrev.com/7696adb0cd2f6aa848a0f6113576d6ed9dfcdcdc/ash/shelf/shelf_layout_manager_unittest.cc
[modify] https://crrev.com/7696adb0cd2f6aa848a0f6113576d6ed9dfcdcdc/ash/system/tray/tray_background_view.cc
[modify] https://crrev.com/7696adb0cd2f6aa848a0f6113576d6ed9dfcdcdc/components/arc/ime/arc_ime_service_unittest.cc
[modify] https://crrev.com/7696adb0cd2f6aa848a0f6113576d6ed9dfcdcdc/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/7696adb0cd2f6aa848a0f6113576d6ed9dfcdcdc/ui/keyboard/keyboard_controller_observer.h
[modify] https://crrev.com/7696adb0cd2f6aa848a0f6113576d6ed9dfcdcdc/ui/keyboard/notification_manager.cc
[modify] https://crrev.com/7696adb0cd2f6aa848a0f6113576d6ed9dfcdcdc/ui/keyboard/notification_manager.h

Project Member

Comment 19 by bugdroid1@chromium.org, Aug 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a6c1752fbfb0dd5868aa826910f00d9f40215d9c

commit a6c1752fbfb0dd5868aa826910f00d9f40215d9c
Author: Darren Shen <shend@chromium.org>
Date: Thu Aug 02 05:27:49 2018

[VK] Remove extraneous calls to NotifyKeyboardBoundsChanged.

In a previous CL, we made it so that changes to the keyboard window
bounds automatically call NotifyKeyboardBoundsChanged. Now we can
remove these extraneous calls and make NotifyKeyboardBoundsChanged
private.

TBR=jamescook@chromium.org

Bug: 845780
Change-Id: I191a22fc50e37a3c57552169f05f609f4fba3005
Reviewed-on: https://chromium-review.googlesource.com/1156114
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Darren Shen <shend@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580079}
[modify] https://crrev.com/a6c1752fbfb0dd5868aa826910f00d9f40215d9c/ash/root_window_controller_unittest.cc
[modify] https://crrev.com/a6c1752fbfb0dd5868aa826910f00d9f40215d9c/ash/wm/always_on_top_controller_unittest.cc
[modify] https://crrev.com/a6c1752fbfb0dd5868aa826910f00d9f40215d9c/chrome/browser/ui/ash/keyboard_controller_browsertest.cc
[modify] https://crrev.com/a6c1752fbfb0dd5868aa826910f00d9f40215d9c/ui/keyboard/keyboard_controller.h

Project Member

Comment 20 by bugdroid1@chromium.org, Oct 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/080ef646977a88aced4419c133478818626fa66b

commit 080ef646977a88aced4419c133478818626fa66b
Author: Darren Shen <shend@chromium.org>
Date: Fri Oct 05 03:54:29 2018

[VK] Avoid calling KeyboardUI::GetKeyboardWindow directly.

KeyboardUI::GetKeyboardWindow lazily creates a keyboard window and
loads web contents. Because the KeyboardController lifecycle is quite
complicated, it's not clear from reading the code whether a call to
GetKeyboardWindow creates a window or not.

In fact, only one particular call to GetKeyboardWindow creates the
keyboard window. All the other calls are guaranteed to occur after
that call. Thus, we keep that one call to GetKeyboardWindow to create
the keyboard window and replace the other calls with KeyboardController
::GetKeyboardWindow, which, by contrast, has no side effects.

Bug: 845780
Change-Id: I56e86932e82ced8b25a6f9eb034c60034e3a3548
Reviewed-on: https://chromium-review.googlesource.com/c/1235353
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596985}
[modify] https://crrev.com/080ef646977a88aced4419c133478818626fa66b/ui/keyboard/keyboard_controller.cc

Project Member

Comment 21 by bugdroid1@chromium.org, Oct 15

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b03ffb6b8056961fdbd373de1a4cb35719acfaa8

commit b03ffb6b8056961fdbd373de1a4cb35719acfaa8
Author: Darren Shen <shend@chromium.org>
Date: Mon Oct 15 23:16:10 2018

[VK] Clarify KeyboardUI interface.

Currently, KeyboardUI has a weird interface where |GetKeyboardWindow|
will start loading the keyboard web page if it is called the first time.
So this resulted in a lot of places where we were calling
GetKeyboardWindow and didn't know whether it would cause a load or not.

We change the KeyboardUI interface a bit to explicitly state when we
are creating / loading a keyboard window, vs just getting an already
loaded one.

This removes ChromeKeyboardUIWebContent's dependency on
KeyboardController as well.

We tried to keep the same behaviour for existing KeyboardUI subclasses.

Bug: 845780
Change-Id: I3178c81c382f2c3cd8217eda5a0c9b9d489df5be
Reviewed-on: https://chromium-review.googlesource.com/c/1264336
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599772}
[modify] https://crrev.com/b03ffb6b8056961fdbd373de1a4cb35719acfaa8/ash/keyboard/test_keyboard_ui.cc
[modify] https://crrev.com/b03ffb6b8056961fdbd373de1a4cb35719acfaa8/ash/keyboard/test_keyboard_ui.h
[modify] https://crrev.com/b03ffb6b8056961fdbd373de1a4cb35719acfaa8/chrome/browser/ui/ash/chrome_keyboard_ui.cc
[modify] https://crrev.com/b03ffb6b8056961fdbd373de1a4cb35719acfaa8/chrome/browser/ui/ash/chrome_keyboard_ui.h
[modify] https://crrev.com/b03ffb6b8056961fdbd373de1a4cb35719acfaa8/chrome/browser/ui/ash/chrome_keyboard_ui_unittest.cc
[modify] https://crrev.com/b03ffb6b8056961fdbd373de1a4cb35719acfaa8/chrome/browser/ui/ash/chrome_keyboard_web_contents.cc
[modify] https://crrev.com/b03ffb6b8056961fdbd373de1a4cb35719acfaa8/chrome/browser/ui/ash/chrome_keyboard_web_contents.h
[modify] https://crrev.com/b03ffb6b8056961fdbd373de1a4cb35719acfaa8/chrome/browser/ui/ash/chrome_keyboard_web_contents_unittest.cc
[modify] https://crrev.com/b03ffb6b8056961fdbd373de1a4cb35719acfaa8/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/b03ffb6b8056961fdbd373de1a4cb35719acfaa8/ui/keyboard/keyboard_ui.cc
[modify] https://crrev.com/b03ffb6b8056961fdbd373de1a4cb35719acfaa8/ui/keyboard/keyboard_ui.h
[modify] https://crrev.com/b03ffb6b8056961fdbd373de1a4cb35719acfaa8/ui/keyboard/test/keyboard_test_util.cc
[modify] https://crrev.com/b03ffb6b8056961fdbd373de1a4cb35719acfaa8/ui/keyboard/test/keyboard_test_util.h

Project Member

Comment 22 by bugdroid1@chromium.org, Oct 18

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aac50024007e214db91dcd64b3d553e4394d2df1

commit aac50024007e214db91dcd64b3d553e4394d2df1
Author: Darren Shen <shend@chromium.org>
Date: Thu Oct 18 06:03:30 2018

Revert "[VK] Clarify KeyboardUI interface."

This reverts commit b03ffb6b8056961fdbd373de1a4cb35719acfaa8.

Reason for revert: b117901291

Original change's description:
> [VK] Clarify KeyboardUI interface.
> 
> Currently, KeyboardUI has a weird interface where |GetKeyboardWindow|
> will start loading the keyboard web page if it is called the first time.
> So this resulted in a lot of places where we were calling
> GetKeyboardWindow and didn't know whether it would cause a load or not.
> 
> We change the KeyboardUI interface a bit to explicitly state when we
> are creating / loading a keyboard window, vs just getting an already
> loaded one.
> 
> This removes ChromeKeyboardUIWebContent's dependency on
> KeyboardController as well.
> 
> We tried to keep the same behaviour for existing KeyboardUI subclasses.
> 
> Bug: 845780
> Change-Id: I3178c81c382f2c3cd8217eda5a0c9b9d489df5be
> Reviewed-on: https://chromium-review.googlesource.com/c/1264336
> Commit-Queue: Darren Shen <shend@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Reviewed-by: James Cook <jamescook@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#599772}

TBR=jamescook@chromium.org,stevenjb@chromium.org,shend@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 845780
Change-Id: Ifb9f421d12eccfc0f6208a952025b84df087228b
Reviewed-on: https://chromium-review.googlesource.com/c/1288094
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600668}
[modify] https://crrev.com/aac50024007e214db91dcd64b3d553e4394d2df1/ash/keyboard/test_keyboard_ui.cc
[modify] https://crrev.com/aac50024007e214db91dcd64b3d553e4394d2df1/ash/keyboard/test_keyboard_ui.h
[modify] https://crrev.com/aac50024007e214db91dcd64b3d553e4394d2df1/chrome/browser/ui/ash/chrome_keyboard_ui.cc
[modify] https://crrev.com/aac50024007e214db91dcd64b3d553e4394d2df1/chrome/browser/ui/ash/chrome_keyboard_ui.h
[modify] https://crrev.com/aac50024007e214db91dcd64b3d553e4394d2df1/chrome/browser/ui/ash/chrome_keyboard_ui_unittest.cc
[modify] https://crrev.com/aac50024007e214db91dcd64b3d553e4394d2df1/chrome/browser/ui/ash/chrome_keyboard_web_contents.cc
[modify] https://crrev.com/aac50024007e214db91dcd64b3d553e4394d2df1/chrome/browser/ui/ash/chrome_keyboard_web_contents.h
[modify] https://crrev.com/aac50024007e214db91dcd64b3d553e4394d2df1/chrome/browser/ui/ash/chrome_keyboard_web_contents_unittest.cc
[modify] https://crrev.com/aac50024007e214db91dcd64b3d553e4394d2df1/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/aac50024007e214db91dcd64b3d553e4394d2df1/ui/keyboard/keyboard_ui.cc
[modify] https://crrev.com/aac50024007e214db91dcd64b3d553e4394d2df1/ui/keyboard/keyboard_ui.h
[modify] https://crrev.com/aac50024007e214db91dcd64b3d553e4394d2df1/ui/keyboard/test/keyboard_test_util.cc
[modify] https://crrev.com/aac50024007e214db91dcd64b3d553e4394d2df1/ui/keyboard/test/keyboard_test_util.h

Project Member

Comment 23 by bugdroid1@chromium.org, Oct 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8a31a94a0f75763dc608c4698b5e4589871bd82d

commit 8a31a94a0f75763dc608c4698b5e4589871bd82d
Author: Darren Shen <shend@chromium.org>
Date: Fri Oct 19 03:42:46 2018

Reland "[VK] Clarify KeyboardUI interface."

This is a reland of b03ffb6b8056961fdbd373de1a4cb35719acfaa8

Ran arc.Boot locally and it passes.

TBR=jamescook@chromium.org

Original change's description:
> [VK] Clarify KeyboardUI interface.
>
> Currently, KeyboardUI has a weird interface where |GetKeyboardWindow|
> will start loading the keyboard web page if it is called the first time.
> So this resulted in a lot of places where we were calling
> GetKeyboardWindow and didn't know whether it would cause a load or not.
>
> We change the KeyboardUI interface a bit to explicitly state when we
> are creating / loading a keyboard window, vs just getting an already
> loaded one.
>
> This removes ChromeKeyboardUIWebContent's dependency on
> KeyboardController as well.
>
> We tried to keep the same behaviour for existing KeyboardUI subclasses.
>
> Bug: 845780
> Change-Id: I3178c81c382f2c3cd8217eda5a0c9b9d489df5be
> Reviewed-on: https://chromium-review.googlesource.com/c/1264336
> Commit-Queue: Darren Shen <shend@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Reviewed-by: James Cook <jamescook@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#599772}

Bug: 845780
Change-Id: I77587e5bb62592df6d7cc9497e370600126c9418
Reviewed-on: https://chromium-review.googlesource.com/c/1288175
Reviewed-by: Darren Shen <shend@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601043}
[modify] https://crrev.com/8a31a94a0f75763dc608c4698b5e4589871bd82d/ash/keyboard/test_keyboard_ui.cc
[modify] https://crrev.com/8a31a94a0f75763dc608c4698b5e4589871bd82d/ash/keyboard/test_keyboard_ui.h
[modify] https://crrev.com/8a31a94a0f75763dc608c4698b5e4589871bd82d/chrome/browser/ui/ash/chrome_keyboard_ui.cc
[modify] https://crrev.com/8a31a94a0f75763dc608c4698b5e4589871bd82d/chrome/browser/ui/ash/chrome_keyboard_ui.h
[modify] https://crrev.com/8a31a94a0f75763dc608c4698b5e4589871bd82d/chrome/browser/ui/ash/chrome_keyboard_ui_unittest.cc
[modify] https://crrev.com/8a31a94a0f75763dc608c4698b5e4589871bd82d/chrome/browser/ui/ash/chrome_keyboard_web_contents.cc
[modify] https://crrev.com/8a31a94a0f75763dc608c4698b5e4589871bd82d/chrome/browser/ui/ash/chrome_keyboard_web_contents.h
[modify] https://crrev.com/8a31a94a0f75763dc608c4698b5e4589871bd82d/chrome/browser/ui/ash/chrome_keyboard_web_contents_unittest.cc
[modify] https://crrev.com/8a31a94a0f75763dc608c4698b5e4589871bd82d/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/8a31a94a0f75763dc608c4698b5e4589871bd82d/ui/keyboard/keyboard_ui.cc
[modify] https://crrev.com/8a31a94a0f75763dc608c4698b5e4589871bd82d/ui/keyboard/keyboard_ui.h
[modify] https://crrev.com/8a31a94a0f75763dc608c4698b5e4589871bd82d/ui/keyboard/test/keyboard_test_util.cc
[modify] https://crrev.com/8a31a94a0f75763dc608c4698b5e4589871bd82d/ui/keyboard/test/keyboard_test_util.h

Project Member

Comment 24 by bugdroid1@chromium.org, Nov 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a6da6f60e35b001983300c3f3d67ee289fffe911

commit a6da6f60e35b001983300c3f3d67ee289fffe911
Author: Darren Shen <shend@chromium.org>
Date: Fri Nov 09 04:29:03 2018

[VK] Remove KeyboardControllerObserver::OnStateChanged.

The KeyboardController state machine is quite complicated (e.g.
transient blurs, background loading) and could be prone to change.
We should not be breaking encapsulation by exposing this implementation
detail to external classes.

We remove KeyboardControllerObserver::OnStateChanged. The only observers
of this event are observing when the keyboard is shown or hidden. This
can be done with the OnKeyboardVisibilityStateChanged event instead.

TBR=edcourtney@chromium.org

Bug: 845780
Change-Id: I19fe82dc97495d8df05c1155a42cea06d3de10c5
Reviewed-on: https://chromium-review.googlesource.com/c/1128661
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606732}
[modify] https://crrev.com/a6da6f60e35b001983300c3f3d67ee289fffe911/ash/login/ui/lock_contents_view.cc
[modify] https://crrev.com/a6da6f60e35b001983300c3f3d67ee289fffe911/ash/login/ui/lock_contents_view.h
[modify] https://crrev.com/a6da6f60e35b001983300c3f3d67ee289fffe911/ash/wm/workspace/workspace_layout_manager.cc
[modify] https://crrev.com/a6da6f60e35b001983300c3f3d67ee289fffe911/ash/wm/workspace/workspace_layout_manager.h
[modify] https://crrev.com/a6da6f60e35b001983300c3f3d67ee289fffe911/chrome/browser/ui/ash/keyboard_controller_browsertest.cc
[modify] https://crrev.com/a6da6f60e35b001983300c3f3d67ee289fffe911/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/a6da6f60e35b001983300c3f3d67ee289fffe911/ui/keyboard/keyboard_controller_observer.h
[modify] https://crrev.com/a6da6f60e35b001983300c3f3d67ee289fffe911/ui/keyboard/test/keyboard_test_util.cc
[modify] https://crrev.com/a6da6f60e35b001983300c3f3d67ee289fffe911/ui/keyboard/test/keyboard_test_util.h

Labels: Hotlist-DesktopUIValid Hotlist-DesktopUIChecked
**UI Mass triage**

Adding respective labels  for expert review.
Project Member

Comment 26 by bugdroid1@chromium.org, Nov 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0dbe34abf54d49cc608862847e8faf7d19ab0400

commit 0dbe34abf54d49cc608862847e8faf7d19ab0400
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Mon Nov 19 20:18:39 2018

KeyoardController: Use composition for InputMethodKeyboardController

This simplifies the public KeybardController interface and helps
prepare for supporting different InputMethodKeyboardController options
in Mash.

Bug: 845780
Change-Id: I8d857ca67c8ea57de76036ae1487abd9dbbbd91a
Reviewed-on: https://chromium-review.googlesource.com/c/1337287
Reviewed-by: Darren Shen <shend@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609412}
[modify] https://crrev.com/0dbe34abf54d49cc608862847e8faf7d19ab0400/chrome/browser/chromeos/input_method/input_method_manager_impl.cc
[modify] https://crrev.com/0dbe34abf54d49cc608862847e8faf7d19ab0400/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/0dbe34abf54d49cc608862847e8faf7d19ab0400/ui/keyboard/keyboard_controller.h

Project Member

Comment 27 by bugdroid1@chromium.org, Nov 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa612fd906b151e5d811756e2b3d6fc7239c56e0

commit fa612fd906b151e5d811756e2b3d6fc7239c56e0
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Mon Nov 19 20:19:35 2018

keyboard::KeyboardController cleanup for Mash

This CL removes a call to OnTextInputStateChanged from
UpdateInputMethodObserver which could trigger a hide immediately
after a show.

Bug: 845780
Change-Id: I57e49ef011f3d3199ff9b72120d6388ebacc0847
Reviewed-on: https://chromium-review.googlesource.com/c/1338232
Reviewed-by: Darren Shen <shend@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609413}
[modify] https://crrev.com/fa612fd906b151e5d811756e2b3d6fc7239c56e0/ui/keyboard/keyboard_controller.cc

Project Member

Comment 28 by bugdroid1@chromium.org, Nov 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1d355151cc33e7ea611fd8aa2789661b681b6773

commit 1d355151cc33e7ea611fd8aa2789661b681b6773
Author: Darren Shen <shend@chromium.org>
Date: Mon Nov 19 21:36:10 2018

[VK] Observe bounds changes for overscrolling, rather than notify.

Currently when keyboard bounds change, KeyboardController notifies
the ChromeKeyboardUI to update the viewports of web contents on the
screen. This adds cruft to the KeyboardUI interface and will pose
problems for mash as well.

Alternatively, we could flip the relationship and have something in
Chrome observing keyboard bounds changes. This simplifies the
KeyboardUI interface and integrates nicely with the existing mojo
set up.

Bug: 845780
Change-Id: I0be697351f52619863f93e2b42dc263cd9dbab82
Reviewed-on: https://chromium-review.googlesource.com/c/1328481
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609444}
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/ash/keyboard/ash_keyboard_controller.cc
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/ash/keyboard/ash_keyboard_controller.h
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/ash/keyboard/ash_keyboard_controller_unittest.cc
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/ash/keyboard/test_keyboard_ui.cc
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/ash/keyboard/test_keyboard_ui.h
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/ash/public/interfaces/keyboard_controller.mojom
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/chrome/browser/ui/ash/chrome_keyboard_bounds_observer.cc
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/chrome/browser/ui/ash/chrome_keyboard_bounds_observer.h
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/chrome/browser/ui/ash/chrome_keyboard_controller_client.cc
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/chrome/browser/ui/ash/chrome_keyboard_controller_client.h
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/chrome/browser/ui/ash/chrome_keyboard_ui.cc
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/chrome/browser/ui/ash/chrome_keyboard_ui.h
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/chrome/browser/ui/ash/chrome_keyboard_web_contents_unittest.cc
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/ui/keyboard/keyboard_ui.h
[modify] https://crrev.com/1d355151cc33e7ea611fd8aa2789661b681b6773/ui/keyboard/test/keyboard_test_util.h

Project Member

Comment 29 by bugdroid1@chromium.org, Jan 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/535bfd6524841e0a4cf9a4bc6e568570965ae968

commit 535bfd6524841e0a4cf9a4bc6e568570965ae968
Author: Darren Shen <shend@chromium.org>
Date: Wed Jan 09 08:22:42 2019

[VK] Don't accept null layout_delegates.

We used to accept null KeyboardLayoutDelegates, but to reduce complexity
we will enforce that layout delegates are not null. There's only one
use case for it and we replace it with a mock KeyboardLayoutDelegates
that does nothing.

Bug: 845780
Change-Id: Iffbe2e35365d4b85c2c6fca6dd30e6092d691e96
Reviewed-on: https://chromium-review.googlesource.com/c/1400922
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621074}
[modify] https://crrev.com/535bfd6524841e0a4cf9a4bc6e568570965ae968/ui/keyboard/BUILD.gn
[modify] https://crrev.com/535bfd6524841e0a4cf9a4bc6e568570965ae968/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/535bfd6524841e0a4cf9a4bc6e568570965ae968/ui/keyboard/keyboard_controller_unittest.cc
[modify] https://crrev.com/535bfd6524841e0a4cf9a4bc6e568570965ae968/ui/keyboard/keyboard_util_unittest.cc
[add] https://crrev.com/535bfd6524841e0a4cf9a4bc6e568570965ae968/ui/keyboard/test/test_keyboard_layout_delegate.h

Project Member

Comment 30 by bugdroid1@chromium.org, Jan 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dc1204ca18cc08b4babece3966c2f2927b10d8e9

commit dc1204ca18cc08b4babece3966c2f2927b10d8e9
Author: Darren Shen <shend@chromium.org>
Date: Thu Jan 10 01:29:27 2019

[VK] Clarify preconditions for ActivateKeyboardForRoot.

AshKeyboardController::ActivateKeyboardForRoot currently activates the
keyboard controller when the keyboard is enabled and the keyboard's root
window is not the primary root window.

However, the VK cannot be activated more than once (we DCHECK that
you can only activate on a deactivated VK). Thus, if the keyboard is
activated on a different root window and this code path executes, we
would hit a DCHECK failure.

We're going to assume that this DCHECK never actually happens in real
executions, so we change ActivateKeyboardForRoot to only activate the
controller when the keyboard's root window is null (i.e. when it is
deactivated).

This will be important when we enforce that enabled keyboards are
always activated.

The original logic for this came from [1].

[1] https://chromium-review.googlesource.com/c/chromium/src/+/851134/

Bug: 845780
Change-Id: Ie6ef952784afd37daf0d252a018c5a16a25c0083
Reviewed-on: https://chromium-review.googlesource.com/c/1404217
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621409}
[modify] https://crrev.com/dc1204ca18cc08b4babece3966c2f2927b10d8e9/ash/keyboard/ash_keyboard_controller.cc

Project Member

Comment 31 by bugdroid1@chromium.org, Jan 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e5451da18f116074ec9632199e7aae4350fb9e6f

commit e5451da18f116074ec9632199e7aae4350fb9e6f
Author: Darren Shen <shend@chromium.org>
Date: Fri Jan 11 00:11:42 2019

[VK] Remove redundant calls to DeactivateKeyboard.

As part of allowing KeyboardController to enable/disable itself (instead
of getting AshKeyboardController to do it), we need KeyboardController
to be able to activate/deactivate itself too.

One goal is to ensure that when the KeyboardController is enabled, it
is always activated (i.e. attached to a root window) and when the
controller is disabled, it is always deactivated (i.e. no root window).
This removes a lot of statefulness from the keyboard controller.

To achieve this, we need to prevent clients from arbitrarily calling
KeyboardController::DeactivateKeyboard. Fortunately, in almost all
cases, the DeactivateKeyboard call is redundant, except:

1. RootWindowController::CloseChildWindows needs to deactivate the
   keyboard in order to prevent the VK window from being deleted.
   For now, we'll add a separate special function for this*.

2. VirtualKeyboardController::MoveKeyboardToDisplayInternal needs to
   activate/deactivate in order to move the VK window across displays.
   This will be refactored later so that KeyboardController does
   the moving of the VK window.

We will make DeactivateKeyboard a private member in a follow up patch.

* Handling CloseChildWindows is tricky, because we don't want to end
  up a with a enabled keyboard with no root window. Hence, we will
  eventually make CloseChildWindows move the VK window somewhere else
  instead of detaching it completely. This assumes that there is always
  at least one RootWindowController, even when the device has no
  monitors.

Bug: 845780
Change-Id: I39dd7e09f81e5375b5ed9df25f832500ea25d30c
Reviewed-on: https://chromium-review.googlesource.com/c/1401827
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621816}
[modify] https://crrev.com/e5451da18f116074ec9632199e7aae4350fb9e6f/ash/keyboard/ash_keyboard_controller.cc
[modify] https://crrev.com/e5451da18f116074ec9632199e7aae4350fb9e6f/ash/keyboard/ash_keyboard_controller.h
[modify] https://crrev.com/e5451da18f116074ec9632199e7aae4350fb9e6f/ash/login/ui/login_keyboard_test_base.cc
[modify] https://crrev.com/e5451da18f116074ec9632199e7aae4350fb9e6f/ash/login/ui/login_keyboard_test_base.h
[modify] https://crrev.com/e5451da18f116074ec9632199e7aae4350fb9e6f/ash/root_window_controller.cc
[modify] https://crrev.com/e5451da18f116074ec9632199e7aae4350fb9e6f/ash/wm/always_on_top_controller_unittest.cc
[modify] https://crrev.com/e5451da18f116074ec9632199e7aae4350fb9e6f/ash/wm/lock_action_handler_layout_manager_unittest.cc
[modify] https://crrev.com/e5451da18f116074ec9632199e7aae4350fb9e6f/ash/wm/system_modal_container_layout_manager_unittest.cc

Project Member

Comment 32 by bugdroid1@chromium.org, Jan 17 (6 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/00f016e7247626471c628715550a3504db97f350

commit 00f016e7247626471c628715550a3504db97f350
Author: Darren Shen <shend@chromium.org>
Date: Thu Jan 17 04:59:19 2019

[VK] Let KeyboardController control activate/deactivate.

In order for KeyboardController to enable/disable by itself, it needs to
also be able to activate/deactivate by itself. This patch changes the
KeyboardLayoutDelegate interface so that KeyboardController does the
activate/deactivate, and the delegate just returns the appropriate
window to move to.

After this patch, whenever the KeyboardController is enabled, it is
guaranteed to be activated and when it is disabled, it is guaranteed to
be deactivated.

Bug: 845780
Change-Id: I73d02e2b258da32fcc22c78bdc56be9c583f3fec
Reviewed-on: https://chromium-review.googlesource.com/c/1401928
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623585}
[modify] https://crrev.com/00f016e7247626471c628715550a3504db97f350/ash/keyboard/ash_keyboard_controller.cc
[modify] https://crrev.com/00f016e7247626471c628715550a3504db97f350/ash/keyboard/ash_keyboard_controller.h
[modify] https://crrev.com/00f016e7247626471c628715550a3504db97f350/ash/keyboard/virtual_keyboard_controller.cc
[modify] https://crrev.com/00f016e7247626471c628715550a3504db97f350/ash/keyboard/virtual_keyboard_controller.h
[modify] https://crrev.com/00f016e7247626471c628715550a3504db97f350/ash/keyboard/virtual_keyboard_controller_unittest.cc
[modify] https://crrev.com/00f016e7247626471c628715550a3504db97f350/ash/login/ui/login_keyboard_test_base.cc
[modify] https://crrev.com/00f016e7247626471c628715550a3504db97f350/ash/wm/system_modal_container_layout_manager_unittest.cc
[modify] https://crrev.com/00f016e7247626471c628715550a3504db97f350/chrome/browser/ui/ash/keyboard/keyboard_controller_browsertest.cc
[modify] https://crrev.com/00f016e7247626471c628715550a3504db97f350/ui/keyboard/BUILD.gn
[modify] https://crrev.com/00f016e7247626471c628715550a3504db97f350/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/00f016e7247626471c628715550a3504db97f350/ui/keyboard/keyboard_controller.h
[modify] https://crrev.com/00f016e7247626471c628715550a3504db97f350/ui/keyboard/keyboard_controller_unittest.cc
[modify] https://crrev.com/00f016e7247626471c628715550a3504db97f350/ui/keyboard/keyboard_layout_delegate.h
[modify] https://crrev.com/00f016e7247626471c628715550a3504db97f350/ui/keyboard/keyboard_util_unittest.cc
[add] https://crrev.com/00f016e7247626471c628715550a3504db97f350/ui/keyboard/test/test_keyboard_layout_delegate.cc
[modify] https://crrev.com/00f016e7247626471c628715550a3504db97f350/ui/keyboard/test/test_keyboard_layout_delegate.h

Sign in to add a comment