Remove the 2-level input method system & InputMethodBridge.

Remove the 2-level input method system & InputMethodBridge.

This is IMF refactoring according to the design:
 - For @google.com:
  https://docs.google.com/document/d/14PQN4fbbSTlJmIk6qk7RzsuNr7O22DUTfNQ6slxKWV0
 - For @chromium.org:
  https://docs.google.com/document/d/1sDES_kuEVUjb_FwCvKsg0Ef48edq5lAx589r0qUtG4g

This cl includes some fundamental changes:
1) Removed ui/views/ime/...
2) All TextInputClient's use InputMethod::SetFocusedTextInputClient() to inform
   InputMethod that itself is focused.
3) Supports mocking input method in InputMethodFactory for TextfieldTest.
4) Removed views::View::GetTextInputClient() which is useless.
The rest changes follow the above major changes.

BUG=474828
TEST=Verified locally on Win7/Win8/Linux/CrOS/Mac. Tests passed.

Committed: https://crrev.com/12f77cb7c9b7b8256df62a4423e03e539a980b3b
Cr-Commit-Position: refs/heads/master@{#337157}

[Shu Chen, 2015-06-10 09:49:27]

shuchen@chromium.org changed reviewers:
+ suzhe@chromium.org

[Shu Chen, 2015-06-10 14:34:58]

shuchen@chromium.org changed reviewers:
+ sadrul@chromium.org, sky@chromium.org, yukawa@chromium.org

[Shu Chen, 2015-06-10 14:34:59]

Guys, can you please help to review this cl? Thanks

James/Yohei, please do the overall review from IMF perspective.
Scott, please cover all UI stuff.
Sadrul, please cover all event stuff.

The cl is big, it's hard to provide file lists for each reviewer. Sorry.

[Shu Chen, 2015-06-11 08:27:56]

Although this cl is big, it would be easier to review by this order:
1) NWA/DNWA/Widget stuff.
2) View controls: View/Textfield/Combobox/etc.
The changes in other files are rather small & easy for review.

When making this cl, there were a lot of test failures, because the tests used
to involve only views::InputMethod & views::InputMethodDelegate, but now needs
to deal with ui::InputMethod. So please also pay attention to the test changes.

Please let me know if you have any questions. Thanks

[yukawa, 2015-06-11 21:48:43]

This is my initial scan and not all the files are checked.

https://codereview.chromium.org/1177503003/diff/40001/ui/base/ime/mock_input_...
File ui/base/ime/mock_input_method.h (right):

https://codereview.chromium.org/1177503003/diff/40001/ui/base/ime/mock_input_...
ui/base/ime/mock_input_method.h:60: internal::InputMethodDelegate* delegate_;
Can we split changes in mock_input_method.{h, cc} into a separate CL?

https://codereview.chromium.org/1177503003/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1177503003/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:713: input_method_ =
ui::CreateInputMethod(NULL, nil);
Well if we almost always need to call |OnFocus()| just after CreateInputMethod,
can we modify CreateInputMethod instead?

Option 1.
    UI_BASE_IME_EXPORT scoped_ptr<InputMethod> CreateInputMethod(
        internal::InputMethodDelegate* delegate,
        gfx::AcceleratedWidget widget,
        bool has_initial_focus);

Option 2.
    UI_BASE_IME_EXPORT scoped_ptr<InputMethod> CreateInputMethodAndFocus(
        internal::InputMethodDelegate* delegate,
        gfx::AcceleratedWidget widget);

We should be able to do that in a separate CL.

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:295: if (GetInputMethod())
Why do we test GetTextInputClient() in Combobox::~Combobox() but not in
Textfield::~Textfield()?

Combobox::~Combobox() {
  model_->RemoveObserver(this);
  if (GetTextInputClient() && GetInputMethod())
    GetInputMethod()->DetachTextInputClient(GetTextInputClient());
}

Since neither Textfield nor Textfield::GetTextInputClient() is marked as final,
so in theory you can override Textfield::GetTextInputClient() to return null.

BTW, if you create a derived class of Textfield, who is responsible to call
GetInputMethod()->DetachTextInputClient() ?  Can we put those clean-up code in
more common place?

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1000: if (GetInputMethod())
ditto.

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1017: if (GetInputMethod())
ditto.

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/tree/...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/tree/...
ui/views/controls/tree/tree_view.cc:101: if (GetTextInputClient() &&
GetInputMethod())
ditto for clean-up code.

[Shu Chen, 2015-06-12 03:34:38]

https://codereview.chromium.org/1177503003/diff/40001/ui/base/ime/mock_input_...
File ui/base/ime/mock_input_method.h (right):

https://codereview.chromium.org/1177503003/diff/40001/ui/base/ime/mock_input_...
ui/base/ime/mock_input_method.h:60: internal::InputMethodDelegate* delegate_;
On 2015/06/11 21:48:42, yukawa wrote:
> Can we split changes in mock_input_method.{h, cc} into a separate CL?

I recall this is to fix some test failures. MockInputMethod::DispatchKeyEvent
mutes all key events, which is wrong. Instead, it should release key events by
calling DispatchKeyEventPostIME.

https://codereview.chromium.org/1177503003/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1177503003/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:713: input_method_ =
ui::CreateInputMethod(NULL, nil);
On 2015/06/11 21:48:42, yukawa wrote:
> Well if we almost always need to call |OnFocus()| just after
CreateInputMethod,
> can we modify CreateInputMethod instead?
> 
> Option 1.
>     UI_BASE_IME_EXPORT scoped_ptr<InputMethod> CreateInputMethod(
>         internal::InputMethodDelegate* delegate,
>         gfx::AcceleratedWidget widget,
>         bool has_initial_focus);
> 
> Option 2.
>     UI_BASE_IME_EXPORT scoped_ptr<InputMethod> CreateInputMethodAndFocus(
>         internal::InputMethodDelegate* delegate,
>         gfx::AcceleratedWidget widget);
> 
> We should be able to do that in a separate CL.

