Set ImeListView's scrollable range when updating

Set ImeListView's scrollable range when updating

For material design, we need to set turns the scroll view (listing all current IMEs) into a bounded scroll view, with a fixed height. When switching IME, this needs to set again and then call Layout() & SchedulePaint().
Since the range should not change during switching IMEs, we pass the range in the constructor of ImeListView instead of ImeListView::Update() every time.

BUG=667650
TEST=Verified on local build.

Committed: https://crrev.com/29b403dc3d47bb52477c4bf5324b46557d38cd38
Cr-Commit-Position: refs/heads/master@{#437168}

[Azure Wei, 2016-11-30 10:28:14]

Description was changed from

==========
Hide and reshow the bubble when switching IMEs.

BUG=667650
TEST=Verified on local build.
==========

to

==========
Hide and reshow the bubble when switching IMEs.

BUG=667650
TEST=Verified on local build.
==========

[Azure Wei, 2016-11-30 10:28:14]

azurewei@chromium.org changed reviewers:
+ tdanderson@chromium.org

[Azure Wei, 2016-11-30 10:30:13]

Please review this CL. Thanks!
This CL may not be a good fix, but I've failed to find out why
ImeListView::Update() cannot work well with opt-in IME menu (note that it works
fine with system IME detailed view).

[Azure Wei, 2016-11-30 11:13:37]

The CQ bit was checked by azurewei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 11:13:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 11:47:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 11:47:14]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2016-11-30 23:04:15]

https://codereview.chromium.org/2541743004/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right):

https://codereview.chromium.org/2541743004/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:477: ShowImeMenuBubble();
I'm not sure that hiding and re-showing the bubble is really the ideal fix for
this bug; wouldn't the menu look like it's "flickering" to the user (when it
hides and re-shows quickly)?

I tried the keyboard shortcut in the IME detailed view in the system menu, and
it seems to work fine. Can you look into how the detailed view handles changes
via keyboard shortcut and do something similar for the opt-in menu?

[Azure Wei, 2016-12-01 00:33:30]

https://codereview.chromium.org/2541743004/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right):

https://codereview.chromium.org/2541743004/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:477: ShowImeMenuBubble();
On 2016/11/30 23:04:15, tdanderson wrote:
> I'm not sure that hiding and re-showing the bubble is really the ideal fix for
> this bug; wouldn't the menu look like it's "flickering" to the user (when it
> hides and re-shows quickly)?
> 
> I tried the keyboard shortcut in the IME detailed view in the system menu, and
> it seems to work fine. Can you look into how the detailed view handles changes
> via keyboard shortcut and do something similar for the opt-in menu?

The IME detailed view and opt-in menu shares the same updating logic. And opt-in
menu used to work fine when switching with shortcuts. I suspects this could be
regression related to scroll content of TrayDetailsView. But I didn't find out
where goes wrong.

Anyway, this isn't a good solution to fix. Let me hold this CL and try other
ways.

[Azure Wei, 2016-12-06 02:56:44]

Description was changed from

==========
Hide and reshow the bubble when switching IMEs.

BUG=667650
TEST=Verified on local build.
==========

to

==========
Set ImeListView's scrollable range when updating

For material design, we need to set turns the scroll view (listing all current
IMEs) into a bounded scroll view, with a fixed height. When switching IME, this
needs to set again and then call Layout() & SchedulePaint(). 
Since the range should not change during switching IMEs, we pass the range in
the constructor of ImeListView instead of ImeListView::Update() every time.


BUG=667650
TEST=Verified on local build.
==========

[Azure Wei, 2016-12-06 02:57:06]

On 2016/12/01 00:33:30, Azure Wei wrote:
>
https://codereview.chromium.org/2541743004/diff/1/ash/common/system/chromeos/...
> File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right):
> 
>
https://codereview.chromium.org/2541743004/diff/1/ash/common/system/chromeos/...
> ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:477: ShowImeMenuBubble();
> On 2016/11/30 23:04:15, tdanderson wrote:
> > I'm not sure that hiding and re-showing the bubble is really the ideal fix
for
> > this bug; wouldn't the menu look like it's "flickering" to the user (when it
> > hides and re-shows quickly)?
> > 
> > I tried the keyboard shortcut in the IME detailed view in the system menu,
and
> > it seems to work fine. Can you look into how the detailed view handles
changes
> > via keyboard shortcut and do something similar for the opt-in menu?
> 
> The IME detailed view and opt-in menu shares the same updating logic. And
opt-in
> menu used to work fine when switching with shortcuts. I suspects this could be
> regression related to scroll content of TrayDetailsView. But I didn't find out
> where goes wrong.
> 
> Anyway, this isn't a good solution to fix. Let me hold this CL and try other
> ways.

This CL is updated. Please take another look. Thanks!

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

https://codereview.chromium.org/2541743004/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right):

https://codereview.chromium.org/2541743004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:305: Layout();
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?

