Get rid of KeyboardController::container_ |
||
Issue description
The current window hierarchy for keyboard looks like this:
KeyboardContainer (ash container)
KeyboardContainer (KeyboardController container)
WebContents (KeyboardUI window)
The KeyboardController KeyboardContainer does not seem to do much. Let's see if we can remove it and just have the WebContents directly attached to the ash KeyboardContainer. This will simplify KeyboardController a lot.
,
Jun 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1a18c3f0a0728393139bc10efdf7b866c61fd605 commit 1a18c3f0a0728393139bc10efdf7b866c61fd605 Author: Darren Shen <shend@chromium.org> Date: Thu Jun 14 14:05:53 2018 [VK] Use semantically-correct bounds when querying keyboard bounds. There are several places where we query the keyboard bounds by grabbing the keyboard container window and calling |bounds|. However, it's unclear whether they mean the bounds relative to parent or bounds relative to the screen, since both are currently the same for the keyboard container. This patch changes several callers to use |GetBoundsInScreen| since they actually want the screen bounds. Note we don't change any behaviour since currently keyboard container |bounds| == |GetBoundsInScreen|, but this will change in future when we get rid of keyboard container. Bug: 849980 Change-Id: I7211073a7697f168824c127319dfc40e6888195a Reviewed-on: https://chromium-review.googlesource.com/1098417 Commit-Queue: James Cook <jamescook@chromium.org> Reviewed-by: James Cook <jamescook@chromium.org> Reviewed-by: Yuichiro Hanada <yhanada@chromium.org> Cr-Commit-Position: refs/heads/master@{#567253} [modify] https://crrev.com/1a18c3f0a0728393139bc10efdf7b866c61fd605/ash/magnifier/magnification_controller.cc [modify] https://crrev.com/1a18c3f0a0728393139bc10efdf7b866c61fd605/chrome/browser/ui/ash/chrome_keyboard_ui.cc [modify] https://crrev.com/1a18c3f0a0728393139bc10efdf7b866c61fd605/ui/keyboard/container_behavior.h [modify] https://crrev.com/1a18c3f0a0728393139bc10efdf7b866c61fd605/ui/keyboard/container_floating_behavior.cc [modify] https://crrev.com/1a18c3f0a0728393139bc10efdf7b866c61fd605/ui/keyboard/container_floating_behavior.h
,
Jun 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/699b7238fa0123cf044d0e7016e565f73c8a99b7 commit 699b7238fa0123cf044d0e7016e565f73c8a99b7 Author: Darren Shen <shend@chromium.org> Date: Wed Jun 20 22:56:16 2018 [VK] Use KeyboardUI contents window in VirtualKeyboardController. In VirtualKeyboardController, we query the keyboard container window to determine which display it is on. As we're getting rid of the container, we need to query the keyboard contents window instead. This should be fine as keyboard contents always has the same bounds as the container. Since we now need keyboard contents window for VirtualKeyboardController / KeyboardLayoutDelegate, we need to create the keyboard contents window before calling the layout delegate in |PopulateKeyboardContents|. Bug: 849980 Change-Id: I3f39358f8d500df0e864a5e88c526ac7a220b467 Reviewed-on: https://chromium-review.googlesource.com/1096586 Commit-Queue: Darren Shen <shend@chromium.org> Reviewed-by: James Cook <jamescook@chromium.org> Reviewed-by: Yuichiro Hanada <yhanada@chromium.org> Cr-Commit-Position: refs/heads/master@{#569065} [modify] https://crrev.com/699b7238fa0123cf044d0e7016e565f73c8a99b7/ash/keyboard/virtual_keyboard_controller.cc [modify] https://crrev.com/699b7238fa0123cf044d0e7016e565f73c8a99b7/ui/keyboard/keyboard_controller.cc [modify] https://crrev.com/699b7238fa0123cf044d0e7016e565f73c8a99b7/ui/keyboard/keyboard_controller.h
,
Jun 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7c24b7ab9edb33137653d82405d60d147d2c8ca7 commit 7c24b7ab9edb33137653d82405d60d147d2c8ca7 Author: Darren Shen <shend@chromium.org> Date: Wed Jun 20 23:14:33 2018 [VK] Add activate/deactivate methods to KeyboardController. As part of initialisation, RootWindowController 'activates' the KeyboardController by adding its container window as a child of another window (the "real" ash keyboard container). KeyboardController then observes the parent change and runs some code. We flip this around by having KeyboardController activate itself on a given window. Change-Id: I9df13eda82e96ab7c7463fe218719ec1f2f7ef8f Bug: 849980 Reviewed-on: https://chromium-review.googlesource.com/1098751 Commit-Queue: Darren Shen <shend@chromium.org> Reviewed-by: James Cook <jamescook@chromium.org> Cr-Commit-Position: refs/heads/master@{#569076} [modify] https://crrev.com/7c24b7ab9edb33137653d82405d60d147d2c8ca7/ash/root_window_controller.cc [modify] https://crrev.com/7c24b7ab9edb33137653d82405d60d147d2c8ca7/ui/keyboard/keyboard_controller.cc [modify] https://crrev.com/7c24b7ab9edb33137653d82405d60d147d2c8ca7/ui/keyboard/keyboard_controller.h [modify] https://crrev.com/7c24b7ab9edb33137653d82405d60d147d2c8ca7/ui/keyboard/keyboard_controller_unittest.cc
,
Jun 21 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/78af93c86adc18b9aa1f5647954b1e9c1af3f500 commit 78af93c86adc18b9aa1f5647954b1e9c1af3f500 Author: Darren Shen <shend@chromium.org> Date: Thu Jun 21 03:21:55 2018 [VK] Remove KeyboardController::container_. We replace instances of KeyboardController::container_ with the keyboard UI contents window. In most cases, we can directly replace them, but there are still some special considerations: - Container used to be created in the ctor, but Contents is created when we load the keyboard UI. So some operations used to be valid when we haven't loaded the UI, but now are not valid. In practice, this doesn't matter as we always preload the keyboard UI when we enable it. However, we should still figure out a sensible lifetime for the keyboard UI. - KeyboardLayoutManager used to be attached to the (old) Container, where we could guarantee that the only child is the contents window. Now it's attached to the (real) container, where we might want to have multiple children in the future. So some changes need to be made there. Change-Id: Ic2b42523565c2acf186ae66f43837ca28888be41 Bug: 849980 Reviewed-on: https://chromium-review.googlesource.com/1098753 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@{#569144} [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/ash/magnifier/magnification_controller.cc [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/ash/magnifier/magnification_controller_unittest.cc [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/ash/root_window_controller.cc [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/ash/root_window_controller_unittest.cc [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/ash/wm/always_on_top_controller_unittest.cc [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/ash/wm/workspace/workspace_layout_manager_unittest.cc [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/chrome/browser/extensions/api/virtual_keyboard_private/chrome_virtual_keyboard_delegate.cc [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/chrome/browser/ui/ash/chrome_keyboard_ui.cc [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/chrome/browser/ui/ash/keyboard_controller_browsertest.cc [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/ui/keyboard/container_floating_behavior.cc [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/ui/keyboard/keyboard_controller.cc [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/ui/keyboard/keyboard_controller.h [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/ui/keyboard/keyboard_controller_unittest.cc [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/ui/keyboard/keyboard_layout_manager.cc [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/ui/keyboard/keyboard_layout_manager.h [modify] https://crrev.com/78af93c86adc18b9aa1f5647954b1e9c1af3f500/ui/keyboard/keyboard_test_util.cc
,
Jun 21 2018
The last CL seems to have broken keyboard_unittests - https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-rel/9688 [ RUN ] KeyboardControllerAnimationTest.FloatingKeyboardEnsureCaretInWorkArea [20580:20580:0620/215026.354445:22158865387:ERROR:input_controller.cc(104)] Not implemented reached in virtual void ui::(anonymous namespace)::StubInputController::SetTouchEventLoggingEnabled(bool) [20580:20580:0620/215026.354536:22158865454:ERROR:input_controller.cc(104)] Not implemented reached in virtual void ui::(anonymous namespace)::StubInputController::SetTouchEventLoggingEnabled(bool) [20580:20580:0620/215026.354637:22158865556:ERROR:input_controller.cc(104)] Not implemented reached in virtual void ui::(anonymous namespace)::StubInputController::SetTouchEventLoggingEnabled(bool) Received signal 11 SEGV_MAPERR 0000000000c9 #0 0x0000007edcbc base::debug::StackTrace::StackTrace() #1 0x0000007ed831 base::debug::(anonymous namespace)::StackDumpSignalHandler() #2 0x7f8e3102d330 <unknown> #3 0x000000ebfb51 keyboard::KeyboardUI::EnsureCaretInWorkArea() #4 0x0000007a7728 keyboard::KeyboardController::HideKeyboard() #5 0x0000007a6db1 keyboard::KeyboardController::DeactivateKeyboard() #6 0x0000007a686c keyboard::KeyboardController::DisableKeyboard() #7 0x00000041dc2f keyboard::KeyboardControllerTest::TearDown() #8 0x0000007529a0 testing::TestInfo::Run() #9 0x000000752eb7 testing::TestCase::Run() #10 0x00000075e397 testing::internal::UnitTestImpl::RunAllTests() #11 0x00000075df0d testing::UnitTest::Run() #12 0x000000806ba1 base::TestSuite::Run() #13 0x00000080827c base::(anonymous namespace)::LaunchUnitTestsInternal() #14 0x000000808130 base::LaunchUnitTests() #15 0x0000004264c7 main #16 0x7f8e2e2d8f45 __libc_start_main #17 0x0000004124ca _start r8: 00000dd60eb55900 r9: 0000000000000001 r10: 0000000000000001 r11: 0000000000050001 r12: 00007ffd89689110 r13: 00007ffd89689170 r14: 00007ffd89689170 r15: 0000000000000000 di: 00007ffd89689148 si: 00007ffd89689170 bp: 00007ffd896890b0 bx: 00000dd60ea6ca10 dx: 0000000000000000 ax: 00007ffd89689148 cx: 0000000000000001 sp: 00007ffd89689020 ip: 0000000000ebfb51 efl: 0000000000010206 cgf: 0000000000000033 erf: 0000000000000004 trp: 000000000000000e msk: 0000000000000000 cr2: 00000000000000c9 [end of stack trace] Calling _exit(1). Core file will not be generated.
,
Jun 21 2018
Ah, nevermind, now I saw issue 854918 and that you already have a fix.
,
Jun 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/beffbcf2fc115386ffbf03158f2c70498f679fc9 commit beffbcf2fc115386ffbf03158f2c70498f679fc9 Author: Darren Shen <shend@chromium.org> Date: Wed Jun 27 03:18:02 2018 [VK] Install pre-event handler for dragging. Forgot to install event handler to handle dragging, which meant dragging stopped working (on mouse). Dragging still worked on touch because IME still has code that handles touch dragging ¯\_(ツ)_/¯ Bug: 849980 Change-Id: Ic19aa886e23bd5f944b41ba066b94e26eed9c99a Reviewed-on: https://chromium-review.googlesource.com/1114562 Reviewed-by: Yuichiro Hanada <yhanada@chromium.org> Reviewed-by: Darren Shen <shend@chromium.org> Commit-Queue: Darren Shen <shend@chromium.org> Cr-Commit-Position: refs/heads/master@{#570651} [modify] https://crrev.com/beffbcf2fc115386ffbf03158f2c70498f679fc9/chrome/browser/ui/ash/keyboard_controller_browsertest.cc [modify] https://crrev.com/beffbcf2fc115386ffbf03158f2c70498f679fc9/ui/keyboard/keyboard_controller.cc
,
Jul 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b18c59fe9b3acc908b484c0fdbcd43ee038bc632 commit b18c59fe9b3acc908b484c0fdbcd43ee038bc632 Author: Darren Shen <shend@chromium.org> Date: Tue Jul 03 01:31:24 2018 [VK] Naming cleanup. After https://crrev.com/c/1098753, "container" now means the parent (ash) container of the virtual keyboard. Thus, there's no more ambiguity between the keyboard "container" vs "contents" window, so we can just refer to the "contents" window as the "keyboard window". Bug: 849980 Change-Id: Iddaca77b8bf37559db75efdedc6a9276491c453d Reviewed-on: https://chromium-review.googlesource.com/1119610 Reviewed-by: Yuichiro Hanada <yhanada@chromium.org> Reviewed-by: James Cook <jamescook@chromium.org> Commit-Queue: Darren Shen <shend@chromium.org> Cr-Commit-Position: refs/heads/master@{#572073} [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ash/app_list/app_list_presenter_delegate_unittest.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ash/keyboard/test_keyboard_ui.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ash/keyboard/test_keyboard_ui.h [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ash/keyboard/virtual_keyboard_controller.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ash/login/ui/login_keyboard_test_base.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ash/magnifier/magnification_controller.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ash/magnifier/magnification_controller_unittest.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ash/root_window_controller.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ash/root_window_controller_unittest.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ash/wm/always_on_top_controller_unittest.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ash/wm/lock_action_handler_layout_manager_unittest.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ash/wm/lock_layout_manager_unittest.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ash/wm/system_modal_container_layout_manager_unittest.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ash/wm/workspace/workspace_layout_manager_unittest.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/chrome/browser/extensions/api/virtual_keyboard_private/chrome_virtual_keyboard_delegate.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/chrome/browser/ui/ash/chrome_keyboard_ui.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/chrome/browser/ui/ash/chrome_keyboard_ui.h [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/chrome/browser/ui/ash/chrome_keyboard_ui_unittest.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/chrome/browser/ui/ash/keyboard_controller_browsertest.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ui/keyboard/container_floating_behavior.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ui/keyboard/keyboard_controller.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ui/keyboard/keyboard_controller.h [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ui/keyboard/keyboard_controller_unittest.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ui/keyboard/keyboard_layout_manager.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ui/keyboard/keyboard_ui.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ui/keyboard/keyboard_ui.h [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ui/keyboard/test/keyboard_test_util.cc [modify] https://crrev.com/b18c59fe9b3acc908b484c0fdbcd43ee038bc632/ui/keyboard/test/keyboard_test_util.h
,
Jul 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/68777662a8218ec4318b8ade62f67a5993fa9281 commit 68777662a8218ec4318b8ade62f67a5993fa9281 Author: Darren Shen <shend@chromium.org> Date: Tue Jul 03 06:20:59 2018 [VK] Follow up on naming cleanup. In 1119610, there were some comments that were addressed, but the updated patch was not uploaded by mistake. This patch just addresses those comments. TBR=yhanada@chromium.org, jamescook@chromium.org Bug: 849980 Change-Id: Ida53707785789fda4b8ee687198932dd266477fb Reviewed-on: https://chromium-review.googlesource.com/1124074 Commit-Queue: Darren Shen <shend@chromium.org> Reviewed-by: Darren Shen <shend@chromium.org> Cr-Commit-Position: refs/heads/master@{#572125} [modify] https://crrev.com/68777662a8218ec4318b8ade62f67a5993fa9281/ash/root_window_controller_unittest.cc [modify] https://crrev.com/68777662a8218ec4318b8ade62f67a5993fa9281/ui/keyboard/keyboard_controller.h [modify] https://crrev.com/68777662a8218ec4318b8ade62f67a5993fa9281/ui/keyboard/keyboard_ui.cc [modify] https://crrev.com/68777662a8218ec4318b8ade62f67a5993fa9281/ui/keyboard/keyboard_ui.h
,
Aug 6
Seems stable enough to close. |
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Jun 8 2018