Condense RootView::DispatchGestureEvent() to a single loop

Condense RootView::DispatchGestureEvent() to a single loop

Remove the unnecessary code from RootView::DispatchGestureEvent()
by condensing it to a single loop and moving parts of
the implementation into RootView::PreDispatchEvent() and
RootView::PostDispatchEvent().

BUG=404228
TEST=covered by existing unit tests

Committed: https://crrev.com/6942fdd7ce53f12cdeb99d4155b29fd90d5ecb69
Cr-Commit-Position: refs/heads/master@{#293055}

[tdanderson, 2014-08-28 21:15:36]

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

[tdanderson, 2014-08-28 21:15:36]

Sadrul, please take a look!

https://codereview.chromium.org/517023004/diff/1/ui/views/widget/root_view.cc
File ui/views/widget/root_view.cc (right):

https://codereview.chromium.org/517023004/diff/1/ui/views/widget/root_view.cc...
ui/views/widget/root_view.cc:675: // not set by the dispatch of a previous
gesture event. Since |event| was
Another reason why I didn't want to name this member
|allow_gesture_event_retargeting_| in the first place was to avoid awkward
explanations such as the first sentence of this comment. Do you think this
documentation would make the logic understandable to someone reading through
this code for the first time?

https://codereview.chromium.org/517023004/diff/1/ui/views/widget/root_view.cc...
ui/views/widget/root_view.cc:750: (!allow_gesture_event_retargeting_ ||
event.handled())) {
Won't the approach of tracking |gesture_handler_| and
|allow_gesture_event_retargeting_| be problematic in cases where we want to have
nested dispatch of gesture events? The processing of the inner event could
change the values of these members and they wouldn't be changed back when the
outer event's processing is resumed.

[sadrul, 2014-08-29 19:35:53]

https://codereview.chromium.org/517023004/diff/1/ui/views/widget/root_view.cc
File ui/views/widget/root_view.cc (right):

https://codereview.chromium.org/517023004/diff/1/ui/views/widget/root_view.cc...
ui/views/widget/root_view.cc:675: // not set by the dispatch of a previous
gesture event. Since |event| was
On 2014/08/28 21:15:36, tdanderson wrote:
> Another reason why I didn't want to name this member
> |allow_gesture_event_retargeting_| in the first place was to avoid awkward
> explanations such as the first sentence of this comment. Do you think this
> documentation would make the logic understandable to someone reading through
> this code for the first time?

I am having a hard time understanding this myself.

Can you add:
  bool gesture_handler_set_before_dispatch = !!gesture_handler_;

at the beginning of this function, and use that here instead?

  if (!gesture_handler_set_before_dispatch)
    gesture_handler_ = NULL;

Or maybe use |gesture_handler_set_before_dispatch_| instead of
|allow_gesture_event_regargeting_|

[tdanderson, 2014-08-29 21:19:51]

Sadrul, can you please take another look?

https://codereview.chromium.org/517023004/diff/1/ui/views/widget/root_view.cc
File ui/views/widget/root_view.cc (right):

https://codereview.chromium.org/517023004/diff/1/ui/views/widget/root_view.cc...
ui/views/widget/root_view.cc:675: // not set by the dispatch of a previous
gesture event. Since |event| was
On 2014/08/29 19:35:53, sadrul wrote:
> On 2014/08/28 21:15:36, tdanderson wrote:
> > Another reason why I didn't want to name this member
> > |allow_gesture_event_retargeting_| in the first place was to avoid awkward
> > explanations such as the first sentence of this comment. Do you think this
> > documentation would make the logic understandable to someone reading through
> > this code for the first time?
> 
> I am having a hard time understanding this myself.
> 
> Can you add:
>   bool gesture_handler_set_before_dispatch = !!gesture_handler_;
> 
> at the beginning of this function, and use that here instead?
> 
>   if (!gesture_handler_set_before_dispatch)
>     gesture_handler_ = NULL;
> 
> Or maybe use |gesture_handler_set_before_dispatch_| instead of
> |allow_gesture_event_regargeting_|

Implemented your first suggestion for the time being. Once
DispatchGestureEvent() is deleted then I will take a look over all documentation
again to see if the variable name is clear.

[sadrul, 2014-08-29 21:43:54]

Sorry, a couple more questions:

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

https://codereview.chromium.org/517023004/diff/20001/ui/views/widget/root_vie...
ui/views/widget/root_view.cc:730: gesture_handler_ = view;
Instead of setting gesture_handler_ here, can we set it in PostDispatchEvent, if
event.handled()? (you can look into this in the next patch)

https://codereview.chromium.org/517023004/diff/20001/ui/views/widget/root_vie...
ui/views/widget/root_view.cc:750: (!allow_gesture_event_retargeting_ ||
event.handled())) {
This seems wrong. Shouldn't we always reset gesture_handler_ to NULL on
ET_GESTURE_END?

[tdanderson, 2014-08-29 21:58:07]

Please see my answers below:

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

https://codereview.chromium.org/517023004/diff/20001/ui/views/widget/root_vie...
ui/views/widget/root_view.cc:730: gesture_handler_ = view;
On 2014/08/29 21:43:54, sadrul wrote:
> Instead of setting gesture_handler_ here, can we set it in PostDispatchEvent,
if
> event.handled()? (you can look into this in the next patch)

We need to set it in PreDispatchEvent() otherwise there will be no way to detect
the case when a target has been removed from the tree during event bubbling (see
RootViewTargeter::FindNextBestTargetForGestureEvent()).

https://codereview.chromium.org/517023004/diff/20001/ui/views/widget/root_vie...
ui/views/widget/root_view.cc:750: (!allow_gesture_event_retargeting_ ||
event.handled())) {
On 2014/08/29 21:43:54, sadrul wrote:
> This seems wrong. Shouldn't we always reset gesture_handler_ to NULL on
> ET_GESTURE_END?

We shouldn't reset |gesture_handler_| to NULL until the GESTURE_END event has
finished bubbling up the views hierarchy. Setting it to NULL in the way you
suggest would actually prevent it from bubbling, which is wrong.

[sadrul, 2014-09-02 14:36:01]

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

https://codereview.chromium.org/517023004/diff/20001/ui/views/widget/root_vie...
ui/views/widget/root_view.cc:730: gesture_handler_ = view;
On 2014/08/29 21:58:07, tdanderson wrote:
> On 2014/08/29 21:43:54, sadrul wrote:
> > Instead of setting gesture_handler_ here, can we set it in
PostDispatchEvent,
> if
> > event.handled()? (you can look into this in the next patch)
> 
> We need to set it in PreDispatchEvent() otherwise there will be no way to
detect
> the case when a target has been removed from the tree during event bubbling
(see
> RootViewTargeter::FindNextBestTargetForGestureEvent()).

Should we use |RV::event_dispatch_target_| to determine if the handler has been
removed from the tree instead?

https://codereview.chromium.org/517023004/diff/20001/ui/views/widget/root_vie...
ui/views/widget/root_view.cc:750: (!allow_gesture_event_retargeting_ ||
event.handled())) {
On 2014/08/29 21:58:07, tdanderson wrote:
> On 2014/08/29 21:43:54, sadrul wrote:
> > This seems wrong. Shouldn't we always reset gesture_handler_ to NULL on
> > ET_GESTURE_END?
> 
> We shouldn't reset |gesture_handler_| to NULL until the GESTURE_END event has
> finished bubbling up the views hierarchy. Setting it to NULL in the way you
> suggest would actually prevent it from bubbling, which is wrong.

Do we need GESTURE_END to bubble up? (i.e. shouldn't we dispatch the GESTURE_END
to a View only if it had already processed some other gesture event (thus
becoming |gesture_handler_|))?

[tdanderson, 2014-09-02 17:26:35]

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

https://codereview.chromium.org/517023004/diff/20001/ui/views/widget/root_vie...
ui/views/widget/root_view.cc:730: gesture_handler_ = view;
On 2014/09/02 14:36:01, sadrul wrote:
> On 2014/08/29 21:58:07, tdanderson wrote:
> > On 2014/08/29 21:43:54, sadrul wrote:
> > > Instead of setting gesture_handler_ here, can we set it in
> PostDispatchEvent,
> > if
> > > event.handled()? (you can look into this in the next patch)
> > 
> > We need to set it in PreDispatchEvent() otherwise there will be no way to
> detect
> > the case when a target has been removed from the tree during event bubbling
> (see
> > RootViewTargeter::FindNextBestTargetForGestureEvent()).
> 
> Should we use |RV::event_dispatch_target_| to determine if the handler has
been
> removed from the tree instead?

I like this idea in theory, but for it to work properly I believe we'd need to
set |event_dispatch_target_| to |new_mh| in RootView::SetMouseHandler() so that
|event_dispatch_target_| is reset in cases that call SetMouseHandler(NULL). I'm
hesitant to make  such a change until I start working on mouse events and have a
better understanding of how all the mouse-related members work and what they're
used for. I suspect that SetMouseHandler() will eventually need to be split into
two or more different methods (or at the very least be re-named).

https://codereview.chromium.org/517023004/diff/20001/ui/views/widget/root_vie...
ui/views/widget/root_view.cc:750: (!allow_gesture_event_retargeting_ ||
event.handled())) {
On 2014/09/02 14:36:01, sadrul wrote:
> On 2014/08/29 21:58:07, tdanderson wrote:
> > On 2014/08/29 21:43:54, sadrul wrote:
> > > This seems wrong. Shouldn't we always reset gesture_handler_ to NULL on
> > > ET_GESTURE_END?
> > 
> > We shouldn't reset |gesture_handler_| to NULL until the GESTURE_END event
has
> > finished bubbling up the views hierarchy. Setting it to NULL in the way you
> > suggest would actually prevent it from bubbling, which is wrong.
> 
> Do we need GESTURE_END to bubble up? (i.e. shouldn't we dispatch the
GESTURE_END
> to a View only if it had already processed some other gesture event (thus
> becoming |gesture_handler_|))?

Yes, this makes sense. I will put up a separate CL to be landed before this one.

[tdanderson, 2014-09-02 20:29:07]

Sadrul, I always reset the gesture-handler on a gesture-end now. Can you please
take another look?

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

https://codereview.chromium.org/517023004/diff/20001/ui/views/widget/root_vie...
ui/views/widget/root_view.cc:750: (!allow_gesture_event_retargeting_ ||
event.handled())) {
On 2014/09/02 17:26:35, tdanderson wrote:
> On 2014/09/02 14:36:01, sadrul wrote:
> > On 2014/08/29 21:58:07, tdanderson wrote:
> > > On 2014/08/29 21:43:54, sadrul wrote:
> > > > This seems wrong. Shouldn't we always reset gesture_handler_ to NULL on
> > > > ET_GESTURE_END?
> > > 
> > > We shouldn't reset |gesture_handler_| to NULL until the GESTURE_END event
> has
> > > finished bubbling up the views hierarchy. Setting it to NULL in the way
you
> > > suggest would actually prevent it from bubbling, which is wrong.
> > 
> > Do we need GESTURE_END to bubble up? (i.e. shouldn't we dispatch the
> GESTURE_END
> > to a View only if it had already processed some other gesture event (thus
> > becoming |gesture_handler_|))?
> 
> Yes, this makes sense. I will put up a separate CL to be landed before this
one.

https://codereview.chromium.org/533793002/ has landed, so I adjusted this
accordingly in the next patch set.

[sadrul, 2014-09-02 22:50:50]

LGTM

[tdanderson, 2014-09-02 22:58:11]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2014-09-02 23:09:15]

The CQ bit was unchecked by tdanderson@chromium.org

[tdanderson, 2014-09-02 23:09:30]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-09-02 23:12:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdanderson@chromium.org/517023004/40001

[commit-bot: I haz the power, 2014-09-03 02:30:12]

Committed patchset #3 (id:40001) as 9b36df6820fe7b9db57dee3c479efabda1445b74

[commit-bot: I haz the power, 2014-09-10 03:23:02]

Patchset 3 (id:??) landed as
https://crrev.com/6942fdd7ce53f12cdeb99d4155b29fd90d5ecb69
Cr-Commit-Position: refs/heads/master@{#293055}