Redesigns the text input focus handling.

Redesigns the text input focus handling.

Introduces the new focus handling of text input behind a flag.  Unless the flag is enabled, the behavior doesn't change.  Once we confirm that the new design works well, we will remove the old text input focus handling code.

Here is a document that describes the approach.
https://docs.google.com/document/d/1tTcJJqh2SE8-9mEHjEzbILa_ebH96B9LCVKMuZ2p6gw/edit?usp=sharing


Notable change in ui/views/widget/desktop_aura/x11_desktop_handler.cc
The timing to update |current_window_| is changed so that the active widget has been changed before a callback function HandleNativeWidgetActivationChanged() is called.
In the new design, the active widget's focus manager controls the text input focus, so it's important that the active widget has been updated before HandleNativeWidgetActivationChanged gets called.

BUG=290701
TEST=Run chrome with --enable-text-input-focus-manager and ensure that text input in the omnibox, find bar, bookmark bubble, bookmark edit dialog, and web content work as expected when you change the text input focus by mouse clicks or tab key, also when you change the active window from/to other windows.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269106

[Yuki, 2014-02-20 15:02:33]

Hi Mike,

Could you review this CL?  This is a rather big change and I don't expect a
quick review.  Please take your time.

[msw, 2014-02-21 21:23:23]

This is interesting; you should link to the design doc on the bug itself and
ping the relevant input method developers about your plans and work. Their
opinions and reviews will be more relevant than mine.

[Yuki, 2014-03-05 12:38:32]

On 2014/02/21 21:23:23, msw wrote:
> This is interesting; you should link to the design doc on the bug itself and
> ping the relevant input method developers about your plans and work. Their
> opinions and reviews will be more relevant than mine.

It seems no one has objection with this design so far.  If you're okay with the
discussion, could you review this CL?  I'll rename views::NoopInputMethod to
views::NullInputMethod later as Yukawa suggested.

[Yuki, 2014-03-10 08:59:16]

Just as a friendly reminder.  If you think we'll need something else before the
review, please let me know.  I'm thinking we've done the design discussion.

[msw, 2014-03-11 00:58:50]

Thanks for doing some hard work here to cleanup the views::InputMethod mess; I'm
sorry it took so long for me to take a look. Hopefully some ui/views InputMethod
authors will also take a look; surely I'm not the foremost expert here.

https://codereview.chromium.org/173803002/diff/200001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/173803002/diff/200001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6987: +      <message
name="IDS_FLAGS_NEW_TEXT_INPUT_FOCUS_HANDLING_NAME" desc="Title for the flag to
enable the new text input focus handling.">
nit: consider IDS_FLAGS_TEXT_INPUT_FOCUS_MANAGER_NAME and s/handling/manager/

https://codereview.chromium.org/173803002/diff/200001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6988: +        Enable the new text input
focus handling.
Consider: "Experimental text input focus manager."

https://codereview.chromium.org/173803002/diff/200001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6990: +      <message
name="IDS_FLAGS_NEW_TEXT_INPUT_FOCUS_HANDLING_DESCRIPTION" desc="Description for
the flag to enable the new text input focus handling.">
nit: consider IDS_FLAGS_TEXT_INPUT_FOCUS_MANAGER_DESCRIPTION and
s/handling/manager/

https://codereview.chromium.org/173803002/diff/200001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6991: +        Enable the re-designed text
input focus handling instead of using the old one.
Consider "Enable an experimental focus manager to track text input clients."

https://codereview.chromium.org/173803002/diff/200001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/173803002/diff/200001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1900: "new-text-input-focus-handling",
nit: text-input-focus-manager