I will do a CL soon to avoid such default-focused behavior.
I put OnFocus() call here to fix some test failures.
Because some tests deal with views::InputMethod which doesn't have
"toplevel_focused" state.
Now they deal with ui::InputMethod which is sensitive about its
"toplevel_focused" state.

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:295: if (GetInputMethod())
On 2015/06/11 21:48:42, yukawa wrote:
> Why do we test GetTextInputClient() in Combobox::~Combobox() but not in
> Textfield::~Textfield()?
> 
> Combobox::~Combobox() {
>   model_->RemoveObserver(this);
>   if (GetTextInputClient() && GetInputMethod())
>     GetInputMethod()->DetachTextInputClient(GetTextInputClient());
> }
> 
> Since neither Textfield nor Textfield::GetTextInputClient() is marked as
final,
> so in theory you can override Textfield::GetTextInputClient() to return null.
You're right. I've updated the code to check GetTextInputClient().

> 
> BTW, if you create a derived class of Textfield, who is responsible to call
> GetInputMethod()->DetachTextInputClient() ?  Can we put those clean-up code in
> more common place?
The derived class of Textfield should be responsible to call
GetInputMethod()->DetachTextInputClient().
And, no, we cannot put this code in common place.

I used to put it in View::~View(), but it caused test failures.
The reason was Combobox::GetTextInputClient() returns PrefixSelector instance:
  ui::TextInputClient* Combobox::GetTextInputClient() {
    if (!selector_)
      selector_.reset(new PrefixSelector(this));
    return selector_.get();
  }
So when destructing Combobox, |selector_| is destructed before its parent's
destructor (~View) being called.
In View's destructor, GetTextInputClient() will get NULL.

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1000: if (GetInputMethod())
On 2015/06/11 21:48:42, yukawa wrote:
> ditto.

Done.

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1017: if (GetInputMethod())
On 2015/06/11 21:48:42, yukawa wrote:
> ditto.

Done.

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/tree/...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/tree/...
ui/views/controls/tree/tree_view.cc:101: if (GetTextInputClient() &&
GetInputMethod())
On 2015/06/11 21:48:43, yukawa wrote:
> ditto for clean-up code.

Ditto.

[yukawa, 2015-06-12 05:20:46]

https://codereview.chromium.org/1177503003/diff/40001/ui/base/ime/mock_input_...
File ui/base/ime/mock_input_method.h (right):

https://codereview.chromium.org/1177503003/diff/40001/ui/base/ime/mock_input_...
ui/base/ime/mock_input_method.h:60: internal::InputMethodDelegate* delegate_;
On 2015/06/12 03:34:38, Shu Chen wrote:
> On 2015/06/11 21:48:42, yukawa wrote:
> > Can we split changes in mock_input_method.{h, cc} into a separate CL?
> 
> I recall this is to fix some test failures. MockInputMethod::DispatchKeyEvent
> mutes all key events, which is wrong. Instead, it should release key events by
> calling DispatchKeyEventPostIME.

Would it be possible to commit changes in mock_input_method.{h, cc} before this
CL?  I'd like to keep the size of this CL as small as possible.

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:295: if (GetInputMethod())
On 2015/06/12 03:34:38, Shu Chen wrote:
> On 2015/06/11 21:48:42, yukawa wrote:
> > Why do we test GetTextInputClient() in Combobox::~Combobox() but not in
> > Textfield::~Textfield()?
> > 
> > Combobox::~Combobox() {
> >   model_->RemoveObserver(this);
> >   if (GetTextInputClient() && GetInputMethod())
> >     GetInputMethod()->DetachTextInputClient(GetTextInputClient());
> > }
> > 
> > Since neither Textfield nor Textfield::GetTextInputClient() is marked as
> final,
> > so in theory you can override Textfield::GetTextInputClient() to return
null.
> You're right. I've updated the code to check GetTextInputClient().
> 
> > 
> > BTW, if you create a derived class of Textfield, who is responsible to call
> > GetInputMethod()->DetachTextInputClient() ?  Can we put those clean-up code
in
> > more common place?
> The derived class of Textfield should be responsible to call
> GetInputMethod()->DetachTextInputClient().

Hmm, if the derived class of TextField calls
|GetInputMethod()->DetachTextInputClient()|, then Textfield::~Textfield() calls
|GetInputMethod()->DetachTextInputClient()| again, right?
Probably it will work, but it sounds a bit confusing.

> And, no, we cannot put this code in common place.
> 
> I used to put it in View::~View(), but it caused test failures.
> The reason was Combobox::GetTextInputClient() returns PrefixSelector instance:
>   ui::TextInputClient* Combobox::GetTextInputClient() {
>     if (!selector_)
>       selector_.reset(new PrefixSelector(this));
>     return selector_.get();
>   }
> So when destructing Combobox, |selector_| is destructed before its parent's
> destructor (~View) being called.
> In View's destructor, GetTextInputClient() will get NULL.

One idea is that adding
  protected:
    void View::DetachTextInputClient();
and call it from Combobox::~Combobox().  Then we can make sure that View can
simply ignore multiple calls of DetachTextInputClient().

[Shu Chen, 2015-06-12 05:32:55]

https://codereview.chromium.org/1177503003/diff/40001/ui/base/ime/mock_input_...
File ui/base/ime/mock_input_method.h (right):

https://codereview.chromium.org/1177503003/diff/40001/ui/base/ime/mock_input_...
ui/base/ime/mock_input_method.h:60: internal::InputMethodDelegate* delegate_;
On 2015/06/12 05:20:45, yukawa wrote:
> On 2015/06/12 03:34:38, Shu Chen wrote:
> > On 2015/06/11 21:48:42, yukawa wrote:
> > > Can we split changes in mock_input_method.{h, cc} into a separate CL?
> > 
> > I recall this is to fix some test failures.
MockInputMethod::DispatchKeyEvent
> > mutes all key events, which is wrong. Instead, it should release key events
by
> > calling DispatchKeyEventPostIME.
> 
> Would it be possible to commit changes in mock_input_method.{h, cc} before
this
> CL?  I'd like to keep the size of this CL as small as possible.

