Introduce EventProcessor::OnEventProcessingStarted()

Introduce EventProcessor::OnEventProcessingStarted()

Introduce EventProcessor::OnEventProcessingStarted(),
to be called at the beginning of event-processing
in EventProcessor::OnEventFromSource() before any
targeting takes place. OnEventProcessingStarted()
marks the event as handled if it is not to be
dispatched, otherwise it optionally prepares the
event for targeting and/or updates any state
variables in the event processor.

OnEventProcessingStarted() replaces
EventProcessor::PrepareEventForDispatch()
and allows us to remove all overrides of
EventProcessor::OnEventFromSource() which are
not part of unit tests.

BUG=412776
TEST=EventProcessorTest.OnEventProcessingStarted,
     WidgetTest.GestureEventsNotProcessed,
     WindowEventDispatcherTest.MouseMovesHeld changed,
     WidgetTest.GestureEndEvents changed

Committed: https://crrev.com/fb66d874c0916cdecd2405b585e7266c29826c5c
Cr-Commit-Position: refs/heads/master@{#297521}

[tdanderson, 2014-09-12 17:23:03]

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

[tdanderson, 2014-09-12 17:23:03]

Sadrul, can you please take a look?

[sadrul, 2014-09-15 14:10:36]

https://codereview.chromium.org/552503003/diff/80001/ui/events/event_processor.h
File ui/events/event_processor.h (right):

https://codereview.chromium.org/552503003/diff/80001/ui/events/event_processo...
ui/events/event_processor.h:38: virtual bool OnEventProcessingStarted(Event*
event);
I remember we briefly discussed making this not return a bool, and instead
marking the event as handled if the event should not be dispatched. Can you
remind me why that wouldn't work?

https://codereview.chromium.org/552503003/diff/80001/ui/views/widget/root_vie...
File ui/views/widget/root_view.cc (right):

https://codereview.chromium.org/552503003/diff/80001/ui/views/widget/root_vie...
ui/views/widget/root_view.cc:254: gesture_handler_set_before_processing_ =
!!gesture_handler_;
Should you move this down so this is set only when returning true?

[tdanderson, 2014-09-15 18:20:27]

Sadrul, please see my replies to your comments below:

https://codereview.chromium.org/552503003/diff/80001/ui/events/event_processor.h
File ui/events/event_processor.h (right):

https://codereview.chromium.org/552503003/diff/80001/ui/events/event_processo...
ui/events/event_processor.h:38: virtual bool OnEventProcessingStarted(Event*
event);
On 2014/09/15 14:10:36, sadrul wrote:
> I remember we briefly discussed making this not return a bool, and instead
> marking the event as handled if the event should not be dispatched. Can you
> remind me why that wouldn't work?

Sorry, I actually don't remember discussing that option. Let me give it a try
(I'll ping you when I've uploaded another CL). The first concern that comes to
mind is that by marking these events as handled we would prevent them from being
forwarded to the menu controller in MenuHostRootView.

https://codereview.chromium.org/552503003/diff/80001/ui/views/widget/root_vie...
File ui/views/widget/root_view.cc (right):

https://codereview.chromium.org/552503003/diff/80001/ui/views/widget/root_vie...
ui/views/widget/root_view.cc:254: gesture_handler_set_before_processing_ =
!!gesture_handler_;
On 2014/09/15 14:10:36, sadrul wrote:
> Should you move this down so this is set only when returning true?

With the CL as-is, this will need to stay where it is otherwise the value of
|gesture_handler_set_before_processing_| may be incorrect by the time we reach
OnEventProcessingFinished(), causing |gesture_handler_| to be reset to NULL when
it shouldn't.

If I end up handling the event instead as per your other comment, then I should
be able to move this line.

[tdanderson, 2014-09-15 18:58:53]

Sadrul, PS6 uploaded containing your suggestions. Running them on the trybots to
see what fails.

[sadrul, 2014-09-15 19:05:46]

https://codereview.chromium.org/552503003/diff/100001/ui/events/event_process...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/552503003/diff/100001/ui/events/event_process...
ui/events/event_processor.cc:29: OnEventProcessingStarted(event);
event_to_dispatch?

https://codereview.chromium.org/552503003/diff/100001/ui/events/event_process...
ui/events/event_processor.cc:30: EventTarget* target =
targeter->FindTargetForEvent(root, event_to_dispatch);
You need to check event_to_dispatch->handled() before doing 30:48, right?

[tdanderson, 2014-09-15 19:14:27]

https://codereview.chromium.org/552503003/diff/100001/ui/events/event_process...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/552503003/diff/100001/ui/events/event_process...
ui/events/event_processor.cc:29: OnEventProcessingStarted(event);
On 2014/09/15 19:05:46, sadrul wrote:
> event_to_dispatch?

Done.

https://codereview.chromium.org/552503003/diff/100001/ui/events/event_process...
ui/events/event_processor.cc:30: EventTarget* target =
targeter->FindTargetForEvent(root, event_to_dispatch);
On 2014/09/15 19:05:46, sadrul wrote:
> You need to check event_to_dispatch->handled() before doing 30:48, right?

Ah yes, of course. Checked this in while(...).

[sadrul, 2014-09-16 17:16:54]

The try failures look related. A comment on the change in EventProcessor
(haven't looked closely at the rest of the changes).

https://codereview.chromium.org/552503003/diff/120001/ui/events/event_process...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/552503003/diff/120001/ui/events/event_process...
ui/events/event_processor.cc:30: EventTarget* target =
targeter->FindTargetForEvent(root, event_to_dispatch);
You don't need to find the target if you aren't going to dispatch the event.

[tdanderson, 2014-09-23 21:30:28]

Sadrul, can you please take a look at the latest patchset + my inline comments?

https://chromiumcodereview.appspot.com/552503003/diff/120001/ui/events/event_...
File ui/events/event_processor.cc (right):

https://chromiumcodereview.appspot.com/552503003/diff/120001/ui/events/event_...
ui/events/event_processor.cc:30: EventTarget* target =
targeter->FindTargetForEvent(root, event_to_dispatch);
On 2014/09/16 17:16:53, sadrul wrote:
> You don't need to find the target if you aren't going to dispatch the event.

Yes, good point... also, FindTargetForEvent() may mutate the event and we don't
want that if we're not going to dispatch. Changed.

https://chromiumcodereview.appspot.com/552503003/diff/140001/ui/events/event_...
File ui/events/event_processor.cc (right):

https://chromiumcodereview.appspot.com/552503003/diff/140001/ui/events/event_...
ui/events/event_processor.cc:31: target = targeter->FindTargetForEvent(root,
event_to_dispatch);
Another option would be to have EventTargeter::FindTargetForEvent() and all of
its overrides just do an early return NULL if the event is handled, then we
could remove the check at line 30. But I think if you prefer that option, it
should be done in a separate CL.

https://chromiumcodereview.appspot.com/552503003/diff/140001/ui/views/widget/...
File ui/views/widget/root_view_targeter.cc (right):

https://chromiumcodereview.appspot.com/552503003/diff/140001/ui/views/widget/...
ui/views/widget/root_view_targeter.cc:37: //                  
RootView::OnEventProcessingStarted() instead of here.
Will do this in a follow-up CL to avoid further cluttering up this one with
unittest changes.

https://chromiumcodereview.appspot.com/552503003/diff/140001/ui/views/widget/...
File ui/views/widget/widget_unittest.cc (right):

https://chromiumcodereview.appspot.com/552503003/diff/140001/ui/views/widget/...
ui/views/widget/widget_unittest.cc:1984: TEST_F(WidgetTest, GestureEndEvents) {
Reduced the scope and size of this test since some of the logic is tested
elsewhere (including in the new test WidgetTest.GestureEventsNotProcessed).

[sadrul, 2014-09-25 16:12:01]

LGTM

https://codereview.chromium.org/552503003/diff/140001/ui/events/event_process...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/552503003/diff/140001/ui/events/event_process...
ui/events/event_processor.cc:31: target = targeter->FindTargetForEvent(root,
event_to_dispatch);
On 2014/09/23 21:30:27, tdanderson wrote:
> Another option would be to have EventTargeter::FindTargetForEvent() and all of
> its overrides just do an early return NULL if the event is handled, then we
> could remove the check at line 30. But I think if you prefer that option, it
> should be done in a separate CL.

I think we should do this here, and not require all targeters to have to look at
the event's dispatch state.

https://codereview.chromium.org/552503003/diff/160001/ui/events/test/test_eve...
File ui/events/test/test_event_processor.cc (right):

https://codereview.chromium.org/552503003/diff/160001/ui/events/test/test_eve...
ui/events/test/test_event_processor.cc:24: void
TestEventProcessor::SetShouldProcessingOccur(bool occur) {
set_should_processing_occur()?

https://codereview.chromium.org/552503003/diff/160001/ui/views/widget/root_vi...
File ui/views/widget/root_view.cc (right):

https://codereview.chromium.org/552503003/diff/160001/ui/views/widget/root_vi...
ui/views/widget/root_view.cc:253: if (event->IsGestureEvent()) {
Early return if !gesture-event instead

[tdanderson, 2014-09-25 17:11:59]

https://codereview.chromium.org/552503003/diff/140001/ui/events/event_process...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/552503003/diff/140001/ui/events/event_process...
ui/events/event_processor.cc:31: target = targeter->FindTargetForEvent(root,
event_to_dispatch);
On 2014/09/25 16:12:00, sadrul wrote:
> On 2014/09/23 21:30:27, tdanderson wrote:
> > Another option would be to have EventTargeter::FindTargetForEvent() and all
of
> > its overrides just do an early return NULL if the event is handled, then we
> > could remove the check at line 30. But I think if you prefer that option, it
> > should be done in a separate CL.
> 
> I think we should do this here, and not require all targeters to have to look
at
> the event's dispatch state.

Sounds good, left as-is then.

https://codereview.chromium.org/552503003/diff/160001/ui/events/test/test_eve...
File ui/events/test/test_event_processor.cc (right):

https://codereview.chromium.org/552503003/diff/160001/ui/events/test/test_eve...
ui/events/test/test_event_processor.cc:24: void
TestEventProcessor::SetShouldProcessingOccur(bool occur) {
On 2014/09/25 16:12:01, sadrul wrote:
> set_should_processing_occur()?

Done.

https://codereview.chromium.org/552503003/diff/160001/ui/views/widget/root_vi...
File ui/views/widget/root_view.cc (right):

https://codereview.chromium.org/552503003/diff/160001/ui/views/widget/root_vi...
ui/views/widget/root_view.cc:253: if (event->IsGestureEvent()) {
On 2014/09/25 16:12:01, sadrul wrote:
> Early return if !gesture-event instead

Done.

[tdanderson, 2014-09-25 17:44:16]

tdanderson@chromium.org changed reviewers:
+ ben@chromium.org

[tdanderson, 2014-09-25 17:44:16]

Ben, can you please review the changes in chrome/ ?

[tdanderson, 2014-09-30 21:04:00]

tdanderson@chromium.org changed reviewers:
+ sky@chromium.org
- ben@chromium.org

[tdanderson, 2014-09-30 21:04:24]

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

[sky, 2014-09-30 21:52:04]

LGTM

[tdanderson, 2014-09-30 22:09:10]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-09-30 22:09:51]

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

[commit-bot: I haz the power, 2014-09-30 22:15:53]

Committed patchset #10 (id:180001) as 3cf12715fb0c9b6099a2a6f56777297d6d4b06c2

[commit-bot: I haz the power, 2014-09-30 22:16:29]

Patchset 10 (id:??) landed as
https://crrev.com/fb66d874c0916cdecd2405b585e7266c29826c5c
Cr-Commit-Position: refs/heads/master@{#297521}