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

Issue 731537 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 6
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocking:
issue 845780
issue 616909


Participants' hotlists:
Fixing-touch


Sign in to add a comment

Simplify the lifetime of KeyboardController

Project Member Reported by yhanada@chromium.org, Jun 9 2017

Issue description

Spun off from http://crrev.com/2916653002.

Building/destroying only the views when keyboard is opened/hidden may make the code simpler.
 
Cc: jamescook@chromium.org

Comment 2 by oka@chromium.org, Jun 19 2017

Currently KeyboardController is recreated when session state changes to ACTIVE.
As far as I understand, it's just for associated the current profile to KeyboardUI [1]. I think in this case also recreating the view is enough. Should I file a separate bug for this?

[1] https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/chrome_shell_delegate.cc?type=cs&q=CreateKeyboardUI+profile&sq=package:chromium&l=599
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 11 2017

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

commit 8909a54471f168be1300c13aaecd2a2985e960ad
Author: yhanada <yhanada@chromium.org>
Date: Tue Jul 11 10:06:19 2017

Use std::unique_ptr for return type of ShellDelegate::CreateKeyboardUI.

Caller takes ownership of the created object. We should use
std::unique_ptr<> for it according to coding style guide.

Bug:  731537 
Test: Build passes.
Change-Id: Ia8f5887219ec8696ed66728de3e65c04d7aaadbe
Reviewed-on: https://chromium-review.googlesource.com/563278
Commit-Queue: Yuichiro Hanada <yhanada@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485575}
[modify] https://crrev.com/8909a54471f168be1300c13aaecd2a2985e960ad/ash/mus/shell_delegate_mus.cc
[modify] https://crrev.com/8909a54471f168be1300c13aaecd2a2985e960ad/ash/mus/shell_delegate_mus.h
[modify] https://crrev.com/8909a54471f168be1300c13aaecd2a2985e960ad/ash/shell.cc
[modify] https://crrev.com/8909a54471f168be1300c13aaecd2a2985e960ad/ash/shell/shell_delegate_impl.cc
[modify] https://crrev.com/8909a54471f168be1300c13aaecd2a2985e960ad/ash/shell/shell_delegate_impl.h
[modify] https://crrev.com/8909a54471f168be1300c13aaecd2a2985e960ad/ash/shell_delegate.h
[modify] https://crrev.com/8909a54471f168be1300c13aaecd2a2985e960ad/ash/test/test_shell_delegate.cc
[modify] https://crrev.com/8909a54471f168be1300c13aaecd2a2985e960ad/ash/test/test_shell_delegate.h
[modify] https://crrev.com/8909a54471f168be1300c13aaecd2a2985e960ad/chrome/browser/ui/ash/chrome_shell_delegate.cc
[modify] https://crrev.com/8909a54471f168be1300c13aaecd2a2985e960ad/chrome/browser/ui/ash/chrome_shell_delegate.h
[modify] https://crrev.com/8909a54471f168be1300c13aaecd2a2985e960ad/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/8909a54471f168be1300c13aaecd2a2985e960ad/ui/keyboard/keyboard_controller.h
[modify] https://crrev.com/8909a54471f168be1300c13aaecd2a2985e960ad/ui/keyboard/keyboard_controller_unittest.cc

Status: Started (was: Assigned)
Re #2: sorry for the belated reply. I will address that problem too.

I'm not sure if this is covered as part of the bug description, but it would be nice if the window container kShellWindowId_VirtualKeyboardContainer always existed, rather than being dynamically created and destroyed.

We've had many bugs in ash related to other containers with this create/destroy behavior. I think we had one for a while that only existed while dragging a window.

Thanks again for your efforts to improve this code.

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 14 2017

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

commit 1f619b96fcd162cada464ff0ab7883f7f3d21df2
Author: yhanada <yhanada@chromium.org>
Date: Fri Jul 14 04:35:51 2017

Clean up in ui/keyboard.

- Update obsolete comments
- Remove UMA recording in virtual_keyboard_private because it's
  recorded in HideKeyboard.
- Remove an unused method.
- Move FullWidthKeyboardBoundsFromRootBounds to keyboard_test_util.
- Mark some methods of KeyboardController const methods.