No problem. I've reverted the changes for MockInputMethod in this cl. 
And I will create another cl for it soon.

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1177503003/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:295: if (GetInputMethod())
On 2015/06/12 05:20:45, yukawa wrote:
> On 2015/06/12 03:34:38, Shu Chen wrote:
> > On 2015/06/11 21:48:42, yukawa wrote:
> > > Why do we test GetTextInputClient() in Combobox::~Combobox() but not in
> > > Textfield::~Textfield()?
> > > 
> > > Combobox::~Combobox() {
> > >   model_->RemoveObserver(this);
> > >   if (GetTextInputClient() && GetInputMethod())
> > >     GetInputMethod()->DetachTextInputClient(GetTextInputClient());
> > > }
> > > 
> > > Since neither Textfield nor Textfield::GetTextInputClient() is marked as
> > final,
> > > so in theory you can override Textfield::GetTextInputClient() to return
> null.
> > You're right. I've updated the code to check GetTextInputClient().
> > 
> > > 
> > > BTW, if you create a derived class of Textfield, who is responsible to
call
> > > GetInputMethod()->DetachTextInputClient() ?  Can we put those clean-up
code
> in
> > > more common place?
> > The derived class of Textfield should be responsible to call
> > GetInputMethod()->DetachTextInputClient().
> 
> Hmm, if the derived class of TextField calls
> |GetInputMethod()->DetachTextInputClient()|, then Textfield::~Textfield()
calls
> |GetInputMethod()->DetachTextInputClient()| again, right?
> Probably it will work, but it sounds a bit confusing.
I think the derived class of TextField overrides GetTextInputClient() doesn't
make sense.
Because TextField is "kind-of" TextInputClient, thus MyTextField should also be
"kind-of" TextInputClient.
If MyTextField overrides GetTextInputClient() which returns non-this, it
conceptually breaks the "kind-of" assumption.
There is no problem to call GetInputMethod()->DetachTextInputClient() for
multiple times, because ui::InputMethodBase's logic can handle it very well.

> 
> > And, no, we cannot put this code in common place.
> > 
> > I used to put it in View::~View(), but it caused test failures.
> > The reason was Combobox::GetTextInputClient() returns PrefixSelector
instance:
> >   ui::TextInputClient* Combobox::GetTextInputClient() {
> >     if (!selector_)
> >       selector_.reset(new PrefixSelector(this));
> >     return selector_.get();
> >   }
> > So when destructing Combobox, |selector_| is destructed before its parent's
> > destructor (~View) being called.
> > In View's destructor, GetTextInputClient() will get NULL.
> 
> One idea is that adding
>   protected:
>     void View::DetachTextInputClient();
> and call it from Combobox::~Combobox().  Then we can make sure that View can
> simply ignore multiple calls of DetachTextInputClient().
Ditto. ui::InputMethodBase's logic can handle multiple DetachTextInputClient
calls very well.

[James Su, 2015-06-12 07:58:51]

https://codereview.chromium.org/1177503003/diff/80001/ui/base/ime/input_metho...
File ui/base/ime/input_method_factory.cc (right):

https://codereview.chromium.org/1177503003/diff/80001/ui/base/ime/input_metho...
ui/base/ime/input_method_factory.cc:43: return
make_scoped_ptr(g_input_method_for_testing);
Not sure if this code is correct. If more than one caller call this method, will
g_input_method_for_testing be deleted for multiple times?

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:295: if (GetInputMethod() &&
GetTextInputClient())
GetTextInputClient() returns 'this', so no need to check it here.

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:296:
GetInputMethod()->DetachTextInputClient(GetTextInputClient());
use 'this' directly?

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:302: if (GetInputMethod() &&
GetTextInputClient())
ditto

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:303:
GetInputMethod()->OnTextInputTypeChanged(GetTextInputClient());
ditto

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:312: if (GetInputMethod() &&
GetTextInputClient())
ditto

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:313:
GetInputMethod()->OnTextInputTypeChanged(GetTextInputClient());
ditto

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:986:
GetInputMethod()->OnTextInputTypeChanged(GetTextInputClient());
ditto

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1001:
GetInputMethod()->SetFocusedTextInputClient(GetTextInputClient());
ditto

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1751:
GetInputMethod()->OnCaretBoundsChanged(GetTextInputClient());
ditto

