Simplify VirtualKeyboardTray

ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc

Index: ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc
diff --git a/ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc b/ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc
index 5e2391ff8370638d3d2e99f623ed125764b3276c..4edd01d809c6b6e2ce979b4bc43258ad512e0fed 100644
--- a/ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc
+++ b/ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc
@@ -21,60 +21,46 @@
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/gfx/vector_icons_public.h"
-#include "ui/views/controls/button/image_button.h"
+#include "ui/keyboard/keyboard_controller.h"
+#include "ui/views/controls/image_view.h"
 
 namespace ash {
 
 VirtualKeyboardTray::VirtualKeyboardTray(WmShelf* wm_shelf)
-    : TrayBackgroundView(wm_shelf), button_(nullptr) {
-  button_ = new views::ImageButton(this);
+    : TrayBackgroundView(wm_shelf), icon_(new views::ImageView) {
   if (MaterialDesignController::IsShelfMaterial()) {
     gfx::ImageSkia image_md =
         CreateVectorIcon(gfx::VectorIconId::SHELF_KEYBOARD, kShelfIconColor);
-    button_->SetImage(views::CustomButton::STATE_NORMAL, &image_md);
+    icon_->SetImage(image_md);
   } else {
     gfx::ImageSkia* image_non_md =
         ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
             IDR_AURA_UBER_TRAY_VIRTUAL_KEYBOARD);
-    button_->SetImage(views::CustomButton::STATE_NORMAL, image_non_md);
+    icon_->SetImage(image_non_md);
   }
-  button_->SetImageAlignment(views::ImageButton::ALIGN_CENTER,
-                             views::ImageButton::ALIGN_MIDDLE);
 
-  tray_container()->AddChildView(button_);
-  button_->SetFocusBehavior(FocusBehavior::NEVER);
+  SetIconBorderForShelfAlignment();
+  tray_container()->AddChildView(icon_);
   SetContentsBackground();
   // The Shell may not exist in some unit tests.
   if (WmShell::HasInstance())
     WmShell::Get()->keyboard_ui()->AddObserver(this);
+  ObserveKeyboardController();
 }
 
 VirtualKeyboardTray::~VirtualKeyboardTray() {
+  UnobserveKeyboardController();
   // The Shell may not exist in some unit tests.
   if (WmShell::HasInstance())
     WmShell::Get()->keyboard_ui()->RemoveObserver(this);
 }
 
 void VirtualKeyboardTray::SetShelfAlignment(ShelfAlignment alignment) {
-  TrayBackgroundView::SetShelfAlignment(alignment);
-  tray_container()->SetBorder(views::Border::NullBorder());
-
-  // Pad button size to align with other controls in the system tray.
-  const gfx::ImageSkia image =
-      button_->GetImage(views::CustomButton::STATE_NORMAL);
-  const int size = GetTrayConstant(VIRTUAL_KEYBOARD_BUTTON_SIZE);
-  const int vertical_padding = (size - image.height()) / 2;
-  int horizontal_padding = (size - image.width()) / 2;
-  if (!ash::MaterialDesignController::IsShelfMaterial() &&
-      IsHorizontalAlignment(alignment)) {
-    // Square up the padding if horizontally aligned. Avoid extra padding when
-    // vertically aligned as the button would violate the width constraint on
-    // the shelf.
-    horizontal_padding += std::max(0, vertical_padding - horizontal_padding);
-  }
+  if (alignment == shelf_alignment())
+    return;
 
-  button_->SetBorder(views::Border::CreateEmptyBorder(
-      gfx::Insets(vertical_padding, horizontal_padding)));
+  TrayBackgroundView::SetShelfAlignment(alignment);
+  SetIconBorderForShelfAlignment();
 }
 
 base::string16 VirtualKeyboardTray::GetAccessibleNameForTray() {
@@ -92,14 +78,47 @@ bool VirtualKeyboardTray::PerformAction(const ui::Event& event) {
   return true;
 }
 
-void VirtualKeyboardTray::ButtonPressed(views::Button* sender,
-                                        const ui::Event& event) {
-  DCHECK_EQ(button_, sender);
-  PerformAction(event);
+void VirtualKeyboardTray::OnKeyboardEnabledStateChanged(bool new_value) {

[sadrul, 2016/08/04 16:36:37]

Call this |enabled|, or |new_enabled|.

[mohsen, 2016/08/04 21:02:39]

Done (Renamed to |new_enabled|).

+  SetVisible(new_value);
+  if (new_value)
+    ObserveKeyboardController();
+  else
+    UnobserveKeyboardController();
 }
 
-void VirtualKeyboardTray::OnKeyboardEnabledStateChanged(bool new_value) {
-  SetVisible(WmShell::Get()->keyboard_ui()->IsEnabled());
+void VirtualKeyboardTray::OnKeyboardBoundsChanging(
+    const gfx::Rect& new_bounds) {
+  SetDrawBackgroundAsActive(!new_bounds.IsEmpty());

[sadrul, 2016/08/04 16:36:37]

It's a bit unfortunate that we don't have anything on KeyboardUIObserver for
'enabled but show/hidden' notifications.

+}
+
+void VirtualKeyboardTray::SetIconBorderForShelfAlignment() {
+  const gfx::ImageSkia image = icon_->GetImage();

[sadrul, 2016/08/04 16:36:37]

const-ref? (moving existing code, I know, but fix since you are touching it?)

[mohsen, 2016/08/04 21:02:39]

Done.

+  const int size = GetTrayConstant(VIRTUAL_KEYBOARD_BUTTON_SIZE);
+  const int vertical_padding = (size - image.height()) / 2;
+  int horizontal_padding = (size - image.width()) / 2;
+  if (!ash::MaterialDesignController::IsShelfMaterial() &&
+      IsHorizontalAlignment(shelf_alignment())) {
+    // Square up the padding if horizontally aligned. Avoid extra padding when
+    // vertically aligned as the button would violate the width constraint on
+    // the shelf.
+    horizontal_padding += std::max(0, vertical_padding - horizontal_padding);
+  }
+  icon_->SetBorder(views::Border::CreateEmptyBorder(
+      gfx::Insets(vertical_padding, horizontal_padding)));
+}
+
+void VirtualKeyboardTray::ObserveKeyboardController() {
+  keyboard::KeyboardController* keyboard_controller =
+      keyboard::KeyboardController::GetInstance();
+  if (keyboard_controller && !keyboard_controller->HasObserver(this))
+    keyboard_controller->AddObserver(this);

[sadrul, 2016/08/04 16:36:37]

Why would you need to check HasObserver() here? (or below in Unobserve?)

[mohsen, 2016/08/04 21:02:38]

OnKeyboardEnabledStateChanged() can be called multiple times. For example, in
one run, looking at the backtrace, it is called once when virtual keyboard prefs
are loaded and once when magnifier prefs are loaded and apparently
KeyboardUIImpl doesn't check that the value has not actually changed (it
actually doesn't keep track of the old value).

The one in the Unobserve() is not really needed as the RemoveObserver() will
ignore the request. Removed.

[sadrul, 2016/08/05 05:50:26]

That seems unfortunate. Can we fix that? At the least, can we track that here?

[mohsen, 2016/08/05 18:35:04]

On every accessibility pref change, AccessibilityManager notifies observers of a
change and the information about the change is thrown away on its way to the
observer (in SystemTrayDelegateChromeOS::OnAccessibilityStatusChanged()). Fixing
that seems out of the scope of this CL. However, I can track that in
KeyboardUIImpl. Done.

+}
+
+void VirtualKeyboardTray::UnobserveKeyboardController() {
+  keyboard::KeyboardController* keyboard_controller =
+      keyboard::KeyboardController::GetInstance();
+  if (keyboard_controller && keyboard_controller->HasObserver(this))
+    keyboard_controller->RemoveObserver(this);
 }
 
 }  // namespace ash