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

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

Remove the pre-material design code paths remaining
in ImeListView and ImeListItemView.

BUG=686286
TEST=manual

Review-Url: https://codereview.chromium.org/2793153004
Cr-Commit-Position: refs/heads/master@{#462228}
Committed: https://chromium.googlesource.com/chromium/src/+/9e3fb2888342b8a8037c5b8288fd53dc6c30cf9a

[tdanderson, 2017-04-04 22:31:13]

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

[commit-bot: I haz the power, 2017-04-04 22:32:09]

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

[tdanderson, 2017-04-04 22:32:45]

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

[tdanderson, 2017-04-04 22:32:45]

estade@ and azurewei@, please take a look.

[commit-bot: I haz the power, 2017-04-04 23:10:26]

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

[commit-bot: I haz the power, 2017-04-04 23:10:27]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-04-04 23:40:48]

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

https://codereview.chromium.org/2793153004/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:137: class
KeyboardStatusRow : public views::View {
where does this actually show up? I can't find a toggle button anywhere in the
system menu; I only see green circle checkmarks. It looks like this always
passes false for showing the keyboard row:

https://cs.chromium.org/chromium/src/ash/common/system/chromeos/ime_menu/ime_...

and it seems like that probably always gets called (even if at construction time
show_keyboard_toggle is true). So maybe we can get rid of KeyboardStatusRow, or
if it's needed improve the class-level docs so I'm not sent on this wild goose
chase.

https://codereview.chromium.org/2793153004/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:286:
keyboard_status_row_->toggle()->SetIsOn(keyboard::IsKeyboardEnabled(), false);
this should probably be in KeyboardStatusRow::Init().

[tdanderson, 2017-04-05 17:54:13]

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

[tdanderson, 2017-04-05 17:54:43]

Please take another look.

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

https://codereview.chromium.org/2793153004/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:137: class
KeyboardStatusRow : public views::View {
On 2017/04/04 23:40:47, Evan Stade wrote:
> where does this actually show up? I can't find a toggle button anywhere in the
> system menu; I only see green circle checkmarks. It looks like this always
> passes false for showing the keyboard row:
> 
>
https://cs.chromium.org/chromium/src/ash/common/system/chromeos/ime_menu/ime_...
> 

Yes that specific call site always passes in false. But there are call sites
which pass in TrayIME::ShouldShowKeyboardToggle(), e.g.,

https://cs.chromium.org/chromium/src/ash/common/system/ime/tray_ime_chromeos....

or ImeMenuTray::ShouldShowKeyboardToggle(), e.g.,

https://cs.chromium.org/chromium/src/ash/common/system/chromeos/ime_menu/ime_...

(As an aside, reviewing the architecture of the IME detailed view and opt-in IME
menu it looks like there are some refactoring opportunities here, but that is
outside of the scope of this particular CL. One example is crbug.com/708692.)

> and it seems like that probably always gets called (even if at construction
time
> show_keyboard_toggle is true). So maybe we can get rid of KeyboardStatusRow,
or
> if it's needed improve the class-level docs so I'm not sent on this wild goose
> chase.

In the next patch set I have tried to clarify the class-level documentation in
various places.

https://codereview.chromium.org/2793153004/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:286:
keyboard_status_row_->toggle()->SetIsOn(keyboard::IsKeyboardEnabled(), false);
On 2017/04/04 23:40:47, Evan Stade wrote:
> this should probably be in KeyboardStatusRow::Init().

Done.

[commit-bot: I haz the power, 2017-04-05 17:54:44]

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

[Evan Stade, 2017-04-05 18:02:56]

Azure might be able to help clear this up.

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

https://codereview.chromium.org/2793153004/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:137: class
KeyboardStatusRow : public views::View {
On 2017/04/05 17:54:42, tdanderson wrote:
> On 2017/04/04 23:40:47, Evan Stade wrote:
> > where does this actually show up? I can't find a toggle button anywhere in
the
> > system menu; I only see green circle checkmarks. It looks like this always
> > passes false for showing the keyboard row:
> > 
> >
>
https://cs.chromium.org/chromium/src/ash/common/system/chromeos/ime_menu/ime_...
> > 
> 
> Yes that specific call site always passes in false. But there are call sites
> which pass in TrayIME::ShouldShowKeyboardToggle(), e.g.,
> 
>
https://cs.chromium.org/chromium/src/ash/common/system/ime/tray_ime_chromeos....
> 
> or ImeMenuTray::ShouldShowKeyboardToggle(), e.g.,
> 
>
https://cs.chromium.org/chromium/src/ash/common/system/chromeos/ime_menu/ime_...
> 
> (As an aside, reviewing the architecture of the IME detailed view and opt-in
IME
> menu it looks like there are some refactoring opportunities here, but that is
> outside of the scope of this particular CL. One example is crbug.com/708692.)
> 
> > and it seems like that probably always gets called (even if at construction
> time
> > show_keyboard_toggle is true). So maybe we can get rid of KeyboardStatusRow,
> or
> > if it's needed improve the class-level docs so I'm not sent on this wild
goose
> > chase.
> 
> In the next patch set I have tried to clarify the class-level documentation in
> various places.

"and it seems like that probably always gets called (even if at construction
time show_keyboard_toggle is true)"

can you actually get the toggle to show?

[tdanderson, 2017-04-05 18:13:45]

On 2017/04/05 18:02:56, Evan Stade wrote:
> Azure might be able to help clear this up.
> 
>
https://codereview.chromium.org/2793153004/diff/1/ash/common/system/chromeos/...
> File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right):
> 
>
https://codereview.chromium.org/2793153004/diff/1/ash/common/system/chromeos/...
> ash/common/system/chromeos/ime_menu/ime_list_view.cc:137: class
> KeyboardStatusRow : public views::View {
> On 2017/04/05 17:54:42, tdanderson wrote:
> > On 2017/04/04 23:40:47, Evan Stade wrote:
> > > where does this actually show up? I can't find a toggle button anywhere in
> the
> > > system menu; I only see green circle checkmarks. It looks like this always
> > > passes false for showing the keyboard row:
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/ash/common/system/chromeos/ime_menu/ime_...
> > > 
> > 
> > Yes that specific call site always passes in false. But there are call sites
> > which pass in TrayIME::ShouldShowKeyboardToggle(), e.g.,
> > 
> >
>
https://cs.chromium.org/chromium/src/ash/common/system/ime/tray_ime_chromeos....
> > 
> > or ImeMenuTray::ShouldShowKeyboardToggle(), e.g.,
> > 
> >
>
https://cs.chromium.org/chromium/src/ash/common/system/chromeos/ime_menu/ime_...
> > 
> > (As an aside, reviewing the architecture of the IME detailed view and opt-in
> IME
> > menu it looks like there are some refactoring opportunities here, but that
is
> > outside of the scope of this particular CL. One example is
crbug.com/708692.)
> > 
> > > and it seems like that probably always gets called (even if at
construction
> > time
> > > show_keyboard_toggle is true). So maybe we can get rid of
KeyboardStatusRow,
> > or
> > > if it's needed improve the class-level docs so I'm not sent on this wild
> goose
> > > chase.
> > 
> > In the next patch set I have tried to clarify the class-level documentation
in
> > various places.
> 
> "and it seems like that probably always gets called (even if at construction
> time show_keyboard_toggle is true)"
> 
> can you actually get the toggle to show?

Yes, the toggle will show if you have a physical keyboard plugged in to the
device, enter TouchView, and then open the IME detailed view.

In TouchView, focusing a text field shows the virtual keyboard by default.
But if you prefer to use your physical keyboard then switching the toggle
to 'off' will suppress this behavior.

[commit-bot: I haz the power, 2017-04-05 18:27:09]

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

[commit-bot: I haz the power, 2017-04-05 18:27:09]

Dry run: This issue passed the CQ dry run.

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

lgtm with updated docs

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

https://codereview.chromium.org/2793153004/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:177: return
GetPreferredSize().height();
I don't actually think this is necessary

https://codereview.chromium.org/2793153004/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:316: if
(keyboard_status_row_ && sender == keyboard_status_row_->toggle()) {
DCHECK this condition?

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

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

https://codereview.chromium.org/2793153004/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:177: return
GetPreferredSize().height();
On 2017/04/05 19:04:44, Evan Stade wrote:
> I don't actually think this is necessary

Done.

https://codereview.chromium.org/2793153004/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:316: if
(keyboard_status_row_ && sender == keyboard_status_row_->toggle()) {
On 2017/04/05 19:04:44, Evan Stade wrote:
> DCHECK this condition?

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).

[tdanderson, 2017-04-05 21:03:53]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2017-04-05 21:03:53]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2793153004/#ps60001
(title: "class level docs for KeyboardStatusRow")

[tdanderson, 2017-04-05 21:03:58]

The CQ bit was unchecked by tdanderson@chromium.org

[tdanderson, 2017-04-05 21:04:32]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2017-04-05 21:05:37]

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, 2017-04-05 21:43:38]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491426272101690,
"parent_rev": "5f51051e2d3a91006a79792440e35f5458d7f30a", "commit_rev":
"9e3fb2888342b8a8037c5b8288fd53dc6c30cf9a"}