https://codereview.chromium.org/1177503003/diff/80001/ui/views/focus/focus_ma...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/1177503003/diff/80001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.cc:451: void
FocusManager::FocusTextInputClient(View* view) {
IMHO, everything related to TextInputClient in this class should be removed.

https://codereview.chromium.org/1177503003/diff/80001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/1177503003/diff/80001/ui/views/view.h#newcode697
ui/views/view.h:697: virtual ui::TextInputClient* GetTextInputClient();
This method should be removed.

[Shu Chen, 2015-06-12 08:51:31]

https://codereview.chromium.org/1177503003/diff/80001/ui/base/ime/input_metho...
File ui/base/ime/input_method_factory.cc (right):

https://codereview.chromium.org/1177503003/diff/80001/ui/base/ime/input_metho...
ui/base/ime/input_method_factory.cc:43: return
make_scoped_ptr(g_input_method_for_testing);
On 2015/06/12 07:58:50, James Su wrote:
> Not sure if this code is correct. If more than one caller call this method,
will
> g_input_method_for_testing be deleted for multiple times?

Done. Doing "g_input_method_for_testing = nullptr;" before returning should be
enough to avoid the problem of multiple calls, although there is no multi-calls
in all existing tests.

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:295: if (GetInputMethod() &&
GetTextInputClient())
On 2015/06/12 07:58:50, James Su wrote:
> GetTextInputClient() returns 'this', so no need to check it here.

yukawa@'s comment pointed out the derived class of Textfield can override
GetTextInputClient() to provide non-this pointer.
I think it is better to mark Textfield::GetTextInputClient() as final.
Done the change.

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:296:
GetInputMethod()->DetachTextInputClient(GetTextInputClient());
On 2015/06/12 07:58:50, James Su wrote:
> use 'this' directly?

Done.

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:302: if (GetInputMethod() &&
GetTextInputClient())
On 2015/06/12 07:58:50, James Su wrote:
> ditto

Done.

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:303:
GetInputMethod()->OnTextInputTypeChanged(GetTextInputClient());
On 2015/06/12 07:58:50, James Su wrote:
> ditto

Done.

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:312: if (GetInputMethod() &&
GetTextInputClient())
On 2015/06/12 07:58:50, James Su wrote:
> ditto

Done.

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:313:
GetInputMethod()->OnTextInputTypeChanged(GetTextInputClient());
On 2015/06/12 07:58:50, James Su wrote:
> ditto

Done.

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:986:
GetInputMethod()->OnTextInputTypeChanged(GetTextInputClient());
On 2015/06/12 07:58:50, James Su wrote:
> ditto

Done.

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1001:
GetInputMethod()->SetFocusedTextInputClient(GetTextInputClient());
On 2015/06/12 07:58:50, James Su wrote:
> ditto

Done.

https://codereview.chromium.org/1177503003/diff/80001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1751:
GetInputMethod()->OnCaretBoundsChanged(GetTextInputClient());
On 2015/06/12 07:58:50, James Su wrote:
> ditto

Done.

https://codereview.chromium.org/1177503003/diff/80001/ui/views/focus/focus_ma...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/1177503003/diff/80001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.cc:451: void
FocusManager::FocusTextInputClient(View* view) {
On 2015/06/12 07:58:50, James Su wrote:
> IMHO, everything related to TextInputClient in this class should be removed.

Removing GetTextInputClient stuff should be done in a separated CL of removing
the whole TextInputFocusManager stuff.

https://codereview.chromium.org/1177503003/diff/80001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/1177503003/diff/80001/ui/views/view.h#newcode697
ui/views/view.h:697: virtual ui::TextInputClient* GetTextInputClient();
On 2015/06/12 07:58:50, James Su wrote:
> This method should be removed.

Removing this method should be included in a separated CL to remove the whole
TextInputFocusManager stuff.

[Shu Chen, 2015-06-15 16:28:50]

I've removed views::View::GetTextInputClient & GetFocusedTextInputClient. Please
take another look.

[Shu Chen, 2015-06-16 02:11:58]

shuchen@chromium.org changed reviewers:
+ oshima@chromium.org

[Shu Chen, 2015-06-16 02:11:59]

+oshima@ for ash changes. Thanks

[yukawa, 2015-06-16 02:45:03]

https://codereview.chromium.org/1177503003/diff/120001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1177503003/diff/120001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:773: focused_now ?
focused_now->GetTextInputClient() : NULL;
Mac build fails with |focused_now->GetTextInputClient()|.

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.h:95: ui::TextInputClient*
GetTextInputClient();
I prefer to use more descriptive name such as
   PrefixSelector* GetPrefixSelector();
What do you think?

Also, can this be private method?

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox_unittest.cc (right):

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/comb...
ui/views/controls/combobox/combobox_unittest.cc:677:
combobox_->GetTextInputClient()->InsertChar('J', ui::EF_NONE);
Can this be rewritten with InputMethod::GetTextInputClient() ?

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/comb...
ui/views/controls/combobox/combobox_unittest.cc:692:
combobox_->GetTextInputClient()->InsertChar('P', ui::EF_NONE);
Is this a realistic scenario?  Can Combobox receive character events via
TextInputClient after Combobox::OnBlur is called?

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:295: if (GetInputMethod())
Is there any chance to move this logic to TextInputClient::~TextInputClient() ?

One idea to do that would be having two more methods in TextInputClient as
follows.

class TextInputClient {
  ~TextInputClient() {
    if (input_method_)
      input_method_->DetachTextInputClient(this);
  }

  void OnSetFocusedTextInputClient(InputMethod *input_method) {
    CHECK_EQ(nullptr, input_method_);
    input_method_ = input_method;
  }

  void OnDetachTextInputClient(const InputMethod *input_method) {
    CHECK_EQ(input_method, input_method_);
    input_method_ = nullptr;
  }

 private:
  InputMethod* input_method_;  // does not take the ownership.

};

Then we can call them back from InputMethod::SetFocusedTextInputClient and
InputMethod::DetachTextInputClient, respectively.

[Shu Chen, 2015-06-16 05:15:27]

https://codereview.chromium.org/1177503003/diff/120001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1177503003/diff/120001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:773: focused_now ?
focused_now->GetTextInputClient() : NULL;
On 2015/06/16 02:45:03, yukawa wrote:
> Mac build fails with |focused_now->GetTextInputClient()|.

Done.

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.h:95: ui::TextInputClient*
GetTextInputClient();
On 2015/06/16 02:45:03, yukawa wrote:
> I prefer to use more descriptive name such as
>    PrefixSelector* GetPrefixSelector();
> What do you think?
> 
> Also, can this be private method?

Done.

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox_unittest.cc (right):

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/comb...
ui/views/controls/combobox/combobox_unittest.cc:677:
combobox_->GetTextInputClient()->InsertChar('J', ui::EF_NONE);
On 2015/06/16 02:45:03, yukawa wrote:
> Can this be rewritten with InputMethod::GetTextInputClient() ?

Done.

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/comb...
ui/views/controls/combobox/combobox_unittest.cc:692:
combobox_->GetTextInputClient()->InsertChar('P', ui::EF_NONE);
On 2015/06/16 02:45:03, yukawa wrote:
> Is this a realistic scenario?  Can Combobox receive character events via
> TextInputClient after Combobox::OnBlur is called?

I think combobox_->RequestFocus() should be added here.

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:295: if (GetInputMethod())
On 2015/06/16 02:45:03, yukawa wrote:
> Is there any chance to move this logic to TextInputClient::~TextInputClient()
?
> 
> One idea to do that would be having two more methods in TextInputClient as
> follows.
> 
> class TextInputClient {
>   ~TextInputClient() {
>     if (input_method_)
>       input_method_->DetachTextInputClient(this);
>   }
> 
>   void OnSetFocusedTextInputClient(InputMethod *input_method) {
>     CHECK_EQ(nullptr, input_method_);
>     input_method_ = input_method;
>   }
> 
>   void OnDetachTextInputClient(const InputMethod *input_method) {
>     CHECK_EQ(input_method, input_method_);
>     input_method_ = nullptr;
>   }
> 
>  private:
>   InputMethod* input_method_;  // does not take the ownership.
> 
> };
> 
> Then we can call them back from InputMethod::SetFocusedTextInputClient and
> InputMethod::DetachTextInputClient, respectively.

I would prefer not to do that. Currently TextInputClient is defined as a pure
interface.
And bound TextInputClient with InputMethod in bi-direction will bring extra
complexity, e.g. maintain the semantics of "only one TextInputClient can be
focused, and only one TextInputClient can have the valid |input_method_|
instance".

[oshima, 2015-06-16 05:22:03]

ash/ lgtm

[yukawa, 2015-06-16 05:34:10]

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:295: if (GetInputMethod())
On 2015/06/16 05:15:27, Shu Chen wrote:
> On 2015/06/16 02:45:03, yukawa wrote:
> > Is there any chance to move this logic to
TextInputClient::~TextInputClient()
> ?
> > 
> > One idea to do that would be having two more methods in TextInputClient as
> > follows.
> > 
> > class TextInputClient {
> >   ~TextInputClient() {
> >     if (input_method_)
> >       input_method_->DetachTextInputClient(this);
> >   }
> > 
> >   void OnSetFocusedTextInputClient(InputMethod *input_method) {
> >     CHECK_EQ(nullptr, input_method_);
> >     input_method_ = input_method;
> >   }
> > 
> >   void OnDetachTextInputClient(const InputMethod *input_method) {
> >     CHECK_EQ(input_method, input_method_);
> >     input_method_ = nullptr;
> >   }
> > 
> >  private:
> >   InputMethod* input_method_;  // does not take the ownership.
> > 
> > };
> > 
> > Then we can call them back from InputMethod::SetFocusedTextInputClient and
> > InputMethod::DetachTextInputClient, respectively.
> 
> I would prefer not to do that. Currently TextInputClient is defined as a pure
> interface.
> And bound TextInputClient with InputMethod in bi-direction will bring extra
> complexity, e.g. maintain the semantics of "only one TextInputClient can be
> focused, and only one TextInputClient can have the valid |input_method_|
> instance".

It may make sense but how do you guarantee that GetInputMethod() is still valid
and non-null here?  I'm not worried about the validity of GetInputMethod() in
View::OnFocus() and View::OnBlur(), but I'm not sure what guarantees that
GetInputMethod() still returns non-null object here?

I'm under the impression that object lifetime management and view focus
management are mixed here.

[Shu Chen, 2015-06-16 06:53:59]

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1177503003/diff/120001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:295: if (GetInputMethod())
On 2015/06/16 05:34:10, yukawa wrote:
> On 2015/06/16 05:15:27, Shu Chen wrote:
> > On 2015/06/16 02:45:03, yukawa wrote:
> > > Is there any chance to move this logic to
> TextInputClient::~TextInputClient()
> > ?
> > > 
> > > One idea to do that would be having two more methods in TextInputClient as
> > > follows.
> > > 
> > > class TextInputClient {
> > >   ~TextInputClient() {
> > >     if (input_method_)
> > >       input_method_->DetachTextInputClient(this);
> > >   }
> > > 
> > >   void OnSetFocusedTextInputClient(InputMethod *input_method) {
> > >     CHECK_EQ(nullptr, input_method_);
> > >     input_method_ = input_method;
> > >   }
> > > 
> > >   void OnDetachTextInputClient(const InputMethod *input_method) {
> > >     CHECK_EQ(input_method, input_method_);
> > >     input_method_ = nullptr;
> > >   }
> > > 
> > >  private:
> > >   InputMethod* input_method_;  // does not take the ownership.
> > > 
> > > };
> > > 
> > > Then we can call them back from InputMethod::SetFocusedTextInputClient and
> > > InputMethod::DetachTextInputClient, respectively.
> > 
> > I would prefer not to do that. Currently TextInputClient is defined as a
pure
> > interface.
> > And bound TextInputClient with InputMethod in bi-direction will bring extra
> > complexity, e.g. maintain the semantics of "only one TextInputClient can be
> > focused, and only one TextInputClient can have the valid |input_method_|
> > instance".
> 
> It may make sense but how do you guarantee that GetInputMethod() is still
valid
> and non-null here?  I'm not worried about the validity of GetInputMethod() in
> View::OnFocus() and View::OnBlur(), but I'm not sure what guarantees that
> GetInputMethod() still returns non-null object here?
> 
> I'm under the impression that object lifetime management and view focus
> management are mixed here.

GetInputMethod() returns NULL only when the View is out of the views hierarchy.
RemoveChildView() is called in View::~View() so Textfield can get non-null input
method instance in its destructor.
However, there will be a problem if some code take out a Textfield from the
views hierarchy, and destroy it without calling its OnBlur(). But IMO, if
FocusManager can handle the View destruction well, it should call OnBlur()
correctly (e.g. if its |focused_view_| is changed, the old |focused_view_|
should be blurred, before it's destroyed).
I'm not familiar with focus management in Views, can you please share some
insights?

[yukawa, 2015-06-16 09:13:26]

Chatting with Shu online and now I think we can go ahead as long as FocusManager
can handle the View destruction well.

At design level this CL looks good to me.  Defer to Views experts' judgement for
Views specific part.

https://codereview.chromium.org/1177503003/diff/180001/ui/views/controls/menu...
File ui/views/controls/menu/submenu_view.h (right):

https://codereview.chromium.org/1177503003/diff/180001/ui/views/controls/menu...
ui/views/controls/menu/submenu_view.h:55: ui::TextInputClient*
GetTextInputClient();
Can this be GetPrefixSelector() ?

[James Su, 2015-06-16 09:18:34]

https://codereview.chromium.org/1177503003/diff/180001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/1177503003/diff/180001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:380:
GetInputMethod()->OnFocus();
I'm wondering why this line is inside if (GetWidget()->HasFocusManager()) block?

And I'm still not quite clear where should InputMethod::On{Focus|Blur}() be
called.

https://codereview.chromium.org/1177503003/diff/180001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:390:
GetInputMethod()->OnBlur();
ditto

https://codereview.chromium.org/1177503003/diff/180001/ui/views/widget/native...
File ui/views/widget/native_widget_private.h (right):

https://codereview.chromium.org/1177503003/diff/180001/ui/views/widget/native...
ui/views/widget/native_widget_private.h:136: // views::InputMethod.
This TODO can be removed?

[Shu Chen, 2015-06-17 02:27:52]

https://codereview.chromium.org/1177503003/diff/180001/ui/views/controls/menu...
File ui/views/controls/menu/submenu_view.h (right):

https://codereview.chromium.org/1177503003/diff/180001/ui/views/controls/menu...
ui/views/controls/menu/submenu_view.h:55: ui::TextInputClient*
GetTextInputClient();
On 2015/06/16 09:13:26, yukawa wrote:
> Can this be GetPrefixSelector() ?

Done.

https://codereview.chromium.org/1177503003/diff/180001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/1177503003/diff/180001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:380:
GetInputMethod()->OnFocus();
On 2015/06/16 09:18:33, James Su wrote:
> I'm wondering why this line is inside if (GetWidget()->HasFocusManager())
block?
> 
> And I'm still not quite clear where should InputMethod::On{Focus|Blur}() be
> called.

This CL doesn't change the original behaviors.

I will make a separated CL to move the OnFocus/OnBlur calls from here to
Xxx-WTH-Yyy, also remove the "default-focused" logic in WTH.

https://codereview.chromium.org/1177503003/diff/180001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:390:
GetInputMethod()->OnBlur();
On 2015/06/16 09:18:33, James Su wrote:
> ditto

ditto.

https://codereview.chromium.org/1177503003/diff/180001/ui/views/widget/native...
File ui/views/widget/native_widget_private.h (right):

https://codereview.chromium.org/1177503003/diff/180001/ui/views/widget/native...
ui/views/widget/native_widget_private.h:136: // views::InputMethod.
On 2015/06/16 09:18:33, James Su wrote:
> This TODO can be removed?

Done.

[James Su, 2015-06-18 01:57:14]

LGTM.

On 2015/06/17 02:27:52, Shu Chen wrote:
>
https://codereview.chromium.org/1177503003/diff/180001/ui/views/controls/menu...
> File ui/views/controls/menu/submenu_view.h (right):
> 
>
https://codereview.chromium.org/1177503003/diff/180001/ui/views/controls/menu...
> ui/views/controls/menu/submenu_view.h:55: ui::TextInputClient*
> GetTextInputClient();
> On 2015/06/16 09:13:26, yukawa wrote:
> > Can this be GetPrefixSelector() ?
> 
> Done.
> 
>
https://codereview.chromium.org/1177503003/diff/180001/ui/views/widget/deskto...
> File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):
> 
>
https://codereview.chromium.org/1177503003/diff/180001/ui/views/widget/deskto...
> ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:380:
> GetInputMethod()->OnFocus();
> On 2015/06/16 09:18:33, James Su wrote:
> > I'm wondering why this line is inside if (GetWidget()->HasFocusManager())
> block?
> > 
> > And I'm still not quite clear where should InputMethod::On{Focus|Blur}() be
> > called.
> 
> This CL doesn't change the original behaviors.
> 
> I will make a separated CL to move the OnFocus/OnBlur calls from here to
> Xxx-WTH-Yyy, also remove the "default-focused" logic in WTH.
> 
>
https://codereview.chromium.org/1177503003/diff/180001/ui/views/widget/deskto...
> ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:390:
> GetInputMethod()->OnBlur();
> On 2015/06/16 09:18:33, James Su wrote:
> > ditto
> 
> ditto.
> 
>
https://codereview.chromium.org/1177503003/diff/180001/ui/views/widget/native...
> File ui/views/widget/native_widget_private.h (right):
> 
>
https://codereview.chromium.org/1177503003/diff/180001/ui/views/widget/native...
> ui/views/widget/native_widget_private.h:136: // views::InputMethod.
> On 2015/06/16 09:18:33, James Su wrote:
> > This TODO can be removed?
> 
> Done.

[Shu Chen, 2015-06-24 00:32:59]

Pinging...

sky@/yukawa@/sadrul@, can you please take a look? Thanks

[yukawa, 2015-06-24 01:00:51]

lgtm

[sadrul, 2015-06-25 05:19:38]

Please try splitting this CL into smaller CLs. Large CLs are difficult to
review, and difficult to revert when something breaks.

[Shu Chen, 2015-06-26 02:49:20]

I've made the CL smaller by reverting the below changes:
 - Removed useless TextInputClient::GetAttachedWindow().
 - Removed GetFocusedTextInputClient() in widget/DNWA/NWA/Window/etc.

sky@/sadrul@, can you please take another look? Thanks

[Shu Chen, 2015-06-26 03:54:47]

shuchen@chromium.org changed reviewers:
+ tapted@chromium.org

[Shu Chen, 2015-06-26 03:55:42]

+tapted@ to review the Mac related stuff.

Also merging Cl https://codereview.chromium.org/1207973002 to this CL.

Thanks tapted@ for catching the issue.

[sky, 2015-06-29 02:17:41]

https://codereview.chromium.org/1177503003/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/find_bar_view.cc:237:
GetWidget()->GetInputMethod()->ShowImeIfNeeded();
Shouldn't focusing find_text_ take care of showing ime?

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.cc:275: model_->RemoveObserver(this);
Here you don't detach from ime, but you do in ~Textfield. Why the difference?

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox_unittest.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/comb...
ui/views/controls/combobox/combobox_unittest.cc:206:
ui::SetUpInputMethodFactoryForTesting();
Should we move this clal to ViewsTestBase?

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/pref...
File ui/views/controls/prefix_selector.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/pref...
ui/views/controls/prefix_selector.cc:35: ui::InputMethod* input_method =
prefix_delegate_->GetInputMethod();
How come PrefixSelector has to detach, but not attach? Doing this from the
destructor seems a bit risky given you don't know the state of the delegate. Can
the users of PrefixSelector detach?

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:295: if (GetInputMethod())
Is it really possible to hit this? I would have assumed we blur before deleting?

[Shu Chen, 2015-06-29 02:56:35]

Thanks for your review, Scott.

Can you please take another look?

https://codereview.chromium.org/1177503003/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/find_bar_view.cc:237:
GetWidget()->GetInputMethod()->ShowImeIfNeeded();
On 2015/06/29 02:17:41, sky wrote:
> Shouldn't focusing find_text_ take care of showing ime?

Textfield::ShowImeIfNeeded() is only called for focusing by mouse/touch events,
not by key events.
There might be some considerations of not calling ShowImeIfNeeded() in
Textfield::OnFocus().
This cl better not change that behavior.

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.cc:275: model_->RemoveObserver(this);
On 2015/06/29 02:17:41, sky wrote:
> Here you don't detach from ime, but you do in ~Textfield. Why the difference?

Textfield is a TextInputClient, while Combobox is not.
Combobox holds PrefixSelector as a TextInputClient, so I made
PrefixSelector::~PrefixSelector() to detach ime.

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox_unittest.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/comb...
ui/views/controls/combobox/combobox_unittest.cc:206:
ui::SetUpInputMethodFactoryForTesting();
On 2015/06/29 02:17:41, sky wrote:
> Should we move this clal to ViewsTestBase?

Done.

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/pref...
File ui/views/controls/prefix_selector.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/pref...
ui/views/controls/prefix_selector.cc:35: ui::InputMethod* input_method =
prefix_delegate_->GetInputMethod();
On 2015/06/29 02:17:41, sky wrote:
> How come PrefixSelector has to detach, but not attach? Doing this from the
> destructor seems a bit risky given you don't know the state of the delegate.
Can
> the users of PrefixSelector detach?

Done.

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:295: if (GetInputMethod())
On 2015/06/29 02:17:41, sky wrote:
> Is it really possible to hit this? I would have assumed we blur before
deleting?

You're right. The FocusManager should make sure the Textfield is blurred before
it being destroyed.
Detaching it at here is just to double make sure for safety.

Do you insist to remove this? Or replace it with a CHECK/DCHECK?

[tapted, 2015-06-29 04:57:21]

https://codereview.chromium.org/1177503003/diff/320001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1177503003/diff/320001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:711: input_method_->OnFocus();
Can you add a comment about this line?

It seems out of place.. e.g. things like Textfield::SetReadOnly(..) call into
this, and the widget might not have focus then. I patched this in, and I see
text input breaks without this line.

The problem seems to be that the new version of
BridgedNativeWidget::OnDidChangeFocus(..) below will get a null TextInputClient
from InputMethodBase::GetTextInputClient()  (i.e. because
InputMethodBase::system_toplevel_window_focused_ is not yet set)

Is the problem that the Mac implementation is currently missing a call to
input_method_->OnFocus() when the widget gains focus? Is there a data race
somewhere? Do we need a different way to get the TextInputClient? (or perhaps
OnFocus really is the right thing to do -- I'm still wrapping my head around the
IME stuff :).

https://codereview.chromium.org/1177503003/diff/320001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:757: focus_manager_->OnKeyEvent(key);
BridgedContentView currently ignores the return value, but this should probably
be 

  return !focus_manager_->OnKeyEvent(key);

[Shu Chen, 2015-06-29 06:05:57]

https://codereview.chromium.org/1177503003/diff/320001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1177503003/diff/320001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:711: input_method_->OnFocus();
On 2015/06/29 04:57:21, tapted wrote:
> Can you add a comment about this line?
> 
> It seems out of place.. e.g. things like Textfield::SetReadOnly(..) call into
> this, and the widget might not have focus then. I patched this in, and I see
> text input breaks without this line.
> 
> The problem seems to be that the new version of
> BridgedNativeWidget::OnDidChangeFocus(..) below will get a null
TextInputClient
> from InputMethodBase::GetTextInputClient()  (i.e. because
> InputMethodBase::system_toplevel_window_focused_ is not yet set)
> 
> Is the problem that the Mac implementation is currently missing a call to
> input_method_->OnFocus() when the widget gains focus? Is there a data race
> somewhere? Do we need a different way to get the TextInputClient? (or perhaps
> OnFocus really is the right thing to do -- I'm still wrapping my head around
the
> IME stuff :).

ui::InputMethod::OnBlur() only gets called in DesktopNativeWidgetAura.
And for Mac, InputMethod::OnBlur() is never called.

So the OnFocus() call here is to make sure the InputMethod under Mac is "always
focused".
ChromeOS/Ash also uses such "always focused" mode.
Please refer to DisplayController::AddWindowTreeHostForDisplay().

https://codereview.chromium.org/1177503003/diff/320001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:757: focus_manager_->OnKeyEvent(key);
On 2015/06/29 04:57:21, tapted wrote:
> BridgedContentView currently ignores the return value, but this should
probably
> be 
> 
>   return !focus_manager_->OnKeyEvent(key);
> 

Done.

[tapted, 2015-06-29 06:34:48]

ui/views/cocoa/* and native_widget_mac lgtm

Thanks!

https://codereview.chromium.org/1177503003/diff/320001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1177503003/diff/320001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:711: input_method_->OnFocus();
On 2015/06/29 06:05:57, Shu Chen wrote:
> On 2015/06/29 04:57:21, tapted wrote:
> > Can you add a comment about this line?
> > 
> > It seems out of place.. e.g. things like Textfield::SetReadOnly(..) call
into
> > this, and the widget might not have focus then. I patched this in, and I see
> > text input breaks without this line.
> > 
> > The problem seems to be that the new version of
> > BridgedNativeWidget::OnDidChangeFocus(..) below will get a null
> TextInputClient
> > from InputMethodBase::GetTextInputClient()  (i.e. because
> > InputMethodBase::system_toplevel_window_focused_ is not yet set)
> > 
> > Is the problem that the Mac implementation is currently missing a call to
> > input_method_->OnFocus() when the widget gains focus? Is there a data race
> > somewhere? Do we need a different way to get the TextInputClient? (or
perhaps
> > OnFocus really is the right thing to do -- I'm still wrapping my head around
> the
> > IME stuff :).
> 
> ui::InputMethod::OnBlur() only gets called in DesktopNativeWidgetAura.
> And for Mac, InputMethod::OnBlur() is never called.
> 
> So the OnFocus() call here is to make sure the InputMethod under Mac is
"always
> focused".
> ChromeOS/Ash also uses such "always focused" mode.
> Please refer to DisplayController::AddWindowTreeHostForDisplay().

Acknowledged. (we discussed this out of band - "always focused" mode is fine to
keep things simple for now. I'm going to follow-up to resolve the TODO with some
test coverage)

[sky, 2015-06-29 17:03:50]

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.cc:275: model_->RemoveObserver(this);
On 2015/06/29 02:56:34, Shu Chen wrote:
> On 2015/06/29 02:17:41, sky wrote:
> > Here you don't detach from ime, but you do in ~Textfield. Why the
difference?
> 
> Textfield is a TextInputClient, while Combobox is not.
> Combobox holds PrefixSelector as a TextInputClient, so I made
> PrefixSelector::~PrefixSelector() to detach ime.

As I mentioned, it is unusual that PrefixSelector does the detach but not the
attach. I think the place that does the attach should be responsible for the
detach.

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:295: if (GetInputMethod())
On 2015/06/29 02:56:34, Shu Chen wrote:
> On 2015/06/29 02:17:41, sky wrote:
> > Is it really possible to hit this? I would have assumed we blur before
> deleting?
> 
> You're right. The FocusManager should make sure the Textfield is blurred
before
> it being destroyed.
> Detaching it at here is just to double make sure for safety.
> 
> Do you insist to remove this? Or replace it with a CHECK/DCHECK?

DCHECK.

[Shu Chen, 2015-06-30 00:54:06]

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.cc:275: model_->RemoveObserver(this);
On 2015/06/29 17:03:49, sky wrote:
> On 2015/06/29 02:56:34, Shu Chen wrote:
> > On 2015/06/29 02:17:41, sky wrote:
> > > Here you don't detach from ime, but you do in ~Textfield. Why the
> difference?
> > 
> > Textfield is a TextInputClient, while Combobox is not.
> > Combobox holds PrefixSelector as a TextInputClient, so I made
> > PrefixSelector::~PrefixSelector() to detach ime.
> 
> As I mentioned, it is unusual that PrefixSelector does the detach but not the
> attach. I think the place that does the attach should be responsible for the
> detach.

Done. I've added the DCHECK here.

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:295: if (GetInputMethod())
On 2015/06/29 17:03:49, sky wrote:
> On 2015/06/29 02:56:34, Shu Chen wrote:
> > On 2015/06/29 02:17:41, sky wrote:
> > > Is it really possible to hit this? I would have assumed we blur before
> > deleting?
> > 
> > You're right. The FocusManager should make sure the Textfield is blurred
> before
> > it being destroyed.
> > Detaching it at here is just to double make sure for safety.
> > 
> > Do you insist to remove this? Or replace it with a CHECK/DCHECK?
> 
> DCHECK.

Done.

[Shu Chen, 2015-06-30 08:43:39]

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

[Shu Chen, 2015-06-30 08:43:39]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
suzhe@chromium.org, yukawa@chromium.org, tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1177503003/#ps400001
(title: "per comments")

[commit-bot: I haz the power, 2015-06-30 08:44:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177503003/400001

[commit-bot: I haz the power, 2015-06-30 09:09:30]

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

[commit-bot: I haz the power, 2015-06-30 09:09:32]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Shu Chen, 2015-06-30 09:50:06]

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

[Shu Chen, 2015-06-30 09:50:07]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
oshima@chromium.org, yukawa@chromium.org, suzhe@chromium.org
Link to the patchset: https://codereview.chromium.org/1177503003/#ps420001
(title: "test green.")

[commit-bot: I haz the power, 2015-06-30 09:50:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177503003/420001

[commit-bot: I haz the power, 2015-06-30 10:50:52]

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

[commit-bot: I haz the power, 2015-06-30 10:50:54]

Dry run: This issue passed the CQ dry run.

[sky, 2015-06-30 16:32:31]

https://codereview.chromium.org/1177503003/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/find_bar_view.cc:237:
GetWidget()->GetInputMethod()->ShowImeIfNeeded();
On 2015/06/29 02:56:34, Shu Chen wrote:
> On 2015/06/29 02:17:41, sky wrote:
> > Shouldn't focusing find_text_ take care of showing ime?
> 
> Textfield::ShowImeIfNeeded() is only called for focusing by mouse/touch
events,
> not by key events.
> There might be some considerations of not calling ShowImeIfNeeded() in
> Textfield::OnFocus().
> This cl better not change that behavior.

That makes sense, but I couldn't readily determine if this
(FindBarView::SetFocusAndSelection()) is always called from mouse/touch events.
What about keyboard accelerators?

https://codereview.chromium.org/1177503003/diff/420001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/1177503003/diff/420001/ui/views/view.h#newcod...
ui/views/view.h:697: ui::InputMethod* GetInputMethod();
Inline this one and call to the const version. See the const_casts in this
header for examples (eg GetNativeTheme).

https://codereview.chromium.org/1177503003/diff/420001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1177503003/diff/420001/ui/views/widget/widget...
ui/views/widget/widget.cc:1123: ui::InputMethod* input_method =
GetInputMethod();
This is the same as 1107-1112. How about a function that each call to in the
anonymous namespace?

[Shu Chen, 2015-07-01 02:38:54]

shuchen@chromium.org changed reviewers:
- sadrul@chromium.org

[Shu Chen, 2015-07-01 02:38:57]

Moving sadrul@ to cc as no specific changes require his attention.

https://codereview.chromium.org/1177503003/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/1177503003/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/find_bar_view.cc:237:
GetWidget()->GetInputMethod()->ShowImeIfNeeded();
On 2015/06/30 16:32:31, sky wrote:
> On 2015/06/29 02:56:34, Shu Chen wrote:
> > On 2015/06/29 02:17:41, sky wrote:
> > > Shouldn't focusing find_text_ take care of showing ime?
> > 
> > Textfield::ShowImeIfNeeded() is only called for focusing by mouse/touch
> events,
> > not by key events.
> > There might be some considerations of not calling ShowImeIfNeeded() in
> > Textfield::OnFocus().
> > This cl better not change that behavior.
> 
> That makes sense, but I couldn't readily determine if this
> (FindBarView::SetFocusAndSelection()) is always called from mouse/touch
events.
> What about keyboard accelerators?

For mouse/touch events, Textfield can take care of calling ShowImeIfNeeded():
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

For keyboard accelerators, according to the UX design, text field may or may not
trigger ShowImeIfNeeded().
For example, Ctrl+F to bring up FindBar will trigger Virtual Keyboard shown.
However, TAB key to focus on omnibox will not trigger Virtual Keyboard shown.

https://codereview.chromium.org/1177503003/diff/420001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/1177503003/diff/420001/ui/views/view.h#newcod...
ui/views/view.h:697: ui::InputMethod* GetInputMethod();
On 2015/06/30 16:32:31, sky wrote:
> Inline this one and call to the const version. See the const_casts in this
> header for examples (eg GetNativeTheme).

Done.

https://codereview.chromium.org/1177503003/diff/420001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1177503003/diff/420001/ui/views/widget/widget...
ui/views/widget/widget.cc:1123: ui::InputMethod* input_method =
GetInputMethod();
On 2015/06/30 16:32:31, sky wrote:
> This is the same as 1107-1112. How about a function that each call to in the
> anonymous namespace?

Done.

[sky, 2015-07-01 16:03:56]

LGTM

[Shu Chen, 2015-07-02 01:10:37]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2015-07-02 01:10:37]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
oshima@chromium.org, yukawa@chromium.org, suzhe@chromium.org
Link to the patchset: https://codereview.chromium.org/1177503003/#ps440001
(title: "nits.")

[commit-bot: I haz the power, 2015-07-02 01:11:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177503003/440001

[commit-bot: I haz the power, 2015-07-02 02:11:29]

Committed patchset #23 (id:440001)

[commit-bot: I haz the power, 2015-07-02 02:12:17]

Patchset 23 (id:??) landed as
https://crrev.com/12f77cb7c9b7b8256df62a4423e03e539a980b3b
Cr-Commit-Position: refs/heads/master@{#337157}