Remove PrepareForShutdown

Remove PrepareForShutdown
Filter events after shutdown in event filter instead.

BUG=None

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

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

[Ben Goodger (Google), 2014-03-17 03:22:42]

+sadrul, see question below:

https://codereview.chromium.org/196573023/diff/30001/ash/display/mouse_cursor...
File ash/display/mouse_cursor_event_filter.cc (right):

https://codereview.chromium.org/196573023/diff/30001/ash/display/mouse_cursor...
ash/display/mouse_cursor_event_filter.cc:84: event->StopPropagation();
So I like the idea to do this from a pre-target event handler, but it seems
somewhat random to do it in this particular handler. Also it seems like even if
you had this done as a distinct pre-target handler, it would be susceptible to
subtle bugs if someone else called PrependPreTargetHandler after this was
attached. I'm wondering if we should extend EventTarget to "lock" calls to
Prepend.. i.e. during init you can prepend some set of handlers, then determine
it "locked" so that no one else can prepend. Sadrul - wdyt?

[oshima, 2014-03-17 04:25:08]

https://codereview.chromium.org/196573023/diff/30001/ash/display/mouse_cursor...
File ash/display/mouse_cursor_event_filter.cc (right):

https://codereview.chromium.org/196573023/diff/30001/ash/display/mouse_cursor...
ash/display/mouse_cursor_event_filter.cc:84: event->StopPropagation();
On 2014/03/17 03:22:42, Ben Goodger (Google) wrote:
> So I like the idea to do this from a pre-target event handler, but it seems
> somewhat random to do it in this particular handler. Also it seems like even
if
> you had this done as a distinct pre-target handler, it would be susceptible to
> subtle bugs if someone else called PrependPreTargetHandler after this was
> attached. I'm wondering if we should extend EventTarget to "lock" calls to
> Prepend.. i.e. during init you can prepend some set of handlers, then
determine
> it "locked" so that no one else can prepend. Sadrul - wdyt?

Yeah, we had a couple of bug in the past which was caused by random code calling
PrependPreTargetHandler because that changed the order of event handling.

In ash, this handler must be the first event handler for mouse event because
this handles cross display mouse handing, and it'd be nice if there is a way to
guarantee that this is always first one.

[sadrul, 2014-03-17 17:43:42]

Another option is to install a window-target on root_window() from
RootWindowController that always returns NULL from FindTargetForEvent().

It looks like we currently have a chromeos::InputEventsBlocker that prepends
itself to the Shell's pre-target list and blocks all input events. Looks like it
is used in login related code. Perhaps we should have a more explicit
EventDispatcher::Pause/UnpauseEventDispatch() method interface for this?

[Ben Goodger (Google), 2014-03-17 20:23:39]

BTW it really feels like Prepend is rife for abuse. Should there just be a
SetFirstPreTargetHandler that DCHECKs to make sure one is not already set?

-Ben


On Mon, Mar 17, 2014 at 10:43 AM, <sadrul@chromium.org> wrote:

> Another option is to install a window-target on root_window() from
> RootWindowController that always returns NULL from FindTargetForEvent().
>
> It looks like we currently have a chromeos::InputEventsBlocker that
> prepends
> itself to the Shell's pre-target list and blocks all input events. Looks
> like it
> is used in login related code. Perhaps we should have a more explicit
> EventDispatcher::Pause/UnpauseEventDispatch() method interface for this?
>
> https://codereview.chromium.org/196573023/
>

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

[Ben Goodger (Google), 2014-03-18 16:06:56]

So I'm going to hold my nose about where this particular call is located because
on balance I think this is better than having the PrepareForShutdown API in aura
itself.

LGTM for this change since it represents a net improvement.
I hope we can improve this further in the future.

[oshima, 2014-03-18 16:53:51]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-18 16:55:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/196573023/30001

[commit-bot: I haz the power, 2014-03-18 17:11:13]

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

[commit-bot: I haz the power, 2014-03-18 17:11:15]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[oshima, 2014-03-18 18:00:49]

The CQ bit was checked by oshima@chromium.org

[oshima, 2014-03-18 18:00:50]

The CQ bit was unchecked by oshima@chromium.org

[oshima, 2014-03-18 18:01:03]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-18 18:30:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/196573023/30001

[commit-bot: I haz the power, 2014-03-18 19:11:27]

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

[commit-bot: I haz the power, 2014-03-18 19:11:29]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[oshima, 2014-03-19 08:50:44]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-19 08:51:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/196573023/30001

[commit-bot: I haz the power, 2014-03-19 10:18:35]

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

[commit-bot: I haz the power, 2014-03-19 10:18:36]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[oshima, 2014-03-19 16:00:44]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-19 16:01:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/196573023/70001

[commit-bot: I haz the power, 2014-03-19 16:42:23]

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

[commit-bot: I haz the power, 2014-03-19 16:42:24]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[oshima, 2014-03-19 17:05:20]

On 2014/03/19 16:42:24, I haz the power (commit-bot) wrote:
> Retried try job too often on win_rel for step(s) base_unittests,
browser_tests,
> interactive_ui_tests, net_unittests, unit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

ben, I also have to change mojo\examples\aura_demo\window_tree_host_mojo.{h|cc}

It's same change, so I'll just run CQ. Please let me know if you have any
concern.

[oshima, 2014-03-19 17:05:25]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-19 17:06:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/196573023/110001

[commit-bot: I haz the power, 2014-03-19 17:11:00]

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

[commit-bot: I haz the power, 2014-03-19 17:11:01]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[oshima, 2014-03-19 17:32:21]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-19 17:33:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/196573023/110001

[commit-bot: I haz the power, 2014-03-19 20:34:47]

Change committed as 258077

[oshima, 2014-03-19 22:31:03]

+jamescook for apps/shell

[James Cook, 2014-03-19 22:33:29]

lgtm

[oshima, 2014-03-19 22:34:55]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-19 22:35:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/196573023/130001

[commit-bot: I haz the power, 2014-03-19 23:58:27]

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

[commit-bot: I haz the power, 2014-03-19 23:58:28]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[oshima, 2014-03-20 00:01:56]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-03-20 00:02:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/196573023/130001

[commit-bot: I haz the power, 2014-03-20 06:14:00]

Change committed as 258217