Only block internal touchpad and allow external mice to continue working in maximize mode.

Only block internal touchpad and allow external mice to continue working in maximize mode.

BUG=362881
TEST=Manual, plug in an external mouse and observe it continues working when device is in maximize mode while internal touchpad does not.

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

[flackr, 2014-05-01 12:36:41]

Please take a look. It looks like the test event generation infrastructure
doesn't currently support creating custom native events (i.e. with XI2 events
containing deviceids) so I don't have a test for this. Let me know if you think
I should add some kind of native event injection in order to test this.

[sadrul, 2014-05-02 01:44:47]

Can you use DeviceDataManager::IsCMTDeviceEvent() instead to detect events
coming from the internal touchpad?

https://codereview.chromium.org/269633005/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_event_blocker.cc (right):

https://codereview.chromium.org/269633005/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_event_blocker.cc:10: #endif
I think these still go below, grouped with the block in lines 21:25

[flackr, 2014-05-07 01:39:23]

Oddly enough DeviceDataManager::IsCMTDeviceEvent() returns true.
DeviceDataManager::IsTouchpadXInputEvent works to discriminate the internal
touchpad from external regular mice, though I'm not sure what this will do with
an external touchpad.

Eventually, we'll have to detect the keyboard as well, so even if we do detect
the internal touchpad some other way we'll probably still have to use the device
name to identify the keyboard.

https://codereview.chromium.org/269633005/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_event_blocker.cc (right):

https://codereview.chromium.org/269633005/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_event_blocker.cc:10: #endif
On 2014/05/02 01:44:47, sadrul wrote:
> I think these still go below, grouped with the block in lines 21:25

Done.

[flackr, 2014-05-07 01:45:00]

On 2014/05/07 01:39:23, flackr wrote:
> Oddly enough DeviceDataManager::IsCMTDeviceEvent() returns true.

To clarify, I mean returns true for my external USB mouse.

> DeviceDataManager::IsTouchpadXInputEvent works to discriminate the internal
> touchpad from external regular mice, though I'm not sure what this will do
with
> an external touchpad.
> 
> Eventually, we'll have to detect the keyboard as well, so even if we do detect
> the internal touchpad some other way we'll probably still have to use the
device
> name to identify the keyboard.
> 
>
https://codereview.chromium.org/269633005/diff/1/ash/wm/maximize_mode/maximiz...
> File ash/wm/maximize_mode/maximize_mode_event_blocker.cc (right):
> 
>
https://codereview.chromium.org/269633005/diff/1/ash/wm/maximize_mode/maximiz...
> ash/wm/maximize_mode/maximize_mode_event_blocker.cc:10: #endif
> On 2014/05/02 01:44:47, sadrul wrote:
> > I think these still go below, grouped with the block in lines 21:25
> 
> Done.

[sadrul, 2014-05-07 13:59:22]

This looks reasonable ... but how difficult would it be to separate out the
device-id related code into an x11-only file? That would make the code cleaner.

https://codereview.chromium.org/269633005/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_event_blocker.cc (right):

https://codereview.chromium.org/269633005/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_event_blocker.cc:52: aura::Window*
root_window_;
We expect that the root-window will outlive this targeter, right?

