[Ash] Remove non-MD code from ImeListView and ImeListItemView

ash/common/system/chromeos/ime_menu/ime_list_view.cc

Index: ash/common/system/chromeos/ime_menu/ime_list_view.cc
diff --git a/ash/common/system/chromeos/ime_menu/ime_list_view.cc b/ash/common/system/chromeos/ime_menu/ime_list_view.cc
index e156720f7b97852873f61e4375e956a9a9f204d7..e4c2a485de632393ef34ec89a6ddceb56db4255f 100644
--- a/ash/common/system/chromeos/ime_menu/ime_list_view.cc
+++ b/ash/common/system/chromeos/ime_menu/ime_list_view.cc
@@ -4,8 +4,7 @@
 
 #include "ash/common/system/chromeos/ime_menu/ime_list_view.h"
 
-#include "ash/common/material_design/material_design_controller.h"
-#include "ash/common/system/tray/hover_highlight_view.h"
+#include "ash/common/system/tray/actionable_view.h"
 #include "ash/common/system/tray/ime_info.h"
 #include "ash/common/system/tray/system_menu_button.h"
 #include "ash/common/system/tray/system_tray_delegate.h"
@@ -27,7 +26,6 @@
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/keyboard/keyboard_util.h"
 #include "ui/views/background.h"
-#include "ui/views/border.h"
 #include "ui/views/controls/button/toggle_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