https://codereview.chromium.org/173803002/diff/200001/content/public/browser/...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/173803002/diff/200001/content/public/browser/...
content/public/browser/render_widget_host_view.h:93: // Returns a
ui::TextInputClient to support text input or NULL if this RWHV
nit: order after GetNativeView* or GetSelectedText() not with Views functions.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/input_metho...
File ui/base/ime/input_method_base.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/input_metho...
ui/base/ime/input_method_base.cc:132: return;
Will the input methods still work okay without calling
On[Will|Did]ChangeFocusedClient and NotifyTextInputStateChanged? Those seem to
cancel/confirm compositions, update other internal state, and notify input
method observers.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
File ui/base/ime/text_input_focus_manager.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.cc:26: void
TextInputFocusManager::UnsetFocusedTextInputClient(
Is this function even necessary? A call could easily do this:
if (client == manager->GetFocusedTextInputClient())
  manager->SetFocusedTextInputClient(NULL);

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
File ui/base/ime/text_input_focus_manager.h (right):

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:17: // The TextInputFocusManager class
manages which instance of TextInputClient
nit: remove "The TextInputFocusManager class"; consider "Manages the focused
TextInputClient across windows and their contents."

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:21: virtual ~TextInputFocusManager();
nit: I don't think this needs to be virtual or public.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:29: void
SetFocusedTextInputClient(TextInputClient* text_input_client);
nit: consider "FocusTextInputClient"

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:33: void
UnsetFocusedTextInputClient(TextInputClient* text_input_client);
nit: consider "BlurTextInputClient" or "UnfocusTextInputClient"

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:38: TextInputClient*
focused_text_input_client_;
Is this better than views::FocusManager having a static TextInputClient* member?

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:40: friend struct
DefaultSingletonTraits<TextInputFocusManager>;
nit: friend types are commonly declared at the top of the private section.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ui_base_switche...
File ui/base/ui_base_switches.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/base/ui_base_switche...
ui/base/ui_base_switches.cc:17: // Disables the new text input focus handling
and uses the old one.
nit: match the language from my other suggestions, if you took them.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ui_base_switches.h
File ui/base/ui_base_switches.h (right):

https://codereview.chromium.org/173803002/diff/200001/ui/base/ui_base_switche...
ui/base/ui_base_switches.h:20: UI_BASE_EXPORT extern const char
kDisableNewTextInputFocus[];
nit: kDisableTextInputFocusManager

https://codereview.chromium.org/173803002/diff/200001/ui/base/ui_base_switche...
ui/base/ui_base_switches.h:24: UI_BASE_EXPORT extern const char
kEnableNewTextInputFocus[];
nit: kEnableTextInputFocusManager

https://codereview.chromium.org/173803002/diff/200001/ui/base/ui_base_switche...
File ui/base/ui_base_switches_util.h (right):

https://codereview.chromium.org/173803002/diff/200001/ui/base/ui_base_switche...
ui/base/ui_base_switches_util.h:12: UI_BASE_EXPORT bool
IsNewTextInputFocusEnabled();
nit: IsTextInputFocusManagerEnabled()

https://codereview.chromium.org/173803002/diff/200001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:161: if
(!switches::IsNewTextInputFocusEnabled())
nit: just make this condition part of the if statement below.

https://codereview.chromium.org/173803002/diff/200001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:212: // WebView::GetTextInputClient()
delegates the text input handling to the
This comment doesn't explain why you'd want to set the focused text input client
to |this| when the underlying render view host is deleted. I'm generally
confused by what's happening here and why.

https://codereview.chromium.org/173803002/diff/200001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.h (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/controls/webvi...
ui/views/controls/webview/webview.h:94: virtual ui::TextInputClient*
GetTextInputClient() OVERRIDE;
nit: declare this below GetClassName, to follow View's decl order/visibility.

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

https://codereview.chromium.org/173803002/diff/200001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.cc:125: if (view != focused_view_)
nit: reverse the condition and call Focus* in this block.

https://codereview.chromium.org/173803002/diff/200001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.cc:568: if (input_method) {
Should this and BlurTextInputClient (or
TextInputFocusManager::SetFocusedTextInputClient) do the other stuff in
InputMethodBase::SetFocusedTextInputClientInternal? ie.
OnWillChangeFocusedClient, OnDidChangeFocusedClient, and
NotifyTextInputStateChanged.

https://codereview.chromium.org/173803002/diff/200001/ui/views/focus/focus_ma...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.h:159: // Changes the text input focus to
|view->GetTextInputClient()| iff |view|
nit: Mention that views should call this when their internal TextInputClient
instance changes.

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
File ui/views/ime/null_input_method.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
ui/views/ime/null_input_method.cc:21: const base::NativeEvent& /* event */,
NativeEventResult* /* result */) {
nit: one parameter per line.

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
ui/views/ime/null_input_method.cc:36: return "";
nit: return std::string();

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
File ui/views/ime/null_input_method.h (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
ui/views/ime/null_input_method.h:9: #include "base/macros.h"
nit: is this explicitly needed here?

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
ui/views/ime/null_input_method.h:16: // We're working on removing
views::InputMethod and all its family and going
nit: I'm not sure what you mean by "family", please revise.

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
ui/views/ime/null_input_method.h:18: // come from views::InputMethodBridge by
replacing it with this instance,
nit: "that come from"

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
ui/views/ime/null_input_method.h:19: // without a big interface change.  This
class also enables that we can switch
This sentence and the above needs some grammar work; instead consider: "Use this
class instead of views::InputMethodBridge with ui::TextInputFocusManager to
effectively eliminate the views::InputMethod layer."

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:631: return new
NullInputMethod();
Is there an advantage to using NullInputMethod here, instead of NULL?

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1096:
views::FocusManager* focus_manager = GetWidget()->GetFocusManager();
If the View focused before the Widget was deactivated was "stored" (blurred on
deactivate) and is going to be focused on Widget activation, has that already
happened? Or is the focused view going to be NULL here?

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1098: } else if
(content_window_ == lost_focus) {
Should we explicitly clear the TextInputClient in this case?

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_handler.cc:189: current_window_ = xid;
q: Why must the order of operations be changed here?

[Yuki, 2014-03-11 15:27:36]

+nona@

Could you review this CL?

https://codereview.chromium.org/173803002/diff/200001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/173803002/diff/200001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6987: +      <message
name="IDS_FLAGS_NEW_TEXT_INPUT_FOCUS_HANDLING_NAME" desc="Title for the flag to
enable the new text input focus handling.">
On 2014/03/11 00:58:50, msw wrote:
> nit: consider IDS_FLAGS_TEXT_INPUT_FOCUS_MANAGER_NAME and s/handling/manager/

Done.

https://codereview.chromium.org/173803002/diff/200001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6988: +        Enable the new text input
focus handling.
On 2014/03/11 00:58:50, msw wrote:
> Consider: "Experimental text input focus manager."

Done.

https://codereview.chromium.org/173803002/diff/200001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6990: +      <message
name="IDS_FLAGS_NEW_TEXT_INPUT_FOCUS_HANDLING_DESCRIPTION" desc="Description for
the flag to enable the new text input focus handling.">
On 2014/03/11 00:58:50, msw wrote:
> nit: consider IDS_FLAGS_TEXT_INPUT_FOCUS_MANAGER_DESCRIPTION and
> s/handling/manager/

Done.

https://codereview.chromium.org/173803002/diff/200001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6991: +        Enable the re-designed text
input focus handling instead of using the old one.
On 2014/03/11 00:58:50, msw wrote:
> Consider "Enable an experimental focus manager to track text input clients."

Done.

https://codereview.chromium.org/173803002/diff/200001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/173803002/diff/200001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1900: "new-text-input-focus-handling",
On 2014/03/11 00:58:50, msw wrote:
> nit: text-input-focus-manager

Done.

https://codereview.chromium.org/173803002/diff/200001/content/public/browser/...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/173803002/diff/200001/content/public/browser/...
content/public/browser/render_widget_host_view.h:93: // Returns a
ui::TextInputClient to support text input or NULL if this RWHV
On 2014/03/11 00:58:50, msw wrote:
> nit: order after GetNativeView* or GetSelectedText() not with Views functions.

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/input_metho...
File ui/base/ime/input_method_base.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/input_metho...
ui/base/ime/input_method_base.cc:132: return;
On 2014/03/11 00:58:50, msw wrote:
> Will the input methods still work okay without calling
> On[Will|Did]ChangeFocusedClient and NotifyTextInputStateChanged? Those seem to
> cancel/confirm compositions, update other internal state, and notify input
> method observers.

It's okay.  views::FocusManager::{Focus,Blur}TextInputClient() take care of
those things.  We'll no longer need those methods.  They'll be removed in
future, and InputMethod will be simplified.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
File ui/base/ime/text_input_focus_manager.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.cc:26: void
TextInputFocusManager::UnsetFocusedTextInputClient(
On 2014/03/11 00:58:50, msw wrote:
> Is this function even necessary? A call could easily do this:
> if (client == manager->GetFocusedTextInputClient())
>   manager->SetFocusedTextInputClient(NULL);

What the client would like to say is "forget me.  good bye."

The implementation about how to forget should be hidden in
TextInputFocusManager.

I think, in general, that it's easier to introduce bugs when we allow (or force)
the client code to check the condition and do the things.  I don't like to trust
the client code too much.

Please consider these three methods as a sort of Get, Add and Remove.  Then it
seems natural to have Unset method here, doesn't it?

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
File ui/base/ime/text_input_focus_manager.h (right):

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:17: // The TextInputFocusManager class
manages which instance of TextInputClient
On 2014/03/11 00:58:50, msw wrote:
> nit: remove "The TextInputFocusManager class"; consider "Manages the focused
> TextInputClient across windows and their contents."

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:21: virtual ~TextInputFocusManager();
On 2014/03/11 00:58:50, msw wrote:
> nit: I don't think this needs to be virtual or public.

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:29: void
SetFocusedTextInputClient(TextInputClient* text_input_client);
On 2014/03/11 00:58:50, msw wrote:
> nit: consider "FocusTextInputClient"

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:33: void
UnsetFocusedTextInputClient(TextInputClient* text_input_client);
On 2014/03/11 00:58:50, msw wrote:
> nit: consider "BlurTextInputClient" or "UnfocusTextInputClient"

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:38: TextInputClient*
focused_text_input_client_;
On 2014/03/11 00:58:50, msw wrote:
> Is this better than views::FocusManager having a static TextInputClient*
member?

ui::InputMethod cannot access to views.  ui::InputMethod and ui::TextInputClient
were designed not to depend on views or anything else.  So
ui::TextInputFocusManager should be, too.

In the design discussion on the doc, people wanted them to be independent from
views.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:40: friend struct
DefaultSingletonTraits<TextInputFocusManager>;
On 2014/03/11 00:58:50, msw wrote:
> nit: friend types are commonly declared at the top of the private section.

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ui_base_switche...
File ui/base/ui_base_switches.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/base/ui_base_switche...
ui/base/ui_base_switches.cc:17: // Disables the new text input focus handling
and uses the old one.
On 2014/03/11 00:58:50, msw wrote:
> nit: match the language from my other suggestions, if you took them.

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ui_base_switches.h
File ui/base/ui_base_switches.h (right):

https://codereview.chromium.org/173803002/diff/200001/ui/base/ui_base_switche...
ui/base/ui_base_switches.h:20: UI_BASE_EXPORT extern const char
kDisableNewTextInputFocus[];
On 2014/03/11 00:58:50, msw wrote:
> nit: kDisableTextInputFocusManager

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ui_base_switche...
ui/base/ui_base_switches.h:24: UI_BASE_EXPORT extern const char
kEnableNewTextInputFocus[];
On 2014/03/11 00:58:50, msw wrote:
> nit: kEnableTextInputFocusManager

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ui_base_switche...
File ui/base/ui_base_switches_util.h (right):

https://codereview.chromium.org/173803002/diff/200001/ui/base/ui_base_switche...
ui/base/ui_base_switches_util.h:12: UI_BASE_EXPORT bool
IsNewTextInputFocusEnabled();
On 2014/03/11 00:58:50, msw wrote:
> nit: IsTextInputFocusManagerEnabled()

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:161: if
(!switches::IsNewTextInputFocusEnabled())
On 2014/03/11 00:58:50, msw wrote:
> nit: just make this condition part of the if statement below.

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:212: // WebView::GetTextInputClient()
delegates the text input handling to the
On 2014/03/11 00:58:50, msw wrote:
> This comment doesn't explain why you'd want to set the focused text input
client
> to |this| when the underlying render view host is deleted. I'm generally
> confused by what's happening here and why.

I've updated the comment trying to make it clearer.

Let me explain in pseudo code.

FocusManager::SetFocusedView(view) {
  TextInputClient* text_input_client = view->GetTextInputClient();
  // Remember the pointer of |text_input_client|.

  // Need to forget the pointer when the object dies.
}

WebView::GetTextInputClient() {
  return rwhv_->GetTextInputClient();
}

WebView::RenderViewDeleted() {
  // This method is called when |rwhv_| was deleted.
  // rwhv_->GetTextInputClient() must have been deleted, too.

  // Note that it's possible that |rwhv_| points to a new RWHV
  // and this WebView is still focused.

  // Tell the focus manager that |this->GetTextInputClient()|
  // will return a new TextInputClient.
  FocusManager->OnTextInputClientChanged(this);
}

https://codereview.chromium.org/173803002/diff/200001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.h (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/controls/webvi...
ui/views/controls/webview/webview.h:94: virtual ui::TextInputClient*
GetTextInputClient() OVERRIDE;
On 2014/03/11 00:58:50, msw wrote:
> nit: declare this below GetClassName, to follow View's decl order/visibility.

Done.

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

https://codereview.chromium.org/173803002/diff/200001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.cc:125: if (view != focused_view_)
On 2014/03/11 00:58:50, msw wrote:
> nit: reverse the condition and call Focus* in this block.

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.cc:568: if (input_method) {
On 2014/03/11 00:58:50, msw wrote:
> Should this and BlurTextInputClient (or
> TextInputFocusManager::SetFocusedTextInputClient) do the other stuff in
> InputMethodBase::SetFocusedTextInputClientInternal? ie.
> OnWillChangeFocusedClient, OnDidChangeFocusedClient, and
> NotifyTextInputStateChanged.

Thre is nothing to do.

We call InputMethod::OnTextInputTypeChanged().  The text input type was
changed...  This means either of 1) the focus changed or 2) the same text input
has the focus, but the type changed (unlikely though).  All of concrete
implementations of ui::InputMethod treat this event as if the focused changed. 
So there is nothing to do as long as we call OnTextInputTypeChanged().  It's
enough.

We can remove On{Will,Did}ChangeFocusedClient(), etc. once the transition gets
done.

https://codereview.chromium.org/173803002/diff/200001/ui/views/focus/focus_ma...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.h:159: // Changes the text input focus to
|view->GetTextInputClient()| iff |view|
On 2014/03/11 00:58:50, msw wrote:
> nit: Mention that views should call this when their internal TextInputClient
> instance changes.

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
File ui/views/ime/null_input_method.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
ui/views/ime/null_input_method.cc:21: const base::NativeEvent& /* event */,
NativeEventResult* /* result */) {
On 2014/03/11 00:58:50, msw wrote:
> nit: one parameter per line.

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
ui/views/ime/null_input_method.cc:36: return "";
On 2014/03/11 00:58:50, msw wrote:
> nit: return std::string();

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
File ui/views/ime/null_input_method.h (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
ui/views/ime/null_input_method.h:9: #include "base/macros.h"
On 2014/03/11 00:58:50, msw wrote:
> nit: is this explicitly needed here?

base/compiler_specific.h is for OVERRIDE macro.
base/macros.h is for DISALLOW_COPY_AND_ASSIGN macro.

Since views::InputMethod (input_method.h) doesn't use either of them, I thought
we should include them here.  However, ui/views/ime/input_method.h indirectly
includes them, so technically we don't need to include them explicitly here.

I think that depending indirect #include could be problematic.  If those
indirect headers are changed, there is no guarantee that this file compiles. 
So, in order to follow the include-what-you-use rule, I think we should include
them here explicitly.

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
ui/views/ime/null_input_method.h:16: // We're working on removing
views::InputMethod and all its family and going
On 2014/03/11 00:58:50, msw wrote:
> nit: I'm not sure what you mean by "family", please revise.

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
ui/views/ime/null_input_method.h:18: // come from views::InputMethodBridge by
replacing it with this instance,
On 2014/03/11 00:58:50, msw wrote:
> nit: "that come from"

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
ui/views/ime/null_input_method.h:19: // without a big interface change.  This
class also enables that we can switch
On 2014/03/11 00:58:50, msw wrote:
> This sentence and the above needs some grammar work; instead consider: "Use
this
> class instead of views::InputMethodBridge with ui::TextInputFocusManager to
> effectively eliminate the views::InputMethod layer."

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:631: return new
NullInputMethod();
On 2014/03/11 00:58:50, msw wrote:
> Is there an advantage to using NullInputMethod here, instead of NULL?

NULL doesn't work.  Some of (not few) clients assume that GetInputMethod()
always returns non-NULL.

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1096:
views::FocusManager* focus_manager = GetWidget()->GetFocusManager();
On 2014/03/11 00:58:50, msw wrote:
> If the View focused before the Widget was deactivated was "stored" (blurred on
> deactivate) and is going to be focused on Widget activation, has that already
> happened? Or is the focused view going to be NULL here?

OnWindowActivated() is called before OnWindowFocused(), and the stored view has
already been restored at this point.  Therefore
|focus_manager->GetFocusedView()| returns the focused view that we want.

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1098: } else if
(content_window_ == lost_focus) {
On 2014/03/11 00:58:50, msw wrote:
> Should we explicitly clear the TextInputClient in this case?

I think we don't need to do so.

Losing the focus means that someone else is getting the focus right away.  So
the new focused window's view will soon set the focused TextInputClient.  So
there is no need to clear the TextInputClient explicitly here.

Actually there are 3 scenarios.
1. Another Chrome window gets the focus. => No problem as I wrote above. 
Another view gets the focus.
2. Another application gets the focus. => No problem because Chrome doesn't get
any input.
3. Chrome is terminating. => No problem because everything is soon gone.

We can clear the TextInputClient here, but I don't like to put such code outside
of FocusManager as much as possible.

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_handler.cc:189: current_window_ = xid;
On 2014/03/11 00:58:50, msw wrote:
> q: Why must the order of operations be changed here?

I've added a comment.  The active widget must be changed before
HandleNativeWidgetActivationChanged(true) is called.

Handle...(false) behaves like Blur()
Handle...(true) behaves like Focus()
So between them, the active widget must be changed, so GetWidget()->IsActive()
returns the right value.

[msw, 2014-03-11 23:24:37]

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/input_metho...
File ui/base/ime/input_method_base.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/input_metho...
ui/base/ime/input_method_base.cc:132: return;
On 2014/03/11 15:27:37, Yuki wrote:
> On 2014/03/11 00:58:50, msw wrote:
> > Will the input methods still work okay without calling
> > On[Will|Did]ChangeFocusedClient and NotifyTextInputStateChanged? Those seem
to
> > cancel/confirm compositions, update other internal state, and notify input
> > method observers.
> 
> It's okay.  views::FocusManager::{Focus,Blur}TextInputClient() take care of
> those things.  We'll no longer need those methods.  They'll be removed in
> future, and InputMethod will be simplified.

Sounds good.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
File ui/base/ime/text_input_focus_manager.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.cc:26: void
TextInputFocusManager::UnsetFocusedTextInputClient(
On 2014/03/11 15:27:37, Yuki wrote:
> On 2014/03/11 00:58:50, msw wrote:
> > Is this function even necessary? A call could easily do this:
> > if (client == manager->GetFocusedTextInputClient())
> >   manager->SetFocusedTextInputClient(NULL);
> 
> What the client would like to say is "forget me.  good bye."
> 
> The implementation about how to forget should be hidden in
> TextInputFocusManager.
> 
> I think, in general, that it's easier to introduce bugs when we allow (or
force)
> the client code to check the condition and do the things.  I don't like to
trust
> the client code too much.
> 
> Please consider these three methods as a sort of Get, Add and Remove.  Then it
> seems natural to have Unset method here, doesn't it?

I'm on the fence about this, but I'll concede to your points.

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
File ui/base/ime/text_input_focus_manager.h (right):

https://codereview.chromium.org/173803002/diff/200001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:38: TextInputClient*
focused_text_input_client_;
On 2014/03/11 15:27:37, Yuki wrote:
> On 2014/03/11 00:58:50, msw wrote:
> > Is this better than views::FocusManager having a static TextInputClient*
> member?
> 
> ui::InputMethod cannot access to views.  ui::InputMethod and
ui::TextInputClient
> were designed not to depend on views or anything else.  So
> ui::TextInputFocusManager should be, too.
> 
> In the design discussion on the doc, people wanted them to be independent from
> views.

Sounds good.

https://codereview.chromium.org/173803002/diff/200001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:212: // WebView::GetTextInputClient()
delegates the text input handling to the
On 2014/03/11 15:27:37, Yuki wrote:
> On 2014/03/11 00:58:50, msw wrote:
> > This comment doesn't explain why you'd want to set the focused text input
> client
> > to |this| when the underlying render view host is deleted. I'm generally
> > confused by what's happening here and why.
> 
> I've updated the comment trying to make it clearer.
> 
> Let me explain in pseudo code.
> 
> FocusManager::SetFocusedView(view) {
>   TextInputClient* text_input_client = view->GetTextInputClient();
>   // Remember the pointer of |text_input_client|.
> 
>   // Need to forget the pointer when the object dies.
> }
> 
> WebView::GetTextInputClient() {
>   return rwhv_->GetTextInputClient();
> }
> 
> WebView::RenderViewDeleted() {
>   // This method is called when |rwhv_| was deleted.
>   // rwhv_->GetTextInputClient() must have been deleted, too.
> 
>   // Note that it's possible that |rwhv_| points to a new RWHV
>   // and this WebView is still focused.
> 
>   // Tell the focus manager that |this->GetTextInputClient()|
>   // will return a new TextInputClient.
>   FocusManager->OnTextInputClientChanged(this);
> }

That makes sense, thanks for the explanation and comment.

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

https://codereview.chromium.org/173803002/diff/200001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.cc:568: if (input_method) {
On 2014/03/11 15:27:37, Yuki wrote:
> On 2014/03/11 00:58:50, msw wrote:
> > Should this and BlurTextInputClient (or
> > TextInputFocusManager::SetFocusedTextInputClient) do the other stuff in
> > InputMethodBase::SetFocusedTextInputClientInternal? ie.
> > OnWillChangeFocusedClient, OnDidChangeFocusedClient, and
> > NotifyTextInputStateChanged.
> 
> Thre is nothing to do.
> 
> We call InputMethod::OnTextInputTypeChanged().  The text input type was
> changed...  This means either of 1) the focus changed or 2) the same text
input
> has the focus, but the type changed (unlikely though).  All of concrete
> implementations of ui::InputMethod treat this event as if the focused changed.

> So there is nothing to do as long as we call OnTextInputTypeChanged().  It's
> enough.
> 
> We can remove On{Will,Did}ChangeFocusedClient(), etc. once the transition gets
> done.

Sounds good.

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
File ui/views/ime/null_input_method.h (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/ime/null_input...
ui/views/ime/null_input_method.h:9: #include "base/macros.h"
On 2014/03/11 15:27:37, Yuki wrote:
> On 2014/03/11 00:58:50, msw wrote:
> > nit: is this explicitly needed here?
> 
> base/compiler_specific.h is for OVERRIDE macro.
> base/macros.h is for DISALLOW_COPY_AND_ASSIGN macro.
> 
> Since views::InputMethod (input_method.h) doesn't use either of them, I
thought
> we should include them here.  However, ui/views/ime/input_method.h indirectly
> includes them, so technically we don't need to include them explicitly here.
> 
> I think that depending indirect #include could be problematic.  If those
> indirect headers are changed, there is no guarantee that this file compiles. 
> So, in order to follow the include-what-you-use rule, I think we should
include
> them here explicitly.

Sounds good.

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:631: return new
NullInputMethod();
On 2014/03/11 15:27:37, Yuki wrote:
> On 2014/03/11 00:58:50, msw wrote:
> > Is there an advantage to using NullInputMethod here, instead of NULL?
> 
> NULL doesn't work.  Some of (not few) clients assume that GetInputMethod()
> always returns non-NULL.

Okay, then this is fine.

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1096:
views::FocusManager* focus_manager = GetWidget()->GetFocusManager();
On 2014/03/11 15:27:37, Yuki wrote:
> On 2014/03/11 00:58:50, msw wrote:
> > If the View focused before the Widget was deactivated was "stored" (blurred
on
> > deactivate) and is going to be focused on Widget activation, has that
already
> > happened? Or is the focused view going to be NULL here?
> 
> OnWindowActivated() is called before OnWindowFocused(), and the stored view
has
> already been restored at this point.  Therefore
> |focus_manager->GetFocusedView()| returns the focused view that we want.

Great, thanks for checking. This may not actually be necessary if the
FocusManager is guaranteed to restore Focus to its last-focused View on
Activate, but I think having this explicit call is better than hoping the
implicit focus restoration does the same thing (at least for now). You might
want to add a comment here, explaining what this does, like "Ensure the focused
view's TextInputClient is used for text input."

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1098: } else if
(content_window_ == lost_focus) {
On 2014/03/11 15:27:37, Yuki wrote:
> On 2014/03/11 00:58:50, msw wrote:
> > Should we explicitly clear the TextInputClient in this case?
> 
> I think we don't need to do so.
> 
> Losing the focus means that someone else is getting the focus right away.  So
> the new focused window's view will soon set the focused TextInputClient.  So
> there is no need to clear the TextInputClient explicitly here.
> 
> Actually there are 3 scenarios.
> 1. Another Chrome window gets the focus. => No problem as I wrote above. 
> Another view gets the focus.
> 2. Another application gets the focus. => No problem because Chrome doesn't
get
> any input.
> 3. Chrome is terminating. => No problem because everything is soon gone.
> 
> We can clear the TextInputClient here, but I don't like to put such code
outside
> of FocusManager as much as possible.

Hmm, I guess this should be okay; the focused View should be Blur()'ed on
deactivate anyway, and that'll do the same thing. I just have a slight
inclination to make this explicit like above, or make both implicit based on the
FocusManager doing a blur/focus on deactivate/active.

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_handler.cc:189: current_window_ = xid;
On 2014/03/11 15:27:37, Yuki wrote:
> On 2014/03/11 00:58:50, msw wrote:
> > q: Why must the order of operations be changed here?
> 
> I've added a comment.  The active widget must be changed before
> HandleNativeWidgetActivationChanged(true) is called.
> 
> Handle...(false) behaves like Blur()
> Handle...(true) behaves like Focus()
> So between them, the active widget must be changed, so GetWidget()->IsActive()
> returns the right value.

Okay, this may have weird implications for code checking the active window
during this process; I guess any problems can be addressed if/when they arise.

https://codereview.chromium.org/173803002/diff/220001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/173803002/diff/220001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:55: void
WebView::SetWebContents(content::WebContents* replacement) {
Should this also call OnTextInputClientChanged?

https://codereview.chromium.org/173803002/diff/220001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:118:
web_contents()->GetRenderWidgetHostView();
Should this ever use web_contents()->GetFullscreenRenderWidgetHostView()?

https://codereview.chromium.org/173803002/diff/220001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:226: void
WebView::WebContentsDestroyed(content::WebContents* web_contents) {
Should this also call OnTextInputClientChanged?

https://codereview.chromium.org/173803002/diff/220001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:247: void WebView::AttachWebContents() {
Should this also call OnTextInputClientChanged?

https://codereview.chromium.org/173803002/diff/220001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:275: void WebView::DetachWebContents() {
Should this also call OnTextInputClientChanged?

https://codereview.chromium.org/173803002/diff/220001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:297: void
WebView::ReattachForFullscreenChange(bool enter_fullscreen) {
Should this also call OnTextInputClientChanged?

https://codereview.chromium.org/173803002/diff/220001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right):

https://codereview.chromium.org/173803002/diff/220001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_handler.cc:189: // Set a new window id.
 This effectively changes the active widget to the new
nit: consider "Update the current window ID to effectively change the active
widget."

[Yuki, 2014-03-12 09:13:14]

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1096:
views::FocusManager* focus_manager = GetWidget()->GetFocusManager();
On 2014/03/11 23:24:37, msw wrote:
> On 2014/03/11 15:27:37, Yuki wrote:
> > On 2014/03/11 00:58:50, msw wrote:
> > > If the View focused before the Widget was deactivated was "stored"
(blurred
> on
> > > deactivate) and is going to be focused on Widget activation, has that
> already
> > > happened? Or is the focused view going to be NULL here?
> > 
> > OnWindowActivated() is called before OnWindowFocused(), and the stored view
> has
> > already been restored at this point.  Therefore
> > |focus_manager->GetFocusedView()| returns the focused view that we want.
> 
> Great, thanks for checking. This may not actually be necessary if the
> FocusManager is guaranteed to restore Focus to its last-focused View on
> Activate, but I think having this explicit call is better than hoping the
> implicit focus restoration does the same thing (at least for now). You might
> want to add a comment here, explaining what this does, like "Ensure the
focused
> view's TextInputClient is used for text input."

Done.

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1098: } else if
(content_window_ == lost_focus) {
On 2014/03/11 23:24:37, msw wrote:
> On 2014/03/11 15:27:37, Yuki wrote:
> > On 2014/03/11 00:58:50, msw wrote:
> > > Should we explicitly clear the TextInputClient in this case?
> > 
> > I think we don't need to do so.
> > 
> > Losing the focus means that someone else is getting the focus right away. 
So
> > the new focused window's view will soon set the focused TextInputClient.  So
> > there is no need to clear the TextInputClient explicitly here.
> > 
> > Actually there are 3 scenarios.
> > 1. Another Chrome window gets the focus. => No problem as I wrote above. 
> > Another view gets the focus.
> > 2. Another application gets the focus. => No problem because Chrome doesn't
> get
> > any input.
> > 3. Chrome is terminating. => No problem because everything is soon gone.
> > 
> > We can clear the TextInputClient here, but I don't like to put such code
> outside
> > of FocusManager as much as possible.
> 
> Hmm, I guess this should be okay; the focused View should be Blur()'ed on
> deactivate anyway, and that'll do the same thing. I just have a slight
> inclination to make this explicit like above, or make both implicit based on
the
> FocusManager doing a blur/focus on deactivate/active.

This has been a tough question for me.  Now I've changed my mind and put calls
to {Focus,Blur}TextInputClient() here and above.

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right):

https://codereview.chromium.org/173803002/diff/200001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_handler.cc:189: current_window_ = xid;
On 2014/03/11 23:24:37, msw wrote:
> On 2014/03/11 15:27:37, Yuki wrote:
> > On 2014/03/11 00:58:50, msw wrote:
> > > q: Why must the order of operations be changed here?
> > 
> > I've added a comment.  The active widget must be changed before
> > HandleNativeWidgetActivationChanged(true) is called.
> > 
> > Handle...(false) behaves like Blur()
> > Handle...(true) behaves like Focus()
> > So between them, the active widget must be changed, so
GetWidget()->IsActive()
> > returns the right value.
> 
> Okay, this may have weird implications for code checking the active window
> during this process; I guess any problems can be addressed if/when they arise.

Will address any issues that would arise.

FocusManager checks if the widget is active or not during the call to
HandleNativeWidgetActivationChanged(), which indirectly calls
GetWidget()->GetFocusManager()->RestoreFocusedView().  At that time, the widget
must be active, otherwise the FocusManager won't set the focused view's
TextInputClient.

https://codereview.chromium.org/173803002/diff/220001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/173803002/diff/220001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:55: void
WebView::SetWebContents(content::WebContents* replacement) {
On 2014/03/11 23:24:39, msw wrote:
> Should this also call OnTextInputClientChanged?

Very good catch.  I added it to {Attach,Detach}WebContents().

{Attach,Detach}WebContents are one-stop place that handles a web contents
changes.  SetWebContents() calls DetachWebContents and then AttachWebContents().

https://codereview.chromium.org/173803002/diff/220001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:118:
web_contents()->GetRenderWidgetHostView();
On 2014/03/11 23:24:39, msw wrote:
> Should this ever use web_contents()->GetFullscreenRenderWidgetHostView()?

Very good catch.  Updated.

https://codereview.chromium.org/173803002/diff/220001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:226: void
WebView::WebContentsDestroyed(content::WebContents* web_contents) {
On 2014/03/11 23:24:39, msw wrote:
> Should this also call OnTextInputClientChanged?

I think no need here.  SetWebContents(NULL) handles it.

In addition, if the web contents is destroyed, its RWHV should be destroyed. 
Then RenderViewDeleted() above should be called.

https://codereview.chromium.org/173803002/diff/220001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:247: void WebView::AttachWebContents() {
On 2014/03/11 23:24:39, msw wrote:
> Should this also call OnTextInputClientChanged?

Done.

https://codereview.chromium.org/173803002/diff/220001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:275: void WebView::DetachWebContents() {
On 2014/03/11 23:24:39, msw wrote:
> Should this also call OnTextInputClientChanged?

Done.

https://codereview.chromium.org/173803002/diff/220001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:297: void
WebView::ReattachForFullscreenChange(bool enter_fullscreen) {
On 2014/03/11 23:24:39, msw wrote:
> Should this also call OnTextInputClientChanged?

This method calls DetachWebContents() and then AttachWebConents().  So it's
okay.

https://codereview.chromium.org/173803002/diff/220001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right):

https://codereview.chromium.org/173803002/diff/220001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_handler.cc:189: // Set a new window id.
 This effectively changes the active widget to the new
On 2014/03/11 23:24:39, msw wrote:
> nit: consider "Update the current window ID to effectively change the active
> widget."

Done.

[msw, 2014-03-12 20:56:14]

This code definitely needs unit testing and additional reviewers. I'm
particularly concerned about the validity of the WebView changes. Otherwise,
this is looking pretty good.

https://codereview.chromium.org/173803002/diff/240001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/173803002/diff/240001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:212: // WebView::GetTextInputClient()
delegates the text input handling to the
With three nearly duplicate code and comment blocks, I suggest either:
A) Generalize the comment to mention any RWHV changes, put it in
WebView::GetTextInputClient, then remove the other comments (or put in stub
comments like "Update the TextInputClient as needed; see GetTextInputClient.")
or B) Make a helper function that does this with the comment there.

https://codereview.chromium.org/173803002/diff/240001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:295:
focus_manager->OnTextInputClientChanged(this);
Will WebView::GetTextInputClient return a different/NULL value yet?

https://codereview.chromium.org/173803002/diff/240001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:299: #if !defined(USE_AURA)
nit: Rebase, this code is no longer present.

https://codereview.chromium.org/173803002/diff/240001/ui/views/focus/focus_ma...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/173803002/diff/240001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.h:221: // Moves the text input focus into/out from
|view|.
nit: now that these are public, group them with OnTextInputClientChanged

[Seigo Nonaka, 2014-03-13 12:13:25]

The approach looks good to me but as msw say, could you add unit tests for
guarding further regressions?

https://codereview.chromium.org/173803002/diff/240001/ui/base/ime/text_input_...
File ui/base/ime/text_input_focus_manager.h (right):

https://codereview.chromium.org/173803002/diff/240001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:17: // Manages the focused
TextInputClient across windows and their contents.
This class seems not thread safe and only designed for UI thread, right?

If so, please add Thread Checker

[Yuki, 2014-03-14 15:25:21]

Thanks for the review.  I tried to add a unittest, but ... I've got stuck about
how to add what tests.

I understand we'd like to test "whenever web contents (or more exactly saying,
the underlying RWHV) changes, the focused text input client must be updated."
However, it's not clear when web contents are changed, especially considering
"whenever".  We can call SetWebContents(replacement) and see if the text input
client changed or not.  But is it a really meaningful test?  I don't think so. 
It doesn't mean that we covered all the cases.  We're not testing what we wanted
to test.

In addition, this new design is guarded by a flag and the flag is disabled by
default.  So we don't need to worry much about regressions.  My plan is to test
the new design behind a flag for the time being, and when we get confident with
the new design, we turn the flag on by default.

Considering toughness of adding unittests, I'm leaning to not introduce new
unittests for the changes in WebView.

What do you guys think?

https://codereview.chromium.org/173803002/diff/240001/ui/base/ime/text_input_...
File ui/base/ime/text_input_focus_manager.h (right):

https://codereview.chromium.org/173803002/diff/240001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:17: // Manages the focused
TextInputClient across windows and their contents.
On 2014/03/13 12:13:26, Seigo Nonaka wrote:
> This class seems not thread safe and only designed for UI thread, right?
> 
> If so, please add Thread Checker

Unfortunately, either of ui/base and ui/views cannot depend on
content/public/browser, so we cannot use a thread checker.

https://codereview.chromium.org/173803002/diff/240001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/173803002/diff/240001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:212: // WebView::GetTextInputClient()
delegates the text input handling to the
On 2014/03/12 20:56:15, msw wrote:
> With three nearly duplicate code and comment blocks, I suggest either:
> A) Generalize the comment to mention any RWHV changes, put it in
> WebView::GetTextInputClient, then remove the other comments (or put in stub
> comments like "Update the TextInputClient as needed; see GetTextInputClient.")
> or B) Make a helper function that does this with the comment there.

Done with option A.

https://codereview.chromium.org/173803002/diff/240001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:295:
focus_manager->OnTextInputClientChanged(this);
On 2014/03/12 20:56:15, msw wrote:
> Will WebView::GetTextInputClient return a different/NULL value yet?

Even after the call to wcv_holder_->Detach(), GetTextInputClient() returns the
same pointer (can be either of valid pointer or null).  However, I've moved the
code at the top of the function just in case.

https://codereview.chromium.org/173803002/diff/240001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:299: #if !defined(USE_AURA)
On 2014/03/12 20:56:15, msw wrote:
> nit: Rebase, this code is no longer present.

Done.

https://codereview.chromium.org/173803002/diff/240001/ui/views/focus/focus_ma...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/173803002/diff/240001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.h:221: // Moves the text input focus into/out from
|view|.
On 2014/03/12 20:56:15, msw wrote:
> nit: now that these are public, group them with OnTextInputClientChanged

Done.

[tfarina, 2014-03-14 19:58:55]

https://codereview.chromium.org/173803002/diff/240001/ui/base/ime/text_input_...
File ui/base/ime/text_input_focus_manager.h (right):

https://codereview.chromium.org/173803002/diff/240001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:17: // Manages the focused
TextInputClient across windows and their contents.
On 2014/03/14 15:25:22, Yuki wrote:
> On 2014/03/13 12:13:26, Seigo Nonaka wrote:
> > This class seems not thread safe and only designed for UI thread, right?
> > 
> > If so, please add Thread Checker
> 
> Unfortunately, either of ui/base and ui/views cannot depend on
> content/public/browser, so we cannot use a thread checker.
I'd say, 'hopefuly' :)

[Yuki, 2014-03-17 12:54:16]

+ben, sadrul,
Could you guys review this CL as owners?

ben:
  content/browser/renderer_host/
  content/public/browser/

sadrul:
  ui/views/

Thanks in advance.

[Seigo Nonaka, 2014-03-17 18:08:06]

https://codereview.chromium.org/173803002/diff/240001/ui/base/ime/text_input_...
File ui/base/ime/text_input_focus_manager.h (right):

https://codereview.chromium.org/173803002/diff/240001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:17: // Manages the focused
TextInputClient across windows and their contents.
On 2014/03/14 15:25:22, Yuki wrote:
> On 2014/03/13 12:13:26, Seigo Nonaka wrote:
> > This class seems not thread safe and only designed for UI thread, right?
> > 
> > If so, please add Thread Checker
> 
> Unfortunately, either of ui/base and ui/views cannot depend on
> content/public/browser, so we cannot use a thread checker.

thread checker is in base/threading so you can use it in anywhere :)

[msw, 2014-03-17 19:48:57]

It might be worthwhile to add automated tests to ensure that the expected
TextInputClient is focused when the user navigates, toggles fullscreen, etc.
Someone that knows WebView better should weigh in.

https://codereview.chromium.org/173803002/diff/240001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/173803002/diff/240001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:295:
focus_manager->OnTextInputClientChanged(this);
On 2014/03/14 15:25:22, Yuki wrote:
> On 2014/03/12 20:56:15, msw wrote:
> > Will WebView::GetTextInputClient return a different/NULL value yet?
> 
> Even after the call to wcv_holder_->Detach(), GetTextInputClient() returns the
> same pointer (can be either of valid pointer or null).  However, I've moved
the
> code at the top of the function just in case.

Actually, that was my concern. [Attach|Detach]WebContents or their callers
should change the output of GetTextInputClient(), to make these
OnTextInputClientChanged() calls necessary. It looks like SetWebContents changes
|web_contents()| and ReattachForFullscreenChange changes
|is_embedding_fullscreen_widget_|, which impact GetTextInputClient(). While
[Attach|Detach]WebContents are called within those functions, perhaps those
those functions themselves are be better places to call
OnTextInputClientChanged(). Hopefully someone that knows WebView better will
review, to give better advice.

Regardless, I would probably move |OnTextInputClientChanged| calls to the end of
whichever functions they are called from, so that any changes are already
applied when GetTextInputClient() is called.

[Yuki, 2014-04-16 09:24:07]

Sorry for leaving this CL for long time.

Could you take another look?  Thanks.

ben@, I believe you're the expert of views::WebView.  Could you review this CL
especially in ui/views/control/webview?

https://codereview.chromium.org/173803002/diff/240001/ui/base/ime/text_input_...
File ui/base/ime/text_input_focus_manager.h (right):

https://codereview.chromium.org/173803002/diff/240001/ui/base/ime/text_input_...
ui/base/ime/text_input_focus_manager.h:17: // Manages the focused
TextInputClient across windows and their contents.
On 2014/03/17 18:08:07, Seigo Nonaka wrote:
> On 2014/03/14 15:25:22, Yuki wrote:
> > On 2014/03/13 12:13:26, Seigo Nonaka wrote:
> > > This class seems not thread safe and only designed for UI thread, right?
> > > 
> > > If so, please add Thread Checker
> > 
> > Unfortunately, either of ui/base and ui/views cannot depend on
> > content/public/browser, so we cannot use a thread checker.
> 
> thread checker is in base/threading so you can use it in anywhere :)
> 

