Set ImeListView's scrollable range when updating

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 1452eeb447d518398a7e8a9e0cff73c60718c128..93a74c9fdc98c694a51f35a9b56e1f9323fd0e86 100644
--- a/ash/common/system/chromeos/ime_menu/ime_list_view.cc
+++ b/ash/common/system/chromeos/ime_menu/ime_list_view.cc
@@ -5,6 +5,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/fixed_sized_scroll_view.h"
 #include "ash/common/system/tray/hover_highlight_view.h"
 #include "ash/common/system/tray/ime_info.h"
 #include "ash/common/system/tray/system_menu_button.h"
@@ -252,8 +253,9 @@ class MaterialKeyboardStatusRowView : public views::View {
 
 ImeListView::ImeListView(SystemTrayItem* owner,
                          bool show_keyboard_toggle,
-                         SingleImeBehavior single_ime_behavior)
-    : TrayDetailsView(owner) {
+                         SingleImeBehavior single_ime_behavior,
+                         const gfx::Range& scrollable_range)
+    : TrayDetailsView(owner), scrollable_range_(scrollable_range) {
   SystemTrayDelegate* delegate = WmShell::Get()->system_tray_delegate();
   IMEInfoList list;
   delegate->GetAvailableIMEList(&list);
@@ -294,13 +296,19 @@ void ImeListView::Update(const IMEInfoList& list,
     }
   }
 
+  if (MaterialDesignController::IsSystemTrayMenuMaterial() &&
+      scrollable_range_.IsValid()) {
+    scroller()->ClipHeightTo(scrollable_range_.start(),
+                             scrollable_range_.end());
+  }
+
   Layout();

[tdanderson, 2016/12/06 23:38:19]

Would it be possible to move the code from lines 299-303 into
ImeListView::Layout() instead of having it here? That would be more consistent
with how TrayDetailsView::Layout() works (note it contains a similar call to
ClipHeightTo(), probably for the same reason as this CL).

Specifically, here is my idea:

* Have ImeDetailedView::Layout() just call up to TrayDetailsView::Layout().

* Implement ImeListView::Layout() as:

void ImeListView::Layout() {
  Range r = const gfx::Range height_range = GetImeListViewRange();
  scroller()->ClipHeightTo(r);
  TrayDetailsView::Layout();
}

Here the call up to TrayDetailsView::Layout() should be a no-op because you've
already clipped the height. Note you would also need to move
GetImeListViewRange() into this class, or at least make it accessible by this
class.

The advantages (assuming my idea works / makes sense) are that you don't need to
introduce the |scrollable_range_| member or modify the constructor of
ImeListView.

WDYT?

[Azure Wei, 2016/12/07 00:58:33]

GetImeListViewRange() is kind of opt-in menu specific behavior, instead of
adding it to ImeListView and making ImeDetailedView override it again, I add a
new class ImeMenuListView as subclass of ImeListView and then override Layout()
change the height range.

Thus, ImeListView and ImeDetailedView could keep the previous range to avoid
introducing any new bugs. And no need for the new |scrollable_range_| member.

Is that OK? PTAL.

   SchedulePaint();
 }
 
 void ImeListView::ResetImeListView() {
   // Children are removed from the view hierarchy and deleted in Reset().
-  Reset();
+  TrayDetailsView::Reset();

[tdanderson, 2016/12/06 23:38:19]

Adding TrayDetailsView:: shouldn't be necessary here.

[Azure Wei, 2016/12/07 00:58:33]

Reverted.

   material_keyboard_status_view_ = nullptr;
   keyboard_status_ = nullptr;
 }