969072 - Do some cleanup on GestureEventListener

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

As part of bug 965381 I was writing a big set of patches to reorder how gestures and touch events get dispatched but I made a mistake (see comment 21). Nonetheless I ended up with some cleanup patches that we can land so I'll put them here for now.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Only pass multitouch events to GEL

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Make GEL only accept MultiTouchInput

The bulk of this is variable renaming.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Move cancel handling into the pinch gesture handler

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Move touch list updating to a helper function

This one introduces a functional change that shouldn't be a problem but that I haven't really tested.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 5 - Split HandleInputEvent into three and rename the bits

This part also we might not want but I wrote it already so I'll save it here.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Note to self: update the bug numbers in the patches before landing.

Comment 7

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8371815 [details] [diff] [review]
Part 1 - Only pass multitouch events to GEL

Review of attachment 8371815 [details] [diff] [review]:
-----------------------------------------------------------------

The reason we didn't want to do this in the past is because we imagined that there may be situations where we want to pass gestures back into GEL, or some other class might generate gestures that GEL should listen for. I don't think that's going to happen, so it's fine to remove the over-generalization.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +484,5 @@
> +    nsRefPtr<GestureEventListener> listener = GetGestureEventListener();
> +    if (listener) {
> +      rv = listener->HandleInputEvent(aEvent);
> +      if (rv == nsEventStatus_eConsumeNoDefault)
> +        return rv;

Can you fix the styling while you're here? It should be:

if (rv == nsEventStatus_eConsumeNoDefault) {
  return rv;
}

Comment 8

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8371816 [details] [diff] [review]
Part 2 - Make GEL only accept MultiTouchInput

Review of attachment 8371816 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/GestureEventListener.cpp
@@ -52,2 @@
>  {
> -  const MultiTouchInput& event = static_cast<const MultiTouchInput&>(aEvent);

This sucked anyways, we should have been using |aEvent.AsMultiTouchInput()|.

Comment 9

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8371817 [details] [diff] [review]
Part 3 - Move cancel handling into the pinch gesture handler

Review of attachment 8371817 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/GestureEventListener.cpp
@@ +193,5 @@
>  
>      break;
>    }
> +  case MultiTouchInput::MULTITOUCH_CANCEL: {
> +    // FIXME: we should probably clear a bunch of gesture state here

Yeah, I think we should. It makes more sense to let individual gesture handlers deal with this, though, since it may vary based on what type of gesture you're in.

Do you know exactly the conditions under which a cancel happens? The only time I've ever observed it is when I did what was written in the comment that used to be here.

@@ +288,2 @@
>      mTouches.Clear();
>    }

Can we just move this block to the top? I think this structure is just the result of removal of code that used to be above this which had to run regardless of the value of aClearTouches.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(In reply to Doug Sherk (:drs) from comment #9)
> Do you know exactly the conditions under which a cancel happens? The only
> time I've ever observed it is when I did what was written in the comment
> that used to be here.

In general it can be issued whenever the hardware feels like it. There are certain conditions when it should always happens but I don't think we should be relying on that. The only thing we should assume is that whatever touch events are in progress are all aborted and all touch points are off.

> 
> @@ +288,2 @@
> >      mTouches.Clear();
> >    }
> 
> Can we just move this block to the top? I think this structure is just the
> result of removal of code that used to be above this which had to run
> regardless of the value of aClearTouches.

Yup, sounds good. I'll upload a new version.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Move cancel handling into the pinch gesture handler (v2)

Comment 12

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8372312 [details] [diff] [review]
Part 3 - Move cancel handling into the pinch gesture handler (v2)

Review of attachment 8372312 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/GestureEventListener.cpp
@@ +192,5 @@
>  
>      break;
>    }
> +  case MultiTouchInput::MULTITOUCH_CANCEL: {
> +    // FIXME: we should probably clear a bunch of gesture state here

OK, I think in that case, it would be best to mention here that individual HandleXXXGestureEvent functions are responsible for dealing with this. I don't think there's really a need for a FIXME, at least in this spot.

@@ +210,5 @@
> +    mState = GESTURE_NONE;
> +    return rv;
> +  }
> +
> +  if (mTouches.Length() > 1) {

Being really pedantic, this could be an else if. It makes the flow clearer and the second block can't possibly be executed if the first is.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to Doug Sherk (:drs) from comment #12)
> OK, I think in that case, it would be best to mention here that individual
> HandleXXXGestureEvent functions are responsible for dealing with this. I
> don't think there's really a need for a FIXME, at least in this spot.

There are no other HandleXXXGestureEvent functions. Who is responsible for clearing things like mLongTapTimeoutTask when we get a CANCEL? Right now nobody does that.

> @@ +210,5 @@
> > +    mState = GESTURE_NONE;
> > +    return rv;
> > +  }
> > +
> > +  if (mTouches.Length() > 1) {
> 
> Being really pedantic, this could be an else if. It makes the flow clearer
> and the second block can't possibly be executed if the first is.

Style guide says otherwise:
https://developer.mozilla.org/en/docs/Developer_Guide/Coding_Style#General_C.2FC.2B.2B_Practices

Comment 14

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8372312 [details] [diff] [review]
Part 3 - Move cancel handling into the pinch gesture handler (v2)

Review of attachment 8372312 [details] [diff] [review]:
-----------------------------------------------------------------

OK, sounds good.

Comment 15

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> (In reply to Doug Sherk (:drs) from comment #9)
> > Do you know exactly the conditions under which a cancel happens? The only
> > time I've ever observed it is when I did what was written in the comment
> > that used to be here.
> 
> In general it can be issued whenever the hardware feels like it. There are
> certain conditions when it should always happens but I don't think we should
> be relying on that. The only thing we should assume is that whatever touch
> events are in progress are all aborted and all touch points are off.
> 
> > 
> > @@ +288,2 @@
> > >      mTouches.Clear();
> > >    }
> > 
> > Can we just move this block to the top? I think this structure is just the
> > result of removal of code that used to be above this which had to run
> > regardless of the value of aClearTouches.
> 
> Yup, sounds good. I'll upload a new version.

(In reply to Doug Sherk (:drs) from comment #14)
> Comment on attachment 8372312 [details] [diff] [review]
> Part 3 - Move cancel handling into the pinch gesture handler (v2)
> 
> Review of attachment 8372312 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK, sounds good.

Oh, sorry. One more nit. Hopefully you haven't landed it yet. The switch/case: MULTITOUCH_CANCEL block shouldn't have {} brackets around it.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Removed the braces and landed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c8a74e6f8159
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c4e6cf313662
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eddf0ede2cb2

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c8a74e6f8159
https://hg.mozilla.org/mozilla-central/rev/c4e6cf313662
https://hg.mozilla.org/mozilla-central/rev/eddf0ede2cb2

Comment 18

[Doug Sherk (:drs) (inactive)]

I think this was meant to have [leave open] in the whiteboard.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

No, I don't plan on landing the other patches anytime soon. They have functional changes and I don't want to risk any more regressions right now.