Bug:  731537 
Test: All tests pass.
Change-Id: I99b1956519465c805c4e8bee3debb41f1f194817
Reviewed-on: https://chromium-review.googlesource.com/568059
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Biao She <bshe@chromium.org>
Commit-Queue: Yuichiro Hanada <yhanada@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486674}
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/ash/BUILD.gn
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/ash/root_window_controller_unittest.cc
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/ash/wm/always_on_top_controller_unittest.cc
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/ash/wm/lock_action_handler_layout_manager_unittest.cc
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/ash/wm/lock_layout_manager_unittest.cc
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/ash/wm/system_modal_container_layout_manager_unittest.cc
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/ash/wm/workspace/workspace_layout_manager_keyboard_unittest.cc
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/chrome/browser/extensions/api/virtual_keyboard_private/chrome_virtual_keyboard_delegate.cc
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/ui/keyboard/BUILD.gn
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/ui/keyboard/keyboard_controller.h
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/ui/keyboard/keyboard_controller_unittest.cc
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/ui/keyboard/keyboard_test_util.cc
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/ui/keyboard/keyboard_test_util.h
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/ui/keyboard/keyboard_util.cc
[modify] https://crrev.com/1f619b96fcd162cada464ff0ab7883f7f3d21df2/ui/keyboard/keyboard_util.h

Blocking: 616909
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 4 2017

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

commit 0c5b078db1822183ca1a7bd35dd6c06a40a297db
Author: Yuichiro Hanada <yhanada@chromium.org>
Date: Mon Dec 04 08:05:32 2017

Don't recreate VirtualKeyboardContainer window when activating VK.

Always create VirtualKeyboardContainer instead of creating on the fly,
so we can register it as an ActivationParent correctly.
This change also deletes code that checks the existence of virtual
keyboard by the existence of the container window.

The location of the container has been changed from between
ImeWindowParentContainer & KeyboardContainer, to between
VirtualKeyboardParentContainer and KeyboardContainer.
The new hierarchy is:
RootWindow ->
ScreenRotationContainer ->
LockScreenRelatedContainersContainer ->
VirtualKeyboardParentContainer ->
VirtualKeyboardContainer ->
KeyboardContainer (which is owned by KeyboardController) ->
keyboard contents window.

Bug:  731537 ,  616909 
Test: Unit tests pass.
Change-Id: Ia9d04fe3ef9b2584dfbe1b785b498f8152fc398b
Reviewed-on: https://chromium-review.googlesource.com/771470
Commit-Queue: Yuichiro Hanada <yhanada@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521288}
[modify] https://crrev.com/0c5b078db1822183ca1a7bd35dd6c06a40a297db/ash/BUILD.gn
[modify] https://crrev.com/0c5b078db1822183ca1a7bd35dd6c06a40a297db/ash/public/cpp/shell_window_ids.h
[modify] https://crrev.com/0c5b078db1822183ca1a7bd35dd6c06a40a297db/ash/root_window_controller.cc
[modify] https://crrev.com/0c5b078db1822183ca1a7bd35dd6c06a40a297db/ash/root_window_controller.h
[modify] https://crrev.com/0c5b078db1822183ca1a7bd35dd6c06a40a297db/ash/root_window_controller_unittest.cc
[modify] https://crrev.com/0c5b078db1822183ca1a7bd35dd6c06a40a297db/ash/shell_unittest.cc
[add] https://crrev.com/0c5b078db1822183ca1a7bd35dd6c06a40a297db/ash/virtual_keyboard_container_layout_manager.cc
[add] https://crrev.com/0c5b078db1822183ca1a7bd35dd6c06a40a297db/ash/virtual_keyboard_container_layout_manager.h
[modify] https://crrev.com/0c5b078db1822183ca1a7bd35dd6c06a40a297db/ash/wm/always_on_top_controller_unittest.cc

Labels: M-66

Comment 11 by shend@chromium.org, Mar 19 2018

Hi Hanada-san, is there any more that needs to be done here?
Yes. Currently we recreate the instance of KeyboardController when prefs related to VK are updated. It caused several problems in observers of KeyboardController state. We might be able to clean up this by recreating only KeyboardUI inside KeyboardController when the prefs are updated and not destroying KeyboardController.
Labels: -M-66
Owner: oka@chromium.org
Status: Assigned (was: Started)

Comment 14 by shend@chromium.org, Apr 23 2018