https://codereview.chromium.org/2541743004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:311:
TrayDetailsView::Reset();
Adding TrayDetailsView:: shouldn't be necessary here.

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

https://codereview.chromium.org/2541743004/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right):

https://codereview.chromium.org/2541743004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:305: Layout();
On 2016/12/06 23:38:19, tdanderson wrote:
> 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?

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.

https://codereview.chromium.org/2541743004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:311:
TrayDetailsView::Reset();
On 2016/12/06 23:38:19, tdanderson wrote:
> Adding TrayDetailsView:: shouldn't be necessary here.

Reverted.

[tdanderson, 2016-12-07 22:53:04]

I like the approach in Patch Set 4 much better, thanks! LGTM with a couple of
nits.

https://codereview.chromium.org/2541743004/diff/60001/ash/common/system/chrom...
File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right):

https://codereview.chromium.org/2541743004/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:306: explicit
ImeMenuListView(SystemTrayItem* owner,
nit: no 'explicit' since the constructor has more than one parameter

https://codereview.chromium.org/2541743004/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:330:
TrayDetailsView::Layout();
I think this should instead call up to IMEListView::Layout(). Currently that
method is not implemented so it will resolve to TrayDetailsView::Layout()
anyway, but if IMEListView::Layout() was ever provided with an implementation
then we would presumably want this method to call up to that rather than
directly up to TrayDetailsView.

[Azure Wei, 2016-12-08 03:04:12]

https://codereview.chromium.org/2541743004/diff/60001/ash/common/system/chrom...
File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right):

https://codereview.chromium.org/2541743004/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:306: explicit
ImeMenuListView(SystemTrayItem* owner,
On 2016/12/07 22:53:04, tdanderson wrote:
> nit: no 'explicit' since the constructor has more than one parameter

Done. Removed 'explicit'.

https://codereview.chromium.org/2541743004/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:330:
TrayDetailsView::Layout();
On 2016/12/07 22:53:04, tdanderson wrote:
> I think this should instead call up to IMEListView::Layout(). Currently that
> method is not implemented so it will resolve to TrayDetailsView::Layout()
> anyway, but if IMEListView::Layout() was ever provided with an implementation
> then we would presumably want this method to call up to that rather than
> directly up to TrayDetailsView.

Yes, this should be ImeListView::Layout(). Thanks.

[Azure Wei, 2016-12-08 03:04:17]

The CQ bit was checked by azurewei@chromium.org

[Azure Wei, 2016-12-08 03:04:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2541743004/#ps80001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2016-12-08 03:04:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 03:40:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481166257609580,
"parent_rev": "c3fbb23bcc2f39e3b34e4b95aa63dbd43ad94be7", "commit_rev":
"5d335bf38957350f8af1b1e2bd583e1d1385d746"}

[commit-bot: I haz the power, 2016-12-08 03:40:39]

Description was changed from

==========
Set ImeListView's scrollable range when updating

For material design, we need to set turns the scroll view (listing all current
IMEs) into a bounded scroll view, with a fixed height. When switching IME, this
needs to set again and then call Layout() & SchedulePaint(). 
Since the range should not change during switching IMEs, we pass the range in
the constructor of ImeListView instead of ImeListView::Update() every time.


BUG=667650
TEST=Verified on local build.
==========

to

==========
Set ImeListView's scrollable range when updating

For material design, we need to set turns the scroll view (listing all current
IMEs) into a bounded scroll view, with a fixed height. When switching IME, this
needs to set again and then call Layout() & SchedulePaint(). 
Since the range should not change during switching IMEs, we pass the range in
the constructor of ImeListView instead of ImeListView::Update() every time.


BUG=667650
TEST=Verified on local build.
==========

[commit-bot: I haz the power, 2016-12-08 03:40:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-08 03:42:51]

Description was changed from

==========
Set ImeListView's scrollable range when updating

For material design, we need to set turns the scroll view (listing all current
IMEs) into a bounded scroll view, with a fixed height. When switching IME, this
needs to set again and then call Layout() & SchedulePaint(). 
Since the range should not change during switching IMEs, we pass the range in
the constructor of ImeListView instead of ImeListView::Update() every time.


BUG=667650
TEST=Verified on local build.
==========

to

==========
Set ImeListView's scrollable range when updating

For material design, we need to set turns the scroll view (listing all current
IMEs) into a bounded scroll view, with a fixed height. When switching IME, this
needs to set again and then call Layout() & SchedulePaint().
Since the range should not change during switching IMEs, we pass the range in
the constructor of ImeListView instead of ImeListView::Update() every time.

BUG=667650
TEST=Verified on local build.

Committed: https://crrev.com/29b403dc3d47bb52477c4bf5324b46557d38cd38
Cr-Commit-Position: refs/heads/master@{#437168}
==========

[commit-bot: I haz the power, 2016-12-08 03:42:52]

Patchset 5 (id:??) landed as
https://crrev.com/29b403dc3d47bb52477c4bf5324b46557d38cd38
Cr-Commit-Position: refs/heads/master@{#437168}