[commit-bot: I haz the power, 2017-04-05 21:44:25]

Description was changed from

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

Remove the pre-material design code paths remaining
in ImeListView and ImeListItemView.

BUG=686286
TEST=manual
==========

to

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

Remove the pre-material design code paths remaining
in ImeListView and ImeListItemView.

BUG=686286
TEST=manual

Review-Url: https://codereview.chromium.org/2793153004
Cr-Commit-Position: refs/heads/master@{#462228}
Committed:
https://chromium.googlesource.com/chromium/src/+/9e3fb2888342b8a8037c5b8288fd...
==========

[commit-bot: I haz the power, 2017-04-05 21:44:26]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9e3fb2888342b8a8037c5b8288fd...

[Evan Stade, 2017-04-05 23:43:06]

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

https://codereview.chromium.org/2793153004/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:316: if
(keyboard_status_row_ && sender == keyboard_status_row_->toggle()) {
On 2017/04/05 21:03:44, tdanderson wrote:
> On 2017/04/05 19:04:44, Evan Stade wrote:
> > DCHECK this condition?
> 
> 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).

I would argue that function should look like

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

[tdanderson, 2017-04-06 18:06:50]

On 2017/04/05 23:43:06, Evan Stade wrote:
>
https://codereview.chromium.org/2793153004/diff/40001/ash/common/system/chrom...
> File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right):
> 
>
https://codereview.chromium.org/2793153004/diff/40001/ash/common/system/chrom...
> ash/common/system/chromeos/ime_menu/ime_list_view.cc:316: if
> (keyboard_status_row_ && sender == keyboard_status_row_->toggle()) {
> On 2017/04/05 21:03:44, tdanderson wrote:
> > On 2017/04/05 19:04:44, Evan Stade wrote:
> > > DCHECK this condition?
> > 
> > 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).
> 
> I would argue that function should look like
> 
> IMEDetailedView::HandleButtonPressed() {
>     if (sender == settings_button_)
>       ShowSettings();
>     else
>       ImeListView::HandleButtonPressed(sender, event);
> }

Hm, good idea. Thanks: https://codereview.chromium.org/2805733002