Use a single ToplevelWindowEventHandler for the ash::Shell

Use a single ToplevelWindowEventHandler for the ash::Shell. This is part 1 in disabling the few cases in which dragging multiple windows is possible (which causes crashiness)

BUG=324038
TEST=None

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

[pkotwicz, 2013-12-12 23:31:19]

Valery, can you please take a look?

[varkha, 2013-12-13 04:05:14]

Great to see so much code and complexity gone, Thanks!

https://codereview.chromium.org/114643003/diff/60001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/114643003/diff/60001/ash/shell.cc#newcode679
ash/shell.cc:679: toplevel_window_event_handler_.reset();
Do we need to reset other handlers and filters that are allocated in Init()?

https://codereview.chromium.org/114643003/diff/60001/ash/wm/panels/panel_wind...
File ash/wm/panels/panel_window_event_handler.cc (right):

https://codereview.chromium.org/114643003/diff/60001/ash/wm/panels/panel_wind...
ash/wm/panels/panel_window_event_handler.cc:25: event->flags() &
ui::EF_IS_DOUBLE_CLICK &&
Do we know if this way of minimizing panels is popular or used? It makes panels
work differently and is a bit redundant with the minimize button also in place.

https://codereview.chromium.org/114643003/diff/60001/ash/wm/toplevel_window_e...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/114643003/diff/60001/ash/wm/toplevel_window_e...
ash/wm/toplevel_window_event_handler.cc:407: event->StopPropagation();
Just a question. Previously the code was stopping propagation in all cases when
component was bounds-relevant. Now you are only stopping events other than
double/triple clicks so double and triple clicks are allowed to propagate. Does
this change any behavior such as double-click, then drag?

https://codereview.chromium.org/114643003/diff/60001/ash/wm/toplevel_window_e...
File ash/wm/toplevel_window_event_handler.h (right):

https://codereview.chromium.org/114643003/diff/60001/ash/wm/toplevel_window_e...
ash/wm/toplevel_window_event_handler.h:37: explicit
ToplevelWindowEventHandler();
Do we still need "explicit"?

https://codereview.chromium.org/114643003/diff/60001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_event_handler.h (right):

https://codereview.chromium.org/114643003/diff/60001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_event_handler.h:25: // Overridden from
ui::EventHandler:
nit: I think the style I was advised to use was "// ui::EventHandler:" which is
a bit shorter. In this case there are no conflicting styles, so it could be a
good time to change.

[pkotwicz, 2013-12-13 20:28:42]

Valery, can you please take another look?

https://codereview.chromium.org/114643003/diff/60001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/114643003/diff/60001/ash/shell.cc#newcode679
ash/shell.cc:679: toplevel_window_event_handler_.reset();
We need to reset |toplevel_window_event_handler_| in particular because it
observes |display_controller_|.

https://codereview.chromium.org/114643003/diff/60001/ash/wm/panels/panel_wind...
File ash/wm/panels/panel_window_event_handler.cc (right):

https://codereview.chromium.org/114643003/diff/60001/ash/wm/panels/panel_wind...
ash/wm/panels/panel_window_event_handler.cc:25: event->flags() &
ui::EF_IS_DOUBLE_CLICK &&
Added some UMA stats. With the UMA stats hopefully we can make an informed
decision about removing this feature. (At the very least this is inconsistent
with panels which not minimize when double clicked)

https://codereview.chromium.org/114643003/diff/60001/ash/wm/toplevel_window_e...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/114643003/diff/60001/ash/wm/toplevel_window_e...
ash/wm/toplevel_window_event_handler.cc:407: event->StopPropagation();
It should not because ToplevelWindowEventHandler and WorkspaceEventHandler are
the only post target event handlers in Chrome.

[pkotwicz, 2013-12-16 19:04:27]

Oshima, can you please take a look?
Valery is away working on innovation week this week

[oshima, 2013-12-17 00:33:46]

wow, i wonder why we were doing this. nice cleanup tho.

lgtm

https://codereview.chromium.org/114643003/diff/110001/ash/root_window_control...
File ash/root_window_controller.h (right):

https://codereview.chromium.org/114643003/diff/110001/ash/root_window_control...
ash/root_window_controller.h:309: // Handles events for the panel container.
can you briefly mention why this still have to be per root window?

https://codereview.chromium.org/114643003/diff/110001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_event_handler.cc (right):

https://codereview.chromium.org/114643003/diff/110001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_event_handler.cc:154: }
can't you just call StopPropagation here once?

https://codereview.chromium.org/114643003/diff/110001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_event_handler.cc:174: }
ditto

[pkotwicz, 2013-12-17 02:16:00]

Sadrul, can you please take a look at the change in ui/views
(modulo me addressing Oshima's nits)

[sadrul, 2013-12-17 04:27:30]

On 2013/12/17 02:16:00, pkotwicz wrote:
> Sadrul, can you please take a look at the change in ui/views
> (modulo me addressing Oshima's nits)

Yep. views/ lgtm

[commit-bot: I haz the power, 2013-12-18 20:46:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/114643003/150001

[commit-bot: I haz the power, 2013-12-18 23:10:08]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-19 16:10:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/114643003/170001

[commit-bot: I haz the power, 2013-12-19 16:10:49]

Failed to apply patch for ash/shell.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ash/shell.h
  Hunk #1 FAILED at 85.
  Hunk #2 FAILED at 97.
  2 out of 4 hunks FAILED -- saving rejects to file ash/shell.h.rej

Patch:       ash/shell.h
Index: ash/shell.h
diff --git a/ash/shell.h b/ash/shell.h
index
339c68c0200a7ff8dd3ac7b084a01466e066bc26..08b90cc24a19a7e4037e073a39e6a18f11dd3fa1
100644
--- a/ash/shell.h
+++ b/ash/shell.h
@@ -85,7 +85,6 @@ class DisplayController;
 class FirstRunHelper;
 class HighContrastController;
 class Launcher;
-class ShelfDelegate;
 class LockStateController;
 class MagnificationController;
 class MediaDelegate;
@@ -97,6 +96,7 @@ class PowerButtonController;
 class RootWindowHostFactory;
 class ScreenAsh;
 class SessionStateDelegate;
+class ShelfDelegate;
 class ShelfItemDelegateManager;
 class ShelfModel;
 class ShellDelegate;
@@ -105,6 +105,7 @@ class StickyKeys;
 class SystemTray;
 class SystemTrayDelegate;
 class SystemTrayNotifier;
+class ToplevelWindowEventHandler;
 class UserActivityDetector;
 class UserWallpaperDelegate;
 class VideoDetector;
@@ -655,6 +656,9 @@ class ASH_EXPORT Shell
   // An event filter for logging keyboard-related metrics.
   scoped_ptr<internal::KeyboardUMAEventFilter> keyboard_metrics_filter_;
 
+  // An event filter which handles moving and resizing windows.
+  scoped_ptr<ToplevelWindowEventHandler> toplevel_window_event_handler_;
+
   // An event filter which handles system level gestures
   scoped_ptr<internal::SystemGestureEventFilter> system_gesture_filter_;

[commit-bot: I haz the power, 2013-12-19 19:46:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/114643003/210001

[commit-bot: I haz the power, 2013-12-20 01:01:21]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-12-20 01:32:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/114643003/210001

[commit-bot: I haz the power, 2013-12-20 09:08:24]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-12-20 15:33:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/114643003/210001

[commit-bot: I haz the power, 2013-12-20 17:36:45]

Change committed as 242116