Cc: oka@chromium.org
Owner: shend@chromium.org
oka-san is OOO, I'll try to finish this.
Project Member

Comment 15 by bugdroid1@chromium.org, Apr 26 2018

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

commit 09e09d32a6847b4743a1dcc289c2589cb747daf9
Author: Darren Shen <shend@chromium.org>
Date: Thu Apr 26 04:54:31 2018

[VK] Add a way to reload IME without destroying KeyboardController.

This patch introduces a new ash::Shell::ReloadKeyboard() method that
reloads the IME without recreating a KeyboardController. It destroys the
keyboard UI window instead and replaces it with a new one.

We use this for accessibility prefs. The accessibility VK and normal VK
are different, so when we switch between the two, we have to reload IME.

Bug:  731537 
Change-Id: I2388839206a48d81873f4bc6e7cf1ad2d0ed30f6
Reviewed-on: https://chromium-review.googlesource.com/1025532
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@{#553920}
[modify] https://crrev.com/09e09d32a6847b4743a1dcc289c2589cb747daf9/ash/accessibility/accessibility_controller.cc
[modify] https://crrev.com/09e09d32a6847b4743a1dcc289c2589cb747daf9/ash/root_window_controller.cc
[modify] https://crrev.com/09e09d32a6847b4743a1dcc289c2589cb747daf9/ash/shell.cc
[modify] https://crrev.com/09e09d32a6847b4743a1dcc289c2589cb747daf9/ash/shell.h
[modify] https://crrev.com/09e09d32a6847b4743a1dcc289c2589cb747daf9/chrome/browser/ui/ash/chrome_keyboard_ui.cc
[modify] https://crrev.com/09e09d32a6847b4743a1dcc289c2589cb747daf9/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/09e09d32a6847b4743a1dcc289c2589cb747daf9/ui/keyboard/keyboard_controller.h
[modify] https://crrev.com/09e09d32a6847b4743a1dcc289c2589cb747daf9/ui/keyboard/keyboard_controller_unittest.cc
[modify] https://crrev.com/09e09d32a6847b4743a1dcc289c2589cb747daf9/ui/keyboard/keyboard_layout_manager.cc
[modify] https://crrev.com/09e09d32a6847b4743a1dcc289c2589cb747daf9/ui/keyboard/keyboard_layout_manager.h

Project Member

Comment 16 by bugdroid1@chromium.org, Apr 29 2018

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

commit 0219ebefc1b05c41be9a4e109a14a013ebedbc2d
Author: Darren Shen <shend@chromium.org>
Date: Sun Apr 29 22:52:54 2018

[VK] Change calls to CreateKeyboard with ReloadKeyboard when appropriate

In a previous patch, we created a dedicated function for reloading the
VK without recreating the KeyboardController. This patch replaces all
the remaining calls of CreateKeyboard that reload the VK to
ReloadKeyboard.

Note that some calls to CreateKeyboard are to enable the keyboard,
so they will be replaced with a different function later.

Bug:  731537 
Change-Id: I2cf964d825433fb3fbeb19231e5c5757213a604f
Reviewed-on: https://chromium-review.googlesource.com/1031970
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@{#554677}
[modify] https://crrev.com/0219ebefc1b05c41be9a4e109a14a013ebedbc2d/ash/shell.cc
[modify] https://crrev.com/0219ebefc1b05c41be9a4e109a14a013ebedbc2d/chrome/browser/extensions/api/virtual_keyboard_private/chrome_virtual_keyboard_delegate.cc
[modify] https://crrev.com/0219ebefc1b05c41be9a4e109a14a013ebedbc2d/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc

Project Member

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

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

commit cfd147ee44254750001d33300472431bc4b6645d
Author: Darren Shen <shend@chromium.org>
Date: Thu May 03 09:24:44 2018

Revert "[VK] Add a way to reload IME without destroying KeyboardController."

This reverts commit 09e09d32a6847b4743a1dcc289c2589cb747daf9.

Reason for revert: borked  crbug.com/838803 

Original change's description:
> [VK] Add a way to reload IME without destroying KeyboardController.
> 
> This patch introduces a new ash::Shell::ReloadKeyboard() method that
> reloads the IME without recreating a KeyboardController. It destroys the
> keyboard UI window instead and replaces it with a new one.
> 
> We use this for accessibility prefs. The accessibility VK and normal VK
> are different, so when we switch between the two, we have to reload IME.
> 
> Bug:  731537 
> Change-Id: I2388839206a48d81873f4bc6e7cf1ad2d0ed30f6
> Reviewed-on: https://chromium-review.googlesource.com/1025532
> 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@{#553920}

TBR=jamescook@chromium.org,yhanada@chromium.org,dvallet@chromium.org,shend@chromium.org

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

Bug:  731537 
Change-Id: Ia8a9996e0c82db4506c587ec8ab937d5c81849ef
Reviewed-on: https://chromium-review.googlesource.com/1041445
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555681}
[modify] https://crrev.com/cfd147ee44254750001d33300472431bc4b6645d/ash/accessibility/accessibility_controller.cc
[modify] https://crrev.com/cfd147ee44254750001d33300472431bc4b6645d/ash/root_window_controller.cc
[modify] https://crrev.com/cfd147ee44254750001d33300472431bc4b6645d/ash/shell.cc
[modify] https://crrev.com/cfd147ee44254750001d33300472431bc4b6645d/ash/shell.h
[modify] https://crrev.com/cfd147ee44254750001d33300472431bc4b6645d/chrome/browser/extensions/api/virtual_keyboard_private/chrome_virtual_keyboard_delegate.cc
[modify] https://crrev.com/cfd147ee44254750001d33300472431bc4b6645d/chrome/browser/ui/ash/chrome_keyboard_ui.cc
[modify] https://crrev.com/cfd147ee44254750001d33300472431bc4b6645d/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
[modify] https://crrev.com/cfd147ee44254750001d33300472431bc4b6645d/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/cfd147ee44254750001d33300472431bc4b6645d/ui/keyboard/keyboard_controller.h
[modify] https://crrev.com/cfd147ee44254750001d33300472431bc4b6645d/ui/keyboard/keyboard_controller_unittest.cc
[modify] https://crrev.com/cfd147ee44254750001d33300472431bc4b6645d/ui/keyboard/keyboard_layout_manager.cc
[modify] https://crrev.com/cfd147ee44254750001d33300472431bc4b6645d/ui/keyboard/keyboard_layout_manager.h

Project Member

Comment 18 by bugdroid1@chromium.org, May 22 2018

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

commit a314c215094e774c6f1d44767fa37aa93f3358be
Author: Darren Shen <shend@chromium.org>
Date: Tue May 22 00:53:17 2018

[VK] Don't activate keyboard if keyboard is not enabled.

The code in Shell::CreateKeyboard() is a bit confusing. We always call
RootWindowController::ActivateKeyboard, even if the keyboard is not
enabled. The current code gives the false impression that calling
ActivateKeyboard when keyboard is not enabled actually does something,
whereas it's a no-op in reality.

This patch makes CreateKeyboard return early if the keyboard is not
enabled to remove this confusion.

Bug:  731537 
Change-Id: If34057bb947b0dd96894fc4598990cddf500ee42
Reviewed-on: https://chromium-review.googlesource.com/1036886
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560437}
[modify] https://crrev.com/a314c215094e774c6f1d44767fa37aa93f3358be/ash/shell.cc

Comment 19 by shend@chromium.org, May 23 2018

Blocking: 845780
Project Member

Comment 20 by bugdroid1@chromium.org, May 28 2018

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

commit ad5dc7290c4be91cdefcb4a4cad87095b4875c21
Author: Darren Shen <shend@chromium.org>
Date: Mon May 28 00:14:45 2018

[VK] Create a KeyboardController only once.

Currently, the global KeyboardController instance is created & destroyed
whenever the keyboard is enabled & disabled. This makes it difficult
to work with KeyboardController (e.g. we can't attach an observer to
the keyboard when it is in a disabled state).

This patch is the first of several patches to simplify the lifetime
of KeyboardController. This patch ensures that KeyboardController
is only created once. The difficulty here is that KeyboardController
has a destructor, which runs every time we disable the keyboard. Thus,
we need to move the destructor code to a separate function that is
called when we disable the keyboard. Similarly for the constructor.

Under the simplified lifetime, |ResetInstance(KeyboardController*)|
becomes |EnableKeyboard| and |ResetInstance(nullptr)| becomes
|DisableKeyboard|.

Another issue is that existing code check if KeyboardController is
null to determine whether the keyboard is enabled. A lot of the
existing code will then break because we will always be returning
a valid KeyboardController. Eventually, we need to change all the null
checks to checks for |enabled|. But to keep this patch small, we do
a trick where |GetInstance| still returns null for disabled keyboards.
This means all the existing code will continue to work as intended.

Unfortunately, that means ash::Shell can't enable the keyboard,
because we can't get a KeyboardController instance to call
|EnableKeyboard|. So we do a temporary workaround by adding an extra
parameter to |GetInstance| that returns a valid instance even when
the keyboard is disabled. This will be removed later.

Bug:  731537 
Change-Id: I0b7fb7ea656e96d057874b65a1174248fd0b810f
Reviewed-on: https://chromium-review.googlesource.com/1068277
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@{#562155}
[modify] https://crrev.com/ad5dc7290c4be91cdefcb4a4cad87095b4875c21/ash/shell.cc
[modify] https://crrev.com/ad5dc7290c4be91cdefcb4a4cad87095b4875c21/ash/shell.h
[modify] https://crrev.com/ad5dc7290c4be91cdefcb4a4cad87095b4875c21/ash/wm/workspace/workspace_layout_manager_unittest.cc
[modify] https://crrev.com/ad5dc7290c4be91cdefcb4a4cad87095b4875c21/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/ad5dc7290c4be91cdefcb4a4cad87095b4875c21/ui/keyboard/keyboard_controller.h
[modify] https://crrev.com/ad5dc7290c4be91cdefcb4a4cad87095b4875c21/ui/keyboard/keyboard_controller_unittest.cc
[modify] https://crrev.com/ad5dc7290c4be91cdefcb4a4cad87095b4875c21/ui/keyboard/keyboard_util_unittest.cc

Project Member

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

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

commit eb29c35590f6f06bae21de2a2f9805a2c440f900
Author: Darren Shen <shend@chromium.org>
Date: Wed Jun 06 03:27:18 2018

[VK] Change KeyboardController::GetInstance to never return null.

Currently, KeyboardController::GetInstance returns null if the keyboard
is disabled. This patch changes it so that |GetInstance| always returns
a valid KeyboardController.

However, existing code determines whether the keyboard is enabled by
checking whether KeyboardController is null or not. With this patch,
KeyboardController will never be null. So we have to replace the null
checks with calls to KeyboardController::enabled(). In some cases,
calls to KeyboardController::enabled() can be omitted, but we leave
that to a future patch for easier review.

Bug:  731537 
Change-Id: Ica487e69bae0a589d79c726f131611f0bf934672
Reviewed-on: https://chromium-review.googlesource.com/1032230
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564779}
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/accelerators/accelerator_controller.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/accessibility/accessibility_controller.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/accessibility/touch_exploration_manager.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/app_list/app_list_controller_impl.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/app_list/app_list_presenter_delegate_unittest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/keyboard/keyboard_observer_register.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/keyboard/keyboard_ui.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/login/ui/lock_contents_view.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/login/ui/login_keyboard_test_base.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/magnifier/magnification_controller.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/magnifier/magnification_controller_unittest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/root_window_controller.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/root_window_controller_unittest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/shelf/shelf_layout_manager_unittest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/shell.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/shell.h
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/shell_unittest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/system/ime_menu/ime_menu_tray.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/system/virtual_keyboard/virtual_keyboard_tray.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/virtual_keyboard_controller.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/virtual_keyboard_controller_unittest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/wm/always_on_top_controller_unittest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/wm/lock_action_handler_layout_manager_unittest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/wm/lock_layout_manager.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/wm/lock_layout_manager_unittest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/wm/lock_window_state.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/wm/system_modal_container_layout_manager.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/wm/system_modal_container_layout_manager_unittest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ash/wm/workspace/workspace_layout_manager_unittest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/chrome/browser/chromeos/extensions/input_method_api.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/chrome/browser/chromeos/input_method/input_method_engine.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/chrome/browser/chromeos/input_method/input_method_engine_unittest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/chrome/browser/chromeos/input_method/input_method_manager_impl.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/chrome/browser/chromeos/login/app_launch_controller.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/chrome/browser/chromeos/login/ui/webui_login_view.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/chrome/browser/extensions/api/omnibox/omnibox_api_testbase.h
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/chrome/browser/extensions/api/virtual_keyboard_private/chrome_virtual_keyboard_delegate.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/chrome/browser/ui/ash/keyboard_controller_browsertest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/chrome/browser/ui/ash/keyboard_end_to_end_browsertest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/components/arc/BUILD.gn
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/components/arc/ime/DEPS
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/components/arc/ime/arc_ime_service.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/components/arc/ime/arc_ime_service.h
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/components/arc/ime/arc_ime_service_unittest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ui/app_list/BUILD.gn
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ui/app_list/views/app_list_folder_view.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ui/app_list/views/app_list_view.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ui/app_list/views/app_list_view_unittest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ui/app_list/views/apps_grid_view_unittest.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ui/app_list/views/search_box_view.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ui/keyboard/keyboard_controller.h
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ui/keyboard/keyboard_event_filter.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ui/keyboard/keyboard_test_util.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ui/keyboard/keyboard_util.cc
[modify] https://crrev.com/eb29c35590f6f06bae21de2a2f9805a2c440f900/ui/keyboard/keyboard_util_unittest.cc

Project Member

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

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

commit 2c1e38af69bf0f4deb7380692486b90129525226
Author: Alexander Potapenko <glider@chromium.org>
Date: Wed Jun 06 10:09:22 2018

Revert "[VK] Change KeyboardController::GetInstance to never return null."

This reverts commit eb29c35590f6f06bae21de2a2f9805a2c440f900.

Reason for revert: broke CrOS MSan bots:  crbug.com/850020 

BUG= 850020 

Original change's description:
> [VK] Change KeyboardController::GetInstance to never return null.
> 
> Currently, KeyboardController::GetInstance returns null if the keyboard
> is disabled. This patch changes it so that |GetInstance| always returns
> a valid KeyboardController.
> 
> However, existing code determines whether the keyboard is enabled by
> checking whether KeyboardController is null or not. With this patch,
> KeyboardController will never be null. So we have to replace the null
> checks with calls to KeyboardController::enabled(). In some cases,
> calls to KeyboardController::enabled() can be omitted, but we leave
> that to a future patch for easier review.
> 
> Bug:  731537 
> Change-Id: Ica487e69bae0a589d79c726f131611f0bf934672
> Reviewed-on: https://chromium-review.googlesource.com/1032230
> Commit-Queue: Darren Shen <shend@chromium.org>
> Reviewed-by: Yusuke Sato <yusukes@chromium.org>
> Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: James Cook <jamescook@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#564779}

TBR=jamescook@chromium.org,yusukes@chromium.org,sky@chromium.org,yhanada@chromium.org,shend@chromium.org

Change-Id: I6bf00360e30529ae5f3a1ac4d8a70efbb169e958
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  731537 
Reviewed-on: https://chromium-review.googlesource.com/1088567
Reviewed-by: Alexander Potapenko <glider@chromium.org>
Commit-Queue: Alexander Potapenko <glider@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564842}
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/accelerators/accelerator_controller.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/accessibility/accessibility_controller.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/accessibility/touch_exploration_manager.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/app_list/app_list_controller_impl.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/app_list/app_list_presenter_delegate_unittest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/keyboard/keyboard_observer_register.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/keyboard/keyboard_ui.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/login/ui/lock_contents_view.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/login/ui/login_keyboard_test_base.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/magnifier/magnification_controller.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/magnifier/magnification_controller_unittest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/root_window_controller.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/root_window_controller_unittest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/shelf/shelf_layout_manager_unittest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/shell.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/shell.h
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/shell_unittest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/system/ime_menu/ime_menu_tray.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/system/virtual_keyboard/virtual_keyboard_tray.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/virtual_keyboard_controller.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/virtual_keyboard_controller_unittest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/wm/always_on_top_controller_unittest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/wm/lock_action_handler_layout_manager_unittest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/wm/lock_layout_manager.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/wm/lock_layout_manager_unittest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/wm/lock_window_state.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/wm/system_modal_container_layout_manager.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/wm/system_modal_container_layout_manager_unittest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ash/wm/workspace/workspace_layout_manager_unittest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/chrome/browser/chromeos/extensions/input_method_api.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/chrome/browser/chromeos/input_method/input_method_engine.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/chrome/browser/chromeos/input_method/input_method_engine_unittest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/chrome/browser/chromeos/input_method/input_method_manager_impl.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/chrome/browser/chromeos/login/app_launch_controller.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/chrome/browser/chromeos/login/ui/webui_login_view.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/chrome/browser/extensions/api/omnibox/omnibox_api_testbase.h
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/chrome/browser/extensions/api/virtual_keyboard_private/chrome_virtual_keyboard_delegate.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/chrome/browser/ui/ash/keyboard_controller_browsertest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/chrome/browser/ui/ash/keyboard_end_to_end_browsertest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/components/arc/BUILD.gn
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/components/arc/ime/DEPS
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/components/arc/ime/arc_ime_service.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/components/arc/ime/arc_ime_service.h
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/components/arc/ime/arc_ime_service_unittest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ui/app_list/BUILD.gn
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ui/app_list/views/app_list_folder_view.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ui/app_list/views/app_list_view.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ui/app_list/views/app_list_view_unittest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ui/app_list/views/apps_grid_view_unittest.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ui/app_list/views/search_box_view.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ui/keyboard/keyboard_controller.h
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ui/keyboard/keyboard_event_filter.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ui/keyboard/keyboard_test_util.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ui/keyboard/keyboard_util.cc
[modify] https://crrev.com/2c1e38af69bf0f4deb7380692486b90129525226/ui/keyboard/keyboard_util_unittest.cc

