Refactoring the ownership of ui::InputMethod.

Refactoring the ownership of ui::InputMethod.

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

This cl includes some fundamental changes:
1) ui::InputMethod is created & owned by aura::WindowTreeHost.
   Therefore, the InputMethod instance can be accessed through
   root_window->GetHost()->GetInputMethod, and no need to save
   kRootWindowInputMethodKey instance to the root window
   anymore.
2) Removed InputMethodEventFilter, therefore no need the flag
   IsTranslated()/SetTranslated() on ui::KeyEvent.
3) Makes aura::WindowTreeHost as a ui::EventSource, because all
   kinds of WindowTreeHost are also ui::EventSource. Therefore,
   WindowTreeHost can override DeliverEventToProcessor method
   to intercept key events for input method processing.

Note:
1) The key events processing by input method happens after event
   rewriters and before the EventHandler's, which remains no
   change to original on desktop environment. For Ash, the IME key
   event handling is moved ahead of some other handles:
   - MagnifierKeyScroller (CrOS only): handles Arrow keys when
     magnification is enabled.
   - SpokenFeedbackToggler (CrOS only): handles F6 key when
     SpokenFeedbackToggler is enabled.
   - OverlayEventFilter: handles ESC/OEM_2/HELP/F14 keys to
     cancel the overlay UI.
2) For Ash, remains the singleton ui::InputMethod instance,
   owned by DisplayController. Because if let each
   AshWindowTreeHost own separated ui::InputMethod instances,
   it will break things:
   a) There is a test: ExtendedDesktopTest.KeyEventsOnLockScreen,
      which tests the text field in root window X can be inserted
      with text while dispatching key events to root window Y. I
      think this test makes no sense and will confirm with oshima@.
   b) For now for Ozone, DrmWindowHost::CanDispatchEvent()
      always returns true, so that only primary window tree host
      can receive the key events, which makes the "primary"
      InputMethod can receive the key events.
   c) For Virtual Keyboard on CrOS, it listens to the notification
      through InputMethodObserver::OnShowImeIfNeeded(), which is
      bound to the specific InputMethod instance. However, the
      Virtual Keyboard should work cross multiple root windows &
      InputMethod instances.

BUG=474828
TEST=Verified on local builds, including linux, cros, win7 & win8.1. And all tests passed.

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

[Shu Chen, 2015-06-01 05:30:28]

shuchen@chromium.org changed reviewers:
+ avi@chromium.org, jamescook@chromium.org, sky@chromium.org, suzhe@chromium.org

[Shu Chen, 2015-06-01 05:30:30]

Hi Guys, can you please review this cl?

