Allow setting different hit test bounds overrides for mouse and touch

Allow setting different hit test bounds overrides for mouse and touch.
This is needed so that the ImmersiveModeController can be used in conjunction with CustomFrameViewAsh (eg for the task manager). Plumbing the content::WebContents* to CustomFrameViewAsh in all cases would be difficult

BUG=307622
TEST=ImmersiveModeControllerAshTestWithBrowserView.RevealViaGestureChildWindowConsumesTouchEvents

[pkotwicz, 2013-10-16 05:21:49]

James, PTAL

I decided to add a kFullscreenTypeKey property and remove the
kFullscreenUsesMinimalChromeKey property (as opposed to adding a
kImmersiveFullscreenKey property) because I am concerned that people would treat
kFullscreenUsesMinimalChromeKey and kImmersiveFullscreenKey as interchangeable
which they are not.

Once it is possible to put any window into immersive fullscreen,
ChromeShellDelegate::ToggleFullscreen() will set
the window's kShowStateKey directly to SHOW_STATE_FULLSCREEN and directly set
the kFullscreenType. For browser windows, we will still go through the
FullscreenController to enter immersive fullscreen.

I am unsure whether kFullscreenTypeKey should be moved to ash::wm::WindowState

[James Cook, 2013-10-16 16:38:34]

https://codereview.chromium.org/27458002/diff/11001/ash/wm/frame_border_hitte...
File ash/wm/frame_border_hittest_controller.cc (right):

https://codereview.chromium.org/27458002/diff/11001/ash/wm/frame_border_hitte...
ash/wm/frame_border_hittest_controller.cc:26: const int
kImmersiveFullscreenTopInnerInsetTouch = 8;
Are you sure you want to do it this way?  We're OK with making part of the
content area un-touchable?

https://codereview.chromium.org/27458002/diff/11001/ash/wm/wm_types.h
File ash/wm/wm_types.h (right):