Project Member

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

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

commit f62db77afbd51cc991da0d18b826fdca4f8b0552
Author: Darren Shen <shend@chromium.org>
Date: Thu Jun 07 06:22:44 2018

Reland "[VK] Change KeyboardController::GetInstance to never return null."

This is a reland of eb29c35590f6f06bae21de2a2f9805a2c440f900

Had to change InputMethodManagerImpl::GetInputMethodKeyboardController
to return nullptr when the keyboard is disabled. Seems like the memory
error was from using a disabled keyboard.

TBR=jamescook@chromium.org, yusukes@chromium.org, sky@chromium.org, yhanada@chromium.org

Original change's description:
> [VK] Change KeyboardController::GetInstance to never return null.
>
> Currently, KeyboardController::GetInstance returns null if the keyboard
> is disabled. This patch changes it so that |GetInstance| always returns
> a valid KeyboardController.
>
> However, existing code determines whether the keyboard is enabled by
> checking whether KeyboardController is null or not. With this patch,
> KeyboardController will never be null. So we have to replace the null
> checks with calls to KeyboardController::enabled(). In some cases,
> calls to KeyboardController::enabled() can be omitted, but we leave
> that to a future patch for easier review.
>
> Bug:  731537 
> Change-Id: Ica487e69bae0a589d79c726f131611f0bf934672
> Reviewed-on: https://chromium-review.googlesource.com/1032230
> Commit-Queue: Darren Shen <shend@chromium.org>
> Reviewed-by: Yusuke Sato <yusukes@chromium.org>
> Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: James Cook <jamescook@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#564779}

