Only exclude workarea from touch-exploration with active shell-surface

Only exclude workarea from touch-exploration with active shell-surface

The previous behavior disabled touch-exploration altogether which
made the shelf un-explorable. This adds support for an exclusion
region in touch_exploration_controller. When a shell-surface is
active, that region gets set to the workarea (area above shelf and
below chromevox banner) so that the shelf remains explorable.

BUG=649524
TEST=unit-test for exclude-area support. manual for shell-surface
behavior.

Committed: https://crrev.com/998c256b3f707cdecdab750239f70b7e893747a1
Cr-Commit-Position: refs/heads/master@{#426299}

[erosky, 2016-09-29 23:36:09]

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

[commit-bot: I haz the power, 2016-09-29 23:36:35]

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

[erosky, 2016-09-30 00:33:34]

erosky@chromium.org changed reviewers:
+ dmazzoni@chromium.org, oshima@chromium.org, reveman@chromium.org,
sky@chromium.org

[commit-bot: I haz the power, 2016-09-30 01:51:40]

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

[commit-bot: I haz the power, 2016-09-30 01:51:41]

Dry run: This issue passed the CQ dry run.

[reveman, 2016-09-30 06:07:48]

components/exo lgtm

[sky, 2016-09-30 15:39:58]

https://codereview.chromium.org/2378773011/diff/1/ash/ash_touch_exploration_m...
File ash/ash_touch_exploration_manager_chromeos.cc (right):

https://codereview.chromium.org/2378773011/diff/1/ash/ash_touch_exploration_m...
ash/ash_touch_exploration_manager_chromeos.cc:31:
Shell::GetInstance()->display_manager()->AddObserver(this);
Use WmShell::Get()->Add/RemoveDisplayObserver (that'll work with mash too).

https://codereview.chromium.org/2378773011/diff/1/ash/ash_touch_exploration_m...
ash/ash_touch_exploration_manager_chromeos.cc:100: if
(ash::GetRootWindowSettings(root_window)->display_id == display.id()) {
no {}

https://codereview.chromium.org/2378773011/diff/1/ash/ash_touch_exploration_m...
ash/ash_touch_exploration_manager_chromeos.cc:134:
touch_exploration_controller_.reset(new ui::TouchExplorationController(
Use MakeUnique (see threads on chromium-dev).

[erosky, 2016-09-30 20:27:24]

https://codereview.chromium.org/2378773011/diff/1/ash/ash_touch_exploration_m...
File ash/ash_touch_exploration_manager_chromeos.cc (right):

https://codereview.chromium.org/2378773011/diff/1/ash/ash_touch_exploration_m...
ash/ash_touch_exploration_manager_chromeos.cc:31:
Shell::GetInstance()->display_manager()->AddObserver(this);
On 2016/09/30 15:39:58, sky wrote:
> Use WmShell::Get()->Add/RemoveDisplayObserver (that'll work with mash too).

WmDisplayObserver::OnDisplayConfigurationChanged takes no arguments. Is
OnDisplayConfigurationChanged() called if any display changes? If so, how do I
find out which display has changed?

https://codereview.chromium.org/2378773011/diff/1/ash/ash_touch_exploration_m...
ash/ash_touch_exploration_manager_chromeos.cc:100: if
(ash::GetRootWindowSettings(root_window)->display_id == display.id()) {
On 2016/09/30 15:39:58, sky wrote:
> no {}

Done.

https://codereview.chromium.org/2378773011/diff/1/ash/ash_touch_exploration_m...
ash/ash_touch_exploration_manager_chromeos.cc:134:
touch_exploration_controller_.reset(new ui::TouchExplorationController(
On 2016/09/30 15:39:58, sky wrote:
> Use MakeUnique (see threads on chromium-dev).

Done.

[erosky, 2016-09-30 20:32:00]

Patchset #2 (id:20001) has been deleted

[erosky, 2016-09-30 20:50:34]

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

[commit-bot: I haz the power, 2016-09-30 20:51:11]

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

[oshima, 2016-09-30 20:59:59]

https://codereview.chromium.org/2378773011/diff/40001/ash/ash_touch_explorati...
File ash/ash_touch_exploration_manager_chromeos.h (right):

https://codereview.chromium.org/2378773011/diff/40001/ash/ash_touch_explorati...
ash/ash_touch_exploration_manager_chromeos.h:52: void
OnDisplayConfigurationChanged() override;
can you use display::DisplayObserver instead?

[commit-bot: I haz the power, 2016-09-30 22:05:43]

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

[commit-bot: I haz the power, 2016-09-30 22:05:44]

Dry run: This issue passed the CQ dry run.

[erosky, 2016-09-30 22:39:56]

https://codereview.chromium.org/2378773011/diff/40001/ash/ash_touch_explorati...
File ash/ash_touch_exploration_manager_chromeos.h (right):

https://codereview.chromium.org/2378773011/diff/40001/ash/ash_touch_explorati...
ash/ash_touch_exploration_manager_chromeos.h:52: void
OnDisplayConfigurationChanged() override;
On 2016/09/30 20:59:59, oshima wrote:
> can you use display::DisplayObserver instead?

I was just asked to switch from that to be forward-compatible with WmShell. What
is the difference here (besides not know which display changed in this one)?

[oshima, 2016-09-30 23:48:16]

https://codereview.chromium.org/2378773011/diff/40001/ash/ash_touch_explorati...
File ash/ash_touch_exploration_manager_chromeos.h (right):

https://codereview.chromium.org/2378773011/diff/40001/ash/ash_touch_explorati...
ash/ash_touch_exploration_manager_chromeos.h:52: void
OnDisplayConfigurationChanged() override;
On 2016/09/30 22:39:55, erosky wrote:
> On 2016/09/30 20:59:59, oshima wrote:
> > can you use display::DisplayObserver instead?
> 
> I was just asked to switch from that to be forward-compatible with WmShell.
What
> is the difference here (besides not know which display changed in this one)?

display::DisplayObserver is a platform independent API, while WmDisplayObserve
is used to bring up ash desktop environment.

[oshima, 2016-10-01 00:07:44]

https://codereview.chromium.org/2378773011/diff/40001/ash/ash_touch_explorati...
File ash/ash_touch_exploration_manager_chromeos.h (right):

https://codereview.chromium.org/2378773011/diff/40001/ash/ash_touch_explorati...
ash/ash_touch_exploration_manager_chromeos.h:52: void
OnDisplayConfigurationChanged() override;
On 2016/09/30 23:48:15, oshima wrote:
> On 2016/09/30 22:39:55, erosky wrote:
> > On 2016/09/30 20:59:59, oshima wrote:
> > > can you use display::DisplayObserver instead?
> > 
> > I was just asked to switch from that to be forward-compatible with WmShell.
> What
> > is the difference here (besides not know which display changed in this one)?
> 
> display::DisplayObserver is a platform independent API, while WmDisplayObserve
> is used to bring up ash desktop environment.

Sorry I wasn't clear enough.

Please use gfx::Screen::GetScreen()->Add/RemoveObserver instead of using
adding/removing directly to/from DisplayManager as it's ash internal.

[erosky, 2016-10-01 00:17:07]

https://codereview.chromium.org/2378773011/diff/40001/ash/ash_touch_explorati...
File ash/ash_touch_exploration_manager_chromeos.h (right):

https://codereview.chromium.org/2378773011/diff/40001/ash/ash_touch_explorati...
ash/ash_touch_exploration_manager_chromeos.h:52: void
OnDisplayConfigurationChanged() override;
On 2016/10/01 00:07:44, oshima wrote:
> On 2016/09/30 23:48:15, oshima wrote:
> > On 2016/09/30 22:39:55, erosky wrote:
> > > On 2016/09/30 20:59:59, oshima wrote:
> > > > can you use display::DisplayObserver instead?
> > > 
> > > I was just asked to switch from that to be forward-compatible with
WmShell.
> > What
> > > is the difference here (besides not know which display changed in this
one)?
> > 
> > display::DisplayObserver is a platform independent API, while
WmDisplayObserve
> > is used to bring up ash desktop environment.
> 
> Sorry I wasn't clear enough.
> 
> Please use gfx::Screen::GetScreen()->Add/RemoveObserver instead of using
> adding/removing directly to/from DisplayManager as it's ash internal.

Done.

[erosky, 2016-10-01 00:17:12]

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

[commit-bot: I haz the power, 2016-10-01 00:17:41]

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

[oshima, 2016-10-01 01:15:15]

https://codereview.chromium.org/2378773011/diff/60001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/2378773011/diff/60001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:81:
root_window_->GetHost()->ConvertPointFromNativeScreen(&location);
This looks wrong. Does this work when you rotate the screen (ctrl-alt-reload)?

Does this event have a target? If so, you should be able to convert
using

Window::ConvertPointToTarget(event_target,
                             root_window_,
                             &location);

https://codereview.chromium.org/2378773011/diff/60001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.h (right):

https://codereview.chromium.org/2378773011/diff/60001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.h:192: void SetExcludeBounds(const
gfx::Rect& bounds);
please document that this is in screen coordinates.

[commit-bot: I haz the power, 2016-10-01 01:23:22]

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

[commit-bot: I haz the power, 2016-10-01 01:23:23]

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

[sky, 2016-10-03 16:21:14]

On 2016/09/30 22:39:56, erosky wrote:
>
https://codereview.chromium.org/2378773011/diff/40001/ash/ash_touch_explorati...
> File ash/ash_touch_exploration_manager_chromeos.h (right):
> 
>
https://codereview.chromium.org/2378773011/diff/40001/ash/ash_touch_explorati...
> ash/ash_touch_exploration_manager_chromeos.h:52: void
> OnDisplayConfigurationChanged() override;
> On 2016/09/30 20:59:59, oshima wrote:
> > can you use display::DisplayObserver instead?
> 
> I was just asked to switch from that to be forward-compatible with WmShell.
What
> is the difference here (besides not know which display changed in this one)?

As Oshima commented, using DisplayObserver is fine as well.

[erosky, 2016-10-03 22:38:41]

https://codereview.chromium.org/2378773011/diff/60001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/2378773011/diff/60001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:81:
root_window_->GetHost()->ConvertPointFromNativeScreen(&location);
On 2016/10/01 01:15:14, oshima wrote:
> This looks wrong. Does this work when you rotate the screen (ctrl-alt-reload)?
> 
> Does this event have a target? If so, you should be able to convert
> using
> 
> Window::ConvertPointToTarget(event_target,
>                              root_window_,
>                              &location);

No target yet here. This conversion is specifically to handle those changes and
it works when I try it.

Would you prefer it if we make event_bounds_ native and then convert from in
callers or is it ok as is?

https://codereview.chromium.org/2378773011/diff/60001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.h (right):

https://codereview.chromium.org/2378773011/diff/60001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.h:192: void SetExcludeBounds(const
gfx::Rect& bounds);
On 2016/10/01 01:15:15, oshima wrote:
> please document that this is in screen coordinates.

Done.

[erosky, 2016-10-03 22:38:49]

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

[commit-bot: I haz the power, 2016-10-03 22:39:57]

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

[commit-bot: I haz the power, 2016-10-04 00:04:21]

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

[commit-bot: I haz the power, 2016-10-04 00:04:22]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2016-10-05 18:21:09]

lgtm

Nice job on the tests and explaining the logic
for continuing to process cancel and release
events.

https://codereview.chromium.org/2378773011/diff/80001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/2378773011/diff/80001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:130: // or if an additional press
occurred within the exclusion bounds.
Thanks for updating this comment. Good catch.

[sky, 2016-10-06 15:56:20]

https://codereview.chromium.org/2378773011/diff/80001/ash/ash_touch_explorati...
File ash/ash_touch_exploration_manager_chromeos.cc (right):

https://codereview.chromium.org/2378773011/diff/80001/ash/ash_touch_explorati...
ash/ash_touch_exploration_manager_chromeos.cc:140: const gfx::Rect& work_area =
ash::Shell::GetInstance()
root_window_controller_->wm_root_window_controller()->GetRootWindow()->GetDisplayNearestWindow().work_area();

The reason for doing this vs display_manager() is this code needs to be ported
to ash/common, and ash/common can't use display_manager.

[erosky, 2016-10-07 23:05:20]

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

[erosky, 2016-10-07 23:05:30]

https://codereview.chromium.org/2378773011/diff/80001/ash/ash_touch_explorati...
File ash/ash_touch_exploration_manager_chromeos.cc (right):

https://codereview.chromium.org/2378773011/diff/80001/ash/ash_touch_explorati...
ash/ash_touch_exploration_manager_chromeos.cc:140: const gfx::Rect& work_area =
ash::Shell::GetInstance()
On 2016/10/06 15:56:19, sky wrote:
>
root_window_controller_->wm_root_window_controller()->GetRootWindow()->GetDisplayNearestWindow().work_area();
> 
> The reason for doing this vs display_manager() is this code needs to be ported
> to ash/common, and ash/common can't use display_manager.

Done. Thanks for explaining!

[commit-bot: I haz the power, 2016-10-07 23:05:48]

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

[commit-bot: I haz the power, 2016-10-07 23:22:40]

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

[commit-bot: I haz the power, 2016-10-07 23:22:41]

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

[erosky, 2016-10-08 00:46:03]

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

[commit-bot: I haz the power, 2016-10-08 00:46:35]

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

[commit-bot: I haz the power, 2016-10-08 01:23:15]

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

[commit-bot: I haz the power, 2016-10-08 01:23:16]

Dry run: This issue passed the CQ dry run.

[sky, 2016-10-10 14:59:08]

LGTM

[erosky, 2016-10-10 20:03:20]

The CQ bit was checked by erosky@chromium.org

[erosky, 2016-10-10 20:03:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2378773011/#ps120001
(title: "resolved merge issues in unittest")

[erosky, 2016-10-10 20:03:30]

The CQ bit was unchecked by erosky@chromium.org

[commit-bot: I haz the power, 2016-10-10 20:03:41]

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

[erosky, 2016-10-19 20:54:12]

The CQ bit was checked by erosky@chromium.org

[commit-bot: I haz the power, 2016-10-19 20:55:03]

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

[commit-bot: I haz the power, 2016-10-19 22:00:34]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-10-21 13:11:58]

Description was changed from

==========
Only exclude workarea from touch-exploration with active shell-surface

The previous behavior disabled touch-exploration altogether which
made the shelf un-explorable. This adds support for an exclusion
region in touch_exploration_controller. When a shell-surface is
active, that region gets set to the workarea (area above shelf and
below chromevox banner) so that the shelf remains explorable.

BUG=649524
TEST=unit-test for exclude-area support. manual for shell-surface
behavior.
==========

to

==========
Only exclude workarea from touch-exploration with active shell-surface

The previous behavior disabled touch-exploration altogether which
made the shelf un-explorable. This adds support for an exclusion
region in touch_exploration_controller. When a shell-surface is
active, that region gets set to the workarea (area above shelf and
below chromevox banner) so that the shelf remains explorable.

BUG=649524
TEST=unit-test for exclude-area support. manual for shell-surface
behavior.

Committed: https://crrev.com/998c256b3f707cdecdab750239f70b7e893747a1
Cr-Commit-Position: refs/heads/master@{#426299}
==========

[commit-bot: I haz the power, 2016-10-21 13:11:59]

Patchset 6 (id:??) landed as
https://crrev.com/998c256b3f707cdecdab750239f70b7e893747a1
Cr-Commit-Position: refs/heads/master@{#426299}