Done.

https://codereview.chromium.org/173803002/diff/240001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/173803002/diff/240001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:295:
focus_manager->OnTextInputClientChanged(this);
On 2014/03/17 19:48:58, msw wrote:
> On 2014/03/14 15:25:22, Yuki wrote:
> > On 2014/03/12 20:56:15, msw wrote:
> > > Will WebView::GetTextInputClient return a different/NULL value yet?
> > 
> > Even after the call to wcv_holder_->Detach(), GetTextInputClient() returns
the
> > same pointer (can be either of valid pointer or null).  However, I've moved
> the
> > code at the top of the function just in case.
> 
> Actually, that was my concern. [Attach|Detach]WebContents or their callers
> should change the output of GetTextInputClient(), to make these
> OnTextInputClientChanged() calls necessary. It looks like SetWebContents
changes
> |web_contents()| and ReattachForFullscreenChange changes
> |is_embedding_fullscreen_widget_|, which impact GetTextInputClient(). While
> [Attach|Detach]WebContents are called within those functions, perhaps those
> those functions themselves are be better places to call
> OnTextInputClientChanged(). Hopefully someone that knows WebView better will
> review, to give better advice.
> 
> Regardless, I would probably move |OnTextInputClientChanged| calls to the end
of
> whichever functions they are called from, so that any changes are already
> applied when GetTextInputClient() is called.