Bug:  731537 
Change-Id: Ifea3cea2c801c27e0d1182c2b3f02b788cd24fe0
Reviewed-on: https://chromium-review.googlesource.com/1089950
Reviewed-by: Darren Shen <shend@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565201}
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/accelerators/accelerator_controller.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/accessibility/accessibility_controller.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/accessibility/touch_exploration_manager.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/app_list/app_list_controller_impl.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/app_list/app_list_presenter_delegate_unittest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/keyboard/keyboard_observer_register.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/keyboard/keyboard_ui.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/login/ui/lock_contents_view.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/login/ui/login_keyboard_test_base.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/magnifier/magnification_controller.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/magnifier/magnification_controller_unittest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/root_window_controller.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/root_window_controller_unittest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/shelf/shelf_layout_manager_unittest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/shell.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/shell.h
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/shell_unittest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/system/ime_menu/ime_menu_tray.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/system/virtual_keyboard/virtual_keyboard_tray.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/virtual_keyboard_controller.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/virtual_keyboard_controller_unittest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/wm/always_on_top_controller_unittest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/wm/lock_action_handler_layout_manager_unittest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/wm/lock_layout_manager.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/wm/lock_layout_manager_unittest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/wm/lock_window_state.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/wm/system_modal_container_layout_manager.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/wm/system_modal_container_layout_manager_unittest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ash/wm/workspace/workspace_layout_manager_unittest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/chrome/browser/chromeos/extensions/input_method_api.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/chrome/browser/chromeos/input_method/input_method_engine.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/chrome/browser/chromeos/input_method/input_method_engine_unittest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/chrome/browser/chromeos/input_method/input_method_manager_impl.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/chrome/browser/chromeos/login/app_launch_controller.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/chrome/browser/chromeos/login/ui/webui_login_view.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/chrome/browser/extensions/api/omnibox/omnibox_api_testbase.h
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/chrome/browser/extensions/api/virtual_keyboard_private/chrome_virtual_keyboard_delegate.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/chrome/browser/ui/ash/keyboard_controller_browsertest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/chrome/browser/ui/ash/keyboard_end_to_end_browsertest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/components/arc/BUILD.gn
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/components/arc/ime/DEPS
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/components/arc/ime/arc_ime_service.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/components/arc/ime/arc_ime_service.h
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/components/arc/ime/arc_ime_service_unittest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ui/app_list/BUILD.gn
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ui/app_list/views/app_list_folder_view.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ui/app_list/views/app_list_view.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ui/app_list/views/app_list_view_unittest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ui/app_list/views/apps_grid_view_unittest.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ui/app_list/views/search_box_view.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ui/keyboard/keyboard_controller.h
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ui/keyboard/keyboard_event_filter.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ui/keyboard/keyboard_test_util.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ui/keyboard/keyboard_util.cc
[modify] https://crrev.com/f62db77afbd51cc991da0d18b826fdca4f8b0552/ui/keyboard/keyboard_util_unittest.cc

