Refactors away method implementations in ui::EventTargeter

Refactors away method implementations in ui::EventTargeter

ui::EventTargeter is now a pure virtual interface with only two methods:
 FindTargetForEvent
 FindNextBestTarget.

Following methods are removed from ui::EventTargeter and got added (with implementations) to aura::WindowTargeter:
 FindTargetForLocatedEvent
 SubtreeCanAcceptEvent
 EventLocationInsideBounds
 SubtreeShouldBeExploredForEvent

Method implementations in aura::WindowTargeter that perform LocatedEvent targeting now take Window* instead of EventTarget*.

Tests moved out of EventProcessorTest into WindowTargeterTest:
+ Bounds
+ TargeterChecksOwningEventTarget
Those tests now require a bit less plumbing since a aura::Window implements coordinates based targeting.

EventProcessorTest tests now use TestEventTarget that provides bubbling behavior optionally and can have an event target changed at runtime with set_target().

BUG=395388
TEST=EventProcessorTest, WindowTargeterTest
TBR=sky@chromium.org

Committed: https://crrev.com/9c6bcf714a44413a5bf00a4cd346197d854464e6
Cr-Commit-Position: refs/heads/master@{#333993}

[varkha, 2015-05-04 20:50:56]

Patchset #1 (id:1) has been deleted

[varkha, 2015-05-04 20:58:37]

Patchset #1 (id:20001) has been deleted

[varkha, 2015-05-04 20:58:49]

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

[commit-bot: I haz the power, 2015-05-04 20:59:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119423003/40001

[commit-bot: I haz the power, 2015-05-04 21:09:18]

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

[commit-bot: I haz the power, 2015-05-04 21:09:19]

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

[varkha, 2015-05-05 16:38:57]

Patchset #1 (id:40001) has been deleted

[varkha, 2015-05-05 16:39:05]

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

[varkha, 2015-05-05 16:40:59]

Patchset #1 (id:60001) has been deleted

[varkha, 2015-05-05 16:41:04]

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

[varkha, 2015-05-05 16:42:34]

Patchset #1 (id:80001) has been deleted

[varkha, 2015-05-05 16:48:13]

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

[commit-bot: I haz the power, 2015-05-05 16:50:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119423003/100001

[commit-bot: I haz the power, 2015-05-05 17:02:20]

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

[commit-bot: I haz the power, 2015-05-05 17:02:20]

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

[varkha, 2015-05-05 19:53:43]

Patchset #1 (id:100001) has been deleted

[varkha, 2015-05-05 19:54:04]

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

[commit-bot: I haz the power, 2015-05-05 19:55:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119423003/120001

[commit-bot: I haz the power, 2015-05-05 21:08:50]

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

[commit-bot: I haz the power, 2015-05-05 21:08:51]

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

[varkha, 2015-05-05 22:39:18]

Patchset #1 (id:120001) has been deleted

[varkha, 2015-05-05 22:40:29]

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

[commit-bot: I haz the power, 2015-05-05 22:41:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119423003/140001

[commit-bot: I haz the power, 2015-05-06 00:37:56]

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

[commit-bot: I haz the power, 2015-05-06 00:37:57]

Dry run: This issue passed the CQ dry run.

[varkha, 2015-05-06 18:39:09]

varkha@chromium.org changed reviewers:
+ sadrul@chromium.org, tdanderson@chromium.org

[varkha, 2015-05-06 18:39:09]

tdanderson@, sadrul@, I have simplified the EventTargeter interface a bit but I
found that some tests like EventProcessorTest.Basic make use of the hierarchical
event handling so something will need to change to move further and remove
FindTargetForEvent implementation from the base class.
WDYT? Would you move those tests out of EventProcessorTest and into aura?
Something else? What did you have in mind when you wanted a pure virtual
EventTargeter?

[tdanderson, 2015-05-07 01:01:26]

Thanks for putting up this CL! With respect to your question, my preference
would be to move the tests into aura, and then move the implementation of
EventTargeter::FindTargetForEvent() into WindowTargeter (since ViewTargeter
doesn't use this logic at all - it's specific to targeting of aura windows).
IIRC the vision I had was for EventTargeter to have only two methods:
FindTargetForEvent() and FindNextBestTarget(), and both of these should be pure
virtual.

https://codereview.chromium.org/1119423003/diff/140001/ui/events/event_target...
File ui/events/event_targeter.cc (right):

https://codereview.chromium.org/1119423003/diff/140001/ui/events/event_target...
ui/events/event_targeter.cc:15: bool IsLocatedEvent(const ui::Event& event) {
It looks like this is already implemented in event.h.

https://codereview.chromium.org/1119423003/diff/140001/ui/events/event_target...
File ui/events/event_targeter.h (left):

https://codereview.chromium.org/1119423003/diff/140001/ui/events/event_target...
ui/events/event_targeter.h:47: //                          non-public.
I don't think SubtreeShouldBeExploredForEvent() belongs in EventTargeter at all,
actually. Its only use is by WindowTargeter and derived classes.

[varkha, 2015-05-07 01:15:30]

Thanks for the insights, that matches my understanding so far. I will try to get
there, but it may need some more reshuffling of the tests.

https://codereview.chromium.org/1119423003/diff/140001/ui/events/event_target...
File ui/events/event_targeter.cc (right):

https://codereview.chromium.org/1119423003/diff/140001/ui/events/event_target...
ui/events/event_targeter.cc:15: bool IsLocatedEvent(const ui::Event& event) {
On 2015/05/07 01:01:26, tdanderson wrote:
> It looks like this is already implemented in event.h.

Yes, acq. I'll change that here and in WindowTargeter that I've stolen this
from.

https://codereview.chromium.org/1119423003/diff/140001/ui/events/event_target...
File ui/events/event_targeter.h (left):

https://codereview.chromium.org/1119423003/diff/140001/ui/events/event_target...
ui/events/event_targeter.h:47: //                          non-public.
On 2015/05/07 01:01:26, tdanderson wrote:
> I don't think SubtreeShouldBeExploredForEvent() belongs in EventTargeter at
all,
> actually. Its only use is by WindowTargeter and derived classes.

Yes, It should not be here, but that will require the tests in
event_processor_unittest.cc to be moved into aura and use WindowTargeter instead
of EventTargeter (since those tests depend on hierarchy and the logic that walks
the tree of targeters). I will try to do that as a next step.

[varkha, 2015-05-11 19:50:31]

PTAL. I have moved two tests into aura. Those tests needed very little change
but largely test the same logic as before.

With this patch a few tests still use the default implementation of
FindTargetforEvent. Did you have in mind moving those implementations into
views::ViewTargeter and using that class in the tests instead of EventTargeter?
This could allow making all the interfaces in EventTargeter pure virtual.

Thanks!

[varkha, 2015-05-11 19:50:38]

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

[commit-bot: I haz the power, 2015-05-11 19:50:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119423003/180001

[commit-bot: I haz the power, 2015-05-11 19:58:22]

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

[commit-bot: I haz the power, 2015-05-11 19:58:23]

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

[tdanderson, 2015-05-13 18:38:25]

Hi Valery, looks good so far, please see my comments inline. 

Regarding your question "With this patch a few tests still use the default
implementation of FindTargetforEvent. Did you have in mind moving those
implementations into views::ViewTargeter?" - as discussed offline, the remaining
tests in event_processor_unittest can be kept where they are, but the
TestEventTarget should instantiate a TestEventTargeter with the initial target
passed in to the constructor.

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targete...
File ui/aura/window_targeter.cc (right):

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targete...
ui/aura/window_targeter.cc:184: targeter->FindTargetForEvent(child, event);
Recursively?

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targeter.h
File ui/aura/window_targeter.h (right):

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targete...
ui/aura/window_targeter.h:34: virtual ui::EventTarget*
FindTargetForLocatedEvent(ui::EventTarget* root,
Since these four methods are being removed from the EventTargeter interface and
will only live in WindowTargeter, I wonder if it would be better to use the
Window* type instead of the ui::EventTarget* type in the method parameter lists
/ return values.

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targete...
ui/aura/window_targeter.h:59: ui::EventTarget*
FindTargetForEventRecursively(Window* root_window,
nit: FindTargetForLocatedEventRecursively?

[varkha, 2015-05-20 21:08:17]

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

[commit-bot: I haz the power, 2015-05-20 21:08:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119423003/200001

[commit-bot: I haz the power, 2015-05-20 23:22:58]

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

[commit-bot: I haz the power, 2015-05-20 23:22:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[varkha, 2015-05-21 17:10:32]

Patchset #5 (id:220001) has been deleted

[varkha, 2015-05-21 17:10:53]

Patchset #4 (id:200001) has been deleted

[varkha, 2015-05-21 18:46:43]

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

[varkha, 2015-05-21 18:57:53]

Patchset #5 (id:260001) has been deleted

[varkha, 2015-05-21 18:58:03]

Patchset #4 (id:240001) has been deleted

[varkha, 2015-05-21 18:58:14]

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

[varkha, 2015-05-21 19:11:04]

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targete...
File ui/aura/window_targeter.cc (right):

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targete...
ui/aura/window_targeter.cc:184: targeter->FindTargetForEvent(child, event);
On 2015/05/13 18:38:25, tdanderson wrote:
> Recursively?

Not so sure. WindowTargeterTest.ScopedWindowTargeter test fails if I do that and
I could not follow the logic chain that confirms that this is not a semantic
change.

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targeter.h
File ui/aura/window_targeter.h (right):

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targete...
ui/aura/window_targeter.h:34: virtual ui::EventTarget*
FindTargetForLocatedEvent(ui::EventTarget* root,
On 2015/05/13 18:38:25, tdanderson wrote:
> Since these four methods are being removed from the EventTargeter interface
and
> will only live in WindowTargeter, I wonder if it would be better to use the
> Window* type instead of the ui::EventTarget* type in the method parameter
lists
> / return values.

Done. Seems like there are fewer casts than before so this is definitely
positive.

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targete...
ui/aura/window_targeter.h:59: ui::EventTarget*
FindTargetForEventRecursively(Window* root_window,
On 2015/05/13 18:38:25, tdanderson wrote:
> nit: FindTargetForLocatedEventRecursively?

Done.

[commit-bot: I haz the power, 2015-05-21 19:38:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119423003/280001

[commit-bot: I haz the power, 2015-05-21 21:28:11]

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

[commit-bot: I haz the power, 2015-05-21 21:28:13]

Dry run: This issue passed the CQ dry run.

[varkha, 2015-05-22 16:06:54]

tdanderson@, ping?

[tdanderson, 2015-05-22 18:29:56]

Hi Valery, please see my latest comments below:

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targete...
File ui/aura/window_targeter.cc (right):

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targete...
ui/aura/window_targeter.cc:184: targeter->FindTargetForEvent(child, event);
On 2015/05/21 19:11:04, varkha wrote:
> On 2015/05/13 18:38:25, tdanderson wrote:
> > Recursively?
> 
> Not so sure. WindowTargeterTest.ScopedWindowTargeter test fails if I do that
and
> I could not follow the logic chain that confirms that this is not a semantic
> change.

Seems OK to leave as-is to me, but I think it's worth getting a second opinion
from Sadrul.

https://codereview.chromium.org/1119423003/diff/280001/ash/accelerators/magni...
File ash/accelerators/magnifier_key_scroller.cc (right):

https://codereview.chromium.org/1119423003/diff/280001/ash/accelerators/magni...
ash/accelerators/magnifier_key_scroller.cc:12: #include "ui/events/event.h"
Not sure why you're including this here (and in several other files)

https://codereview.chromium.org/1119423003/diff/280001/ash/wm/resize_handle_w...
File ash/wm/resize_handle_window_targeter.cc (right):

https://codereview.chromium.org/1119423003/diff/280001/ash/wm/resize_handle_w...
ash/wm/resize_handle_window_targeter.cc:92: WindowTargeter* targeter =
aura::WindowTargeter? (as as done on line 80)

https://codereview.chromium.org/1119423003/diff/280001/ash/wm/resize_handle_w...
ash/wm/resize_handle_window_targeter.cc:99: }
Side question: have you tried this on device as a sanity check to make sure
targeting still works as intended?

https://codereview.chromium.org/1119423003/diff/280001/ui/aura/window_targete...
File ui/aura/window_targeter.cc (right):

https://codereview.chromium.org/1119423003/diff/280001/ui/aura/window_targete...
ui/aura/window_targeter.cc:88: target = NULL;
nit: nullptr (here and elsewhere within the file).

https://codereview.chromium.org/1119423003/diff/280001/ui/aura/window_targeter.h
File ui/aura/window_targeter.h (right):

https://codereview.chromium.org/1119423003/diff/280001/ui/aura/window_targete...
ui/aura/window_targeter.h:13: 
nit: remove newlines

https://codereview.chromium.org/1119423003/diff/280001/ui/aura/window_targete...
ui/aura/window_targeter.h:28: // Returns true if |target| or one of its
descendants can be a target of
nit: update |target| to |window| in docs (many places).

https://codereview.chromium.org/1119423003/diff/280001/ui/events/event_proces...
File ui/events/event_processor_unittest.cc (right):

https://codereview.chromium.org/1119423003/diff/280001/ui/events/event_proces...
ui/events/event_processor_unittest.cc:22: class RecursiveEventTargeter : public
EventTargeter {
I'm not sure if this class is necessary. WindowTargeterTest* should (and does)
test the logic of WindowTargeter's implementation of FindTargetForEvent() and
FindNextBestTarget(). Similarly for ViewTargeterTest*. Since the targeting logic
is tested elsewhere, it seems reasonable for RecursiveEventTargeter to be a
small test double like BubblingEventTargeter. This would avoid having to
duplicate the implementation of WindowTargeter::FindTargetForEvent(), which can
become out of date if the implementation ever changes.

What do you think of the following idea? Remove RecursiveEventTargeter and
instead change BubblingEventTargeter to a more robust class:

class TestEventTargeter : public EventTargeter {
 public:
  TestEventTargeter(TestEventTarget* initial_target, bool bubble)
    : initial_target_(initial_target),
      should_bubble_(bubble) {}

  void set_initial_target(TestEventTarget* initial_target) {
    initial_target_ = initial_target;
  }

  EventTarget* FindTargetForEvent(...) override {
    return initial_target_;
  }

  EventTarget* FindNextBestTarget(...) override {
    if (!should_bubble_)
      return nullptr;
    return previous_target->GetParentTarget();
  }

 private:
  TestEventTarget* initial_target_;
  bool should_bubble_;

  DISALLOW_COPY_AND_ASSIGN(TestEventTargeter);
};

Then in SetUp() you can initialize TestEventTargeter(nullptr, false) and call
set_initial_target() within each test as needed.

Ideally TestEventTargeter should be defined in its own file located in
ui/events/test, similar to what is done for TestEventTarget.

WDYT?

https://codereview.chromium.org/1119423003/diff/280001/ui/events/event_target...
File ui/events/event_targeter.h (right):

https://codereview.chromium.org/1119423003/diff/280001/ui/events/event_target...
ui/events/event_targeter.h:9: //#include "ui/events/event.h"
Looks like you've left // in here.

https://codereview.chromium.org/1119423003/diff/280001/ui/events/null_event_t...
File ui/events/null_event_targeter.h (right):

https://codereview.chromium.org/1119423003/diff/280001/ui/events/null_event_t...
ui/events/null_event_targeter.h:6: #include "base/macros.h"
Not sure why you're including macros.h in here, view_targeter.h, and
window_targeter.h.

https://codereview.chromium.org/1119423003/diff/280001/ui/events/null_event_t...
ui/events/null_event_targeter.h:12: class EVENTS_EXPORT NullEventTargeter :
public EventTargeter {
I know you didn't add this class, but could you include a brief description of
the class above line 12? It wasn't obvious to me how/when this would ever need
to be used without using codesearch.

https://codereview.chromium.org/1119423003/diff/280001/ui/views/test/event_ge...
File ui/views/test/event_generator_delegate_mac.mm (right):

https://codereview.chromium.org/1119423003/diff/280001/ui/views/test/event_ge...
ui/views/test/event_generator_delegate_mac.mm:264: return NULL;
nit: nullptr?

[varkha, 2015-05-22 22:09:17]

Patchset #5 (id:300001) has been deleted

[varkha, 2015-05-22 22:18:34]

PTAL, thanks for the suggestions!

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targete...
File ui/aura/window_targeter.cc (right):

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targete...
ui/aura/window_targeter.cc:184: targeter->FindTargetForEvent(child, event);
On 2015/05/22 18:29:55, tdanderson wrote:
> On 2015/05/21 19:11:04, varkha wrote:
> > On 2015/05/13 18:38:25, tdanderson wrote:
> > > Recursively?
> > 
> > Not so sure. WindowTargeterTest.ScopedWindowTargeter test fails if I do that
> and
> > I could not follow the logic chain that confirms that this is not a semantic
> > change.
> 
> Seems OK to leave as-is to me, but I think it's worth getting a second opinion
> from Sadrul.

Yes, the code now reproduces the original flow, not sure if it can be refactored
in such a way that FindTargetForEventRecursively would recursively call itself
rather than just a general FindTargetForEvent.

https://codereview.chromium.org/1119423003/diff/280001/ash/accelerators/magni...
File ash/accelerators/magnifier_key_scroller.cc (right):

https://codereview.chromium.org/1119423003/diff/280001/ash/accelerators/magni...
ash/accelerators/magnifier_key_scroller.cc:12: #include "ui/events/event.h"
On 2015/05/22 18:29:56, tdanderson wrote:
> Not sure why you're including this here (and in several other files)

Because I have removed #include "ui/events/event.h" in event_targeter.h. This is
in general spirit of putting includes where they are necessary in implementation
files rather than in headers.

https://codereview.chromium.org/1119423003/diff/280001/ash/wm/resize_handle_w...
File ash/wm/resize_handle_window_targeter.cc (right):

https://codereview.chromium.org/1119423003/diff/280001/ash/wm/resize_handle_w...
ash/wm/resize_handle_window_targeter.cc:92: WindowTargeter* targeter =
On 2015/05/22 18:29:56, tdanderson wrote:
> aura::WindowTargeter? (as as done on line 80)

Done.

https://codereview.chromium.org/1119423003/diff/280001/ash/wm/resize_handle_w...
ash/wm/resize_handle_window_targeter.cc:99: }
On 2015/05/22 18:29:56, tdanderson wrote:
> Side question: have you tried this on device as a sanity check to make sure
> targeting still works as intended?

No, not yet. So far I don't think there should be any semantic changes. I have
tried in simulation, in particular I've tried the cases around resizing a window
next to the shelf or next to the other windows.

https://codereview.chromium.org/1119423003/diff/280001/ui/aura/window_targete...
File ui/aura/window_targeter.cc (right):

https://codereview.chromium.org/1119423003/diff/280001/ui/aura/window_targete...
ui/aura/window_targeter.cc:88: target = NULL;
On 2015/05/22 18:29:56, tdanderson wrote:
> nit: nullptr (here and elsewhere within the file).

Done.

https://codereview.chromium.org/1119423003/diff/280001/ui/aura/window_targeter.h
File ui/aura/window_targeter.h (right):

https://codereview.chromium.org/1119423003/diff/280001/ui/aura/window_targete...
ui/aura/window_targeter.h:13: 
On 2015/05/22 18:29:56, tdanderson wrote:
> nit: remove newlines

Done.

https://codereview.chromium.org/1119423003/diff/280001/ui/aura/window_targete...
ui/aura/window_targeter.h:28: // Returns true if |target| or one of its
descendants can be a target of
On 2015/05/22 18:29:56, tdanderson wrote:
> nit: update |target| to |window| in docs (many places).

Done.

https://codereview.chromium.org/1119423003/diff/280001/ui/events/event_proces...
File ui/events/event_processor_unittest.cc (right):

https://codereview.chromium.org/1119423003/diff/280001/ui/events/event_proces...
ui/events/event_processor_unittest.cc:22: class RecursiveEventTargeter : public
EventTargeter {
On 2015/05/22 18:29:56, tdanderson wrote:
> I'm not sure if this class is necessary. WindowTargeterTest* should (and does)
> test the logic of WindowTargeter's implementation of FindTargetForEvent() and
> FindNextBestTarget(). Similarly for ViewTargeterTest*. Since the targeting
logic
> is tested elsewhere, it seems reasonable for RecursiveEventTargeter to be a
> small test double like BubblingEventTargeter. This would avoid having to
> duplicate the implementation of WindowTargeter::FindTargetForEvent(), which
can
> become out of date if the implementation ever changes.
> 
> What do you think of the following idea? Remove RecursiveEventTargeter and
> instead change BubblingEventTargeter to a more robust class:
> 
> class TestEventTargeter : public EventTargeter {
>  public:
>   TestEventTargeter(TestEventTarget* initial_target, bool bubble)
>     : initial_target_(initial_target),
>       should_bubble_(bubble) {}
> 
>   void set_initial_target(TestEventTarget* initial_target) {
>     initial_target_ = initial_target;
>   }
> 
>   EventTarget* FindTargetForEvent(...) override {
>     return initial_target_;
>   }
> 
>   EventTarget* FindNextBestTarget(...) override {
>     if (!should_bubble_)
>       return nullptr;
>     return previous_target->GetParentTarget();
>   }
> 
>  private:
>   TestEventTarget* initial_target_;
>   bool should_bubble_;
> 
>   DISALLOW_COPY_AND_ASSIGN(TestEventTargeter);
> };
> 
> Then in SetUp() you can initialize TestEventTargeter(nullptr, false) and call
> set_initial_target() within each test as needed.
> 
> Ideally TestEventTargeter should be defined in its own file located in
> ui/events/test, similar to what is done for TestEventTarget.
> 
> WDYT?

Done. I was considering it but at the time it seemed that the logic with a
targeter responding to adding and removing children should be automated in the
targeter. I think your suggestion has a net positive impact though since the
payback in the test is localized and simple and we would not need to maintain
two copies of recursive logic. Thanks!

https://codereview.chromium.org/1119423003/diff/280001/ui/events/event_target...
File ui/events/event_targeter.h (right):

https://codereview.chromium.org/1119423003/diff/280001/ui/events/event_target...
ui/events/event_targeter.h:9: //#include "ui/events/event.h"
On 2015/05/22 18:29:56, tdanderson wrote:
> Looks like you've left // in here.

Yes, forgot to remove completely after experimenting. It is not needed here
since we can rely on forward declarations. This however triggers a need to add a
few includes in some files. Done.

https://codereview.chromium.org/1119423003/diff/280001/ui/events/null_event_t...
File ui/events/null_event_targeter.h (right):

https://codereview.chromium.org/1119423003/diff/280001/ui/events/null_event_t...
ui/events/null_event_targeter.h:6: #include "base/macros.h"
On 2015/05/22 18:29:56, tdanderson wrote:
> Not sure why you're including macros.h in here, view_targeter.h, and
> window_targeter.h.

Needed to define DISALLOW_COPY_AND_ASSIGN. It is needed because I have removed
#include "ui/events/event.h" in event_targeter and it was bringing in the
base/macros.h. This is in general spirit of putting includes where they are
necessary in implementation files rather than in headers.

https://codereview.chromium.org/1119423003/diff/280001/ui/events/null_event_t...
ui/events/null_event_targeter.h:12: class EVENTS_EXPORT NullEventTargeter :
public EventTargeter {
On 2015/05/22 18:29:56, tdanderson wrote:
> I know you didn't add this class, but could you include a brief description of
> the class above line 12? It wasn't obvious to me how/when this would ever need
> to be used without using codesearch.

Done.

https://codereview.chromium.org/1119423003/diff/280001/ui/views/test/event_ge...
File ui/views/test/event_generator_delegate_mac.mm (right):

https://codereview.chromium.org/1119423003/diff/280001/ui/views/test/event_ge...
ui/views/test/event_generator_delegate_mac.mm:264: return NULL;
On 2015/05/22 18:29:56, tdanderson wrote:
> nit: nullptr?

Done.

[varkha, 2015-05-22 22:19:11]

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

[commit-bot: I haz the power, 2015-05-22 22:23:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119423003/320001

[commit-bot: I haz the power, 2015-05-23 00:33:59]

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

[commit-bot: I haz the power, 2015-05-23 00:34:00]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2015-05-25 15:00:08]

LGTM with the inline nits and:

* It would be nice to perform a manual sanity check before uploading.

* Be sure to get Sadrul's opinion on
WindowTargeter::FindTargetForLocatedEventRecursively() calling
WT::FindTargetForEvent().

* Please include a CL description which explains the key changes made in this
CL.

https://codereview.chromium.org/1119423003/diff/280001/ash/accelerators/magni...
File ash/accelerators/magnifier_key_scroller.cc (right):

https://codereview.chromium.org/1119423003/diff/280001/ash/accelerators/magni...
ash/accelerators/magnifier_key_scroller.cc:12: #include "ui/events/event.h"
On 2015/05/22 22:18:34, varkha wrote:
> On 2015/05/22 18:29:56, tdanderson wrote:
> > Not sure why you're including this here (and in several other files)
> 
> Because I have removed #include "ui/events/event.h" in event_targeter.h. This
is
> in general spirit of putting includes where they are necessary in
implementation
> files rather than in headers.

Acknowledged.

https://codereview.chromium.org/1119423003/diff/280001/ui/events/null_event_t...
File ui/events/null_event_targeter.h (right):

https://codereview.chromium.org/1119423003/diff/280001/ui/events/null_event_t...
ui/events/null_event_targeter.h:6: #include "base/macros.h"
On 2015/05/22 22:18:34, varkha wrote:
> On 2015/05/22 18:29:56, tdanderson wrote:
> > Not sure why you're including macros.h in here, view_targeter.h, and
> > window_targeter.h.
> 
> Needed to define DISALLOW_COPY_AND_ASSIGN. It is needed because I have removed
> #include "ui/events/event.h" in event_targeter and it was bringing in the
> base/macros.h. This is in general spirit of putting includes where they are
> necessary in implementation files rather than in headers.

Acknowledged.

https://codereview.chromium.org/1119423003/diff/320001/ui/events/event_proces...
File ui/events/event_processor_unittest.cc (right):

https://codereview.chromium.org/1119423003/diff/320001/ui/events/event_proces...
ui/events/event_processor_unittest.cc:247: // When the root has a
TestEventTargeter installed, events targeted
nit: "When the root has a TestEventTargeter installed which permits bubbling..."

https://codereview.chromium.org/1119423003/diff/320001/ui/events/event_proces...
ui/events/event_processor_unittest.cc:302: // a TestEventTargeter is installed
on the root target.
nit: "a TestEventTargeter which permits bubbling"

https://codereview.chromium.org/1119423003/diff/320001/ui/events/test/test_ev...
File ui/events/test/test_event_targeter.h (right):

https://codereview.chromium.org/1119423003/diff/320001/ui/events/test/test_ev...
ui/events/test/test_event_targeter.h:24: explicit
TestEventTargeter(TestEventTarget* initial_target,
nit: not explicit since ctor has two parameters.

https://codereview.chromium.org/1119423003/diff/320001/ui/events/test/test_ev...
ui/events/test/test_event_targeter.h:33: 
nit: remove newline

[varkha, 2015-05-25 15:19:40]

varkha@chromium.org changed reviewers:
+ benwells@chromium.org, oshima@chromium.org

[varkha, 2015-05-25 15:19:41]

sadrul@, PTAL with nits ACQed.
+oshima for ash/,
+benwells for chrome/browser/ui/views/apps/

[varkha, 2015-05-25 15:50:51]

Patchset #6 (id:340001) has been deleted

[varkha, 2015-05-25 15:53:30]

https://codereview.chromium.org/1119423003/diff/320001/ui/events/event_proces...
File ui/events/event_processor_unittest.cc (right):

https://codereview.chromium.org/1119423003/diff/320001/ui/events/event_proces...
ui/events/event_processor_unittest.cc:247: // When the root has a
TestEventTargeter installed, events targeted
On 2015/05/25 15:00:08, tdanderson wrote:
> nit: "When the root has a TestEventTargeter installed which permits
bubbling..."

Done.

https://codereview.chromium.org/1119423003/diff/320001/ui/events/event_proces...
ui/events/event_processor_unittest.cc:302: // a TestEventTargeter is installed
on the root target.
On 2015/05/25 15:00:08, tdanderson wrote:
> nit: "a TestEventTargeter which permits bubbling"

Done.

https://codereview.chromium.org/1119423003/diff/320001/ui/events/test/test_ev...
File ui/events/test/test_event_targeter.h (right):

https://codereview.chromium.org/1119423003/diff/320001/ui/events/test/test_ev...
ui/events/test/test_event_targeter.h:24: explicit
TestEventTargeter(TestEventTarget* initial_target,
On 2015/05/25 15:00:08, tdanderson wrote:
> nit: not explicit since ctor has two parameters.

Done.

https://codereview.chromium.org/1119423003/diff/320001/ui/events/test/test_ev...
ui/events/test/test_event_targeter.h:33: 
On 2015/05/25 15:00:08, tdanderson wrote:
> nit: remove newline

Done.

[varkha, 2015-05-25 15:53:31]

https://codereview.chromium.org/1119423003/diff/320001/ui/events/event_proces...
File ui/events/event_processor_unittest.cc (right):

https://codereview.chromium.org/1119423003/diff/320001/ui/events/event_proces...
ui/events/event_processor_unittest.cc:247: // When the root has a
TestEventTargeter installed, events targeted
On 2015/05/25 15:00:08, tdanderson wrote:
> nit: "When the root has a TestEventTargeter installed which permits
bubbling..."

Done.

https://codereview.chromium.org/1119423003/diff/320001/ui/events/event_proces...
ui/events/event_processor_unittest.cc:302: // a TestEventTargeter is installed
on the root target.
On 2015/05/25 15:00:08, tdanderson wrote:
> nit: "a TestEventTargeter which permits bubbling"

Done.

https://codereview.chromium.org/1119423003/diff/320001/ui/events/test/test_ev...
File ui/events/test/test_event_targeter.h (right):

https://codereview.chromium.org/1119423003/diff/320001/ui/events/test/test_ev...
ui/events/test/test_event_targeter.h:24: explicit
TestEventTargeter(TestEventTarget* initial_target,
On 2015/05/25 15:00:08, tdanderson wrote:
> nit: not explicit since ctor has two parameters.

Done.

https://codereview.chromium.org/1119423003/diff/320001/ui/events/test/test_ev...
ui/events/test/test_event_targeter.h:33: 
On 2015/05/25 15:00:08, tdanderson wrote:
> nit: remove newline

Done.

[benwells, 2015-05-25 23:12:51]

c/b/ui/views/apps lgtm

[oshima, 2015-05-26 20:22:16]

ash/ lgtm

[varkha, 2015-05-27 14:37:15]

sadrul@, ping?

[varkha, 2015-05-27 14:37:16]

sadrul@, ping?

[sadrul, 2015-05-28 04:37:41]

On 2015/05/27 14:37:16, varkha wrote:
> sadrul@, ping?

[sadrul, 2015-05-28 04:37:42]

On 2015/05/27 14:37:16, varkha wrote:
> sadrul@, ping?

[sadrul, 2015-05-28 04:48:38]

On 2015/05/28 04:37:42, sadrul wrote:
> On 2015/05/27 14:37:16, varkha wrote:
> > sadrul@, ping?

What I meant is: sorry about the delay. I have responded to one of the comments
below. I want to take a little closer look at the rest of the CL though, so
please give me another day for review.

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targete...
File ui/aura/window_targeter.cc (right):

https://codereview.chromium.org/1119423003/diff/180001/ui/aura/window_targete...
ui/aura/window_targeter.cc:184: targeter->FindTargetForEvent(child, event);
On 2015/05/22 22:18:33, varkha wrote:
> On 2015/05/22 18:29:55, tdanderson wrote:
> > On 2015/05/21 19:11:04, varkha wrote:
> > > On 2015/05/13 18:38:25, tdanderson wrote:
> > > > Recursively?
> > > 
> > > Not so sure. WindowTargeterTest.ScopedWindowTargeter test fails if I do
that
> > and
> > > I could not follow the logic chain that confirms that this is not a
semantic
> > > change.
> > 
> > Seems OK to leave as-is to me, but I think it's worth getting a second
opinion
> > from Sadrul.
> 
> Yes, the code now reproduces the original flow, not sure if it can be
refactored
> in such a way that FindTargetForEventRecursively would recursively call itself
> rather than just a general FindTargetForEvent.

I think calling FindTargetForEvent is the right thing to do here. It allows the
child window's targeter to customize the behaviour by overriding
FindTargetForEvent if it wants to.

[varkha, 2015-06-03 14:38:16]

sadrul@, ping?

[sadrul, 2015-06-11 03:55:43]

On 2015/06/03 14:38:16, varkha wrote:
> sadrul@, ping?

lgtm. Sorry about the delay :(

[varkha, 2015-06-11 04:25:17]

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

[varkha, 2015-06-11 04:25:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/1119423003/#ps360001
(title: "Refactors ui::EventTargeter (nits)")

[commit-bot: I haz the power, 2015-06-11 04:25:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119423003/360001

[commit-bot: I haz the power, 2015-06-11 04:48:51]

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

[commit-bot: I haz the power, 2015-06-11 04:48:53]

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

[varkha, 2015-06-11 16:32:34]

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

[varkha, 2015-06-11 16:32:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, benwells@chromium.org, oshima@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1119423003/#ps380001
(title: "Refactors ui::EventTargeter (rebased)")

[commit-bot: I haz the power, 2015-06-11 16:34:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119423003/380001

[commit-bot: I haz the power, 2015-06-11 17:36:56]

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

[commit-bot: I haz the power, 2015-06-11 17:36:58]

Dry run: This issue passed the CQ dry run.

[varkha, 2015-06-11 17:48:43]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2015-06-11 17:49:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119423003/380001

[commit-bot: I haz the power, 2015-06-11 18:01:15]

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

[commit-bot: I haz the power, 2015-06-11 18:01:18]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[varkha, 2015-06-11 18:17:12]

varkha@chromium.org changed reviewers:
+ sky@chromium.org

[varkha, 2015-06-11 18:17:13]

+TBR:sky for a trivial change in
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc thta had
to be made after rebase to conform to the changed interface prototype.

[varkha, 2015-06-11 18:18:14]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2015-06-11 18:19:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119423003/380001

[commit-bot: I haz the power, 2015-06-11 18:33:11]

Committed patchset #7 (id:380001)

[commit-bot: I haz the power, 2015-06-11 18:34:01]

Patchset 7 (id:??) landed as
https://crrev.com/9c6bcf714a44413a5bf00a4cd346197d854464e6
Cr-Commit-Position: refs/heads/master@{#333993}