Thanks for the comment.  I'll add a webview reviewer.

As you suggested, I added a helper function NotifyMaybeTextInputClientChanged()
and made SetWebContents() and ReattachForFullscreenChange() call it.

[Yuki, 2014-04-18 04:41:43]

As a friendly reminder,
could you guys review this CL?

[msw, 2014-04-18 19:57:57]

https://codereview.chromium.org/173803002/diff/240001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/173803002/diff/240001/ui/views/views.gyp#newc...
ui/views/views.gyp:7: },
Are these changes from a bad rebase or something?

https://codereview.chromium.org/173803002/diff/310001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/173803002/diff/310001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7016: +      <message
name="IDS_FLAGS_NEW_TEXT_INPUT_FOCUS_HANDLING_NAME" desc="Title for the flag to
enable the new text input focus handling.">
Remove this redundant string...

https://codereview.chromium.org/173803002/diff/310001/content/public/browser/...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/173803002/diff/310001/content/public/browser/...
content/public/browser/render_widget_host_view.h:88: // doesn't support text
input.
nit: should this note that certain platforms are not expected to return a
ui::TextInputClient, even if they support text input, like mac and android?

https://codereview.chromium.org/173803002/diff/310001/ui/base/ui_base_switche...
File ui/base/ui_base_switches_util.h (right):