Project Member

Comment 24 by bugdroid1@chromium.org, Jun 17 2018

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

commit 6017edbbf558d8d85363f3020d788d58039f930d
Author: Darren Shen <shend@chromium.org>
Date: Sun Jun 17 22:50:12 2018

[VK] Remove unnecessary observers of OnVirtualKeyboardStateChanged.

Some classes listen to ShellObserver::OnVirtualKeyboardStateChanged in
order to add/remove KeyboardController observer when the keyboard is
enabled/disabled. However, with the simplified KeyboardController
lifetimes, we can add/remove observers whenever.

So we just add an observer in the constructor and remove it in the
destructor.

After this change, there will be no more observers of
OnVirtualKeyboardStateChanged.

Bug:  731537 
Change-Id: I7274484229ff5f64ac973806e565f178a271094f
Reviewed-on: https://chromium-review.googlesource.com/1101598
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567918}
[modify] https://crrev.com/6017edbbf558d8d85363f3020d788d58039f930d/ash/app_list/app_list_controller_impl.cc
[modify] https://crrev.com/6017edbbf558d8d85363f3020d788d58039f930d/ash/app_list/app_list_controller_impl.h
[modify] https://crrev.com/6017edbbf558d8d85363f3020d788d58039f930d/ash/root_window_controller.cc
[modify] https://crrev.com/6017edbbf558d8d85363f3020d788d58039f930d/ash/shell.cc
[modify] https://crrev.com/6017edbbf558d8d85363f3020d788d58039f930d/ash/shell.h
[modify] https://crrev.com/6017edbbf558d8d85363f3020d788d58039f930d/ash/shell_observer.h
[modify] https://crrev.com/6017edbbf558d8d85363f3020d788d58039f930d/chrome/browser/chromeos/login/ui/webui_login_view.cc
[modify] https://crrev.com/6017edbbf558d8d85363f3020d788d58039f930d/chrome/browser/chromeos/login/ui/webui_login_view.h

Project Member

Comment 25 by bugdroid1@chromium.org, Jun 29 2018

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

commit 27bd1f258ee86989474682832e7db43762ee1739
Author: Darren Shen <shend@chromium.org>
Date: Fri Jun 29 02:53:12 2018

[VK] Only register observer once in VirtualKeyboardController.

Bug:  731537 
Change-Id: I9383c7a646cb12a4136cc1ac8999fbdb4958196d
Reviewed-on: https://chromium-review.googlesource.com/1111490
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571369}
[modify] https://crrev.com/27bd1f258ee86989474682832e7db43762ee1739/ash/keyboard/virtual_keyboard_controller.cc

Status: Fixed (was: Assigned)

Sign in to add a comment