vk: Delay notifying observers when VK is enabled / disabled.

When observers are notified about the VK being enabled / disabled, the
state of the KeyboardController at the time is wrong. For example,
when notified about disabling the VK, KeyboardController::IsEnabled()
still returns true (!).

We move the observer notification code to the correct place and add
tests to verify the state of the KeyboardController at the time of
notification.

Bug: 943446
Change-Id: I663042f743826afdb9880c538e6110ca13b4a6d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1572983
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#654787}

ui/keyboard/keyboard_controller.cc

@@ -288,15 +288,16 @@ void KeyboardController::EnableKeyboard() {
   time_of_last_blur_ = base::Time::UnixEpoch();
   UpdateInputMethodObserver();
 
-  for (KeyboardControllerObserver& observer : observer_list_)
-    observer.OnKeyboardEnabledChanged(true);
-
   ActivateKeyboardInContainer(
       layout_delegate_->GetContainerForDefaultDisplay());
 
   // Start preloading the virtual keyboard UI in the background, so that it
   // shows up faster when needed.
   LoadKeyboardWindowInBackground();
+
+  // Notify observers after the keyboard window has a root window.
+  for (KeyboardControllerObserver& observer : observer_list_)
+    observer.OnKeyboardEnabledChanged(true);
 }
 
 void KeyboardController::DisableKeyboard() {
@@ -323,10 +324,12 @@ void KeyboardController::DisableKeyboard() {
   animation_observer_.reset();
 
   ime_observer_.RemoveAll();
-  for (KeyboardControllerObserver& observer : observer_list_)
-    observer.OnKeyboardEnabledChanged(false);
   ui_->SetController(nullptr);
   ui_.reset();
+
+  // Notify observers after |ui_| is reset so that IsEnabled() is false.
+  for (KeyboardControllerObserver& observer : observer_list_)
+    observer.OnKeyboardEnabledChanged(false);
 }
 
 void KeyboardController::ActivateKeyboardInContainer(aura::Window* parent) {

ui/keyboard/keyboard_controller_unittest.cc

@@ -772,4 +772,55 @@ TEST_F(KeyboardControllerTest, DontClearObserverList) {
   EXPECT_TRUE(IsKeyboardDisabled());
 }
 
+class MockKeyboardControllerObserver : public KeyboardControllerObserver {
+ public:
+  MockKeyboardControllerObserver() = default;
+  ~MockKeyboardControllerObserver() override = default;
+
+  // KeyboardControllerObserver:
+  MOCK_METHOD1(OnKeyboardEnabledChanged, void(bool is_enabled));
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockKeyboardControllerObserver);
+};
+
+TEST_F(KeyboardControllerTest, OnKeyboardEnabledChangedToEnabled) {
+  // Start with the keyboard disabled.
+  keyboard::SetTouchKeyboardEnabled(false);
+
+  MockKeyboardControllerObserver mock_observer;
+  controller().AddObserver(&mock_observer);
+
+  EXPECT_CALL(mock_observer, OnKeyboardEnabledChanged(true))
+      .WillOnce(testing::InvokeWithoutArgs([]() {
+        auto* controller = keyboard::KeyboardController::Get();
+        ASSERT_TRUE(controller);
+        EXPECT_TRUE(controller->IsEnabled());
+        EXPECT_TRUE(controller->GetKeyboardWindow());
+        EXPECT_TRUE(controller->GetRootWindow());
+      }));
+
+  keyboard::SetTouchKeyboardEnabled(true);
+
+  controller().RemoveObserver(&mock_observer);
+}
+
+TEST_F(KeyboardControllerTest, OnKeyboardEnabledChangedToDisabled) {
+  MockKeyboardControllerObserver mock_observer;
+  controller().AddObserver(&mock_observer);
+
+  EXPECT_CALL(mock_observer, OnKeyboardEnabledChanged(false))
+      .WillOnce(testing::InvokeWithoutArgs([]() {
+        auto* controller = keyboard::KeyboardController::Get();
+        ASSERT_TRUE(controller);
+        EXPECT_FALSE(controller->IsEnabled());
+        EXPECT_FALSE(controller->GetKeyboardWindow());
+        EXPECT_FALSE(controller->GetRootWindow());
+      }));
+
+  keyboard::SetTouchKeyboardEnabled(false);
+
+  controller().RemoveObserver(&mock_observer);
+}
+
 }  // namespace keyboard