https://codereview.chromium.org/173803002/diff/310001/ui/base/ui_base_switche...
ui/base/ui_base_switches_util.h:18: class UI_BASE_EXPORT
ScopedTextInputFocusManagerForTesting {
Remove this and instead append kEnableTextInputFocusManager to the command line
in the WebView unit test if needed.

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:114: 
nit: remove blank line

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:343: FocusManager* const focus_manager =
GetFocusManager();
nit: remove const.

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
File ui/views/controls/webview/webview_tests.gyp (right):

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_tests.gyp:14:
'../../../../content/content_shell_and_tests.gyp:test_support_content',
Is it necessary to list the dependencies in common with webview_test_support, if
that itself is a dependency?

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_tests.gyp:26: 'webview_unittest.cc',
Could this instead simply be part of views_unittests?
(that target would probably need to add some dependencies)

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
File ui/views/controls/webview/webview_unittest.cc (right):

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:24: : widget_(NULL),
webview_(NULL),
nit: one initializer per line

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:62: views::View* container = new
views::View();
nit: is container needed, can you just widget_->SetContentsView(webview_);?

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:76:
scoped_ptr<content::TestBrowserContext> browser_context_;
nit: drop scoped_ptr for content::TestBrowserContext browser_context_;

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:77:
scoped_ptr<views::WebViewTestHelper> webview_test_helper_;
nit: drop scoped_ptr for views::WebViewTestHelper webview_test_helper_;

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:89: ui::TextInputFocusManager*
const text_input_focus_manager =
nit: this usage of const is uncommon, is it intended?

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:91: const ui::TextInputClient*
const null_text_input_client = NULL;
nit: this usage of const is uncommon, is it intended?

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:98: webview()->GetWebContents(); 
// Creates a new WebContents.
nit: remove comment, it's redundant with the one above.

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:106: content::WebContents*
web_contents = content::WebContents::Create(params);
Use a ScopedPtr, WebView doesn't take ownership below.

https://codereview.chromium.org/173803002/diff/310001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right):

https://codereview.chromium.org/173803002/diff/310001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_handler.cc:193: current_window_ = xid;
Can you explain why this change is needed for this CL? (just reply here and
perhaps explain in the CL description, which is woefully lacking in details).

[Yuki, 2014-04-22 09:09:01]

https://codereview.chromium.org/173803002/diff/240001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/173803002/diff/240001/ui/views/views.gyp#newc...
ui/views/views.gyp:7: },
On 2014/04/18 19:57:57, msw wrote:
> Are these changes from a bad rebase or something?

I don't understand what lines you're talking about.

null_input_method.{h,cc} are added intentionally.
input_method.h is just moved in alphabet order.

I don't see any other changes in this file.

https://codereview.chromium.org/173803002/diff/310001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/173803002/diff/310001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7016: +      <message
name="IDS_FLAGS_NEW_TEXT_INPUT_FOCUS_HANDLING_NAME" desc="Title for the flag to
enable the new text input focus handling.">
On 2014/04/18 19:57:57, msw wrote:
> Remove this redundant string...

Done.

https://codereview.chromium.org/173803002/diff/310001/content/public/browser/...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/173803002/diff/310001/content/public/browser/...
content/public/browser/render_widget_host_view.h:88: // doesn't support text
input.
On 2014/04/18 19:57:57, msw wrote:
> nit: should this note that certain platforms are not expected to return a
> ui::TextInputClient, even if they support text input, like mac and android?

Added a note.

https://codereview.chromium.org/173803002/diff/310001/ui/base/ui_base_switche...
File ui/base/ui_base_switches_util.h (right):

https://codereview.chromium.org/173803002/diff/310001/ui/base/ui_base_switche...
ui/base/ui_base_switches_util.h:18: class UI_BASE_EXPORT
ScopedTextInputFocusManagerForTesting {
On 2014/04/18 19:57:57, msw wrote:
> Remove this and instead append kEnableTextInputFocusManager to the command
line
> in the WebView unit test if needed.

Done.

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:114: 
On 2014/04/18 19:57:57, msw wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:343: FocusManager* const focus_manager =
GetFocusManager();
On 2014/04/18 19:57:57, msw wrote:
> nit: remove const.

I'm okay to remove this const, but this is the style used in this file.  I just
followed the style used on other lines in this file.  Should I remove 'const'
from all the lines which call GetFocusManager()?

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
File ui/views/controls/webview/webview_tests.gyp (right):

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_tests.gyp:14:
'../../../../content/content_shell_and_tests.gyp:test_support_content',
On 2014/04/18 19:57:57, msw wrote:
> Is it necessary to list the dependencies in common with webview_test_support,
if
> that itself is a dependency?

Removed, but it turned out that we need skiya.gyp:skia because webview.h
indirectly includes third_party/skia/include/utils/SkMatrix44.h.

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_tests.gyp:26: 'webview_unittest.cc',
On 2014/04/18 19:57:57, msw wrote:
> Could this instead simply be part of views_unittests?
> (that target would probably need to add some dependencies)

Seeing webview.gyp:webview is separated from views.gyp:views, I thought that we
should have webviews_unittest separated from views_unittest, too, in the same
manner.

We may not want to combine them.  I'll check it with tfarina@.

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
File ui/views/controls/webview/webview_unittest.cc (right):

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:24: : widget_(NULL),
webview_(NULL),
On 2014/04/18 19:57:57, msw wrote:
> nit: one initializer per line

After applying your comment, we don't need webview_test_helper_ here.  And now
everything can be on one line.

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:62: views::View* container = new
views::View();
On 2014/04/18 19:57:57, msw wrote:
> nit: is container needed, can you just widget_->SetContentsView(webview_);?

Done.

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:76:
scoped_ptr<content::TestBrowserContext> browser_context_;
On 2014/04/18 19:57:57, msw wrote:
> nit: drop scoped_ptr for content::TestBrowserContext browser_context_;

Done.

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:77:
scoped_ptr<views::WebViewTestHelper> webview_test_helper_;
On 2014/04/18 19:57:57, msw wrote:
> nit: drop scoped_ptr for views::WebViewTestHelper webview_test_helper_;

Done.

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:89: ui::TextInputFocusManager*
const text_input_focus_manager =
On 2014/04/18 19:57:57, msw wrote:
> nit: this usage of const is uncommon, is it intended?

Removed.  It was intentional, but I don't care much about this.

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:91: const ui::TextInputClient*
const null_text_input_client = NULL;
On 2014/04/18 19:57:57, msw wrote:
> nit: this usage of const is uncommon, is it intended?

Done.

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:98: webview()->GetWebContents(); 
// Creates a new WebContents.
On 2014/04/18 19:57:57, msw wrote:
> nit: remove comment, it's redundant with the one above.

Done.

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:106: content::WebContents*
web_contents = content::WebContents::Create(params);
On 2014/04/18 19:57:57, msw wrote:
> Use a ScopedPtr, WebView doesn't take ownership below.

Done.

https://codereview.chromium.org/173803002/diff/310001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right):

https://codereview.chromium.org/173803002/diff/310001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_handler.cc:193: current_window_ = xid;
On 2014/04/18 19:57:57, msw wrote:
> Can you explain why this change is needed for this CL? (just reply here and
> perhaps explain in the CL description, which is woefully lacking in details).

It's because we have to change the active widget before calling a callback
function HandleNativeWidgetActivationChanged(), so in that callback function,
|new_host|'s widget has already got activated.

Note that, in this CL, the active widget has an important role.  We allow the
active widget's focus manager to control the text input focus.  So it's
important if the widget is active or not.

Updated the description.

[msw, 2014-04-22 17:12:55]

Please update the TEST= to something like "run chrome with
--enable-text-input-focus-manager and ensure that text input in the omnibox,
find bar, bookmark bubble, bookmark edit dialog, and web content work as
expected." Otherwise, I just have one comment about appending the switch for the
test.

https://codereview.chromium.org/173803002/diff/240001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/173803002/diff/240001/ui/views/views.gyp#newc...
ui/views/views.gyp:7: },
On 2014/04/22 09:09:02, Yuki wrote:
> On 2014/04/18 19:57:57, msw wrote:
> > Are these changes from a bad rebase or something?
> 
> I don't understand what lines you're talking about.
> 
> null_input_method.{h,cc} are added intentionally.
> input_method.h is just moved in alphabet order.
> 
> I don't see any other changes in this file.

