Closed Bug 969072 Opened 8 years ago Closed 8 years ago

Do some cleanup on GestureEventListener

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: kats, Assigned: kats)

Details

Attachments

(5 files, 1 obsolete file)

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.
The bulk of this is variable renaming.
Attachment #8371816 - Flags: review?(bugzilla)
This one introduces a functional change that shouldn't be a problem but that I haven't really tested.
This part also we might not want but I wrote it already so I'll save it here.
Note to self: update the bug numbers in the patches before landing.
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;
}
Attachment #8371815 - Flags: review?(bugzilla) → review+
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()|.
Attachment #8371816 - Flags: review?(bugzilla) → review+
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.
(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.
Attachment #8372312 - Attachment description: 3-facebook-cleanup-f45247f → Part 3 - Move cancel handling into the pinch gesture handler (v2)
Attachment #8372312 - Attachment is patch: true
Attachment #8372312 - Flags: review?(bugzilla)
Attachment #8371817 - Attachment is obsolete: true
Attachment #8371817 - Flags: review?(bugzilla)
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.
(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 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.
Attachment #8372312 - Flags: review?(bugzilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
I think this was meant to have [leave open] in the whiteboard.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave open]
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.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.