To dive into such big CL quickly, please review it by the following sequence:
1) ui/aura/window_tree_host*
2) ui/views/widget/desktop_aura/*
3) ash/display/display_controller.*
4) The rest changes just follow the major changes above.

Thanks!

[Avi (use Gerrit), 2015-06-01 15:10:50]

On 2015/06/01 05:30:30, Shu Chen wrote:
> Hi Guys, can you please review this cl?
> 
> To dive into such big CL quickly, please review it by the following sequence:
> 1) ui/aura/window_tree_host*
> 2) ui/views/widget/desktop_aura/*
> 3) ash/display/display_controller.*
> 4) The rest changes just follow the major changes above.
> 
> Thanks!

Please specify which reviewers you want to review which files.

[Avi (use Gerrit), 2015-06-01 15:13:40]

Also, random nits.

(You should use nullptr rather than NULL.)

https://codereview.chromium.org/1155013005/diff/360001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1155013005/diff/360001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2471: //return
root_window->GetProperty(aura::client::kRootWindowInputMethodKey);
don't commit commented-out code

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:291: CHECK(!details.dispatcher_destroyed);
DCHECK

[James Cook, 2015-06-01 22:21:01]

Thank you for taking on a complex refactor to clean up the code.

I'm not an expert on input methods -- I focused on your changes in ash/* with a
few comments elsewhere.

https://codereview.chromium.org/1155013005/diff/360001/ash/magnifier/magnific...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/1155013005/diff/360001/ash/magnifier/magnific...
ash/magnifier/magnification_controller.cc:68: return NULL;
nullptr

https://codereview.chromium.org/1155013005/diff/360001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/1155013005/diff/360001/ash/shell.cc#newcode741
ash/shell.cc:741: keyboard::KeyboardController::ResetInstance(NULL);
This used to run on non-Chrome OS platforms. Are you sure it's only needed on
Chrome OS?

Also, nullptr

https://codereview.chromium.org/1155013005/diff/360001/ash/wm/window_manager_...
File ash/wm/window_manager_unittest.cc (right):

https://codereview.chromium.org/1155013005/diff/360001/ash/wm/window_manager_...
ash/wm/window_manager_unittest.cc:136: // The IME event filter interferes with
the basic key event propagation we
Remove this comment.

https://codereview.chromium.org/1155013005/diff/360001/ash/wm/window_manager_...
ash/wm/window_manager_unittest.cc:138: test::ShellTestApi
shell_test(Shell::GetInstance());
Do you still need this variable?

https://codereview.chromium.org/1155013005/diff/360001/ash/wm/window_manager_...
ash/wm/window_manager_unittest.cc:653: test::ShellTestApi
shell_test(Shell::GetInstance());
same comments

https://codereview.chromium.org/1155013005/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/input_method/textinput_test_helper.cc (right):

https://codereview.chromium.org/1155013005/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/textinput_test_helper.cc:15: #include
"ui/aura/client/aura_constants.h"
Here and elsewhere that you've removed aura::client::kRootWindowInputMethodKey
-- do you still need to #include this header?

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:47: input_method_ = NULL;
nullptr

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:202: if (input_method_)
Should it be an error and/or warning to call this twice?

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.h:14: #include "ui/base/ime/input_method.h"
optional: you could forward-declare ui::InputMethod

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.h:125: void SetInputMethod(ui::InputMethod*
input_method);
Document how this behaves with respect to ownership.

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.h:210: ui::InputMethod* input_method_;
Document how ownership works for this member.

https://codereview.chromium.org/1155013005/diff/360001/ui/base/accelerators/a...
File ui/base/accelerators/accelerator_manager_unittest.cc (left):

https://codereview.chromium.org/1155013005/diff/360001/ui/base/accelerators/a...
ui/base/accelerators/accelerator_manager_unittest.cc:155:
EXPECT_FALSE(manager_.Process(accelerator)) << text;  // different type
Just curious, why aren't these needed anymore? It seems odd that there's a
KEY_RELEASED but not a KEY_PRESSED.

https://codereview.chromium.org/1155013005/diff/360001/ui/events/event.h
File ui/events/event.h (left):

https://codereview.chromium.org/1155013005/diff/360001/ui/events/event.h#oldc...
ui/events/event.h:749: bool IsTranslated() const;
Seeing things like this being removed makes me happy!

https://codereview.chromium.org/1155013005/diff/360001/ui/keyboard/keyboard_u...
File ui/keyboard/keyboard_util.cc (right):

https://codereview.chromium.org/1155013005/diff/360001/ui/keyboard/keyboard_u...
ui/keyboard/keyboard_util.cc:17: #include "ui/aura/client/aura_constants.h"
Can this be removed?

[Shu Chen, 2015-06-02 04:11:17]

https://codereview.chromium.org/1155013005/diff/360001/ash/magnifier/magnific...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/1155013005/diff/360001/ash/magnifier/magnific...
ash/magnifier/magnification_controller.cc:68: return NULL;
On 2015/06/01 22:20:59, James Cook wrote:
> nullptr

Done.

https://codereview.chromium.org/1155013005/diff/360001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/1155013005/diff/360001/ash/shell.cc#newcode741
ash/shell.cc:741: keyboard::KeyboardController::ResetInstance(NULL);
On 2015/06/01 22:21:00, James Cook wrote:
> This used to run on non-Chrome OS platforms. Are you sure it's only needed on
> Chrome OS?
> 
> Also, nullptr

Done. Thanks for catching this. Btw, the destruction of KeyboardController needs
to be before the destruction of DisplayController.

https://codereview.chromium.org/1155013005/diff/360001/ash/wm/window_manager_...
File ash/wm/window_manager_unittest.cc (right):

https://codereview.chromium.org/1155013005/diff/360001/ash/wm/window_manager_...
ash/wm/window_manager_unittest.cc:136: // The IME event filter interferes with
the basic key event propagation we
On 2015/06/01 22:21:00, James Cook wrote:
> Remove this comment.

Done.

https://codereview.chromium.org/1155013005/diff/360001/ash/wm/window_manager_...
ash/wm/window_manager_unittest.cc:138: test::ShellTestApi
shell_test(Shell::GetInstance());
On 2015/06/01 22:21:00, James Cook wrote:
> Do you still need this variable?

Done.

https://codereview.chromium.org/1155013005/diff/360001/ash/wm/window_manager_...
ash/wm/window_manager_unittest.cc:653: test::ShellTestApi
shell_test(Shell::GetInstance());
On 2015/06/01 22:21:00, James Cook wrote:
> same comments

Done.

https://codereview.chromium.org/1155013005/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/input_method/textinput_test_helper.cc (right):

https://codereview.chromium.org/1155013005/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/textinput_test_helper.cc:15: #include
"ui/aura/client/aura_constants.h"
On 2015/06/01 22:21:00, James Cook wrote:
> Here and elsewhere that you've removed aura::client::kRootWindowInputMethodKey
> -- do you still need to #include this header?

Done.

https://codereview.chromium.org/1155013005/diff/360001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1155013005/diff/360001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2471: //return
root_window->GetProperty(aura::client::kRootWindowInputMethodKey);
On 2015/06/01 15:13:39, Avi wrote:
> don't commit commented-out code

Done.

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:47: input_method_ = NULL;
On 2015/06/01 22:21:00, James Cook wrote:
> nullptr

Done.

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:202: if (input_method_)
On 2015/06/01 22:21:00, James Cook wrote:
> Should it be an error and/or warning to call this twice?

Done.

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:291: CHECK(!details.dispatcher_destroyed);
On 2015/06/01 15:13:39, Avi wrote:
> DCHECK

Done.

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.h:14: #include "ui/base/ime/input_method.h"
On 2015/06/01 22:21:00, James Cook wrote:
> optional: you could forward-declare ui::InputMethod

Done.

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.h:125: void SetInputMethod(ui::InputMethod*
input_method);
On 2015/06/01 22:21:00, James Cook wrote:
> Document how this behaves with respect to ownership.

Done.

https://codereview.chromium.org/1155013005/diff/360001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.h:210: ui::InputMethod* input_method_;
On 2015/06/01 22:21:00, James Cook wrote:
> Document how ownership works for this member.

Done.

https://codereview.chromium.org/1155013005/diff/360001/ui/base/accelerators/a...
File ui/base/accelerators/accelerator_manager_unittest.cc (left):

https://codereview.chromium.org/1155013005/diff/360001/ui/base/accelerators/a...
ui/base/accelerators/accelerator_manager_unittest.cc:155:
EXPECT_FALSE(manager_.Process(accelerator)) << text;  // different type
On 2015/06/01 22:21:00, James Cook wrote:
> Just curious, why aren't these needed anymore? It seems odd that there's a
> KEY_RELEASED but not a KEY_PRESSED.

The line 148 is to verify EXPECT_TRUE for KEY_PRESSED.
These lines are to verify EXPECT_FALSE's.

https://codereview.chromium.org/1155013005/diff/360001/ui/events/event.h
File ui/events/event.h (left):

https://codereview.chromium.org/1155013005/diff/360001/ui/events/event.h#oldc...
ui/events/event.h:749: bool IsTranslated() const;
On 2015/06/01 22:21:00, James Cook wrote:
> Seeing things like this being removed makes me happy!

Me too! :-)

https://codereview.chromium.org/1155013005/diff/360001/ui/keyboard/keyboard_u...
File ui/keyboard/keyboard_util.cc (right):

https://codereview.chromium.org/1155013005/diff/360001/ui/keyboard/keyboard_u...
ui/keyboard/keyboard_util.cc:17: #include "ui/aura/client/aura_constants.h"
On 2015/06/01 22:21:00, James Cook wrote:
> Can this be removed?

Yes, it can be remvoed.

[Shu Chen, 2015-06-02 04:16:21]

For each reviewer:

sky@ please focus on overall design.
avi@ please focus on aura/views stuff.
jamescook@ please focus on ash stuff.

Below is the detailed list from "git cl owner". Thanks!

avi:
  content/browser/renderer_host/render_widget_host_view_aura.cc
  content/shell/browser/shell_platform_data_aura.cc
  content/shell/browser/shell_platform_data_aura.h
  ui/aura/client/aura_constants.cc
  ui/aura/client/aura_constants.h
  ui/aura/remote_window_tree_host_win.cc
  ui/aura/remote_window_tree_host_win.h
  ui/aura/test/aura_test_helper.cc
  ui/aura/test/aura_test_helper.h
  ui/aura/window_tree_host.cc
  ui/aura/window_tree_host.h
  ui/aura/window_tree_host_ozone.cc
  ui/aura/window_tree_host_ozone.h
  ui/aura/window_tree_host_win.cc
  ui/aura/window_tree_host_win.h
  ui/aura/window_tree_host_x11.cc
  ui/aura/window_tree_host_x11.h
  ui/base/accelerators/accelerator_manager_unittest.cc
  ui/base/ime/input_method_chromeos.cc
  ui/base/ime/input_method_factory.cc
  ui/base/ime/mock_input_method.cc
  ui/events/event.cc
  ui/events/event.h
  ui/events/event_constants.h
  ui/events/event_source.h
  ui/events/event_utils.cc
  ui/keyboard/keyboard_util.cc
  ui/views/ime/input_method_bridge.cc
  ui/views/ime/input_method_bridge_unittest.cc
  ui/views/widget/desktop_aura/desktop_native_widget_aura.cc
  ui/views/widget/desktop_aura/desktop_native_widget_aura.h
  ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc
  ui/views/widget/desktop_aura/desktop_window_tree_host_win.h
  ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
  ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h
  ui/views/widget/native_widget_aura.cc
  ui/views/win/hwnd_message_handler_delegate.h
  ui/wm/BUILD.gn
  ui/wm/core/input_method_event_filter.cc
  ui/wm/core/input_method_event_filter.h
  ui/wm/core/input_method_event_filter_unittest.cc
  ui/wm/test/wm_test_helper.cc
  ui/wm/test/wm_test_helper.h
  ui/wm/wm.gyp

sky:
  ash/accelerators/accelerator_controller_unittest.cc
  ash/display/display_controller.cc
  ash/display/display_controller.h
  ash/host/ash_window_tree_host_unified.cc
  ash/host/ash_window_tree_host_unified.h
  ash/magnifier/magnification_controller.cc
  ash/magnifier/magnification_controller_unittest.cc
  ash/shell.cc
  ash/shell.h
  ash/shell/keyboard_controller_proxy_stub.cc
  ash/shell/shell_delegate_impl.cc
  ash/test/shell_test_api.cc
  ash/test/shell_test_api.h
  ash/wm/overlay_event_filter.cc
  ash/wm/power_button_controller.cc
  ash/wm/window_manager_unittest.cc
  ash/wm/workspace/workspace_layout_manager.cc
  ash/wm/workspace/workspace_layout_manager_unittest.cc
  chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.cc
  chrome/browser/chromeos/input_method/input_method_engine.cc
  chrome/browser/chromeos/input_method/textinput_test_helper.cc
  chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc
  mandoline/ui/aura/native_widget_view_manager.cc
  mandoline/ui/aura/native_widget_view_manager.h
  mandoline/ui/aura/window_tree_host_mojo.cc
  mandoline/ui/aura/window_tree_host_mojo.h
  ui/aura/client/aura_constants.cc
  ui/aura/client/aura_constants.h
  ui/aura/remote_window_tree_host_win.cc
  ui/aura/remote_window_tree_host_win.h
  ui/aura/test/aura_test_helper.cc
  ui/aura/test/aura_test_helper.h
  ui/aura/window_tree_host.cc
  ui/aura/window_tree_host.h
  ui/aura/window_tree_host_ozone.cc
  ui/aura/window_tree_host_ozone.h
  ui/aura/window_tree_host_win.cc
  ui/aura/window_tree_host_win.h
  ui/aura/window_tree_host_x11.cc
  ui/aura/window_tree_host_x11.h
  ui/base/accelerators/accelerator_manager_unittest.cc
  ui/base/ime/input_method_chromeos.cc
  ui/base/ime/input_method_factory.cc
  ui/base/ime/mock_input_method.cc
  ui/events/event.cc
  ui/events/event.h
  ui/events/event_constants.h
  ui/events/event_source.h
  ui/events/event_utils.cc
  ui/keyboard/keyboard_util.cc
  ui/views/ime/input_method_bridge.cc
  ui/views/ime/input_method_bridge_unittest.cc
  ui/views/widget/desktop_aura/desktop_native_widget_aura.cc
  ui/views/widget/desktop_aura/desktop_native_widget_aura.h
  ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc
  ui/views/widget/desktop_aura/desktop_window_tree_host_win.h
  ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
  ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h
  ui/views/widget/native_widget_aura.cc
  ui/views/win/hwnd_message_handler_delegate.h
  ui/wm/BUILD.gn
  ui/wm/core/input_method_event_filter.cc
  ui/wm/core/input_method_event_filter.h
  ui/wm/core/input_method_event_filter_unittest.cc
  ui/wm/test/wm_test_helper.cc
  ui/wm/test/wm_test_helper.h
  ui/wm/wm.gyp

jamescook:
  ash/accelerators/accelerator_controller_unittest.cc
  ash/display/display_controller.cc
  ash/display/display_controller.h
  ash/host/ash_window_tree_host_unified.cc
  ash/host/ash_window_tree_host_unified.h
  ash/magnifier/magnification_controller.cc
  ash/magnifier/magnification_controller_unittest.cc
  ash/shell.cc
  ash/shell.h
  ash/shell/keyboard_controller_proxy_stub.cc
  ash/shell/shell_delegate_impl.cc
  ash/test/shell_test_api.cc
  ash/test/shell_test_api.h
  ash/wm/overlay_event_filter.cc
  ash/wm/power_button_controller.cc
  ash/wm/window_manager_unittest.cc
  ash/wm/workspace/workspace_layout_manager.cc
  ash/wm/workspace/workspace_layout_manager_unittest.cc
  chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc
  extensions/shell/browser/shell_desktop_controller_aura.cc
  extensions/shell/browser/shell_desktop_controller_aura.h

[James Su, 2015-06-02 07:05:27]

https://codereview.chromium.org/1155013005/diff/400001/ui/aura/remote_window_...
File ui/aura/remote_window_tree_host_win.cc (right):

https://codereview.chromium.org/1155013005/diff/400001/ui/aura/remote_window_...
ui/aura/remote_window_tree_host_win.cc:431: ui::InputMethod* input_method =
GetAshWindow()->GetHost()->GetInputMethod();
shouldn't GetAshWindow()->GetHost() == this ?

https://codereview.chromium.org/1155013005/diff/400001/ui/views/widget/native...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1155013005/diff/400001/ui/views/widget/native...
ui/views/widget/native_widget_aura.cc:279: ui::InputMethod* host =
root_window->GetHost()->GetInputMethod();
You can just call GetHostInputMethod() here.

[Shu Chen, 2015-06-02 07:20:21]

https://codereview.chromium.org/1155013005/diff/400001/ui/aura/remote_window_...
File ui/aura/remote_window_tree_host_win.cc (right):

https://codereview.chromium.org/1155013005/diff/400001/ui/aura/remote_window_...
ui/aura/remote_window_tree_host_win.cc:431: ui::InputMethod* input_method =
GetAshWindow()->GetHost()->GetInputMethod();
On 2015/06/02 07:05:26, James Su wrote:
> shouldn't GetAshWindow()->GetHost() == this ?

Yes. Done.

https://codereview.chromium.org/1155013005/diff/400001/ui/views/widget/native...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1155013005/diff/400001/ui/views/widget/native...
ui/views/widget/native_widget_aura.cc:279: ui::InputMethod* host =
root_window->GetHost()->GetInputMethod();
On 2015/06/02 07:05:27, James Su wrote:
> You can just call GetHostInputMethod() here.

Done.

[James Su, 2015-06-02 08:28:22]

https://codereview.chromium.org/1155013005/diff/440001/ash/shell/keyboard_con...
File ash/shell/keyboard_controller_proxy_stub.cc (right):

https://codereview.chromium.org/1155013005/diff/440001/ash/shell/keyboard_con...
ash/shell/keyboard_controller_proxy_stub.cc:39: return
ash::Shell::GetPrimaryRootWindow()->GetHost()->GetInputMethod();
GetTargetRootWindow ?

[Shu Chen, 2015-06-02 08:44:06]

https://codereview.chromium.org/1155013005/diff/440001/ash/shell/keyboard_con...
File ash/shell/keyboard_controller_proxy_stub.cc (right):

https://codereview.chromium.org/1155013005/diff/440001/ash/shell/keyboard_con...
ash/shell/keyboard_controller_proxy_stub.cc:39: return
ash::Shell::GetPrimaryRootWindow()->GetHost()->GetInputMethod();
On 2015/06/02 08:28:21, James Su wrote:
> GetTargetRootWindow ?

Done.

[Avi (use Gerrit), 2015-06-02 14:49:04]

On 2015/06/02 04:16:21, Shu Chen wrote:
> avi@ please focus on aura/views stuff.

No, sorry.

In ui/OWNERS is:

# Mac stuff
avi@chromium.org

I don't know Aura. I don't know Views. I am deeply unqualified to review any of
this.

Also in ui/OWNERS is sky@. Scott, can you review these files instead?

[Shu Chen, 2015-06-02 14:56:01]

On 2015/06/02 14:49:04, Avi wrote:
> On 2015/06/02 04:16:21, Shu Chen wrote:
> > avi@ please focus on aura/views stuff.
> 
> No, sorry.
> 
> In ui/OWNERS is:
> 
> # Mac stuff
> mailto:avi@chromium.org
> 
> I don't know Aura. I don't know Views. I am deeply unqualified to review any
of
> this.
> 
> Also in ui/OWNERS is sky@. Scott, can you review these files instead?

Oh, sorry I didn't check the OWNERS.

Avi, can you please focus on the changes under content/... then? Thanks

Scott, can you please review the overall ui stuff? Thanks

[Avi (use Gerrit), 2015-06-02 15:14:51]

On 2015/06/02 14:56:01, Shu Chen wrote:
> Avi, can you please focus on the changes under content/... then? Thanks

Usually I'm happy to review content/, but I know the non-platform-specific
stuff. All your content/ changes are Aura, again out of my expertise.

Once you get the Aura experts to LG this change, ping me and I will LG it too.

Sorry I can't help further right now. :(

[James Cook, 2015-06-02 16:29:10]

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

[James Cook, 2015-06-02 16:29:12]

ash/ and extensions/ are looking good, just a couple questions.

oshima, can you comment on the use of Shell::GetTargetRootWindow() in these
situations?  (See ash/display/display_controller.cc)

https://codereview.chromium.org/1155013005/diff/480001/ash/display/display_co...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/1155013005/diff/480001/ash/display/display_co...
ash/display/display_controller.cc:872: return
Shell::GetTargetRootWindow()->GetHost()->DispatchKeyEventPostIME(
Are you sure this is right? GetTargetRootWindow is supposed to be used for
creating new windows. Do you want the active window instead?  (oshima might be
able to advise on this too)

https://codereview.chromium.org/1155013005/diff/480001/ash/display/display_co...
File ash/display/display_controller.h (right):

https://codereview.chromium.org/1155013005/diff/480001/ash/display/display_co...
ash/display/display_controller.h:169: // ui::internal::InputMethodDelegate:
nit: match the style of the other ones ("overrides:") or change the others to
match this one

https://codereview.chromium.org/1155013005/diff/480001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/1155013005/diff/480001/ash/shell.cc#newcode754
ash/shell.cc:754: keyboard::KeyboardController::ResetInstance(nullptr);
Does this need to be done here or can it be done closer to the destruction of
DisplayController?  (I'm a little nervous about moving it, as there's a lot of
cleanup happening between the new and old locations.)

https://codereview.chromium.org/1155013005/diff/480001/ash/shell/keyboard_con...
File ash/shell/keyboard_controller_proxy_stub.cc (right):

https://codereview.chromium.org/1155013005/diff/480001/ash/shell/keyboard_con...
ash/shell/keyboard_controller_proxy_stub.cc:39: return
ash::Shell::GetTargetRootWindow()->GetHost()->GetInputMethod();
Again, are you sure about GetTargetRootWindow?

[oshima, 2015-06-02 17:35:54]

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

[oshima, 2015-06-02 17:35:56]

+sadrul, the master of events.

https://codereview.chromium.org/1155013005/diff/480001/ash/display/display_co...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/1155013005/diff/480001/ash/display/display_co...
ash/display/display_controller.cc:873: event);
James is correct. If you want to send key events to the root window which has a
focused window, then you should use
GetActiveWindow()->GetRootWindow()->GetHost(), then fallback to the target root
window if there is no active window)

https://codereview.chromium.org/1155013005/diff/480001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/480001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:288: ui::KeyEvent copied_event(event);
+sadrul in case this copy has some side effect

[Shu Chen, 2015-06-03 03:31:54]

https://codereview.chromium.org/1155013005/diff/480001/ash/display/display_co...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/1155013005/diff/480001/ash/display/display_co...
ash/display/display_controller.cc:872: return
Shell::GetTargetRootWindow()->GetHost()->DispatchKeyEventPostIME(
On 2015/06/02 16:29:11, James Cook wrote:
> Are you sure this is right? GetTargetRootWindow is supposed to be used for
> creating new windows. Do you want the active window instead?  (oshima might be
> able to advise on this too)

Done.

https://codereview.chromium.org/1155013005/diff/480001/ash/display/display_co...
ash/display/display_controller.cc:873: event);
On 2015/06/02 17:35:55, oshima wrote:
> James is correct. If you want to send key events to the root window which has
a
> focused window, then you should use
> GetActiveWindow()->GetRootWindow()->GetHost(), then fallback to the target
root
> window if there is no active window)

Done.

https://codereview.chromium.org/1155013005/diff/480001/ash/display/display_co...
File ash/display/display_controller.h (right):

https://codereview.chromium.org/1155013005/diff/480001/ash/display/display_co...
ash/display/display_controller.h:169: // ui::internal::InputMethodDelegate:
On 2015/06/02 16:29:12, James Cook wrote:
> nit: match the style of the other ones ("overrides:") or change the others to
> match this one

Done.

https://codereview.chromium.org/1155013005/diff/480001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/1155013005/diff/480001/ash/shell.cc#newcode754
ash/shell.cc:754: keyboard::KeyboardController::ResetInstance(nullptr);
On 2015/06/02 16:29:12, James Cook wrote:
> Does this need to be done here or can it be done closer to the destruction of
> DisplayController?  (I'm a little nervous about moving it, as there's a lot of
> cleanup happening between the new and old locations.)

I recall I changed this due to some test failures during the refactoring, but
later I figured out the test failures were caused by other things. So probably
the change here can be reverted. Let me try and see the whether trybots will be
green.

https://codereview.chromium.org/1155013005/diff/480001/ash/shell/keyboard_con...
File ash/shell/keyboard_controller_proxy_stub.cc (right):

https://codereview.chromium.org/1155013005/diff/480001/ash/shell/keyboard_con...
ash/shell/keyboard_controller_proxy_stub.cc:39: return
ash::Shell::GetTargetRootWindow()->GetHost()->GetInputMethod();
On 2015/06/02 16:29:12, James Cook wrote:
> Again, are you sure about GetTargetRootWindow?

Done.

https://codereview.chromium.org/1155013005/diff/480001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/480001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:288: ui::KeyEvent copied_event(event);
On 2015/06/02 17:35:55, oshima wrote:
> +sadrul in case this copy has some side effect

Actually the code here is copied from
InputMethodEventFilter::DispatchKeyEventPostIME().
So I think the event copy should be safe.

[James Su, 2015-06-03 06:41:17]

https://codereview.chromium.org/1155013005/diff/520001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/520001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:187: input_method_(NULL),
nullptr?

[Shu Chen, 2015-06-03 07:29:57]

https://codereview.chromium.org/1155013005/diff/480001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/1155013005/diff/480001/ash/shell.cc#newcode754
ash/shell.cc:754: keyboard::KeyboardController::ResetInstance(nullptr);
On 2015/06/03 03:31:54, Shu Chen wrote:
> On 2015/06/02 16:29:12, James Cook wrote:
> > Does this need to be done here or can it be done closer to the destruction
of
> > DisplayController?  (I'm a little nervous about moving it, as there's a lot
of
> > cleanup happening between the new and old locations.)
> 
> I recall I changed this due to some test failures during the refactoring, but
> later I figured out the test failures were caused by other things. So probably
> the change here can be reverted. Let me try and see the whether trybots will
be
> green.

The trybots are green. :)

https://codereview.chromium.org/1155013005/diff/520001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/520001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:187: input_method_(NULL),
On 2015/06/03 06:41:16, James Su wrote:
> nullptr?

Done.

[Shu Chen, 2015-06-03 10:01:23]

FYI. Just verified on a win8.1 VM, and this cl doesn't cause any regressions for
major scenarios.

[oshima, 2015-06-03 16:18:56]

On 2015/06/03 10:01:23, Shu Chen wrote:
> FYI. Just verified on a win8.1 VM, and this cl doesn't cause any regressions
for
> major scenarios.

Re a) The test still makes sense, at least on X. X and aura has its own focus
system,
and that test is to make sure that aura focus works regardless of which native X
window has focus.

[James Cook, 2015-06-03 16:26:40]

On 2015/06/03 16:18:56, oshima wrote:
> On 2015/06/03 10:01:23, Shu Chen wrote:
> > FYI. Just verified on a win8.1 VM, and this cl doesn't cause any regressions
> for
> > major scenarios.
> 
> Re a) The test still makes sense, at least on X. X and aura has its own focus
> system,
> and that test is to make sure that aura focus works regardless of which native
X
> window has focus.

oshima, which (a) are you referring to?

[James Cook, 2015-06-03 16:33:40]

On 2015/06/03 16:26:40, James Cook wrote:
> On 2015/06/03 16:18:56, oshima wrote:
> > On 2015/06/03 10:01:23, Shu Chen wrote:
> > > FYI. Just verified on a win8.1 VM, and this cl doesn't cause any
regressions
> > for
> > > major scenarios.
> > 
> > Re a) The test still makes sense, at least on X. X and aura has its own
focus
> > system,
> > and that test is to make sure that aura focus works regardless of which
native
> X
> > window has focus.
> 
> oshima, which (a) are you referring to?

Oh, I see, 2.a in the CL description. Nevermind.

[James Cook, 2015-06-03 16:34:28]

ash/ and extensions/shell/ LGTM with nit

https://codereview.chromium.org/1155013005/diff/540001/ash/display/display_co...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/1155013005/diff/540001/ash/display/display_co...
ash/display/display_controller.cc:877: root_window =
Shell::GetTargetRootWindow();
nit: I would use GetPrimaryRootWindow() as the fallback here.

https://codereview.chromium.org/1155013005/diff/540001/ash/shell/keyboard_con...
File ash/shell/keyboard_controller_proxy_stub.cc (right):

https://codereview.chromium.org/1155013005/diff/540001/ash/shell/keyboard_con...
ash/shell/keyboard_controller_proxy_stub.cc:44: root_window =
Shell::GetTargetRootWindow();
same nit, GetPrimaryRootWindow()?

https://codereview.chromium.org/1155013005/diff/540001/ash/test/shell_test_api.h
File ash/test/shell_test_api.h (left):

https://codereview.chromium.org/1155013005/diff/540001/ash/test/shell_test_ap...
ash/test/shell_test_api.h:11: class InputMethodEventFilter;
Thanks for cleaning up forward declares when removing code.

[oshima, 2015-06-03 16:48:01]

https://codereview.chromium.org/1155013005/diff/540001/ash/display/display_co...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/1155013005/diff/540001/ash/display/display_co...
ash/display/display_controller.cc:877: root_window =
Shell::GetTargetRootWindow();
active window must have a root window, so the following would be better

aura::Window* active_window = wm::GetActiveWindow();
aura::Window* root_window = active_window ? active_window->GetRootWindow() :
GetTargetRootWindow();
root_window->GetHost()->DispatchKeyEventPostIME(..)

It shouldn't really matter which root window to send though, because it'll end
up using WindowTargeter::FindTargetForKeyEvent, which uses the focus_client to
find where to send to.

I'm okay with the logic above, or just send it to primary root window,
but could you please add comment that this isn't significant as the event
will be sent to the window resolved by aura::client::FocusClient (which is
FocusController in ash)?

[Shu Chen, 2015-06-04 01:59:44]

https://codereview.chromium.org/1155013005/diff/540001/ash/display/display_co...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/1155013005/diff/540001/ash/display/display_co...
ash/display/display_controller.cc:877: root_window =
Shell::GetTargetRootWindow();
On 2015/06/03 16:48:00, oshima wrote:
> active window must have a root window, so the following would be better
> 
> aura::Window* active_window = wm::GetActiveWindow();
> aura::Window* root_window = active_window ? active_window->GetRootWindow() :
> GetTargetRootWindow();
> root_window->GetHost()->DispatchKeyEventPostIME(..)
> 
> It shouldn't really matter which root window to send though, because it'll end
> up using WindowTargeter::FindTargetForKeyEvent, which uses the focus_client to
> find where to send to.
> 
> I'm okay with the logic above, or just send it to primary root window,
> but could you please add comment that this isn't significant as the event
> will be sent to the window resolved by aura::client::FocusClient (which is
> FocusController in ash)?

Done. And comments added.

https://codereview.chromium.org/1155013005/diff/540001/ash/shell/keyboard_con...
File ash/shell/keyboard_controller_proxy_stub.cc (right):

https://codereview.chromium.org/1155013005/diff/540001/ash/shell/keyboard_con...
ash/shell/keyboard_controller_proxy_stub.cc:44: root_window =
Shell::GetTargetRootWindow();
On 2015/06/03 16:34:27, James Cook wrote:
> same nit, GetPrimaryRootWindow()?

Done.

[oshima, 2015-06-04 02:14:20]

my bits lgtm

[James Su, 2015-06-04 04:10:24]

On 2015/06/03 16:18:56, oshima wrote:
> On 2015/06/03 10:01:23, Shu Chen wrote:
> > FYI. Just verified on a win8.1 VM, and this cl doesn't cause any regressions
> for
> > major scenarios.
> 
> Re a) The test still makes sense, at least on X. X and aura has its own focus
> system,
> and that test is to make sure that aura focus works regardless of which native
X
> window has focus.

IMHO, native window focus should always match aura window focus (i.e. the native
window, in which the active aura window lives, should always have native focus),
otherwise unpredictable things may happen.
For example: the native input method may use the focused native window to
position its floating suggestion UI, if the native focus and aura focus don't
match, then native input method's floating suggestion UI may be wrongly
positioned on the screen.
Other bad things may happen if such mismatch happens on Windows, because key
events are dispatched to the native input method context bound to the focused
hwnd by system before being dispatched to Chrome. So, we can't know what will
happen if the active aura window is not inside the focused hwnd.

So I'm wondering in which case such focus mismatch will happen? And why we allow
such mismatch? Can we avoid it on all platforms? If such focus mismatch is not
by design, then I think we should treat it as an invalid case and remove this
test, and add a new test to ensure the native focus and aura focus always match.

[oshima, 2015-06-04 04:36:49]

On 2015/06/04 04:10:24, James Su wrote:
> On 2015/06/03 16:18:56, oshima wrote:
> > On 2015/06/03 10:01:23, Shu Chen wrote:
> > > FYI. Just verified on a win8.1 VM, and this cl doesn't cause any
regressions
> > for
> > > major scenarios.
> > 
> > Re a) The test still makes sense, at least on X. X and aura has its own
focus
> > system,
> > and that test is to make sure that aura focus works regardless of which
native
> X
> > window has focus.
> 
> IMHO, native window focus should always match aura window focus (i.e. the
native
> window, in which the active aura window lives, should always have native
focus),
> otherwise unpredictable things may happen.
> For example: the native input method may use the focused native window to
> position its floating suggestion UI, if the native focus and aura focus don't
> match, then native input method's floating suggestion UI may be wrongly
> positioned on the screen.
> Other bad things may happen if such mismatch happens on Windows, because key
> events are dispatched to the native input method context bound to the focused
> hwnd by system before being dispatched to Chrome. So, we can't know what will
> happen if the active aura window is not inside the focused hwnd.
> 
> So I'm wondering in which case such focus mismatch will happen? And why we
allow
> such mismatch? Can we avoid it on all platforms? If such focus mismatch is not
> by design, then I think we should treat it as an invalid case and remove this
> test, and add a new test to ensure the native focus and aura focus always
match.

This is for ash/chromeos case, not for desktop aura. On ash/chromeos, native
window
isn't really window, just a thing that host desktop for each display

I agree that they have to match in desktop aura, but that's not this test is
for.

[James Su, 2015-06-04 05:15:14]

On 2015/06/04 04:36:49, oshima wrote:
> On 2015/06/04 04:10:24, James Su wrote:
> > On 2015/06/03 16:18:56, oshima wrote:
> > > On 2015/06/03 10:01:23, Shu Chen wrote:
> > > > FYI. Just verified on a win8.1 VM, and this cl doesn't cause any
> regressions
> > > for
> > > > major scenarios.
> > > 
> > > Re a) The test still makes sense, at least on X. X and aura has its own
> focus
> > > system,
> > > and that test is to make sure that aura focus works regardless of which
> native
> > X
> > > window has focus.
> > 
> > IMHO, native window focus should always match aura window focus (i.e. the
> native
> > window, in which the active aura window lives, should always have native
> focus),
> > otherwise unpredictable things may happen.
> > For example: the native input method may use the focused native window to
> > position its floating suggestion UI, if the native focus and aura focus
don't
> > match, then native input method's floating suggestion UI may be wrongly
> > positioned on the screen.
> > Other bad things may happen if such mismatch happens on Windows, because key
> > events are dispatched to the native input method context bound to the
focused
> > hwnd by system before being dispatched to Chrome. So, we can't know what
will
> > happen if the active aura window is not inside the focused hwnd.
> > 
> > So I'm wondering in which case such focus mismatch will happen? And why we
> allow
> > such mismatch? Can we avoid it on all platforms? If such focus mismatch is
not
> > by design, then I think we should treat it as an invalid case and remove
this
> > test, and add a new test to ensure the native focus and aura focus always
> match.
> 
> This is for ash/chromeos case, not for desktop aura. On ash/chromeos, native
> window
> isn't really window, just a thing that host desktop for each display
> 
> I agree that they have to match in desktop aura, but that's not this test is
> for.

So this test will never run for other platforms (e.g. ash/windows) except
ash/chromeos?

We probably need to add some comments to clarify it somewhere.

And IMHO, the event reroute logic in WindowTargeter::FindTargetForEvent()
method:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_tar...

looks so tricky. At least it breaks a common code practice: a method should only
do one thing, and should not have side effect. This method clearly has a side
effect when the given root target is not focused: it reroutes the even to the
real focused target and returns null to caller.
By looking at the method declaration, people won't know this method will
dispatch the event sometime.

Is it possible to move this logic into a different place to make the code easier
to understand?

[oshima, 2015-06-04 05:49:27]

On 2015/06/04 05:15:14, James Su wrote:
> On 2015/06/04 04:36:49, oshima wrote:
> > On 2015/06/04 04:10:24, James Su wrote:
> > > On 2015/06/03 16:18:56, oshima wrote:
> > > > On 2015/06/03 10:01:23, Shu Chen wrote:
> > > > > FYI. Just verified on a win8.1 VM, and this cl doesn't cause any
> > regressions
> > > > for
> > > > > major scenarios.
> > > > 
> > > > Re a) The test still makes sense, at least on X. X and aura has its own
> > focus
> > > > system,
> > > > and that test is to make sure that aura focus works regardless of which
> > native
> > > X
> > > > window has focus.
> > > 
> > > IMHO, native window focus should always match aura window focus (i.e. the
> > native
> > > window, in which the active aura window lives, should always have native
> > focus),
> > > otherwise unpredictable things may happen.
> > > For example: the native input method may use the focused native window to
> > > position its floating suggestion UI, if the native focus and aura focus
> don't
> > > match, then native input method's floating suggestion UI may be wrongly
> > > positioned on the screen.
> > > Other bad things may happen if such mismatch happens on Windows, because
key
> > > events are dispatched to the native input method context bound to the
> focused
> > > hwnd by system before being dispatched to Chrome. So, we can't know what
> will
> > > happen if the active aura window is not inside the focused hwnd.
> > > 
> > > So I'm wondering in which case such focus mismatch will happen? And why we
> > allow
> > > such mismatch? Can we avoid it on all platforms? If such focus mismatch is
> not
> > > by design, then I think we should treat it as an invalid case and remove
> this
> > > test, and add a new test to ensure the native focus and aura focus always
> > match.
> > 
> > This is for ash/chromeos case, not for desktop aura. On ash/chromeos, native
> > window
> > isn't really window, just a thing that host desktop for each display
> > 
> > I agree that they have to match in desktop aura, but that's not this test is
> > for.
> 
> So this test will never run for other platforms (e.g. ash/windows) except
> ash/chromeos?
> 
> We probably need to add some comments to clarify it somewhere.

The test is in ash/ so it's very clear.

> 
> And IMHO, the event reroute logic in WindowTargeter::FindTargetForEvent()
> method:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_tar...
> 
> looks so tricky. At least it breaks a common code practice: a method should
only
> do one thing, and should not have side effect. This method clearly has a side
> effect when the given root target is not focused: it reroutes the even to the
> real focused target and returns null to caller.
> By looking at the method declaration, people won't know this method will
> dispatch the event sometime.

That method has nothing to do with focus. This is to deal with cross-root-window
native mouse capture.
It can have another out parameter for modified event, but i'll leave it to
sadrul@ who designed it.

> 
> Is it possible to move this logic into a different place to make the code
easier
> to understand?

[James Su, 2015-06-04 06:05:32]

On 2015/06/04 05:49:27, oshima wrote:
> On 2015/06/04 05:15:14, James Su wrote:
> > On 2015/06/04 04:36:49, oshima wrote:
> > > On 2015/06/04 04:10:24, James Su wrote:
> > > > On 2015/06/03 16:18:56, oshima wrote:
> > > > > On 2015/06/03 10:01:23, Shu Chen wrote:
> > > > > > FYI. Just verified on a win8.1 VM, and this cl doesn't cause any
> > > regressions
> > > > > for
> > > > > > major scenarios.
> > > > > 
> > > > > Re a) The test still makes sense, at least on X. X and aura has its
own
> > > focus
> > > > > system,
> > > > > and that test is to make sure that aura focus works regardless of
which
> > > native
> > > > X
> > > > > window has focus.
> > > > 
> > > > IMHO, native window focus should always match aura window focus (i.e.
the
> > > native
> > > > window, in which the active aura window lives, should always have native
> > > focus),
> > > > otherwise unpredictable things may happen.
> > > > For example: the native input method may use the focused native window
to
> > > > position its floating suggestion UI, if the native focus and aura focus
> > don't
> > > > match, then native input method's floating suggestion UI may be wrongly
> > > > positioned on the screen.
> > > > Other bad things may happen if such mismatch happens on Windows, because
> key
> > > > events are dispatched to the native input method context bound to the
> > focused
> > > > hwnd by system before being dispatched to Chrome. So, we can't know what
> > will
> > > > happen if the active aura window is not inside the focused hwnd.
> > > > 
> > > > So I'm wondering in which case such focus mismatch will happen? And why
we
> > > allow
> > > > such mismatch? Can we avoid it on all platforms? If such focus mismatch
is
> > not
> > > > by design, then I think we should treat it as an invalid case and remove
> > this
> > > > test, and add a new test to ensure the native focus and aura focus
always
> > > match.
> > > 
> > > This is for ash/chromeos case, not for desktop aura. On ash/chromeos,
native
> > > window
> > > isn't really window, just a thing that host desktop for each display
> > > 
> > > I agree that they have to match in desktop aura, but that's not this test
is
> > > for.
> > 
> > So this test will never run for other platforms (e.g. ash/windows) except
> > ash/chromeos?
> > 
> > We probably need to add some comments to clarify it somewhere.
> 
> The test is in ash/ so it's very clear.
I understand that ash is also used on Windows8 metro, does it also have the same
focus mismatch issue? And how about ash/chromeos/x11? (Do we still need to care
about ash/chromeos/x11?)

> 
> > 
> > And IMHO, the event reroute logic in WindowTargeter::FindTargetForEvent()
> > method:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_tar...
> > 
> > looks so tricky. At least it breaks a common code practice: a method should
> only
> > do one thing, and should not have side effect. This method clearly has a
side
> > effect when the given root target is not focused: it reroutes the even to
the
> > real focused target and returns null to caller.
> > By looking at the method declaration, people won't know this method will
> > dispatch the event sometime.
> 
> That method has nothing to do with focus. This is to deal with
cross-root-window
> native mouse capture.
> It can have another out parameter for modified event, but i'll leave it to
> sadrul@ who designed it.
According to the code, this method takes effect for key events as well. If there
is no focus mismatch issue between native window and active aura window, then no
necessary to reroute key events in this method at all.

> 
> > 
> > Is it possible to move this logic into a different place to make the code
> easier
> > to understand?

[James Su, 2015-06-04 06:07:20]

Anyway, this CL LGTM to me.

Shu will document all of the issues raised in previous comments somewhere and
maybe file bugs for them.

[oshima, 2015-06-04 06:50:26]

On Wed, Jun 3, 2015 at 11:05 PM, <suzhe@chromium.org> wrote:

> On 2015/06/04 05:49:27, oshima wrote:
>
>> On 2015/06/04 05:15:14, James Su wrote:
>> > On 2015/06/04 04:36:49, oshima wrote:
>> > > On 2015/06/04 04:10:24, James Su wrote:
>> > > > On 2015/06/03 16:18:56, oshima wrote:
>> > > > > On 2015/06/03 10:01:23, Shu Chen wrote:
>> > > > > > FYI. Just verified on a win8.1 VM, and this cl doesn't cause any
>> > > regressions
>> > > > > for
>> > > > > > major scenarios.
>> > > > >
>> > > > > Re a) The test still makes sense, at least on X. X and aura has
>> its
>>
> own
>
>> > > focus
>> > > > > system,
>> > > > > and that test is to make sure that aura focus works regardless of
>>
> which
>
>> > > native
>> > > > X
>> > > > > window has focus.
>> > > >
>> > > > IMHO, native window focus should always match aura window focus
>> (i.e.
>>
> the
>
>> > > native
>> > > > window, in which the active aura window lives, should always have
>> native
>> > > focus),
>> > > > otherwise unpredictable things may happen.
>> > > > For example: the native input method may use the focused native
>> window
>>
> to
>
>> > > > position its floating suggestion UI, if the native focus and aura
>> focus
>> > don't
>> > > > match, then native input method's floating suggestion UI may be
>> wrongly
>> > > > positioned on the screen.
>> > > > Other bad things may happen if such mismatch happens on Windows,
>> because
>> key
>> > > > events are dispatched to the native input method context bound to
>> the
>> > focused
>> > > > hwnd by system before being dispatched to Chrome. So, we can't know
>> what
>> > will
>> > > > happen if the active aura window is not inside the focused hwnd.
>> > > >
>> > > > So I'm wondering in which case such focus mismatch will happen? And
>> why
>>
> we
>
>> > > allow
>> > > > such mismatch? Can we avoid it on all platforms? If such focus
>> mismatch
>>
> is
>
>> > not
>> > > > by design, then I think we should treat it as an invalid case and
>> remove
>> > this
>> > > > test, and add a new test to ensure the native focus and aura focus
>>
> always
>
>> > > match.
>> > >
>> > > This is for ash/chromeos case, not for desktop aura. On ash/chromeos,
>>
> native
>
>> > > window
>> > > isn't really window, just a thing that host desktop for each display
>> > >
>> > > I agree that they have to match in desktop aura, but that's not this
>> test
>>
> is
>
>> > > for.
>> >
>> > So this test will never run for other platforms (e.g. ash/windows)
>> except
>> > ash/chromeos?
>> >
>> > We probably need to add some comments to clarify it somewhere.
>>
>
>  The test is in ash/ so it's very clear.
>>
> I understand that ash is also used on Windows8 metro, does it also have
> the same
> focus mismatch issue? And how about ash/chromeos/x11? (Do we still need to
> care
> about ash/chromeos/x11?)


metro mode won't have more than one host window. The test is designed to
run only
when there can be multiple host windows in ash.


>
>  >
>> > And IMHO, the event reroute logic in
>> WindowTargeter::FindTargetForEvent()
>> > method:
>> >
>>
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_tar...
>
>> >
>> > looks so tricky. At least it breaks a common code practice: a method
>> should
>> only
>> > do one thing, and should not have side effect. This method clearly has a
>>
> side
>
>> > effect when the given root target is not focused: it reroutes the even
>> to
>>
> the
>
>> > real focused target and returns null to caller.
>> > By looking at the method declaration, people won't know this method will
>> > dispatch the event sometime.
>>
>
>  That method has nothing to do with focus. This is to deal with
>>
> cross-root-window
>
>> native mouse capture.
>> It can have another out parameter for modified event, but i'll leave it to
>> sadrul@ who designed it.
>>
> According to the code, this method takes effect for key events as well. If
> there
> is no focus mismatch issue between native window and active aura window,
> then no
> necessary to reroute key events in this method at all.
>
>
Maybe we were talking about different thing? I thought you were referring
to the code that
update the location in event. That side effect only happens for located
event, not key event.

And the focused aura window may or may not be in the host window that
received the key event
at ui/aura level. It's certainly possible to have different targeter for
ash and desktop aura.
I don't have strong opinion about which is better though.


>
>  >
>> > Is it possible to move this logic into a different place to make the
>> code
>> easier
>> > to understand?
>>
>
>
>
> https://codereview.chromium.org/1155013005/
>

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

[James Su, 2015-06-04 07:25:17]

On 2015/06/04 06:50:26, oshima wrote:
> On Wed, Jun 3, 2015 at 11:05 PM, <mailto:suzhe@chromium.org> wrote:
> 
> > On 2015/06/04 05:49:27, oshima wrote:
> >
> >> On 2015/06/04 05:15:14, James Su wrote:
> >> > On 2015/06/04 04:36:49, oshima wrote:
> >> > > On 2015/06/04 04:10:24, James Su wrote:
> >> > > > On 2015/06/03 16:18:56, oshima wrote:
> >> > > > > On 2015/06/03 10:01:23, Shu Chen wrote:
> >> > > > > > FYI. Just verified on a win8.1 VM, and this cl doesn't cause any
> >> > > regressions
> >> > > > > for
> >> > > > > > major scenarios.
> >> > > > >
> >> > > > > Re a) The test still makes sense, at least on X. X and aura has
> >> its
> >>
> > own
> >
> >> > > focus
> >> > > > > system,
> >> > > > > and that test is to make sure that aura focus works regardless of
> >>
> > which
> >
> >> > > native
> >> > > > X
> >> > > > > window has focus.
> >> > > >
> >> > > > IMHO, native window focus should always match aura window focus
> >> (i.e.
> >>
> > the
> >
> >> > > native
> >> > > > window, in which the active aura window lives, should always have
> >> native
> >> > > focus),
> >> > > > otherwise unpredictable things may happen.
> >> > > > For example: the native input method may use the focused native
> >> window
> >>
> > to
> >
> >> > > > position its floating suggestion UI, if the native focus and aura
> >> focus
> >> > don't
> >> > > > match, then native input method's floating suggestion UI may be
> >> wrongly
> >> > > > positioned on the screen.
> >> > > > Other bad things may happen if such mismatch happens on Windows,
> >> because
> >> key
> >> > > > events are dispatched to the native input method context bound to
> >> the
> >> > focused
> >> > > > hwnd by system before being dispatched to Chrome. So, we can't know
> >> what
> >> > will
> >> > > > happen if the active aura window is not inside the focused hwnd.
> >> > > >
> >> > > > So I'm wondering in which case such focus mismatch will happen? And
> >> why
> >>
> > we
> >
> >> > > allow
> >> > > > such mismatch? Can we avoid it on all platforms? If such focus
> >> mismatch
> >>
> > is
> >
> >> > not
> >> > > > by design, then I think we should treat it as an invalid case and
> >> remove
> >> > this
> >> > > > test, and add a new test to ensure the native focus and aura focus
> >>
> > always
> >
> >> > > match.
> >> > >
> >> > > This is for ash/chromeos case, not for desktop aura. On ash/chromeos,
> >>
> > native
> >
> >> > > window
> >> > > isn't really window, just a thing that host desktop for each display
> >> > >
> >> > > I agree that they have to match in desktop aura, but that's not this
> >> test
> >>
> > is
> >
> >> > > for.
> >> >
> >> > So this test will never run for other platforms (e.g. ash/windows)
> >> except
> >> > ash/chromeos?
> >> >
> >> > We probably need to add some comments to clarify it somewhere.
> >>
> >
> >  The test is in ash/ so it's very clear.
> >>
> > I understand that ash is also used on Windows8 metro, does it also have
> > the same
> > focus mismatch issue? And how about ash/chromeos/x11? (Do we still need to
> > care
> > about ash/chromeos/x11?)
> 
> 
> metro mode won't have more than one host window. The test is designed to
> run only
> when there can be multiple host windows in ash.
I see. Then we can focus on ash/chromeos. Do we still need to consider
ash/chromeos/x11?

> 
> 
> >
> >  >
> >> > And IMHO, the event reroute logic in
> >> WindowTargeter::FindTargetForEvent()
> >> > method:
> >> >
> >>
> >
> >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_tar...
> >
> >> >
> >> > looks so tricky. At least it breaks a common code practice: a method
> >> should
> >> only
> >> > do one thing, and should not have side effect. This method clearly has a
> >>
> > side
> >
> >> > effect when the given root target is not focused: it reroutes the even
> >> to
> >>
> > the
> >
> >> > real focused target and returns null to caller.
> >> > By looking at the method declaration, people won't know this method will
> >> > dispatch the event sometime.
> >>
> >
> >  That method has nothing to do with focus. This is to deal with
> >>
> > cross-root-window
> >
> >> native mouse capture.
> >> It can have another out parameter for modified event, but i'll leave it to
> >> sadrul@ who designed it.
> >>
> > According to the code, this method takes effect for key events as well. If
> > there
> > is no focus mismatch issue between native window and active aura window,
> > then no
> > necessary to reroute key events in this method at all.
> >
> >
> Maybe we were talking about different thing? I thought you were referring
> to the code that
> update the location in event. That side effect only happens for located
> event, not key event.
I'm talking about the reroute logic related to both located events and key
events in WindowTargeter::FindTargetForEvent().
IMHO, calling new_root->GetHost()->event_processor()->OnEventFromSource(event)
to reroute events in WindowTargeter::FindTargetForEvent() is very confusing.
If only look at the method definition in EventTargeter, nobody would know that
an event maybe re-dispatched by this method:

  // Returns the target |event| should be dispatched to. If there is no such
  // target, return NULL. If |event| is a located event, the location of |event|
  // is in the coordinate space of |root|. Furthermore, the targeter can mutate
  // the event (e.g., by changing the location of the event to be in the
  // returned target's coordinate space) so that it can be dispatched to the
  // target without any further modification.
  virtual EventTarget* FindTargetForEvent(EventTarget* root,
                                          Event* event);

> 
> And the focused aura window may or may not be in the host window that
> received the key event
> at ui/aura level. It's certainly possible to have different targeter for
> ash and desktop aura.
> I don't have strong opinion about which is better though.
As I mentioned in comment #30, if focused aura window is not in the focused
native window (which receives the native key events), then the behavior of
system native IME maybe broken.
It maybe ok for chromeos, as there is no system native IME at all (IME extension
is fully controlled by us). But it may not be ok for Windows, Linux and Mac,
where system native IME is out of our control, and we don't know if the system
native IME relies on the focused native window to work.

That's why I want to know whether this focus mismatch issue only happens on
chromeos or not.

Even for chromeos, the code maybe more easier to understand if we can make sure
key events are always directly dispatched to the window tree host which contains
the active aura window, rather than allowing such focus mismatch and
re-dispatching the key events to the correct window tree host in
WindowTargeter::FindTargetForEvent().

> 
> 
> >
> >  >
> >> > Is it possible to move this logic into a different place to make the
> >> code
> >> easier
> >> > to understand?
> >>
> >
> >
> >
> > https://codereview.chromium.org/1155013005/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-06-04 13:40:28]

https://codereview.chromium.org/1155013005/diff/560001/chrome/browser/chromeo...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/1155013005/diff/560001/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/input_method_engine.cc:295:
ui::InputMethod* input_method =
Is there a reason you need to explicitly call through to the InputMethod here
and not the dispatcher as before? Shouldn't the later still work?

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:191: ui::InputMethod*
WindowTreeHost::GetInputMethod() {
Make order match header.

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:195: // Makes sure the input method is focused by
default when created.
This doesn't make sense to me. Why does the input_method_ need an initial focus.

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.h:125: // Sets the InputMethod instance, and it is not
owned by this WindowTreeHost.
Sets a shared unowned InputMethod. This is used when there is a singleton
InputMethod shared between multiple instances.

Can this be protected?

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.h:219: // So that the destructor can determine whether
to release the InputMethod
Nuke second sentence.

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host_win.h (left):

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_win.h:12: #include "ui/events/event_source.h"
Can you remove this include from all tree hosts now?

https://codereview.chromium.org/1155013005/diff/560001/ui/keyboard/keyboard_u...
File ui/keyboard/keyboard_util.cc (right):

https://codereview.chromium.org/1155013005/diff/560001/ui/keyboard/keyboard_u...
ui/keyboard/keyboard_util.cc:285: ui::InputMethod* input_method =
host->GetInputMethod();
How about moving this to where it's needed, eg 326?

[Shu Chen, 2015-06-04 15:59:26]

FYI. I've added more detailed comments for WindowTreeHost::SetInputMethod().

https://codereview.chromium.org/1155013005/diff/560001/chrome/browser/chromeo...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/1155013005/diff/560001/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/input_method_engine.cc:295:
ui::InputMethod* input_method =
On 2015/06/04 13:40:28, sky wrote:
> Is there a reason you need to explicitly call through to the InputMethod here
> and not the dispatcher as before? Shouldn't the later still work?

The dispatcher->OnEventFromSource() now doesn't work because it happens after
IME.
The EventSource::SendEventToProcessor() happens before IME but it is protected.
Should make it as public?

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:191: ui::InputMethod*
WindowTreeHost::GetInputMethod() {
On 2015/06/04 13:40:28, sky wrote:
> Make order match header.

Done.

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:195: // Makes sure the input method is focused by
default when created.
On 2015/06/04 13:40:28, sky wrote:
> This doesn't make sense to me. Why does the input_method_ need an initial
focus.

InputMethod::OnFocus() should be called when a native root window
(WindowTreeHost) is activated/focused.
However, for Ash, WindowTreeHost doesn't have the activation/focus state, and
WindowTreeHost::OnHostActivated() never gets called.
So here to make InputMethod by default focused is to make Ash work.
I've added this into the comment.

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.h:125: // Sets the InputMethod instance, and it is not
owned by this WindowTreeHost.
On 2015/06/04 13:40:28, sky wrote:
> Sets a shared unowned InputMethod. This is used when there is a singleton
> InputMethod shared between multiple instances.
> 
> Can this be protected?

Revised the comment.

To make it protected, a new param ui::InputMethod* needs to be added to
constructors of AshWindowTreeHostXxx. And add a new member ui::InputMethod* to
AshWindowTreeHostInitParams.

I would prefer to remain this as public because the singleton/shared InputMethod
is a temporary solution.
My later CL will try to make each AshWindowTreeHost to own a separated
InputMethod instance.

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.h:219: // So that the destructor can determine whether
to release the InputMethod
On 2015/06/04 13:40:28, sky wrote:
> Nuke second sentence.

Done.

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host_win.h (left):

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_win.h:12: #include "ui/events/event_source.h"
On 2015/06/04 13:40:28, sky wrote:
> Can you remove this include from all tree hosts now?

Done.

https://codereview.chromium.org/1155013005/diff/560001/ui/keyboard/keyboard_u...
File ui/keyboard/keyboard_util.cc (right):

https://codereview.chromium.org/1155013005/diff/560001/ui/keyboard/keyboard_u...
ui/keyboard/keyboard_util.cc:285: ui::InputMethod* input_method =
host->GetInputMethod();
On 2015/06/04 13:40:28, sky wrote:
> How about moving this to where it's needed, eg 326?

line 293 also needs it.

[oshima, 2015-06-04 16:20:51]

On 2015/06/04 07:25:17, James Su wrote:
> On 2015/06/04 06:50:26, oshima wrote:
> > On Wed, Jun 3, 2015 at 11:05 PM, <mailto:suzhe@chromium.org> wrote:
> > 
> > > On 2015/06/04 05:49:27, oshima wrote:
> > >
> > >> On 2015/06/04 05:15:14, James Su wrote:
> > >> > On 2015/06/04 04:36:49, oshima wrote:
> > >> > > On 2015/06/04 04:10:24, James Su wrote:
> > >> > > > On 2015/06/03 16:18:56, oshima wrote:
> > >> > > > > On 2015/06/03 10:01:23, Shu Chen wrote:
> > >> > > > > > FYI. Just verified on a win8.1 VM, and this cl doesn't cause
any
> > >> > > regressions
> > >> > > > > for
> > >> > > > > > major scenarios.
> > >> > > > >
> > >> > > > > Re a) The test still makes sense, at least on X. X and aura has
> > >> its
> > >>
> > > own
> > >
> > >> > > focus
> > >> > > > > system,
> > >> > > > > and that test is to make sure that aura focus works regardless of
> > >>
> > > which
> > >
> > >> > > native
> > >> > > > X
> > >> > > > > window has focus.
> > >> > > >
> > >> > > > IMHO, native window focus should always match aura window focus
> > >> (i.e.
> > >>
> > > the
> > >
> > >> > > native
> > >> > > > window, in which the active aura window lives, should always have
> > >> native
> > >> > > focus),
> > >> > > > otherwise unpredictable things may happen.
> > >> > > > For example: the native input method may use the focused native
> > >> window
> > >>
> > > to
> > >
> > >> > > > position its floating suggestion UI, if the native focus and aura
> > >> focus
> > >> > don't
> > >> > > > match, then native input method's floating suggestion UI may be
> > >> wrongly
> > >> > > > positioned on the screen.
> > >> > > > Other bad things may happen if such mismatch happens on Windows,
> > >> because
> > >> key
> > >> > > > events are dispatched to the native input method context bound to
> > >> the
> > >> > focused
> > >> > > > hwnd by system before being dispatched to Chrome. So, we can't know
> > >> what
> > >> > will
> > >> > > > happen if the active aura window is not inside the focused hwnd.
> > >> > > >
> > >> > > > So I'm wondering in which case such focus mismatch will happen? And
> > >> why
> > >>
> > > we
> > >
> > >> > > allow
> > >> > > > such mismatch? Can we avoid it on all platforms? If such focus
> > >> mismatch
> > >>
> > > is
> > >
> > >> > not
> > >> > > > by design, then I think we should treat it as an invalid case and
> > >> remove
> > >> > this
> > >> > > > test, and add a new test to ensure the native focus and aura focus
> > >>
> > > always
> > >
> > >> > > match.
> > >> > >
> > >> > > This is for ash/chromeos case, not for desktop aura. On ash/chromeos,
> > >>
> > > native
> > >
> > >> > > window
> > >> > > isn't really window, just a thing that host desktop for each display
> > >> > >
> > >> > > I agree that they have to match in desktop aura, but that's not this
> > >> test
> > >>
> > > is
> > >
> > >> > > for.
> > >> >
> > >> > So this test will never run for other platforms (e.g. ash/windows)
> > >> except
> > >> > ash/chromeos?
> > >> >
> > >> > We probably need to add some comments to clarify it somewhere.
> > >>
> > >
> > >  The test is in ash/ so it's very clear.
> > >>
> > > I understand that ash is also used on Windows8 metro, does it also have
> > > the same
> > > focus mismatch issue? And how about ash/chromeos/x11? (Do we still need to
> > > care
> > > about ash/chromeos/x11?)
> > 
> > 
> > metro mode won't have more than one host window. The test is designed to
> > run only
> > when there can be multiple host windows in ash.
> I see. Then we can focus on ash/chromeos. Do we still need to consider
> ash/chromeos/x11?

Yes, X11 is still in use and will be used for development.

> 
> > 
> > 
> > >
> > >  >
> > >> > And IMHO, the event reroute logic in
> > >> WindowTargeter::FindTargetForEvent()
> > >> > method:
> > >> >
> > >>
> > >
> > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_tar...
> > >
> > >> >
> > >> > looks so tricky. At least it breaks a common code practice: a method
> > >> should
> > >> only
> > >> > do one thing, and should not have side effect. This method clearly has
a
> > >>
> > > side
> > >
> > >> > effect when the given root target is not focused: it reroutes the even
> > >> to
> > >>
> > > the
> > >
> > >> > real focused target and returns null to caller.
> > >> > By looking at the method declaration, people won't know this method
will
> > >> > dispatch the event sometime.
> > >>
> > >
> > >  That method has nothing to do with focus. This is to deal with
> > >>
> > > cross-root-window
> > >
> > >> native mouse capture.
> > >> It can have another out parameter for modified event, but i'll leave it
to
> > >> sadrul@ who designed it.
> > >>
> > > According to the code, this method takes effect for key events as well. If
> > > there
> > > is no focus mismatch issue between native window and active aura window,
> > > then no
> > > necessary to reroute key events in this method at all.
> > >
> > >
> > Maybe we were talking about different thing? I thought you were referring
> > to the code that
> > update the location in event. That side effect only happens for located
> > event, not key event.
> I'm talking about the reroute logic related to both located events and key
> events in WindowTargeter::FindTargetForEvent().
> IMHO, calling new_root->GetHost()->event_processor()->OnEventFromSource(event)
> to reroute events in WindowTargeter::FindTargetForEvent() is very confusing.
> If only look at the method definition in EventTargeter, nobody would know that
> an event maybe re-dispatched by this method:
> 
>   // Returns the target |event| should be dispatched to. If there is no such
>   // target, return NULL. If |event| is a located event, the location of
|event|
>   // is in the coordinate space of |root|. Furthermore, the targeter can
mutate
>   // the event (e.g., by changing the location of the event to be in the
>   // returned target's coordinate space) so that it can be dispatched to the
>   // target without any further modification.
>   virtual EventTarget* FindTargetForEvent(EventTarget* root,
>                                           Event* event);

I see. It's orthogonal to the issue (it has to be re-dispatched one way or
another),
so you should talk to sadrul if you want to change the design.

> 
> > 
> > And the focused aura window may or may not be in the host window that
> > received the key event
> > at ui/aura level. It's certainly possible to have different targeter for
> > ash and desktop aura.
> > I don't have strong opinion about which is better though.
> As I mentioned in comment #30, if focused aura window is not in the focused
> native window (which receives the native key events), then the behavior of
> system native IME maybe broken.
> It maybe ok for chromeos, as there is no system native IME at all (IME
extension
> is fully controlled by us). But it may not be ok for Windows, Linux and Mac,
> where system native IME is out of our control, and we don't know if the system
> native IME relies on the focused native window to work.
> 
> That's why I want to know whether this focus mismatch issue only happens on
> chromeos or not.
> 
> Even for chromeos, the code maybe more easier to understand if we can make
sure
> key events are always directly dispatched to the window tree host which
contains
> the active aura window, rather than allowing such focus mismatch and
> re-dispatching the key events to the correct window tree host in
> WindowTargeter::FindTargetForEvent().

There is no such "native window" that has a focus in chromeos (conceptually).
It's just a mechanism that host the screen, and ash code is designed to be
independent on the native window behavior. (another example is display layout)

Key events should be dispatched to the focused window. It shouldn't really
matter
who receives first. It even doesn't have to be WTH, and I think this is the
case on ozone (at least, I can't find key handling code in WTHOzone code).

Again, I'm talking about ash/chromeos only. I fully understand that they should
match
on desktop aura.

> 
> > 
> > 
> > >
> > >  >
> > >> > Is it possible to move this logic into a different place to make the
> > >> code
> > >> easier
> > >> > to understand?
> > >>
> > >
> > >
> > >
> > > https://codereview.chromium.org/1155013005/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-06-04 19:42:52]

https://codereview.chromium.org/1155013005/diff/560001/chrome/browser/chromeo...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/1155013005/diff/560001/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/input_method_engine.cc:295:
ui::InputMethod* input_method =
On 2015/06/04 15:59:25, Shu Chen wrote:
> On 2015/06/04 13:40:28, sky wrote:
> > Is there a reason you need to explicitly call through to the InputMethod
here
> > and not the dispatcher as before? Shouldn't the later still work?
> 
> The dispatcher->OnEventFromSource() now doesn't work because it happens after
> IME.
> The EventSource::SendEventToProcessor() happens before IME but it is
protected.
> Should make it as public?

No, keep it as you have it.

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:195: // Makes sure the input method is focused by
default when created.
On 2015/06/04 15:59:25, Shu Chen wrote:
> On 2015/06/04 13:40:28, sky wrote:
> > This doesn't make sense to me. Why does the input_method_ need an initial
> focus.
> 
> InputMethod::OnFocus() should be called when a native root window
> (WindowTreeHost) is activated/focused.
> However, for Ash, WindowTreeHost doesn't have the activation/focus state, and
> WindowTreeHost::OnHostActivated() never gets called.
> So here to make InputMethod by default focused is to make Ash work.
> I've added this into the comment.

This code effects desktop too though. Is that ok? You should add a more
descriptive comment too.

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.h:125: // Sets the InputMethod instance, and it is not
owned by this WindowTreeHost.
On 2015/06/04 15:59:25, Shu Chen wrote:
> On 2015/06/04 13:40:28, sky wrote:
> > Sets a shared unowned InputMethod. This is used when there is a singleton
> > InputMethod shared between multiple instances.
> > 
> > Can this be protected?
> 
> Revised the comment.
> 
> To make it protected, a new param ui::InputMethod* needs to be added to
> constructors of AshWindowTreeHostXxx. And add a new member ui::InputMethod* to
> AshWindowTreeHostInitParams.
> 
> I would prefer to remain this as public because the singleton/shared
InputMethod
> is a temporary solution.
> My later CL will try to make each AshWindowTreeHost to own a separated
> InputMethod instance.
> 

Fair enough.

[Shu Chen, 2015-06-05 00:48:32]

On 2015/06/04 16:20:51, oshima wrote:
> On 2015/06/04 07:25:17, James Su wrote:
> > On 2015/06/04 06:50:26, oshima wrote:
> > > On Wed, Jun 3, 2015 at 11:05 PM, <mailto:suzhe@chromium.org> wrote:
> > > 
> > > > On 2015/06/04 05:49:27, oshima wrote:
> > > >
> > > >> On 2015/06/04 05:15:14, James Su wrote:
> > > >> > On 2015/06/04 04:36:49, oshima wrote:
> > > >> > > On 2015/06/04 04:10:24, James Su wrote:
> > > >> > > > On 2015/06/03 16:18:56, oshima wrote:
> > > >> > > > > On 2015/06/03 10:01:23, Shu Chen wrote:
> > > >> > > > > > FYI. Just verified on a win8.1 VM, and this cl doesn't cause
> any
> > > >> > > regressions
> > > >> > > > > for
> > > >> > > > > > major scenarios.
> > > >> > > > >
> > > >> > > > > Re a) The test still makes sense, at least on X. X and aura has
> > > >> its
> > > >>
> > > > own
> > > >
> > > >> > > focus
> > > >> > > > > system,
> > > >> > > > > and that test is to make sure that aura focus works regardless
of
> > > >>
> > > > which
> > > >
> > > >> > > native
> > > >> > > > X
> > > >> > > > > window has focus.
> > > >> > > >
> > > >> > > > IMHO, native window focus should always match aura window focus
> > > >> (i.e.
> > > >>
> > > > the
> > > >
> > > >> > > native
> > > >> > > > window, in which the active aura window lives, should always have
> > > >> native
> > > >> > > focus),
> > > >> > > > otherwise unpredictable things may happen.
> > > >> > > > For example: the native input method may use the focused native
> > > >> window
> > > >>
> > > > to
> > > >
> > > >> > > > position its floating suggestion UI, if the native focus and aura
> > > >> focus
> > > >> > don't
> > > >> > > > match, then native input method's floating suggestion UI may be
> > > >> wrongly
> > > >> > > > positioned on the screen.
> > > >> > > > Other bad things may happen if such mismatch happens on Windows,
> > > >> because
> > > >> key
> > > >> > > > events are dispatched to the native input method context bound to
> > > >> the
> > > >> > focused
> > > >> > > > hwnd by system before being dispatched to Chrome. So, we can't
know
> > > >> what
> > > >> > will
> > > >> > > > happen if the active aura window is not inside the focused hwnd.
> > > >> > > >
> > > >> > > > So I'm wondering in which case such focus mismatch will happen?
And
> > > >> why
> > > >>
> > > > we
> > > >
> > > >> > > allow
> > > >> > > > such mismatch? Can we avoid it on all platforms? If such focus
> > > >> mismatch
> > > >>
> > > > is
> > > >
> > > >> > not
> > > >> > > > by design, then I think we should treat it as an invalid case and
> > > >> remove
> > > >> > this
> > > >> > > > test, and add a new test to ensure the native focus and aura
focus
> > > >>
> > > > always
> > > >
> > > >> > > match.
> > > >> > >
> > > >> > > This is for ash/chromeos case, not for desktop aura. On
ash/chromeos,
> > > >>
> > > > native
> > > >
> > > >> > > window
> > > >> > > isn't really window, just a thing that host desktop for each
display
> > > >> > >
> > > >> > > I agree that they have to match in desktop aura, but that's not
this
> > > >> test
> > > >>
> > > > is
> > > >
> > > >> > > for.
> > > >> >
> > > >> > So this test will never run for other platforms (e.g. ash/windows)
> > > >> except
> > > >> > ash/chromeos?
> > > >> >
> > > >> > We probably need to add some comments to clarify it somewhere.
> > > >>
> > > >
> > > >  The test is in ash/ so it's very clear.
> > > >>
> > > > I understand that ash is also used on Windows8 metro, does it also have
> > > > the same
> > > > focus mismatch issue? And how about ash/chromeos/x11? (Do we still need
to
> > > > care
> > > > about ash/chromeos/x11?)
> > > 
> > > 
> > > metro mode won't have more than one host window. The test is designed to
> > > run only
> > > when there can be multiple host windows in ash.
> > I see. Then we can focus on ash/chromeos. Do we still need to consider
> > ash/chromeos/x11?
> 
> Yes, X11 is still in use and will be used for development.
> 
> > 
> > > 
> > > 
> > > >
> > > >  >
> > > >> > And IMHO, the event reroute logic in
> > > >> WindowTargeter::FindTargetForEvent()
> > > >> > method:
> > > >> >
> > > >>
> > > >
> > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_tar...
> > > >
> > > >> >
> > > >> > looks so tricky. At least it breaks a common code practice: a method
> > > >> should
> > > >> only
> > > >> > do one thing, and should not have side effect. This method clearly
has
> a
> > > >>
> > > > side
> > > >
> > > >> > effect when the given root target is not focused: it reroutes the
even
> > > >> to
> > > >>
> > > > the
> > > >
> > > >> > real focused target and returns null to caller.
> > > >> > By looking at the method declaration, people won't know this method
> will
> > > >> > dispatch the event sometime.
> > > >>
> > > >
> > > >  That method has nothing to do with focus. This is to deal with
> > > >>
> > > > cross-root-window
> > > >
> > > >> native mouse capture.
> > > >> It can have another out parameter for modified event, but i'll leave it
> to
> > > >> sadrul@ who designed it.
> > > >>
> > > > According to the code, this method takes effect for key events as well.
If
> > > > there
> > > > is no focus mismatch issue between native window and active aura window,
> > > > then no
> > > > necessary to reroute key events in this method at all.
> > > >
> > > >
> > > Maybe we were talking about different thing? I thought you were referring
> > > to the code that
> > > update the location in event. That side effect only happens for located
> > > event, not key event.
> > I'm talking about the reroute logic related to both located events and key
> > events in WindowTargeter::FindTargetForEvent().
> > IMHO, calling
new_root->GetHost()->event_processor()->OnEventFromSource(event)
> > to reroute events in WindowTargeter::FindTargetForEvent() is very confusing.
> > If only look at the method definition in EventTargeter, nobody would know
that
> > an event maybe re-dispatched by this method:
> > 
> >   // Returns the target |event| should be dispatched to. If there is no such
> >   // target, return NULL. If |event| is a located event, the location of
> |event|
> >   // is in the coordinate space of |root|. Furthermore, the targeter can
> mutate
> >   // the event (e.g., by changing the location of the event to be in the
> >   // returned target's coordinate space) so that it can be dispatched to the
> >   // target without any further modification.
> >   virtual EventTarget* FindTargetForEvent(EventTarget* root,
> >                                           Event* event);
> 
> I see. It's orthogonal to the issue (it has to be re-dispatched one way or
> another),
> so you should talk to sadrul if you want to change the design.
> 
> > 
> > > 
> > > And the focused aura window may or may not be in the host window that
> > > received the key event
> > > at ui/aura level. It's certainly possible to have different targeter for
> > > ash and desktop aura.
> > > I don't have strong opinion about which is better though.
> > As I mentioned in comment #30, if focused aura window is not in the focused
> > native window (which receives the native key events), then the behavior of
> > system native IME maybe broken.
> > It maybe ok for chromeos, as there is no system native IME at all (IME
> extension
> > is fully controlled by us). But it may not be ok for Windows, Linux and Mac,
> > where system native IME is out of our control, and we don't know if the
system
> > native IME relies on the focused native window to work.
> > 
> > That's why I want to know whether this focus mismatch issue only happens on
> > chromeos or not.
> > 
> > Even for chromeos, the code maybe more easier to understand if we can make
> sure
> > key events are always directly dispatched to the window tree host which
> contains
> > the active aura window, rather than allowing such focus mismatch and
> > re-dispatching the key events to the correct window tree host in
> > WindowTargeter::FindTargetForEvent().
> 
> There is no such "native window" that has a focus in chromeos (conceptually).
> It's just a mechanism that host the screen, and ash code is designed to be
> independent on the native window behavior. (another example is display layout)
> 
> Key events should be dispatched to the focused window. It shouldn't really
> matter
> who receives first. It even doesn't have to be WTH, and I think this is the
> case on ozone (at least, I can't find key handling code in WTHOzone code).
> 
> Again, I'm talking about ash/chromeos only. I fully understand that they
should
> match
> on desktop aura.
> 
> > 
> > > 
> > > 
> > > >
> > > >  >
> > > >> > Is it possible to move this logic into a different place to make the
> > > >> code
> > > >> easier
> > > >> > to understand?
> > > >>
> > > >
> > > >
> > > >
> > > > https://codereview.chromium.org/1155013005/
> > > >
> > > 
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.

Thanks, Oshima. I will talk with Sadrul about it.

[Shu Chen, 2015-06-05 00:52:19]

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/560001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:195: // Makes sure the input method is focused by
default when created.
On 2015/06/04 19:42:52, sky wrote:
> On 2015/06/04 15:59:25, Shu Chen wrote:
> > On 2015/06/04 13:40:28, sky wrote:
> > > This doesn't make sense to me. Why does the input_method_ need an initial
> > focus.
> > 
> > InputMethod::OnFocus() should be called when a native root window
> > (WindowTreeHost) is activated/focused.
> > However, for Ash, WindowTreeHost doesn't have the activation/focus state,
and
> > WindowTreeHost::OnHostActivated() never gets called.
> > So here to make InputMethod by default focused is to make Ash work.
> > I've added this into the comment.
> 
> This code effects desktop too though. Is that ok? You should add a more
> descriptive comment too.

It is ok, because this behavior remains no change as original:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/input_m...

I can move this OnFocus call to DisplayController so it will be Ash only, but it
would be safer to have a separated CL to do that for easier rollback on
regressions.

[sky, 2015-06-05 15:10:51]

LGTM

[Avi (use Gerrit), 2015-06-05 15:14:53]

On 2015/06/05 15:10:51, sky wrote:
> LGTM

Since everyone's happy here, I'll stamp the content/ files. LGTM

[sadrul, 2015-06-05 15:39:34]

A couple of questions:

In the CL description:
> 1) The key events processing by input method happens after event
>    rewriters and before the EventHandler's, which remains no
>    change to original.

This is incorrect, right? Before this change, the input-method would receive the
event from InputMethodEventFilter. There could be several pre-target handlers
before the IMEF in the list, and so those pre-target handlers would receive the
key-events before the input method. After this change, the input-method gets the
event first?

> Refactoring the ownership of ui::InputMethod.

This CL does more than refactoring the ownership. Can you move the actual
ownership change in a separate CL?

[Shu Chen, 2015-06-06 04:58:29]

On 2015/06/05 15:39:34, sadrul wrote:
> A couple of questions:
> 
> In the CL description:
> > 1) The key events processing by input method happens after event
> >    rewriters and before the EventHandler's, which remains no
> >    change to original.
> 
> This is incorrect, right? Before this change, the input-method would receive
the
> event from InputMethodEventFilter. There could be several pre-target handlers
> before the IMEF in the list, and so those pre-target handlers would receive
the
> key-events before the input method. After this change, the input-method gets
the
> event first?
You're right. This cl will essentially change the behavior.
For desktop browser scenario, see in DNWA::InitNativeWidget(), the
InputMethodEventFilter (put in root_window_event_filter_) is the first event
handler.
For Ash scenario, see in Shell::Init(), there are handlers planted before
InputMethodEventFilter (FocusController is excluded because it isn't interested
in key events):
 - MagnifierKeyScroller (CrOS only): handles Arrow keys when magnification is
enabled.
 - SpokenFeedbackToggler (CrOS only): handles F6 key when SpokenFeedbackToggler
is enabled.
 - OverlayEventFilter: handles ESC/OEM_2/HELP/F14 keys to cancel the overlay UI.
I went through all above cases and cannot think of potential regressions could
be caused by this cl.
IME does handle some keys which may conflict with above, e.g. ESC & Arrow keys.
However, IME usually handles those keys when the suggestion popup UI is at
present.
IMO, once the suggestion popup UI is shown, it would be debatable that who (IME
or Magnifier/Overlay) should handle those keys first.
Since the IMEs of CrOS are fully controller by us, I would say it is pretty safe
to move the InputMethod ahead of above handlers.

> 
> > Refactoring the ownership of ui::InputMethod.
> 
> This CL does more than refactoring the ownership. Can you move the actual
> ownership change in a separate CL?

The core change of this cl is to change the ownership of InputMethod:
from InputMethodEventFilter (which is owned by Shell and DNWA) to
DisplayController (for Ash) and WTH.
Other changes are based on the core ownership change, including:
 - removing InputMethodEventFilter
 - removing event.SetTranslated()/IsTranslated() flag.
 - make WTH inherit form EventSource (so that the InputMethod can receive the
event after rewriters and before the followup handlers).
I don't see any benefits to split the cl into smaller ones.

[sky, 2015-06-06 17:50:49]

On Fri, Jun 5, 2015 at 9:58 PM,  <shuchen@chromium.org> wrote:
> On 2015/06/05 15:39:34, sadrul wrote:
>>
>> A couple of questions:
>
>
>> In the CL description:
>> > 1) The key events processing by input method happens after event
>> >    rewriters and before the EventHandler's, which remains no
>> >    change to original.
>
>
>> This is incorrect, right? Before this change, the input-method would
>> receive
>
> the
>>
>> event from InputMethodEventFilter. There could be several pre-target
>> handlers
>> before the IMEF in the list, and so those pre-target handlers would
>> receive
>
> the
>>
>> key-events before the input method. After this change, the input-method
>> gets
>
> the
>>
>> event first?
>
> You're right. This cl will essentially change the behavior.
> For desktop browser scenario, see in DNWA::InitNativeWidget(), the
> InputMethodEventFilter (put in root_window_event_filter_) is the first event
> handler.

Will this effect events dispatched to web pages then?

> For Ash scenario, see in Shell::Init(), there are handlers planted before
> InputMethodEventFilter (FocusController is excluded because it isn't
> interested
> in key events):
>  - MagnifierKeyScroller (CrOS only): handles Arrow keys when magnification
> is
> enabled.
>  - SpokenFeedbackToggler (CrOS only): handles F6 key when
> SpokenFeedbackToggler
> is enabled.
>  - OverlayEventFilter: handles ESC/OEM_2/HELP/F14 keys to cancel the overlay
> UI.
> I went through all above cases and cannot think of potential regressions
> could
> be caused by this cl.
> IME does handle some keys which may conflict with above, e.g. ESC & Arrow
> keys.
> However, IME usually handles those keys when the suggestion popup UI is at
> present.
> IMO, once the suggestion popup UI is shown, it would be debatable that who
> (IME
> or Magnifier/Overlay) should handle those keys first.
> Since the IMEs of CrOS are fully controller by us, I would say it is pretty
> safe
> to move the InputMethod ahead of above handlers.
>
>
>> > Refactoring the ownership of ui::InputMethod.
>
>
>> This CL does more than refactoring the ownership. Can you move the actual
>> ownership change in a separate CL?
>
>
> The core change of this cl is to change the ownership of InputMethod:
> from InputMethodEventFilter (which is owned by Shell and DNWA) to
> DisplayController (for Ash) and WTH.
> Other changes are based on the core ownership change, including:
>  - removing InputMethodEventFilter
>  - removing event.SetTranslated()/IsTranslated() flag.
>  - make WTH inherit form EventSource (so that the InputMethod can receive
> the
> event after rewriters and before the followup handlers).
> I don't see any benefits to split the cl into smaller ones.
>
>
> https://codereview.chromium.org/1155013005/

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

[Shu Chen, 2015-06-08 01:54:28]

On 2015/06/06 17:50:49, sky wrote:
> On Fri, Jun 5, 2015 at 9:58 PM,  <mailto:shuchen@chromium.org> wrote:
> > On 2015/06/05 15:39:34, sadrul wrote:
> >>
> >> A couple of questions:
> >
> >
> >> In the CL description:
> >> > 1) The key events processing by input method happens after event
> >> >    rewriters and before the EventHandler's, which remains no
> >> >    change to original.
> >
> >
> >> This is incorrect, right? Before this change, the input-method would
> >> receive
> >
> > the
> >>
> >> event from InputMethodEventFilter. There could be several pre-target
> >> handlers
> >> before the IMEF in the list, and so those pre-target handlers would
> >> receive
> >
> > the
> >>
> >> key-events before the input method. After this change, the input-method
> >> gets
> >
> > the
> >>
> >> event first?
> >
> > You're right. This cl will essentially change the behavior.
> > For desktop browser scenario, see in DNWA::InitNativeWidget(), the
> > InputMethodEventFilter (put in root_window_event_filter_) is the first event
> > handler.
> 
> Will this effect events dispatched to web pages then?
No.
For desktop, no behavior will be changed.
For Ash, the handling order change will not effect events dispatched to web
pages.

> 
> > For Ash scenario, see in Shell::Init(), there are handlers planted before
> > InputMethodEventFilter (FocusController is excluded because it isn't
> > interested
> > in key events):
> >  - MagnifierKeyScroller (CrOS only): handles Arrow keys when magnification
> > is
> > enabled.
> >  - SpokenFeedbackToggler (CrOS only): handles F6 key when
> > SpokenFeedbackToggler
> > is enabled.
> >  - OverlayEventFilter: handles ESC/OEM_2/HELP/F14 keys to cancel the overlay
> > UI.
> > I went through all above cases and cannot think of potential regressions
> > could
> > be caused by this cl.
> > IME does handle some keys which may conflict with above, e.g. ESC & Arrow
> > keys.
> > However, IME usually handles those keys when the suggestion popup UI is at
> > present.
> > IMO, once the suggestion popup UI is shown, it would be debatable that who
> > (IME
> > or Magnifier/Overlay) should handle those keys first.
> > Since the IMEs of CrOS are fully controller by us, I would say it is pretty
> > safe
> > to move the InputMethod ahead of above handlers.
> >
> >
> >> > Refactoring the ownership of ui::InputMethod.
> >
> >
> >> This CL does more than refactoring the ownership. Can you move the actual
> >> ownership change in a separate CL?
> >
> >
> > The core change of this cl is to change the ownership of InputMethod:
> > from InputMethodEventFilter (which is owned by Shell and DNWA) to
> > DisplayController (for Ash) and WTH.
> > Other changes are based on the core ownership change, including:
> >  - removing InputMethodEventFilter
> >  - removing event.SetTranslated()/IsTranslated() flag.
> >  - make WTH inherit form EventSource (so that the InputMethod can receive
> > the
> > event after rewriters and before the followup handlers).
> > I don't see any benefits to split the cl into smaller ones.
> >
> >
> > https://codereview.chromium.org/1155013005/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2015-06-08 02:02:59]

On 2015/06/06 04:58:29, Shu Chen wrote:
> On 2015/06/05 15:39:34, sadrul wrote:
> > A couple of questions:
> > 
> > In the CL description:
> > > 1) The key events processing by input method happens after event
> > >    rewriters and before the EventHandler's, which remains no
> > >    change to original.
> > 
> > This is incorrect, right? Before this change, the input-method would receive
> the
> > event from InputMethodEventFilter. There could be several pre-target
handlers
> > before the IMEF in the list, and so those pre-target handlers would receive
> the
> > key-events before the input method. After this change, the input-method gets
> the
> > event first?
> You're right. This cl will essentially change the behavior.
> For desktop browser scenario, see in DNWA::InitNativeWidget(), the
> InputMethodEventFilter (put in root_window_event_filter_) is the first event
> handler.
> For Ash scenario, see in Shell::Init(), there are handlers planted before
> InputMethodEventFilter (FocusController is excluded because it isn't
interested
> in key events):
>  - MagnifierKeyScroller (CrOS only): handles Arrow keys when magnification is
> enabled.
>  - SpokenFeedbackToggler (CrOS only): handles F6 key when
SpokenFeedbackToggler
> is enabled.
>  - OverlayEventFilter: handles ESC/OEM_2/HELP/F14 keys to cancel the overlay
UI.
> I went through all above cases and cannot think of potential regressions could
> be caused by this cl.
> IME does handle some keys which may conflict with above, e.g. ESC & Arrow
keys.
> However, IME usually handles those keys when the suggestion popup UI is at
> present.
> IMO, once the suggestion popup UI is shown, it would be debatable that who
(IME
> or Magnifier/Overlay) should handle those keys first.
> Since the IMEs of CrOS are fully controller by us, I would say it is pretty
safe
> to move the InputMethod ahead of above handlers.
> 
> > 
> > > Refactoring the ownership of ui::InputMethod.
> > 
> > This CL does more than refactoring the ownership. Can you move the actual
> > ownership change in a separate CL?
> 
> The core change of this cl is to change the ownership of InputMethod:
> from InputMethodEventFilter (which is owned by Shell and DNWA) to
> DisplayController (for Ash) and WTH.
> Other changes are based on the core ownership change, including:
>  - removing InputMethodEventFilter
>  - removing event.SetTranslated()/IsTranslated() flag.
>  - make WTH inherit form EventSource (so that the InputMethod can receive the
> event after rewriters and before the followup handlers).
> I don't see any benefits to split the cl into smaller ones.

OK. Thanks for looking into the ordering issue to make sure there is not
behavioural change in this CL (perhaps update the CL description accordingly
too)

[Shu Chen, 2015-06-08 02:05:12]

On 2015/06/08 02:02:59, sadrul wrote:
> On 2015/06/06 04:58:29, Shu Chen wrote:
> > On 2015/06/05 15:39:34, sadrul wrote:
> > > A couple of questions:
> > > 
> > > In the CL description:
> > > > 1) The key events processing by input method happens after event
> > > >    rewriters and before the EventHandler's, which remains no
> > > >    change to original.
> > > 
> > > This is incorrect, right? Before this change, the input-method would
receive
> > the
> > > event from InputMethodEventFilter. There could be several pre-target
> handlers
> > > before the IMEF in the list, and so those pre-target handlers would
receive
> > the
> > > key-events before the input method. After this change, the input-method
gets
> > the
> > > event first?
> > You're right. This cl will essentially change the behavior.
> > For desktop browser scenario, see in DNWA::InitNativeWidget(), the
> > InputMethodEventFilter (put in root_window_event_filter_) is the first event
> > handler.
> > For Ash scenario, see in Shell::Init(), there are handlers planted before
> > InputMethodEventFilter (FocusController is excluded because it isn't
> interested
> > in key events):
> >  - MagnifierKeyScroller (CrOS only): handles Arrow keys when magnification
is
> > enabled.
> >  - SpokenFeedbackToggler (CrOS only): handles F6 key when
> SpokenFeedbackToggler
> > is enabled.
> >  - OverlayEventFilter: handles ESC/OEM_2/HELP/F14 keys to cancel the overlay
> UI.
> > I went through all above cases and cannot think of potential regressions
could
> > be caused by this cl.
> > IME does handle some keys which may conflict with above, e.g. ESC & Arrow
> keys.
> > However, IME usually handles those keys when the suggestion popup UI is at
> > present.
> > IMO, once the suggestion popup UI is shown, it would be debatable that who
> (IME
> > or Magnifier/Overlay) should handle those keys first.
> > Since the IMEs of CrOS are fully controller by us, I would say it is pretty
> safe
> > to move the InputMethod ahead of above handlers.
> > 
> > > 
> > > > Refactoring the ownership of ui::InputMethod.
> > > 
> > > This CL does more than refactoring the ownership. Can you move the actual
> > > ownership change in a separate CL?
> > 
> > The core change of this cl is to change the ownership of InputMethod:
> > from InputMethodEventFilter (which is owned by Shell and DNWA) to
> > DisplayController (for Ash) and WTH.
> > Other changes are based on the core ownership change, including:
> >  - removing InputMethodEventFilter
> >  - removing event.SetTranslated()/IsTranslated() flag.
> >  - make WTH inherit form EventSource (so that the InputMethod can receive
the
> > event after rewriters and before the followup handlers).
> > I don't see any benefits to split the cl into smaller ones.
> 
> OK. Thanks for looking into the ordering issue to make sure there is not
> behavioural change in this CL (perhaps update the CL description accordingly
> too)

Thanks, I've updated the CL description for the event handling order change for
Ash.

[Shu Chen, 2015-06-08 02:25:23]

Sadrul, may I get your LGTM for this cl? Thanks

[Shu Chen, 2015-06-08 22:38:32]

pinging...
Sadrul, do you think it is good to commit this cl?

[sadrul, 2015-06-08 22:45:12]

Hi! Sorry, I am OOO today. I have a draft comment from last night. I will take a
second look later tonight and let you know if I have any more feedback.

https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:200: bool
WindowTreeHost::DispatchKeyEventPostIME(const ui::KeyEvent& event) {
Not for this CL, but would it be possible for DispatchKeyEventPostIME() (and
InputMethod::DispatchKeyEvent()) to return an EventDispatchDetails? It is
possible for the dispatched event to actually destroy the target/dispatcher. So
it would be good to retain that information and pass it on to the caller.

[Shu Chen, 2015-06-09 02:28:49]

https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:200: bool
WindowTreeHost::DispatchKeyEventPostIME(const ui::KeyEvent& event) {
On 2015/06/08 22:45:12, sadrul wrote:
> Not for this CL, but would it be possible for DispatchKeyEventPostIME() (and
> InputMethod::DispatchKeyEvent()) to return an EventDispatchDetails? It is
> possible for the dispatched event to actually destroy the target/dispatcher.
So
> it would be good to retain that information and pass it on to the caller.

The caller of InputMethod::DispatchKeyEvent() wants to know whether the
InputMethod has handled the key event, so that it can stop the propagation as
appropriate (see WindowTreeHost::DeliverEventToProcessor below).
I would prefer to put EventDispatchDetails as the second param for the
DispatchKeyEvent().
So that the bool return value can be kept for above reason.
It is doable in a separated CL.

However, I'd like to raise a concern to the design of event dispatching.
Currently, the re-writer logics are implemented in EventSource, which is WTH.
So to dispatch events from a WTH, the re-writer logics run.
For re-dispatching logics in WindowTargeter::FindTargetForEvent(), which happens
after re-writers, will cause the re-writers run again in another WTH.
Is it feasible to put the re-writer logics close to the platform-dependent
layer? so that they can:
1) directly read/write the native events instead of ui::KeyEvent's.
2) become global, instead of bound to certain WTH.
3) InputMethod can intercept the key events from there too.
Therefore, there would be no need to deal with EventDispatchDetails.

[Shu Chen, 2015-06-09 03:08:59]

On 2015/06/09 02:28:49, Shu Chen wrote:
>
https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
> File ui/aura/window_tree_host.cc (right):
> 
>
https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
> ui/aura/window_tree_host.cc:200: bool
> WindowTreeHost::DispatchKeyEventPostIME(const ui::KeyEvent& event) {
> On 2015/06/08 22:45:12, sadrul wrote:
> > Not for this CL, but would it be possible for DispatchKeyEventPostIME() (and
> > InputMethod::DispatchKeyEvent()) to return an EventDispatchDetails? It is
> > possible for the dispatched event to actually destroy the target/dispatcher.
> So
> > it would be good to retain that information and pass it on to the caller.
> 
> The caller of InputMethod::DispatchKeyEvent() wants to know whether the
> InputMethod has handled the key event, so that it can stop the propagation as
> appropriate (see WindowTreeHost::DeliverEventToProcessor below).
> I would prefer to put EventDispatchDetails as the second param for the
> DispatchKeyEvent().
> So that the bool return value can be kept for above reason.
> It is doable in a separated CL.
> 
> However, I'd like to raise a concern to the design of event dispatching.
> Currently, the re-writer logics are implemented in EventSource, which is WTH.
> So to dispatch events from a WTH, the re-writer logics run.
> For re-dispatching logics in WindowTargeter::FindTargetForEvent(), which
happens
> after re-writers, will cause the re-writers run again in another WTH.
Please ignore this, the re-dispatching uses OnEventFromSource() instead of
SendEventToProcessor() so no more re-writers are involved there.
> Is it feasible to put the re-writer logics close to the platform-dependent
> layer? so that they can:
> 1) directly read/write the native events instead of ui::KeyEvent's.
> 2) become global, instead of bound to certain WTH.
> 3) InputMethod can intercept the key events from there too.
> Therefore, there would be no need to deal with EventDispatchDetails.

[sadrul, 2015-06-09 15:29:33]

https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:200: bool
WindowTreeHost::DispatchKeyEventPostIME(const ui::KeyEvent& event) {
On 2015/06/09 02:28:49, Shu Chen wrote:
> On 2015/06/08 22:45:12, sadrul wrote:
> > Not for this CL, but would it be possible for DispatchKeyEventPostIME() (and
> > InputMethod::DispatchKeyEvent()) to return an EventDispatchDetails? It is
> > possible for the dispatched event to actually destroy the target/dispatcher.
> So
> > it would be good to retain that information and pass it on to the caller.
> 
> The caller of InputMethod::DispatchKeyEvent() wants to know whether the
> InputMethod has handled the key event, so that it can stop the propagation as
> appropriate (see WindowTreeHost::Deliver tarEventToProcessor below).
> I would prefer to put EventDispatchDetails as the second param for the
> DispatchKeyEvent().
> So that the bool return value can be kept for above reason.
> It is doable in a separated CL.

You could send a pointer to the KeyEvent in DispatchKeyEvent(), so that the
consumer can set the handled bit on the event if it handled the event. This
would be consistent with the normal event dispatch logic. (passing the EDD as
the second param would also work, but maybe not as nice because of
inconsistency).

> 
> However, I'd like to raise a concern to the design of event dispatching.
> Currently, the re-writer logics are implemented in EventSource, which is WTH.
> So to dispatch events from a WTH, the re-writer logics run.
> For re-dispatching logics in WindowTargeter::FindTargetForEvent(), which
happens
> after re-writers, will cause the re-writers run again in another WTH.
> Is it feasible to put the re-writer logics close to the platform-dependent
> layer? so that they can:
> 1) directly read/write the native events instead of ui::KeyEvent's.
> 2) become global, instead of bound to certain WTH.
> 3) InputMethod can intercept the key events from there too.
> Therefore, there would be no need to deal with EventDispatchDetails.

EventDispatchDetails is used to track whether the event-target or the host is
destroyed during the dispatch, and the InputMethod is also involved during the
dispatch phase. So I do not see how changing anything in the rewriter/targeter
logic obviates its usefulness.

> Is it feasible to put the re-writer logics close to the platform-dependent
> layer? so that they can:
> 1) directly read/write the native events instead of ui::KeyEvent's.

We should use less of 'platform dependent' events, and not more. (e.g. we used
to have chromeos event-rewriter that used to deal with raw X11 events. But for
ozone, it has been converted to work with ui::KeyEvent (kpschoedel@ can give you
more details)).

> 2) become global, instead of bound to certain WTH.

This can maybe happen, yes (maybe these should hang off of Env).

> 3) InputMethod can intercept the key events from there too.

It would be terrific if IM could be implemented as an event-rewriter. I seem to
remember I made this suggestion earlier in the doc.

https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.h:144: void SetInputMethod(ui::InputMethod*
input_method);
Call this SetSharedInputMethod()?

https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.h:147: bool DispatchKeyEventPostIME(const ui::KeyEvent&
event) override;
Can this go in the protected/private section?

https://codereview.chromium.org/1155013005/diff/610001/ui/base/ime/input_meth...
File ui/base/ime/input_method_factory.cc (right):

https://codereview.chromium.org/1155013005/diff/610001/ui/base/ime/input_meth...
ui/base/ime/input_method_factory.cc:17: #elif defined(USE_AURA) &&
defined(OS_LINUX) && defined(USE_X11) && \
I don't see anything x11 specific in InputMethodAuraLinux. Why make this change?
(I think intel folks use this for non-x11 desktop-ozone builds)

[sadrul, 2015-06-09 15:30:16]

LGTM with the comments addressed.

[Shu Chen, 2015-06-10 03:01:35]

https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:200: bool
WindowTreeHost::DispatchKeyEventPostIME(const ui::KeyEvent& event) {
On 2015/06/09 15:29:32, sadrul wrote:
> On 2015/06/09 02:28:49, Shu Chen wrote:
> > On 2015/06/08 22:45:12, sadrul wrote:
> > > Not for this CL, but would it be possible for DispatchKeyEventPostIME()
(and
> > > InputMethod::DispatchKeyEvent()) to return an EventDispatchDetails? It is
> > > possible for the dispatched event to actually destroy the
target/dispatcher.
> > So
> > > it would be good to retain that information and pass it on to the caller.
> > 
> > The caller of InputMethod::DispatchKeyEvent() wants to know whether the
> > InputMethod has handled the key event, so that it can stop the propagation
as
> > appropriate (see WindowTreeHost::Deliver tarEventToProcessor below).
> > I would prefer to put EventDispatchDetails as the second param for the
> > DispatchKeyEvent().
> > So that the bool return value can be kept for above reason.
> > It is doable in a separated CL.
> 
> You could send a pointer to the KeyEvent in DispatchKeyEvent(), so that the
> consumer can set the handled bit on the event if it handled the event. This
> would be consistent with the normal event dispatch logic. (passing the EDD as
> the second param would also work, but maybe not as nice because of
> inconsistency).
> 
> > 
> > However, I'd like to raise a concern to the design of event dispatching.
> > Currently, the re-writer logics are implemented in EventSource, which is
WTH.
> > So to dispatch events from a WTH, the re-writer logics run.
> > For re-dispatching logics in WindowTargeter::FindTargetForEvent(), which
> happens
> > after re-writers, will cause the re-writers run again in another WTH.
> > Is it feasible to put the re-writer logics close to the platform-dependent
> > layer? so that they can:
> > 1) directly read/write the native events instead of ui::KeyEvent's.
> > 2) become global, instead of bound to certain WTH.
> > 3) InputMethod can intercept the key events from there too.
> > Therefore, there would be no need to deal with EventDispatchDetails.
> 
> EventDispatchDetails is used to track whether the event-target or the host is
> destroyed during the dispatch, and the InputMethod is also involved during the
> dispatch phase. So I do not see how changing anything in the rewriter/targeter
> logic obviates its usefulness.
> 
> > Is it feasible to put the re-writer logics close to the platform-dependent
> > layer? so that they can:
> > 1) directly read/write the native events instead of ui::KeyEvent's.
> 
> We should use less of 'platform dependent' events, and not more. (e.g. we used
> to have chromeos event-rewriter that used to deal with raw X11 events. But for
> ozone, it has been converted to work with ui::KeyEvent (kpschoedel@ can give
you
> more details)).
> 
> > 2) become global, instead of bound to certain WTH.
> 
> This can maybe happen, yes (maybe these should hang off of Env).
> 
> > 3) InputMethod can intercept the key events from there too.
> 
> It would be terrific if IM could be implemented as an event-rewriter. I seem
to
> remember I made this suggestion earlier in the doc.
Actually I meant "InputMethod can intercept the key events from the 'platform
dependent' layer instead of being a event rewriter.

Thanks for the elaboration. I've added a TODO in DeliverEventToProcessor().

https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.h:144: void SetInputMethod(ui::InputMethod*
input_method);
On 2015/06/09 15:29:33, sadrul wrote:
> Call this SetSharedInputMethod()?

Done.

https://codereview.chromium.org/1155013005/diff/610001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.h:147: bool DispatchKeyEventPostIME(const ui::KeyEvent&
event) override;
On 2015/06/09 15:29:33, sadrul wrote:
> Can this go in the protected/private section?

This should remain public because DisplayController wants to call it.

https://codereview.chromium.org/1155013005/diff/610001/ui/base/ime/input_meth...
File ui/base/ime/input_method_factory.cc (right):

https://codereview.chromium.org/1155013005/diff/610001/ui/base/ime/input_meth...
ui/base/ime/input_method_factory.cc:17: #elif defined(USE_AURA) &&
defined(OS_LINUX) && defined(USE_X11) && \
On 2015/06/09 15:29:33, sadrul wrote:
> I don't see anything x11 specific in InputMethodAuraLinux. Why make this
change?
> (I think intel folks use this for non-x11 desktop-ozone builds)

Done. I recall this was to fix tests but cannot remember the exact reason. Let
me revert this and see the trybots.

[Shu Chen, 2015-06-10 04:11:43]

https://codereview.chromium.org/1155013005/diff/610001/ui/base/ime/input_meth...
File ui/base/ime/input_method_factory.cc (right):

https://codereview.chromium.org/1155013005/diff/610001/ui/base/ime/input_meth...
ui/base/ime/input_method_factory.cc:17: #elif defined(USE_AURA) &&
defined(OS_LINUX) && defined(USE_X11) && \
On 2015/06/09 15:29:33, sadrul wrote:
> I don't see anything x11 specific in InputMethodAuraLinux. Why make this
change?
> (I think intel folks use this for non-x11 desktop-ozone builds)

Looks like the ChromecastShellBrowserTest in bot cast_shell_linux will be broken
if revert this change.
The CastShell should use InputMethodMinimal instead of InputMethodAuraLinux.

InputMethodAuraLinux uses ui::LinuxInputMethodContext which is implemented by
X11InputMethodContextImplGtk2 which depends on x11 stuff.

So for linux-desktop-ozone builds, there is no native IME support in Chromium
yet.

[Shu Chen, 2015-06-10 04:13:11]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2015-06-10 04:13:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, oshima@chromium.org, suzhe@chromium.org,
avi@chromium.org, sadrul@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1155013005/#ps650001
(title: "fixed bot failure: cast_shell_linux")

[commit-bot: I haz the power, 2015-06-10 04:13:16]

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

[commit-bot: I haz the power, 2015-06-10 07:48:37]

Committed patchset #34 (id:650001)

[commit-bot: I haz the power, 2015-06-10 07:49:35]

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

[oshima, 2015-06-19 00:43:40]

https://codereview.chromium.org/1155013005/diff/650001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1155013005/diff/650001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:323: }
Hi, we noticed that with this CL, InputMethod receives key event before
EventHandlers receive them.

I thought EventHandler should receive key events first so that we can stop
propagation even to IME (InputEventBlocker for example).

Or am I missing something?

[oshima, 2015-06-19 01:50:42]

On 2015/06/19 00:43:40, oshima wrote:
>
https://codereview.chromium.org/1155013005/diff/650001/ui/aura/window_tree_ho...
> File ui/aura/window_tree_host.cc (right):
> 
>
https://codereview.chromium.org/1155013005/diff/650001/ui/aura/window_tree_ho...
> ui/aura/window_tree_host.cc:323: }
> Hi, we noticed that with this CL, InputMethod receives key event before
> EventHandlers receive them.
> 
> I thought EventHandler should receive key events first so that we can stop
> propagation even to IME (InputEventBlocker for example).
> 
> Or am I missing something?

Ugh, this also broke key input in Unified Desktop mode. Filed crbug.com/502113.

[James Su, 2015-06-19 02:05:07]

On 2015/06/19 00:43:40, oshima wrote:
>
https://codereview.chromium.org/1155013005/diff/650001/ui/aura/window_tree_ho...
> File ui/aura/window_tree_host.cc (right):
> 
>
https://codereview.chromium.org/1155013005/diff/650001/ui/aura/window_tree_ho...
> ui/aura/window_tree_host.cc:323: }
> Hi, we noticed that with this CL, InputMethod receives key event before
> EventHandlers receive them.
> 
> I thought EventHandler should receive key events first so that we can stop
> propagation even to IME (InputEventBlocker for example).
> 
> Or am I missing something?

Having IME to handle all key events before anything else is a common practice.
For example, on Windows, key events will be dispatched to IME by system even
before entering chrome. On Mac, key events are dispatched to IME by system when
we call NSResponder's - interpretKeyEvents: method, which is called at very
beginning in Chrome's code.

From the user's point of view, if the user is using an IME, the IME may want to
handle all key events. Surprise behavior may happen if some key events get
intercepted by something else.
And when the user is not using the IME, s/he will turn off the IME and then the
IME will not intercept any key event, so that other functionality will not be
affected at all.

Thus, one of the most important purpose of this refactor is to allow IME handles
key events at the very beginning, especially before all event filters.
If this refactor causes any regression, then we should fix the regression
without changing this key event handling sequence.

And btw, what is InputEventBlocker? I can't find it in code search.

[oshima, 2015-06-19 04:07:16]

On 2015/06/19 02:05:07, James Su wrote:
> On 2015/06/19 00:43:40, oshima wrote:
> >
>
https://codereview.chromium.org/1155013005/diff/650001/ui/aura/window_tree_ho...
> > File ui/aura/window_tree_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/1155013005/diff/650001/ui/aura/window_tree_ho...
> > ui/aura/window_tree_host.cc:323: }
> > Hi, we noticed that with this CL, InputMethod receives key event before
> > EventHandlers receive them.
> > 
> > I thought EventHandler should receive key events first so that we can stop
> > propagation even to IME (InputEventBlocker for example).
> > 
> > Or am I missing something?
> 
> Having IME to handle all key events before anything else is a common practice.
> For example, on Windows, key events will be dispatched to IME by system even
> before entering chrome. On Mac, key events are dispatched to IME by system
when
> we call NSResponder's - interpretKeyEvents: method, which is called at very
> beginning in Chrome's code.

Windows can intercept the key event before sending to IME. In ChromeOS,
ash is the part of OS, and should have an ability to intercept key event before
IME.

> 
> From the user's point of view, if the user is using an IME, the IME may want
to
> handle all key events. Surprise behavior may happen if some key events get
> intercepted by something else.
> And when the user is not using the IME, s/he will turn off the IME and then
the
> IME will not intercept any key event, so that other functionality will not be
> affected at all.
> 
> Thus, one of the most important purpose of this refactor is to allow IME
handles
> key events at the very beginning, especially before all event filters.
> If this refactor causes any regression, then we should fix the regression
> without changing this key event handling sequence.
> 
> And btw, what is InputEventBlocker? I can't find it in code search.

[James Su, 2015-06-19 04:24:58]

2015-06-19 12:07 GMT+08:00 <oshima@chromium.org>:

> On 2015/06/19 02:05:07, James Su wrote:
>
>> On 2015/06/19 00:43:40, oshima wrote:
>> >
>>
>
>
>
https://codereview.chromium.org/1155013005/diff/650001/ui/aura/window_tree_ho...
>
>> > File ui/aura/window_tree_host.cc (right):
>> >
>> >
>>
>
>
>
https://codereview.chromium.org/1155013005/diff/650001/ui/aura/window_tree_ho...
>
>> > ui/aura/window_tree_host.cc:323: }
>> > Hi, we noticed that with this CL, InputMethod receives key event before
>> > EventHandlers receive them.
>> >
>> > I thought EventHandler should receive key events first so that we can
>> stop
>> > propagation even to IME (InputEventBlocker for example).
>> >
>> > Or am I missing something?
>>
>
>  Having IME to handle all key events before anything else is a common
>> practice.
>> For example, on Windows, key events will be dispatched to IME by system
>> even
>> before entering chrome. On Mac, key events are dispatched to IME by system
>>
> when
>
>> we call NSResponder's - interpretKeyEvents: method, which is called at
>> very
>> beginning in Chrome's code.
>>
>
> Windows can intercept the key event before sending to IME. In ChromeOS,
> ash is the part of OS, and should have an ability to intercept key event
> before
> IME.


Just wondering how to do it on Windows? Can you point me the related doc?
AFAIK, it's not possible, unless you do some tricky thing. At least for
Chrome on Windows, all key events sent to Chrome are already intercepted by
IME.

If we want ash to handle key events before IME, then I think we need to
introduce a different logic to achieve it. But I think it's better to keep
the relationship between WTH and IME and the event dispatching sequence
inside WTH unified across all platforms.


>
>
>
>   From the user's point of view, if the user is using an IME, the IME may
>> want
>>
> to
>
>> handle all key events. Surprise behavior may happen if some key events get
>> intercepted by something else.
>> And when the user is not using the IME, s/he will turn off the IME and
>> then
>>
> the
>
>> IME will not intercept any key event, so that other functionality will
>> not be
>> affected at all.
>>
>
>  Thus, one of the most important purpose of this refactor is to allow IME
>>
> handles
>
>> key events at the very beginning, especially before all event filters.
>> If this refactor causes any regression, then we should fix the regression
>> without changing this key event handling sequence.
>>
>
>  And btw, what is InputEventBlocker? I can't find it in code search.
>>
>
>
>
> https://codereview.chromium.org/1155013005/
>



-- 
- James Su

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

[oshima, 2015-06-19 04:50:14]

On Thu, Jun 18, 2015 at 9:24 PM, James Su <suzhe@chromium.org> wrote:

>
>
> 2015-06-19 12:07 GMT+08:00 <oshima@chromium.org>:
>
>> On 2015/06/19 02:05:07, James Su wrote:
>>
>>> On 2015/06/19 00:43:40, oshima wrote:
>>> >
>>>
>>
>>
>>
https://codereview.chromium.org/1155013005/diff/650001/ui/aura/window_tree_ho...
>>
>>> > File ui/aura/window_tree_host.cc (right):
>>> >
>>> >
>>>
>>
>>
>>
https://codereview.chromium.org/1155013005/diff/650001/ui/aura/window_tree_ho...
>>
>>> > ui/aura/window_tree_host.cc:323: }
>>> > Hi, we noticed that with this CL, InputMethod receives key event before
>>> > EventHandlers receive them.
>>> >
>>> > I thought EventHandler should receive key events first so that we can
>>> stop
>>> > propagation even to IME (InputEventBlocker for example).
>>> >
>>> > Or am I missing something?
>>>
>>
>>  Having IME to handle all key events before anything else is a common
>>> practice.
>>> For example, on Windows, key events will be dispatched to IME by system
>>> even
>>> before entering chrome. On Mac, key events are dispatched to IME by
>>> system
>>>
>> when
>>
>>> we call NSResponder's - interpretKeyEvents: method, which is called at
>>> very
>>> beginning in Chrome's code.
>>>
>>
>> Windows can intercept the key event before sending to IME. In ChromeOS,
>> ash is the part of OS, and should have an ability to intercept key event
>> before
>> IME.
>
>
> Just wondering how to do it on Windows? Can you point me the related doc?
> AFAIK, it's not possible, unless you do some tricky thing. At least for
> Chrome on Windows, all key events sent to Chrome are already intercepted by
> IME.
>

What I meant is that ash is NOT chrome. It's the part of "OS" portion of
the ChromeOS.
Windows (as OS) first gets key event and can do whatever they need before
dispatching to IME.


>
> If we want ash to handle key events before IME, then I think we need to
> introduce a different logic to achieve it. But I think it's better to keep
> the relationship between WTH and IME and the event dispatching sequence
> inside WTH unified across all platforms.
>
>

Let's wait for sadrul's feedback first as he owns ui/events. For
InputEventsBlocker, see

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...



>
>>
>>
>>   From the user's point of view, if the user is using an IME, the IME may
>>> want
>>>
>> to
>>
>>> handle all key events. Surprise behavior may happen if some key events
>>> get
>>> intercepted by something else.
>>> And when the user is not using the IME, s/he will turn off the IME and
>>> then
>>>
>> the
>>
>>> IME will not intercept any key event, so that other functionality will
>>> not be
>>> affected at all.
>>>
>>
>>  Thus, one of the most important purpose of this refactor is to allow IME
>>>
>> handles
>>
>>> key events at the very beginning, especially before all event filters.
>>> If this refactor causes any regression, then we should fix the regression
>>> without changing this key event handling sequence.
>>>
>>
>>  And btw, what is InputEventBlocker? I can't find it in code search.
>>>
>>
>>
>>
>> https://codereview.chromium.org/1155013005/
>>
>
>
>
> --
> - James Su
>

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

[Shu Chen, 2015-06-19 17:37:04]

oshima@, I understand the concern is Ash sometimes tires to touch key events
before IMEs, because it tries to behave like OS.

To better understand the key problem and adopt better design, I am wondering:
 - Why the issue only affect unified mode? If changing key sequence would mess
up key event handling by Ash-OS, should normal mode of Ash be affected too?
 - can you please review the CL description, Note 1)? Does my analysis sound
correct to you?

Thanks!

[oshima, 2015-06-19 18:19:08]

On 2015/06/19 17:37:04, Shu Chen wrote:
> oshima@, I understand the concern is Ash sometimes tires to touch key events
> before IMEs, because it tries to behave like OS.
>
> To better understand the key problem and adopt better design, I am wondering:
>  - Why the issue only affect unified mode? If changing key sequence would mess
> up key event handling by Ash-OS, should normal mode of Ash be affected too?

This is just an example. afark@ was affected, and I think there are other
features affected but not just detected. see below.

>  - can you please review the CL description, Note 1)? Does my analysis sound
> correct to you?

Note 1) wasn't correct.

Any event handlers that are added by PrependPreTargetHandler will also be called
before IME. By looking at the code I think following handlers are affected by
this change.

ParticalScreenshotController
InputEventsBlocker
JavascriptAppModalEventBlockerX11 (this is probably less important than others,
but still used in testing)

This also overrides the targeter behavior, and following targeters are affected

UnifiedEventTargeter
NullEventTargeter

Sorry I should have read it more carefully, and that's my fault.

> 
> Thanks!

[James Su, 2015-06-20 04:14:54]

On 2015/06/19 18:19:08, oshima wrote:
> On 2015/06/19 17:37:04, Shu Chen wrote:
> > oshima@, I understand the concern is Ash sometimes tires to touch key events
> > before IMEs, because it tries to behave like OS.
> >
> > To better understand the key problem and adopt better design, I am
wondering:
> >  - Why the issue only affect unified mode? If changing key sequence would
mess
> > up key event handling by Ash-OS, should normal mode of Ash be affected too?
> 
> This is just an example. afark@ was affected, and I think there are other
> features affected but not just detected. see below.
> 
> >  - can you please review the CL description, Note 1)? Does my analysis sound
> > correct to you?
> 
> Note 1) wasn't correct.
> 
> Any event handlers that are added by PrependPreTargetHandler will also be
called
> before IME. By looking at the code I think following handlers are affected by
> this change.
> 
> ParticalScreenshotController
> InputEventsBlocker
> JavascriptAppModalEventBlockerX11 (this is probably less important than
others,
> but still used in testing)
> 
> This also overrides the targeter behavior, and following targeters are
affected
> 
> UnifiedEventTargeter
> NullEventTargeter
> 
> Sorry I should have read it more carefully, and that's my fault.
> 
> > 
> > Thanks!

Is there a design doc about event handling in Ash/ChromeOS? IMHO, it's extremely
complicated.