Weird, now I don't see any odd changes either, perhaps I was accidentally
diff'ing against an earlier patch set. Ignore that comment, sorry.

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:343: FocusManager* const focus_manager =
GetFocusManager();
On 2014/04/22 09:09:02, Yuki wrote:
> On 2014/04/18 19:57:57, msw wrote:
> > nit: remove const.
> 
> I'm okay to remove this const, but this is the style used in this file.  I
just
> followed the style used on other lines in this file.  Should I remove 'const'
> from all the lines which call GetFocusManager()?

Okay, the style guide says encourages putting const first (const FocusManager*
focus_manager), but does not require it; it's more important to be consistent
with the surrounding code. So, you can leave the const as-is here to be
consistent.

https://codereview.chromium.org/173803002/diff/350001/ui/views/controls/webvi...
File ui/views/controls/webview/run_all_unittests.cc (right):

https://codereview.chromium.org/173803002/diff/350001/ui/views/controls/webvi...
ui/views/controls/webview/run_all_unittests.cc:47:
cmd_line->AppendSwitch(switches::kEnableTextInputFocusManager);
Can this be done in webview_unittest.cc?

[Yuki, 2014-04-23 03:53:39]

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:343: FocusManager* const focus_manager =
GetFocusManager();
On 2014/04/22 17:12:56, msw wrote:
> On 2014/04/22 09:09:02, Yuki wrote:
> > On 2014/04/18 19:57:57, msw wrote:
> > > nit: remove const.
> > 
> > I'm okay to remove this const, but this is the style used in this file.  I
> just
> > followed the style used on other lines in this file.  Should I remove
'const'
> > from all the lines which call GetFocusManager()?
> 
> Okay, the style guide says encourages putting const first (const FocusManager*
> focus_manager), but does not require it; it's more important to be consistent
> with the surrounding code. So, you can leave the const as-is here to be
> consistent.

Thanks.

Just FYI, const T* and T* const have different effects while const T* and T
const* have the same effect.

const T* p = The value pointed to is const.  The pointer is not const, so ++p is
valid.

T* const p = The pointer is const, so ++p is invalid.  But the value pointed to
is not const, so *p = x is valid.

const T* const p = Both of the value pointed to and the pointer itself are
const.  Either of ++p or *p = x are invalid.

https://codereview.chromium.org/173803002/diff/350001/ui/views/controls/webvi...
File ui/views/controls/webview/run_all_unittests.cc (right):

https://codereview.chromium.org/173803002/diff/350001/ui/views/controls/webvi...
ui/views/controls/webview/run_all_unittests.cc:47:
cmd_line->AppendSwitch(switches::kEnableTextInputFocusManager);
On 2014/04/22 17:12:56, msw wrote:
> Can this be done in webview_unittest.cc?

Done.

[msw, 2014-04-23 04:50:11]

We shouldn't waste so much time on a nit :)

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/173803002/diff/310001/ui/views/controls/webvi...
ui/views/controls/webview/webview.cc:343: FocusManager* const focus_manager =
GetFocusManager();
On 2014/04/23 03:53:40, Yuki wrote:
> On 2014/04/22 17:12:56, msw wrote:
> > On 2014/04/22 09:09:02, Yuki wrote:
> > > On 2014/04/18 19:57:57, msw wrote:
> > > > nit: remove const.
> > > 
> > > I'm okay to remove this const, but this is the style used in this file.  I
> > just
> > > followed the style used on other lines in this file.  Should I remove
> 'const'
> > > from all the lines which call GetFocusManager()?
> > 
> > Okay, the style guide says encourages putting const first (const
FocusManager*
> > focus_manager), but does not require it; it's more important to be
consistent
> > with the surrounding code. So, you can leave the const as-is here to be
> > consistent.
> 
> Thanks.
> 
> Just FYI, const T* and T* const have different effects while const T* and T
> const* have the same effect.
> 
> const T* p = The value pointed to is const.  The pointer is not const, so ++p
is
> valid.
> 
> T* const p = The pointer is const, so ++p is invalid.  But the value pointed
to
> is not const, so *p = x is valid.
> 
> const T* const p = Both of the value pointed to and the pointer itself are
> const.  Either of ++p or *p = x are invalid.

Right, I'm aware of the significance, but I believe using only one const
qualifier to protect against mutation of the object is more prevalent. Here,
|const FocusManager* const p| is viable, but it's just not frequently done (at
least in my experience). |FocusManager* const| is only used twice (and only in
this file), whereas the other form occurs four times (in view and widget).

[Yuki, 2014-04-23 06:27:44]

+brettw@ as an owner of content/ directory.

Brett, could you review files under content/ directory?

[sadrul, 2014-04-23 14:22:17]

+sky@ since he commented on the design doc.

(I would've thought aura::client::FocusClient or wm::FocusController would also
need to change somehow)

[Ben Goodger (Google), 2014-04-23 19:59:21]

lgtm

https://codereview.chromium.org/173803002/diff/370001/ui/base/ui_base_switche...
File ui/base/ui_base_switches_util.cc (right):