https://codereview.chromium.org/269633005/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_event_blocker.cc:98: last_mouse_location_);
Add a comment here about why we are doing this (it isn't immediately clear that
we don't want the cursor to move in this mode).

https://codereview.chromium.org/269633005/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_event_blocker.cc:104: &last_mouse_location_);
Use WindowTreeHost::ConvertPointToHost on the event's root_location() instead?

https://codereview.chromium.org/269633005/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_event_blocker.cc:131:
base::TrimWhitespaceASCII(device_name, base::TRIM_TRAILING, &device_name);
How safe is it to assume that the name is ASCII?

[flackr, 2014-05-14 16:46:56]

Separated out the device detection into an X11 only file.

https://codereview.chromium.org/269633005/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_event_blocker.cc (right):

https://codereview.chromium.org/269633005/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_event_blocker.cc:52: aura::Window*
root_window_;
On 2014/05/07 13:59:23, sadrul wrote:
> We expect that the root-window will outlive this targeter, right?

Yes, commented.

https://codereview.chromium.org/269633005/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_event_blocker.cc:98: last_mouse_location_);
On 2014/05/07 13:59:23, sadrul wrote:
> Add a comment here about why we are doing this (it isn't immediately clear
that
> we don't want the cursor to move in this mode).

Done.

https://codereview.chromium.org/269633005/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_event_blocker.cc:104: &last_mouse_location_);
On 2014/05/07 13:59:23, sadrul wrote:
> Use WindowTreeHost::ConvertPointToHost on the event's root_location() instead?

Done.

https://codereview.chromium.org/269633005/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_event_blocker.cc:131:
base::TrimWhitespaceASCII(device_name, base::TRIM_TRAILING, &device_name);
On 2014/05/07 13:59:23, sadrul wrote:
> How safe is it to assume that the name is ASCII?

It should be safe, TrimWhitespaceASCII uses std::string::find_last_not_of
basically ignoring encoding. Afaik at worst trimming a UTF8 string would give an
invalid UTF8 string but since we don't use the string as anything but ASCII it
wouldn't matter.

[sadrul, 2014-05-14 18:26:08]

LGTM. Thanks!

+kpschoedel@ FYI. The newly added InternalInputDeviceList will need to be merged
with the fix for http://crbug.com/360377

https://codereview.chromium.org/269633005/diff/40001/ash/wm/maximize_mode/int...
File ash/wm/maximize_mode/internal_input_device_list_x11.h (right):

https://codereview.chromium.org/269633005/diff/40001/ash/wm/maximize_mode/int...
ash/wm/maximize_mode/internal_input_device_list_x11.h:22: virtual bool
IsEventFromInternalDevice(const ui::Event* event) OVERRIDE;
// InternalInputDeviceList:

[flackr, 2014-05-14 18:41:56]

https://codereview.chromium.org/269633005/diff/40001/ash/wm/maximize_mode/int...
File ash/wm/maximize_mode/internal_input_device_list_x11.h (right):

https://codereview.chromium.org/269633005/diff/40001/ash/wm/maximize_mode/int...
ash/wm/maximize_mode/internal_input_device_list_x11.h:22: virtual bool
IsEventFromInternalDevice(const ui::Event* event) OVERRIDE;
On 2014/05/14 18:26:09, sadrul wrote:
> // InternalInputDeviceList:

Done.

[flackr, 2014-05-14 18:43:14]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-05-14 18:43:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/flackr@chromium.org/269633005/60001

[commit-bot: I haz the power, 2014-05-14 21:55:05]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-14 22:10:14]

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

[commit-bot: I haz the power, 2014-05-14 22:10:15]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[flackr, 2014-05-16 21:18:59]

Hey, after merging I had to modify how the internal input device list is
accessed so that it can be mocked out for
MaximizeModeControllerTest.BlocksKeyboardAndMouse. Still look good?

[flackr, 2014-05-20 20:41:54]

ping, do the changes in maximize_mode_controller_unittest.cc and
maximize_mode_event_blocker.h/cc in patch set 5 look reasonable? (Pardon the
ugliness of the merge, the upstream change added tests which require mouse
events to be identified as internal and blocked)

[sadrul, 2014-05-21 15:29:17]

sorry about the delay. slgtm

[flackr, 2014-05-21 15:30:31]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-05-21 19:56:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/flackr@chromium.org/269633005/80001

[commit-bot: I haz the power, 2014-05-22 01:15:38]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-22 01:21:13]

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

[commit-bot: I haz the power, 2014-05-22 01:21:14]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[flackr, 2014-05-22 14:15:05]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-05-22 14:16:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/flackr@chromium.org/269633005/100001

[commit-bot: I haz the power, 2014-05-22 18:49:04]

Change committed as 272254