Right now there are two code paths in APZCTreeManager to deal with touch input. There is ProcessTouchEvent which deals with WidgetTouchEvent and ReceiveInputEvent(.. InputData ..) which deals with MultiTouchInput. The two code paths are doing exactly the same thing but are separate because WidgetTouchEvent can only be used on the main thread. We should refactor the code to reuse more of this code across the two code paths, and extract the non-reusable bits out.

Markus, feel free to steal this review if you're comfortable with it.

Comment on attachment 8445920 [details] [diff] [review] Patch, rebased on top of bug 1027309 patches Review of attachment 8445920 [details] [diff] [review]: ----------------------------------------------------------------- Cool! Looks fine overall. Just see my comment about widget/InputData.(h|cpp) ::: gfx/layers/apz/src/APZCTreeManager.cpp @@ -457,5 @@ > - // TODO(botond): Once we get rid of ReceiveInputEvent(WidgetInputEvent), > - // the signature of this function will change to take the InputData > - // via non-const reference. We can then remove the touch point from > - // multiTouchInput rather than the copy (inputForApzc), so that > - // content doesn't get it either. I remember there was a bug a long time ago due to the mismatch of the events that content and APZC were processing, but I can't remember anything else about it or what the bug number is. I hope this doesn't reintroduce that. ::: widget/InputData.h @@ +14,5 @@ > #include "mozilla/TimeStamp.h" > > namespace mozilla { > > +namespace dom { These changes to InputData.h and InputData.cpp look ok to me, but you'll need a widget peer to review them. Maybe ask Olli (smaug) since he reviewed this file originally. I'm not sure how strict we are about this rule. You can tell me, and if we don't need a widget peer, then I'm fine with this.

Comment on attachment 8445920 [details] [diff] [review] Patch, rebased on top of bug 1027309 patches Review of attachment 8445920 [details] [diff] [review]: ----------------------------------------------------------------- r? to smaug for the changes to InputData.*. Note: I also had to add an #include "nsCOMPtr.h" to InputData.h locally to make it compile on OS X, so that already_AddRefed was available.

There shouldn't be need to include nsCOMPtr.h. You could just forward declare already_AddRefed, something like template<class E> class already_AddRefed;

Comment on attachment 8445920 [details] [diff] [review] Patch, rebased on top of bug 1027309 patches >-APZCTreeManager::ProcessTouchEvent(WidgetTouchEvent& aEvent, >+APZCTreeManager::ProcessTouchEvent(MultiTouchInput& aEvent, I would rename the method and the param so that they don't talk about events but about input. >+ touchEvent.touches.Clear(); Could you call then here touchEvent.touches.SetCapacity(touchInput.mTouches.Length()); (or perhaps store the length in a variable and use it here and elsewhere) >+ for (size_t i = 0; i < touchInput.mTouches.Length(); i++) { >+ nsRefPtr<dom::Touch> touch = touchInput.mTouches[i].ToNewDOMTouch(); >+ touchEvent.touches.AppendElement(touch); I think touchEvent.touches.AppendElement() = touch.forget(); would work here.

(In reply to Olli Pettay [:smaug] from comment #5) > There shouldn't be need to include nsCOMPtr.h. > You could just forward declare already_AddRefed, something like > template<class E> class already_AddRefed; You were right, forward declaring it did the job. (In reply to Olli Pettay [:smaug] from comment #6) > I would rename the method and the param so that they don't talk about events > but about input. Done. > >+ touchEvent.touches.Clear(); > Could you call then here > touchEvent.touches.SetCapacity(touchInput.mTouches.Length()); > (or perhaps store the length in a variable and use it here and elsewhere) Considering the array will generally have at most 2 or 3 elements this seems kind of premature optimization to me, but done anyway. > >+ for (size_t i = 0; i < touchInput.mTouches.Length(); i++) { > >+ nsRefPtr<dom::Touch> touch = touchInput.mTouches[i].ToNewDOMTouch(); > >+ touchEvent.touches.AppendElement(touch); > I think > touchEvent.touches.AppendElement() = touch.forget(); > would work here. Interesting. Based on other code snippets I think it would have to have a |*| at the front of that statement as well. I think that's less easy to read but if I change it to this: *touchEvent.touches.AppendElement() = touchInput.mTouches[i].ToNewDOMTouch(); then I can get rid of the temp variable entirely, so I'll go with that. Compiling locally to ensure it works, will land shortly.

Fix typo I introduced in this change: https://hg.mozilla.org/integration/mozilla-inbound/rev/d2a102f1ef78