@@ -42,40 +40,8 @@ namespace {
 
 const int kMinFontSizeDelta = -10;
 
-const SkColor kKeyboardRowSeparatorColor = SkColorSetA(SK_ColorBLACK, 0x1F);
-
-// A |HoverHighlightView| that uses bold or normal font depending on whether it
-// is selected.  This view exposes itself as a checkbox to the accessibility
-// framework.
-class SelectableHoverHighlightView : public HoverHighlightView {
- public:
-  SelectableHoverHighlightView(ViewClickListener* listener,
-                               const base::string16& label,
-                               bool selected)
-      : HoverHighlightView(listener), selected_(selected) {
-    AddLabelDeprecated(label, gfx::ALIGN_LEFT, selected);
-  }
-
-  ~SelectableHoverHighlightView() override {}
-
- protected:
-  // views::View:
-  void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
-    HoverHighlightView::GetAccessibleNodeData(node_data);
-    node_data->role = ui::AX_ROLE_CHECK_BOX;
-    if (selected_)
-      node_data->AddStateFlag(ui::AX_STATE_CHECKED);
-  }
-
- private:
-  bool selected_;
-
-  DISALLOW_COPY_AND_ASSIGN(SelectableHoverHighlightView);
-};
-
-// The IME list item view used in the material design. It contains IME info
-// (name and label) and a check button if the item is selected. It's also used
-// for IME property item, which has no name but label and a gray checked icon.
+// Represents a row in the scrollable IME list; each row is either an IME or
+// an IME property. A checkmark icon is shown in the row if selected.
 class ImeListItemView : public ActionableView {
  public:
   ImeListItemView(SystemTrayItem* owner,
@@ -87,14 +53,13 @@ class ImeListItemView : public ActionableView {
       : ActionableView(owner, TrayPopupInkDropStyle::FILL_BOUNDS),
         ime_list_view_(list_view),
         selected_(selected) {
-    if (MaterialDesignController::IsSystemTrayMenuMaterial())
-      SetInkDropMode(InkDropHostView::InkDropMode::ON);
+    SetInkDropMode(InkDropHostView::InkDropMode::ON);
 
     TriView* tri_view = TrayPopupUtils::CreateDefaultRowView();
     AddChildView(tri_view);
     SetLayoutManager(new views::FillLayout);
 
-    // The id button shows the IME short name.
+    // |id_label| contains the IME short name (e.g., 'US', 'GB', 'IT').
     views::Label* id_label = TrayPopupUtils::CreateDefaultLabel();
     id_label->SetText(id);
     ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
@@ -102,9 +67,9 @@ class ImeListItemView : public ActionableView {
         rb.GetFontList(ui::ResourceBundle::MediumBoldFont);
     id_label->SetFontList(base_font_list);
 
-    // For IMEs whose short name are more than 2 characters (INTL, EXTD, etc.),
-    // |kMenuIconSize| is not enough. The label will trigger eliding as "I..."
-    // or "...". So we shrink the font size until it fits within the bounds.
+    // IMEs having a short name of more than two characters (e.g., 'INTL') will
+    // elide if rendered within |kMenuIconSize|. Shrink the font size until the
+    // entire short name fits within the bounds.
     int size_delta = -1;
     while ((id_label->GetPreferredSize().width() -
             id_label->GetInsets().width()) > kMenuIconSize &&
@@ -114,7 +79,7 @@ class ImeListItemView : public ActionableView {
     }
     tri_view->AddView(TriView::Container::START, id_label);
 
-    // The label shows the IME name.
+    // The label shows the IME full name.
     auto* label_view = TrayPopupUtils::CreateDefaultLabel();
     label_view->SetText(label);
     TrayPopupItemStyle style(
@@ -138,13 +103,9 @@ class ImeListItemView : public ActionableView {
 
   // ActionableView:
   bool PerformAction(const ui::Event& event) override {
-    if (ime_list_view_->should_focus_ime_after_selection_with_keyboard() &&
-        event.type() == ui::EventType::ET_KEY_PRESSED) {
-      ime_list_view_->set_last_item_selected_with_keyboard(true);
-    } else {
-      ime_list_view_->set_last_item_selected_with_keyboard(false);
-    }
-
+    ime_list_view_->set_last_item_selected_with_keyboard(
+        ime_list_view_->should_focus_ime_after_selection_with_keyboard() &&
+        event.type() == ui::EventType::ET_KEY_PRESSED);
     ime_list_view_->HandleViewClicked(this);
     return true;
   }
@@ -171,28 +132,17 @@ class ImeListItemView : public ActionableView {
 
 }  // namespace
 
-// The view that contains a |KeyboardButtonView| and a toggle button.
-class MaterialKeyboardStatusRowView : public views::View {
+// Contains a toggle button to let the user enable/disable whether the
+// on-screen keyboard should be shown on touch.
+class KeyboardStatusRow : public views::View {
  public:
-  MaterialKeyboardStatusRowView(views::ButtonListener* listener, bool enabled)
-      : listener_(listener), toggle_(nullptr) {
-    Init();
-    toggle_->SetIsOn(enabled, false);
-  }
-
-  ~MaterialKeyboardStatusRowView() override {}
+  KeyboardStatusRow() {}
+  ~KeyboardStatusRow() override {}
 
-  views::Button* toggle() const { return toggle_; }
+  views::ToggleButton* toggle() const { return toggle_; }
   bool is_toggled() const { return toggle_->is_on(); }
 
- protected:
-  // views::View:
-  int GetHeightForWidth(int w) const override {
-    return GetPreferredSize().height();
-  }
-
- private:
-  void Init() {
+  void Init(views::ButtonListener* listener) {
     TrayPopupUtils::ConfigureAsStickyHeader(this);
     SetLayoutManager(new views::FillLayout);
 
@@ -216,17 +166,22 @@ class MaterialKeyboardStatusRowView : public views::View {
 
     // The on-screen keyboard toggle button.
     toggle_ = TrayPopupUtils::CreateToggleButton(
-        listener_, IDS_ASH_STATUS_TRAY_ACCESSIBILITY_VIRTUAL_KEYBOARD);
+        listener, IDS_ASH_STATUS_TRAY_ACCESSIBILITY_VIRTUAL_KEYBOARD);
+    toggle_->SetIsOn(keyboard::IsKeyboardEnabled(), false);
     tri_view->AddView(TriView::Container::END, toggle_);
   }
 
-  // ButtonListener to notify when |toggle_| is clicked.
-  views::ButtonListener* listener_;
+ protected:
+  // views::View:
+  int GetHeightForWidth(int w) const override {
+    return GetPreferredSize().height();

[Evan Stade, 2017/04/05 19:04:44]

I don't actually think this is necessary

[tdanderson, 2017/04/05 21:03:44]

Done.

+  }
 
+ private:
   // ToggleButton to toggle keyboard on or off.
-  views::ToggleButton* toggle_;
+  views::ToggleButton* toggle_ = nullptr;
 
-  DISALLOW_COPY_AND_ASSIGN(MaterialKeyboardStatusRowView);
+  DISALLOW_COPY_AND_ASSIGN(KeyboardStatusRow);
 };
 
 ImeListView::ImeListView(SystemTrayItem* owner)
@@ -256,26 +211,11 @@ void ImeListView::Update(const IMEInfoList& list,
   property_map_.clear();
   CreateScrollableList();
 
-  // Appends IME list and IME properties.
-  if (single_ime_behavior == ImeListView::SHOW_SINGLE_IME || list.size() > 1) {
-    if (MaterialDesignController::IsSystemTrayMenuMaterial()) {
-      AppendImeListAndProperties(list, property_list);
-    } else {
-      AppendIMEList(list);
-      if (!property_list.empty())
-        AppendIMEProperties(property_list);
-    }
-  }
+  if (single_ime_behavior == ImeListView::SHOW_SINGLE_IME || list.size() > 1)
+    AppendImeListAndProperties(list, property_list);
 
-  if (show_keyboard_toggle) {
-    if (MaterialDesignController::IsSystemTrayMenuMaterial()) {
-      PrependMaterialKeyboardStatus();
-    } else {
-      if (list.size() > 1 || !property_list.empty())
-        AddScrollSeparator();
-      AppendKeyboardStatus();
-    }
-  }
+  if (show_keyboard_toggle)
+    PrependKeyboardStatusRow();
 
   Layout();
   SchedulePaint();
@@ -291,8 +231,7 @@ void ImeListView::Update(const IMEInfoList& list,
 void ImeListView::ResetImeListView() {
   // Children are removed from the view hierarchy and deleted in Reset().
   Reset();
-  material_keyboard_status_view_ = nullptr;
-  keyboard_status_ = nullptr;
+  keyboard_status_row_ = nullptr;
   current_ime_view_ = nullptr;
 }
 
@@ -303,30 +242,6 @@ void ImeListView::CloseImeListView() {
   GetWidget()->Close();
 }
 
-void ImeListView::AppendIMEList(const IMEInfoList& list) {
-  DCHECK(ime_map_.empty());
-  for (size_t i = 0; i < list.size(); i++) {
-    HoverHighlightView* container =
-        new SelectableHoverHighlightView(this, list[i].name, list[i].selected);
-    scroll_content()->AddChildView(container);
-    ime_map_[container] = list[i].id;
-  }
-}
-
-void ImeListView::AppendIMEProperties(
-    const IMEPropertyInfoList& property_list) {
-  DCHECK(property_map_.empty());
-  for (size_t i = 0; i < property_list.size(); i++) {
-    HoverHighlightView* container = new SelectableHoverHighlightView(
-        this, property_list[i].name, property_list[i].selected);
-    if (i == 0)
-      container->SetBorder(
-          views::CreateSolidSidedBorder(1, 0, 0, 0, kBorderLightColor));
-    scroll_content()->AddChildView(container);
-    property_map_[container] = property_list[i].key;
-  }
-}
-
 void ImeListView::AppendImeListAndProperties(
     const IMEInfoList& list,
     const IMEPropertyInfoList& property_list) {
@@ -341,8 +256,7 @@ void ImeListView::AppendImeListAndProperties(
     if (list[i].selected)
       current_ime_view_ = ime_view;
 
-    // In material design, the property items will be added after the current
-    // selected IME item.
+    // Add the properties, if any, of the currently-selected IME.
     if (list[i].selected && !property_list.empty()) {
       // Adds a separator on the top of property items.
       scroll_content()->AddChildView(
@@ -366,35 +280,14 @@ void ImeListView::AppendImeListAndProperties(
   }
 }
 
-void ImeListView::AppendKeyboardStatus() {
-  DCHECK(!MaterialDesignController::IsSystemTrayMenuMaterial());
-  HoverHighlightView* container = new HoverHighlightView(this);
-  int id = keyboard::IsKeyboardEnabled() ? IDS_ASH_STATUS_TRAY_DISABLE_KEYBOARD
-                                         : IDS_ASH_STATUS_TRAY_ENABLE_KEYBOARD;
-  container->AddLabelDeprecated(
-      ui::ResourceBundle::GetSharedInstance().GetLocalizedString(id),
-      gfx::ALIGN_LEFT, false /* highlight */);
-  scroll_content()->AddChildView(container);
-  keyboard_status_ = container;
-}
-
-void ImeListView::PrependMaterialKeyboardStatus() {
-  DCHECK(MaterialDesignController::IsSystemTrayMenuMaterial());
-  DCHECK(!material_keyboard_status_view_);
-  MaterialKeyboardStatusRowView* view =
-      new MaterialKeyboardStatusRowView(this, keyboard::IsKeyboardEnabled());
-  scroll_content()->AddChildViewAt(view, 0);
-  material_keyboard_status_view_ = view;
+void ImeListView::PrependKeyboardStatusRow() {
+  DCHECK(!keyboard_status_row_);
+  keyboard_status_row_ = new KeyboardStatusRow;
+  keyboard_status_row_->Init(this);
+  scroll_content()->AddChildViewAt(keyboard_status_row_, 0);
 }
 
 void ImeListView::HandleViewClicked(views::View* view) {
-  if (view == keyboard_status_) {
-    WmShell::Get()->ToggleIgnoreExternalKeyboard();
-    last_selected_item_id_.clear();
-    last_item_selected_with_keyboard_ = false;
-    return;
-  }
-
   SystemTrayDelegate* delegate = Shell::Get()->system_tray_delegate();
   std::map<views::View*, std::string>::const_iterator ime = ime_map_.find(view);
   if (ime != ime_map_.end()) {
@@ -420,8 +313,7 @@ void ImeListView::HandleViewClicked(views::View* view) {
 
 void ImeListView::HandleButtonPressed(views::Button* sender,
                                       const ui::Event& event) {
-  if (material_keyboard_status_view_ &&
-      sender == material_keyboard_status_view_->toggle()) {
+  if (keyboard_status_row_ && sender == keyboard_status_row_->toggle()) {

[Evan Stade, 2017/04/05 19:04:44]

DCHECK this condition?

[tdanderson, 2017/04/05 21:03:44]

We should leave as-is since this method is called by its override
IMEDetailedView::HandleButtonPressed(), and the condition is not necessarily
true (if the user clicks the settings button, for instance).

[Evan Stade, 2017/04/05 23:43:05]

I would argue that function should look like

IMEDetailedView::HandleButtonPressed() {
    if (sender == settings_button_)
      ShowSettings();
    else
      ImeListView::HandleButtonPressed(sender, event);
}

     WmShell::Get()->ToggleIgnoreExternalKeyboard();
     last_selected_item_id_.clear();
     last_item_selected_with_keyboard_ = false;
@@ -464,7 +356,7 @@ ImeListViewTestApi::ImeListViewTestApi(ImeListView* ime_list_view)
 ImeListViewTestApi::~ImeListViewTestApi() {}
 
 views::View* ImeListViewTestApi::GetToggleView() const {
-  return ime_list_view_->material_keyboard_status_view_->toggle();
+  return ime_list_view_->keyboard_status_row_->toggle();
 }
 
 }  // namespace ash