https://codereview.chromium.org/27458002/diff/11001/ash/wm/wm_types.h#newcode52
ash/wm/wm_types.h:52: enum FullscreenType {
This seems like a good idea. I would put it in WindowState.  The goal is that
any Ash-specific window properties that could potentially interact with each
other should live in WindowState so we can enforce consistency between them.

[pkotwicz, 2013-10-17 04:45:07]

James, can you please take another look?

https://codereview.chromium.org/27458002/diff/11001/ash/wm/frame_border_hitte...
File ash/wm/frame_border_hittest_controller.cc (right):

https://codereview.chromium.org/27458002/diff/11001/ash/wm/frame_border_hitte...
ash/wm/frame_border_hittest_controller.cc:26: const int
kImmersiveFullscreenTopInnerInsetTouch = 8;
This is not a change. This is just moving what we were doing in
ImmersiveModeControllerAsh::SetRenderWindowTopInsetsForTouch() to
FrameBorderHittestController. (In the sense that it is equivalent to what we are
already doing, I am sure). I asked sadrul@ and he did not have any suggestions
as to a better way of doing this.
The problem is that if anyone consumes touch events, ET_GESTURE_SCROLL.* is not
dispatched at all.

https://codereview.chromium.org/27458002/diff/11001/ash/wm/wm_types.h
File ash/wm/wm_types.h (right):

https://codereview.chromium.org/27458002/diff/11001/ash/wm/wm_types.h#newcode52
ash/wm/wm_types.h:52: enum FullscreenType {
On 2013/10/16 16:38:35, James Cook wrote:
> This seems like a good idea. I would put it in WindowState.  The goal is that
> any Ash-specific window properties that could potentially interact with each
> other should live in WindowState so we can enforce consistency between them.

Done.

[James Cook, 2013-10-17 16:56:24]

https://codereview.chromium.org/27458002/diff/17001/ash/wm/frame_border_hit_t...
File ash/wm/frame_border_hit_test_controller.cc (right):

https://codereview.chromium.org/27458002/diff/17001/ash/wm/frame_border_hit_t...
ash/wm/frame_border_hit_test_controller.cc:24: const int
kImmersiveFullscreenTopInnerInsetTouch = 8;
OK, sounds good to me.

https://codereview.chromium.org/27458002/diff/17001/ash/wm/frame_border_hit_t...
ash/wm/frame_border_hit_test_controller.cc:103: // web contents consumes all
touch events.
This is a helpful comment, thanks for expanding on it.

https://codereview.chromium.org/27458002/diff/17001/ash/wm/window_state.cc
File ash/wm/window_state.cc (right):

https://codereview.chromium.org/27458002/diff/17001/ash/wm/window_state.cc#ne...
ash/wm/window_state.cc:202: void WindowState::SetFullscreenType(FullscreenType
type) {
Do you need to be able to change the fullscreen type even when you are not in
fullscreen?  If not, it might be clearer to require IsFullscreen() to make these
changes, either by returning early or DCHECK(IsFullScreen()). This would also
make it more clear about when ShowType change notifications happen vs.
FullscreenType change notifications.

If it has to be this way I might document why the notification pattern for
OnWindowFullscreenTypeChanged() has to be the way it is, maybe in the header.

[pkotwicz, 2013-10-17 22:14:26]

James, can you please take another look?

https://codereview.chromium.org/27458002/diff/17001/ash/wm/window_state.cc
File ash/wm/window_state.cc (right):

https://codereview.chromium.org/27458002/diff/17001/ash/wm/window_state.cc#ne...
ash/wm/window_state.cc:202: void WindowState::SetFullscreenType(FullscreenType
type) {
I now ignore calls to SetFullscreenType() when not in fullscreen.
However, in order to accomodate the new behavior, I have to reset
|fullscreen_type_| in OnWindowPropertyChanged().
I think this way is slightly easier to understand.

[James Cook, 2013-10-17 22:40:56]

LGTM

[pkotwicz, 2013-10-17 23:01:48]

Scott, can you please take a look at the changes in ui/

Sadrul, can you please take a look at the comment for
kImmersiveFullscreenTopInnerInsetTouch in frame_border_hit_test_controller.cc (I
do not know much about gestures so I am not sure if it is as correct as it can
be)

[Ben Goodger (Google), 2013-10-17 23:04:48]

https://codereview.chromium.org/27458002/diff/40001/ui/aura/window.h
File ui/aura/window.h (right):

https://codereview.chromium.org/27458002/diff/40001/ui/aura/window.h#newcode261
ui/aura/window.h:261: void SetHitTestBoundsOverrideInner(const gfx::Insets&
mouse_insets,
Can we try and remove this from Window?

I'm trying to slim down Aura and I wonder if this is something that could be
carried as a property.

[pkotwicz, 2013-10-18 00:25:58]

I do not see an easy way of removing this from aura::Window. (This is used in
aura::Window::GetWindowForPoint()).
Refactoring toplevel_window_event_handler.cc may be able to reduce the amount of
uses of set_hit_test_bounds_override_inner() on the CrOS side.

The new touch insets passed to SetHitTestBoundsOverrideInner() are because it is
not possible to get scroll gestures (even in a global pre target event filter)
if any aura::Window* consumes all touch events. sadrul@ can correct me and speak
as to whether there is a better way of doing this.
The use case is detecting an edge gestures when in a fullscreen web page with:
<html ontouchstart="event.preventDefault()"></html>

[Ben Goodger (Google), 2013-10-18 04:42:05]

I actually think we could add a single WindowDelegate method. The delegate
would be consulted after the visibility check in GetWindowForPoint. The
modified function would look something like this:

Window* Window::GetWindowForPoint(..) {
  if (!IsVisible())
    return NULL;
  if (for_event_handling && delegate_) {
    Window* event_handler = delegate_GetEventHandlerForPoint(local_point);
    if (event_handler)
      return event_handler;
  }
  if (!return_tightest && delegate_)
    return this;
...

This is on the assumption that GetWindowForPoint is the only call stack for
HitTest. Are there others?

-Ben


On Thu, Oct 17, 2013 at 5:25 PM, <pkotwicz@chromium.org> wrote:

> I do not see an easy way of removing this from aura::Window. (This is used
> in
> aura::Window::**GetWindowForPoint()).
> Refactoring toplevel_window_event_handler.**cc may be able to reduce the
> amount of
> uses of set_hit_test_bounds_override_**inner() on the CrOS side.
>
> The new touch insets passed to SetHitTestBoundsOverrideInner(**) are
> because it is
> not possible to get scroll gestures (even in a global pre target event
> filter)
> if any aura::Window* consumes all touch events. sadrul@ can correct me
> and speak
> as to whether there is a better way of doing this.
> The use case is detecting an edge gestures when in a fullscreen web page
> with:
> <html ontouchstart="event.**preventDefault()"></html>
>
>
https://codereview.chromium.**org/27458002/<https://codereview.chromium.org/2...
>

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

[pkotwicz, 2013-10-18 19:04:04]

There are other callers (exactly 3) of aura::Window::HitTest() but I think we
can deal with them all.

Are we ok with duplicating the logic in aura::Window::GetWindowForPoint() in
NativeWidgetAura and DesktopNativeWidgetAura?
We may be able to simplify the logic in DesktopNativeWidgetAura by ignoring
hit_test_bounds_override_outer_*, but we cannot ignore
hit_test_bounds_override_inner_.*

[pkotwicz, 2013-10-27 23:38:10]

I am closing this CL because I have split it up into 2:
https://codereview.chromium.org/32943006/ (jamescook@ is a reviewer)
https://codereview.chromium.org/30923002/ (ben@ is a reviewer) ben@, I will post
a new version of that CL shortly