https://codereview.chromium.org/173803002/diff/370001/ui/base/ui_base_switche...
ui/base/ui_base_switches_util.cc:20: ||
CommandLine::ForCurrentProcess()->HasSwitch(
nit: wrap || up

[Yuki, 2014-04-24 05:24:54]

https://codereview.chromium.org/173803002/diff/370001/ui/base/ui_base_switche...
File ui/base/ui_base_switches_util.cc (right):

https://codereview.chromium.org/173803002/diff/370001/ui/base/ui_base_switche...
ui/base/ui_base_switches_util.cc:20: ||
CommandLine::ForCurrentProcess()->HasSwitch(
On 2014/04/23 19:59:22, Ben Goodger (Google) wrote:
> nit: wrap || up

Done.
And removed unnecessary code that I forgot to remove.

[sky, 2014-04-24 16:04:49]

On 2014/04/23 14:22:17, sadrul wrote:
> +sky@ since he commented on the design doc.
> 
> (I would've thought aura::client::FocusClient or wm::FocusController would
also
> need to change somehow)

I think you're right and they should call into TextInputFocusManager as well.

[sky, 2014-04-24 16:05:04]

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

https://codereview.chromium.org/173803002/diff/410001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.cc:438: void
FocusManager::OnTextInputClientChanged(View* view) {
Wouldn't all of these functions fit better on view (as protected methods)?

[Ben Goodger (Google), 2014-04-24 16:07:00]

related: is there a design doc somewhere for how text input etc. is
supposed to work these days? can someone write one if not :-)

-Ben


On Thu, Apr 24, 2014 at 9:05 AM, <sky@chromium.org> wrote:

>
> https://codereview.chromium.org/173803002/diff/410001/ui/
> views/focus/focus_manager.cc
> File ui/views/focus/focus_manager.cc (right):
>
> https://codereview.chromium.org/173803002/diff/410001/ui/
> views/focus/focus_manager.cc#newcode438
> ui/views/focus/focus_manager.cc:438: void
> FocusManager::OnTextInputClientChanged(View* view) {
> Wouldn't all of these functions fit better on view (as protected
> methods)?
>
> https://codereview.chromium.org/173803002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[msw, 2014-04-24 16:48:12]

On 2014/04/24 16:07:00, Ben Goodger (Google) wrote:
> related: is there a design doc somewhere for how text input etc. is
> supposed to work these days? can someone write one if not :-)
> 
> -Ben

Yuki's design doc link is posted in the CL description:
https://docs.google.com/a/chromium.org/document/d/1tTcJJqh2SE8-9mEHjEzbILa_eb...

[Yuki, 2014-04-25 15:05:47]

> > (I would've thought aura::client::FocusClient or wm::FocusController would
> also
> > need to change somehow)
> 
> I think you're right and they should call into TextInputFocusManager as well.

With this CL, the focused View and TextInputClient are under control of
views::FocusManager, so we can't tell which TextInputClient should get focused
without views::FocusManager.  Since aura does not depend on views,
aura::client::FocusClient cannot access to views::FocusManager and it's hard to
tell which TextInputClient should get focused.

views::corewm::DesktopNativeWidgetAura implements
aura::client::{Activation,Focus}ChangeObserver, so
DesktopNativeWidgetAura::OnWindow{Activated,Focused}() are called from
aura::client::FocusClient::FocusWindow().  With this CL, we're handling the
focus change there where views::FocusManager is accessible because
DesktopNativeWidgetAura is in views::corewm.

So, we could say that what you're talking has already been done in
DesktopNativeWidgetAura::OnWindow{Activated,Focused}() and no change needed in
aura::client::FocusClient or wm::FocusController.

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

https://codereview.chromium.org/173803002/diff/410001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.cc:438: void
FocusManager::OnTextInputClientChanged(View* view) {
On 2014/04/24 16:05:05, sky wrote:
> Wouldn't all of these functions fit better on view (as protected methods)?

No, I don't think so.

I expect these functions to be called at only a few places.

OnTextInputClientChanged() is called from views::WebView only.  Other View (such
as Textfield) never change TextInputClient, so there is no need to call this
function, except for WebView.

{Focus,Blur}TextInputClient() are mostly called within FocusManager itself.  The
other client is DesktopNativeWidgetAura and there is no other client.

So I don't think it's worth putting these functions in views::View.

[sky, 2014-04-25 15:45:06]

On Fri, Apr 25, 2014 at 8:05 AM,  <yukishiino@chromium.org> wrote:
>> > (I would've thought aura::client::FocusClient or wm::FocusController
>> > would
>> also
>> > need to change somehow)
>
>
>> I think you're right and they should call into TextInputFocusManager as
>> well.
>
>
> With this CL, the focused View and TextInputClient are under control of
> views::FocusManager, so we can't tell which TextInputClient should get
> focused
> without views::FocusManager.  Since aura does not depend on views,
> aura::client::FocusClient cannot access to views::FocusManager and it's hard
> to
> tell which TextInputClient should get focused.
>
> views::corewm::DesktopNativeWidgetAura implements
> aura::client::{Activation,Focus}ChangeObserver, so
> DesktopNativeWidgetAura::OnWindow{Activated,Focused}() are called from
> aura::client::FocusClient::FocusWindow().  With this CL, we're handling the
> focus change there where views::FocusManager is accessible because
> DesktopNativeWidgetAura is in views::corewm.
>
> So, we could say that what you're talking has already been done in
> DesktopNativeWidgetAura::OnWindow{Activated,Focused}() and no change needed
> in
> aura::client::FocusClient or wm::FocusController.

I raised this issue in the design doc. All this code should work
regardless of using views or not. Changing the active aura::Window
should reset the textinputclient just as changing the focused view
does.

>
>
>
https://codereview.chromium.org/173803002/diff/410001/ui/views/focus/focus_ma...
> File ui/views/focus/focus_manager.cc (right):
>
>
https://codereview.chromium.org/173803002/diff/410001/ui/views/focus/focus_ma...
> ui/views/focus/focus_manager.cc:438: void
> FocusManager::OnTextInputClientChanged(View* view) {
> On 2014/04/24 16:05:05, sky wrote:
>>
>> Wouldn't all of these functions fit better on view (as protected
>
> methods)?
>
> No, I don't think so.
>
> I expect these functions to be called at only a few places.
>
> OnTextInputClientChanged() is called from views::WebView only.  Other
> View (such as Textfield) never change TextInputClient, so there is no
> need to call this function, except for WebView.
>
> {Focus,Blur}TextInputClient() are mostly called within FocusManager
> itself.  The other client is DesktopNativeWidgetAura and there is no
> other client.
>
> So I don't think it's worth putting these functions in views::View.

If it's only needed by WebView, move it there. If we ever need it
somewhere else it can be moved.

  -Scott

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yuki, 2014-04-25 17:48:36]

On 2014/04/25 15:45:06, sky wrote:
> On Fri, Apr 25, 2014 at 8:05 AM,  <mailto:yukishiino@chromium.org> wrote:
> >> > (I would've thought aura::client::FocusClient or wm::FocusController
> >> > would
> >> also
> >> > need to change somehow)
> >
> >
> >> I think you're right and they should call into TextInputFocusManager as
> >> well.
> >
> >
> > With this CL, the focused View and TextInputClient are under control of
> > views::FocusManager, so we can't tell which TextInputClient should get
> > focused
> > without views::FocusManager.  Since aura does not depend on views,
> > aura::client::FocusClient cannot access to views::FocusManager and it's hard
> > to
> > tell which TextInputClient should get focused.
> >
> > views::corewm::DesktopNativeWidgetAura implements
> > aura::client::{Activation,Focus}ChangeObserver, so
> > DesktopNativeWidgetAura::OnWindow{Activated,Focused}() are called from
> > aura::client::FocusClient::FocusWindow().  With this CL, we're handling the
> > focus change there where views::FocusManager is accessible because
> > DesktopNativeWidgetAura is in views::corewm.
> >
> > So, we could say that what you're talking has already been done in
> > DesktopNativeWidgetAura::OnWindow{Activated,Focused}() and no change needed
> > in
> > aura::client::FocusClient or wm::FocusController.
> 
> I raised this issue in the design doc. All this code should work
> regardless of using views or not. Changing the active aura::Window
> should reset the textinputclient just as changing the focused view
> does.

Well, then, what about Patch Set #15 ?


>
https://codereview.chromium.org/173803002/diff/410001/ui/views/focus/focus_ma...
> > File ui/views/focus/focus_manager.cc (right):
> >
> >
>
https://codereview.chromium.org/173803002/diff/410001/ui/views/focus/focus_ma...
> > ui/views/focus/focus_manager.cc:438: void
> > FocusManager::OnTextInputClientChanged(View* view) {
> > On 2014/04/24 16:05:05, sky wrote:
> >>
> >> Wouldn't all of these functions fit better on view (as protected
> >
> > methods)?
> >
> > No, I don't think so.
> >
> > I expect these functions to be called at only a few places.
> >
> > OnTextInputClientChanged() is called from views::WebView only.  Other
> > View (such as Textfield) never change TextInputClient, so there is no
> > need to call this function, except for WebView.
> >
> > {Focus,Blur}TextInputClient() are mostly called within FocusManager
> > itself.  The other client is DesktopNativeWidgetAura and there is no
> > other client.
> >
> > So I don't think it's worth putting these functions in views::View.
> 
> If it's only needed by WebView, move it there. If we ever need it
> somewhere else it can be moved.

Technically it's easy to move the function.  But I don't want WebView to
directly touch the implementation details of FocusManager.

I'd like FocusManager to control the text input focus, not View or WebView. 
Central control is my design goal.  So I don't want WebView to call
FocusManager::FocusTextInputClient().

If we moved the function to WebView, A WebView's bug (in that function) could
cause an issue of text input focus.  I'd like to have all text input focus
issues in FocusManager (otherwise, FocusClient or such things), not in View or
WebView.  Please let FocusManager keep the control of text input focus.

[sky, 2014-04-25 18:20:20]

Yes, patchset 15 seems right. Be sure and add test coverage to the wm/core side.

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

https://codereview.chromium.org/173803002/diff/410001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.cc:438: void
FocusManager::OnTextInputClientChanged(View* view) {
On 2014/04/25 15:05:48, Yuki wrote:
> On 2014/04/24 16:05:05, sky wrote:
> > Wouldn't all of these functions fit better on view (as protected methods)?
> 
> No, I don't think so.
> 
> I expect these functions to be called at only a few places.
> 
> OnTextInputClientChanged() is called from views::WebView only.  Other View
(such
> as Textfield) never change TextInputClient, so there is no need to call this
> function, except for WebView.
> 
> {Focus,Blur}TextInputClient() are mostly called within FocusManager itself. 
The
> other client is DesktopNativeWidgetAura and there is no other client.
> 
> So I don't think it's worth putting these functions in views::View.

I didn't realize FocusManager needs to use these functions too. Ok, leaving them
here makes sense.

https://codereview.chromium.org/173803002/diff/430001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/173803002/diff/430001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1082: if
(!switches::IsTextInputFocusManagerEnabled()) {
Can all this code be moved to Widget::OnNativeWidgetFocus/Blur?

https://codereview.chromium.org/173803002/diff/430001/ui/wm/core/focus_contro...
File ui/wm/core/focus_controller.cc (right):

https://codereview.chromium.org/173803002/diff/430001/ui/wm/core/focus_contro...
ui/wm/core/focus_controller.cc:143:
text_input_focus_manager->FocusTextInputClient(NULL);
Shouldn't all this be in SetFocusedWindow?

[sky, 2014-04-25 18:21:50]

https://codereview.chromium.org/173803002/diff/430001/ui/views/controls/webvi...
File ui/views/controls/webview/webview_unittest.cc (right):

https://codereview.chromium.org/173803002/diff/430001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:69: widget_->Activate();
If these rely on activation shouldn't they be in interactive ui tests? Otherwise
aren't they going to be racy?

[Yuki, 2014-04-28 12:59:28]

Will add unittests to wm/core/ in the next patch set.

https://codereview.chromium.org/173803002/diff/430001/ui/views/controls/webvi...
File ui/views/controls/webview/webview_unittest.cc (right):

https://codereview.chromium.org/173803002/diff/430001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:69: widget_->Activate();
On 2014/04/25 18:21:51, sky wrote:
> If these rely on activation shouldn't they be in interactive ui tests?
Otherwise
> aren't they going to be racy?

My understanding is that we have only one widget and one webview here, and we
don't run the message loop, so there shouldn't be a race condition.

The reason of the call to Activate() is because
FocusManager::FocusTextInputClient() checks widget_->IsActive().  That's the
only reason.

I want to keep this test small and simple as much as possible.  So I don't want
to change this to an interactive ui test as long as there is no problem.

https://codereview.chromium.org/173803002/diff/430001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/173803002/diff/430001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1082: if
(!switches::IsTextInputFocusManagerEnabled()) {
On 2014/04/25 18:20:21, sky wrote:
> Can all this code be moved to Widget::OnNativeWidgetFocus/Blur?

I'd like to guard the existing implementation with a new flag.  Since this code
has been here, I'd like to leave the code here so nothing changes when the flag
is disabled.

https://codereview.chromium.org/173803002/diff/430001/ui/wm/core/focus_contro...
File ui/wm/core/focus_controller.cc (right):

https://codereview.chromium.org/173803002/diff/430001/ui/wm/core/focus_contro...
ui/wm/core/focus_controller.cc:143:
text_input_focus_manager->FocusTextInputClient(NULL);
On 2014/04/25 18:20:21, sky wrote:
> Shouldn't all this be in SetFocusedWindow?

You're right.  Moved.

[sky, 2014-04-28 14:46:38]

https://codereview.chromium.org/173803002/diff/430001/ui/views/controls/webvi...
File ui/views/controls/webview/webview_unittest.cc (right):

https://codereview.chromium.org/173803002/diff/430001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:69: widget_->Activate();
On 2014/04/28 12:59:29, Yuki wrote:
> On 2014/04/25 18:21:51, sky wrote:
> > If these rely on activation shouldn't they be in interactive ui tests?
> Otherwise
> > aren't they going to be racy?
> 
> My understanding is that we have only one widget and one webview here, and we
> don't run the message loop, so there shouldn't be a race condition.
> 
> The reason of the call to Activate() is because
> FocusManager::FocusTextInputClient() checks widget_->IsActive().  That's the
> only reason.
> 
> I want to keep this test small and simple as much as possible.  So I don't
want
> to change this to an interactive ui test as long as there is no problem.

I agree with you on keeping the test small, but Activate and other similar
messages are going to effectively pump some messages. I think this can lead to a
racy test.

https://codereview.chromium.org/173803002/diff/430001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/173803002/diff/430001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1082: if
(!switches::IsTextInputFocusManagerEnabled()) {
On 2014/04/28 12:59:29, Yuki wrote:
> On 2014/04/25 18:20:21, sky wrote:
> > Can all this code be moved to Widget::OnNativeWidgetFocus/Blur?
> 
> I'd like to guard the existing implementation with a new flag.  Since this
code
> has been here, I'd like to leave the code here so nothing changes when the
flag
> is disabled.

Not sure I follow. You can still access the flag in Widget too. The reason this
should be in Widget is that way it'll take care of both NativeWidgetAura and
DesktopNativeWidgetAura.

[Yuki, 2014-05-01 14:11:50]

I added some tests into ui/wm/core/focus_controller_unittest.cc.

Could you take another look?

https://codereview.chromium.org/173803002/diff/430001/ui/views/controls/webvi...
File ui/views/controls/webview/webview_unittest.cc (right):

https://codereview.chromium.org/173803002/diff/430001/ui/views/controls/webvi...
ui/views/controls/webview/webview_unittest.cc:69: widget_->Activate();
On 2014/04/28 14:46:39, sky wrote:
> On 2014/04/28 12:59:29, Yuki wrote:
> > On 2014/04/25 18:21:51, sky wrote:
> > > If these rely on activation shouldn't they be in interactive ui tests?
> > Otherwise
> > > aren't they going to be racy?
> > 
> > My understanding is that we have only one widget and one webview here, and
we
> > don't run the message loop, so there shouldn't be a race condition.
> > 
> > The reason of the call to Activate() is because
> > FocusManager::FocusTextInputClient() checks widget_->IsActive().  That's the
> > only reason.
> > 
> > I want to keep this test small and simple as much as possible.  So I don't
> want
> > to change this to an interactive ui test as long as there is no problem.
> 
> I agree with you on keeping the test small, but Activate and other similar
> messages are going to effectively pump some messages. I think this can lead to
a
> racy test.

Okay, I've moved the test to interactive_ui_tests following the way of
WidgetTestInteractive in ui/views/widget/widget_interactive_uitest.cc.  Since
I'm not familiar with interactive_ui_tests, please let me know if I'm wrong with
something.

https://codereview.chromium.org/173803002/diff/430001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/173803002/diff/430001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1082: if
(!switches::IsTextInputFocusManagerEnabled()) {
On 2014/04/28 14:46:39, sky wrote:
> On 2014/04/28 12:59:29, Yuki wrote:
> > On 2014/04/25 18:20:21, sky wrote:
> > > Can all this code be moved to Widget::OnNativeWidgetFocus/Blur?
> > 
> > I'd like to guard the existing implementation with a new flag.  Since this
> code
> > has been here, I'd like to leave the code here so nothing changes when the
> flag
> > is disabled.
> 
> Not sure I follow. You can still access the flag in Widget too. The reason
this
> should be in Widget is that way it'll take care of both NativeWidgetAura and
> DesktopNativeWidgetAura.

Got it.  Moved the new code to Widget.

[sky, 2014-05-01 15:21:18]

https://codereview.chromium.org/173803002/diff/570001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/173803002/diff/570001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:29:
'../ui/views/controls/webview/webview_tests.gyp:webview_test_support',
Shouldn't you only add this dependency when using views?

https://codereview.chromium.org/173803002/diff/570001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:50:
'../ui/views/controls/webview/webview_interactive_uitest.cc',
How does this test get excluded on other platforms?

https://codereview.chromium.org/173803002/diff/570001/ui/views/controls/webvi...
File ui/views/controls/webview/webview_interactive_uitest.cc (right):

https://codereview.chromium.org/173803002/diff/570001/ui/views/controls/webvi...
ui/views/controls/webview/webview_interactive_uitest.cc:39: };
DISALLOW_...

https://codereview.chromium.org/173803002/diff/570001/ui/views/controls/webvi...
ui/views/controls/webview/webview_interactive_uitest.cc:41: #if
defined(USE_AURA)
You shouldn't need this ifdef anymore.

[Yuki, 2014-05-02 06:38:09]

https://codereview.chromium.org/173803002/diff/570001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/173803002/diff/570001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:29:
'../ui/views/controls/webview/webview_tests.gyp:webview_test_support',
On 2014/05/01 15:21:19, sky wrote:
> Shouldn't you only add this dependency when using views?

You're right.  Moved to 'toolkit_views==1' section.

https://codereview.chromium.org/173803002/diff/570001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:50:
'../ui/views/controls/webview/webview_interactive_uitest.cc',
On 2014/05/01 15:21:19, sky wrote:
> How does this test get excluded on other platforms?

I forgot it, but now I've found the following rules work fine. (around line 243)

        }, { # else: toolkit_views == 0
          'sources/': [
            ['exclude', '^browser/ui/views/'],
            ['exclude', '^../ui/views/'],
          ],
        }],

https://codereview.chromium.org/173803002/diff/570001/ui/views/controls/webvi...
File ui/views/controls/webview/webview_interactive_uitest.cc (right):

https://codereview.chromium.org/173803002/diff/570001/ui/views/controls/webvi...
ui/views/controls/webview/webview_interactive_uitest.cc:39: };
On 2014/05/01 15:21:19, sky wrote:
> DISALLOW_...

Done.

https://codereview.chromium.org/173803002/diff/570001/ui/views/controls/webvi...
ui/views/controls/webview/webview_interactive_uitest.cc:41: #if
defined(USE_AURA)
On 2014/05/01 15:21:19, sky wrote:
> You shouldn't need this ifdef anymore.

Done.

[sky, 2014-05-02 14:39:14]

LGTM

[msw, 2014-05-02 18:46:02]

LGTM with nits.

https://codereview.chromium.org/173803002/diff/580037/content/public/browser/...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/173803002/diff/580037/content/public/browser/...
content/public/browser/render_widget_host_view.h:86: // text input.  Some of
platforms (Mac and Android for example) use their own
nit: "Some platforms" or "Some of the platforms"

https://codereview.chromium.org/173803002/diff/580037/ui/views/controls/webvi...
File ui/views/controls/webview/webview_interactive_uitest.cc (right):

https://codereview.chromium.org/173803002/diff/580037/ui/views/controls/webvi...
ui/views/controls/webview/webview_interactive_uitest.cc:23: virtual void SetUp()
OVERRIDE {
nit q: is this SetUp override necessary for this test?
(I see that this is copied from WidgetTestInteractive)

https://codereview.chromium.org/173803002/diff/580037/ui/wm/core/focus_contro...
File ui/wm/core/focus_controller.cc (right):

https://codereview.chromium.org/173803002/diff/580037/ui/wm/core/focus_contro...
ui/wm/core/focus_controller.cc:277:
text_input_focus_manager->FocusTextInputClient(NULL);
nit: the value of |window| doesn't change from line 240, afaict. Is it possible
to just call text_input_focus_manager->FocusTextInputClient(NULL); once above,
regardless of the value of |window|? Perhaps I'm missing some nuance here.

https://codereview.chromium.org/173803002/diff/580037/ui/wm/core/focus_contro...
File ui/wm/core/focus_controller_unittest.cc (right):

https://codereview.chromium.org/173803002/diff/580037/ui/wm/core/focus_contro...
ui/wm/core/focus_controller_unittest.cc:524: ui::TextInputFocusManager*
text_input_focus_manager =
nit: would this new test code be better suited by a new test function?

[Yuki, 2014-05-07 05:58:01]

https://codereview.chromium.org/173803002/diff/580037/content/public/browser/...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/173803002/diff/580037/content/public/browser/...
content/public/browser/render_widget_host_view.h:86: // text input.  Some of
platforms (Mac and Android for example) use their own
On 2014/05/02 18:46:03, msw wrote:
> nit: "Some platforms" or "Some of the platforms"

Done.

https://codereview.chromium.org/173803002/diff/580037/ui/views/controls/webvi...
File ui/views/controls/webview/webview_interactive_uitest.cc (right):

https://codereview.chromium.org/173803002/diff/580037/ui/views/controls/webvi...
ui/views/controls/webview/webview_interactive_uitest.cc:23: virtual void SetUp()
OVERRIDE {
On 2014/05/02 18:46:03, msw wrote:
> nit q: is this SetUp override necessary for this test?
> (I see that this is copied from WidgetTestInteractive)

It seems we can simplify this code because we don't need a resource bundle, but
GL surface initialization seems needed for (probably) web contents.  This test
doesn't run without initializing GL surface.

https://codereview.chromium.org/173803002/diff/580037/ui/wm/core/focus_contro...
File ui/wm/core/focus_controller.cc (right):

https://codereview.chromium.org/173803002/diff/580037/ui/wm/core/focus_contro...
ui/wm/core/focus_controller.cc:277:
text_input_focus_manager->FocusTextInputClient(NULL);
On 2014/05/02 18:46:03, msw wrote:
> nit: the value of |window| doesn't change from line 240, afaict. Is it
possible
> to just call text_input_focus_manager->FocusTextInputClient(NULL); once above,
> regardless of the value of |window|? Perhaps I'm missing some nuance here.

Some observers above might change the focused text input client (if so, it must
be NULL though), we'd like to make sure here that the focused text input client
is NULL when losing the window focus regardless of observer's behavior.

https://codereview.chromium.org/173803002/diff/580037/ui/wm/core/focus_contro...
File ui/wm/core/focus_controller_unittest.cc (right):

https://codereview.chromium.org/173803002/diff/580037/ui/wm/core/focus_contro...
ui/wm/core/focus_controller_unittest.cc:524: ui::TextInputFocusManager*
text_input_focus_manager =
On 2014/05/02 18:46:03, msw wrote:
> nit: would this new test code be better suited by a new test function?

Done.  Created FocusedTextInputClient as a test function and moved the test code
into it.

[msw, 2014-05-07 06:50:14]

lgtm with one last nit

https://codereview.chromium.org/173803002/diff/620001/ui/wm/core/focus_contro...
File ui/wm/core/focus_controller_unittest.cc (right):

https://codereview.chromium.org/173803002/diff/620001/ui/wm/core/focus_contro...
ui/wm/core/focus_controller_unittest.cc:831:
text_input_focus_manager->FocusTextInputClient(&text_input_client);
nit: Add an EXPECT_EQ(&text_input_client,
text_input_focus_manager->GetFocusedTextInputClient()); after this statement.

[Yuki, 2014-05-07 07:05:18]

Thanks for your kind review.

https://codereview.chromium.org/173803002/diff/620001/ui/wm/core/focus_contro...
File ui/wm/core/focus_controller_unittest.cc (right):

https://codereview.chromium.org/173803002/diff/620001/ui/wm/core/focus_contro...
ui/wm/core/focus_controller_unittest.cc:831:
text_input_focus_manager->FocusTextInputClient(&text_input_client);
On 2014/05/07 06:50:15, msw wrote:
> nit: Add an EXPECT_EQ(&text_input_client,
> text_input_focus_manager->GetFocusedTextInputClient()); after this statement.

Done.

[Yuki, 2014-05-07 07:35:09]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-05-07 07:38:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yukishiino@chromium.org/173803002/660001

[commit-bot: I haz the power, 2014-05-07 20:14:29]

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

[commit-bot: I haz the power, 2014-05-07 20:14:30]

Failed to apply patch for chrome/browser/about_flags.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/about_flags.cc
  Hunk #1 FAILED at 1842.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/about_flags.cc.rej

Patch:       chrome/browser/about_flags.cc
Index: chrome/browser/about_flags.cc
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index
fc66aa990ff5ea72adcce080b816b30497701928..c9235a2b854943ca99d50eef4b81c4d67d421691
100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -1842,6 +1842,16 @@ const Experiment kExperiments[] = {
     SINGLE_VALUE_TYPE(switches::kEnableAppInstallAlerts)
   },
 #endif
+#if defined(USE_AURA)
+  {
+    "text-input-focus-manager",
+    IDS_FLAGS_TEXT_INPUT_FOCUS_MANAGER_NAME,
+    IDS_FLAGS_TEXT_INPUT_FOCUS_MANAGER_DESCRIPTION,
+    kOsCrOS | kOsLinux | kOsWin,
+    ENABLE_DISABLE_VALUE_TYPE(switches::kEnableTextInputFocusManager,
+                              switches::kDisableTextInputFocusManager)
+  },
+#endif
 };
 
 const Experiment* experiments = kExperiments;

[Yuki, 2014-05-08 03:31:58]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-05-08 03:34:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yukishiino@chromium.org/173803002/680001

[commit-bot: I haz the power, 2014-05-08 23:51:57]

Change committed as 269106

[falken, 2014-05-09 02:26:55]

On 2014/05/08 23:51:57, I haz the power (commit-bot) wrote:
> Change committed as 269106

Is it possible this patch made interactive_ui_tests fail?

Log:
http://build.chromium.org/p/chromium.webkit/builders/Win7%20%28dbg%29/builds/...

Tests:
ClickingMovesFocus
PopupStaysClosed

Snippet:
[4368:4808:0507/212523:FATAL:omnibox_edit_model.cc(772)] Check failed:
!last_omnibox_focus_.is_null(). An omnibox focus should have occurred before
opening a match.
Backtrace:
	base::debug::StackTrace::StackTrace [0x0AAB9F11+33]
	logging::LogMessage::~LogMessage [0x0AB6031E+94]
	OmniboxEditModel::OpenMatch [0x034B76AD+2173]
	OmniboxView::OpenMatch [0x034C27D8+184]
	OmniboxEditModel::AcceptInput [0x034B1810+1744]
	LocationBarView::AcceptInput [0x037B4647+39]

[falken, 2014-05-09 02:43:42]

On 2014/05/09 02:26:55, falken wrote:
> On 2014/05/08 23:51:57, I haz the power (commit-bot) wrote:
> > Change committed as 269106
> 
> Is it possible this patch made interactive_ui_tests fail?
> 
> Log:
>
http://build.chromium.org/p/chromium.webkit/builders/Win7%20%28dbg%29/builds/...
> 
> Tests:
> ClickingMovesFocus
> PopupStaysClosed
> 
> Snippet:
> [4368:4808:0507/212523:FATAL:omnibox_edit_model.cc(772)] Check failed:
> !last_omnibox_focus_.is_null(). An omnibox focus should have occurred before
> opening a match.
> Backtrace:
> 	base::debug::StackTrace::StackTrace [0x0AAB9F11+33]
> 	logging::LogMessage::~LogMessage [0x0AB6031E+94]
> 	OmniboxEditModel::OpenMatch [0x034B76AD+2173]
> 	OmniboxView::OpenMatch [0x034C27D8+184]
> 	OmniboxEditModel::AcceptInput [0x034B1810+1744]
> 	LocationBarView::AcceptInput [0x037B4647+39]

Sorry, I think I made a false alarm. The suspected Chromium revision range is
268997:269024 which this patch doesn't fall in.