Closed Bug 1299488 Opened 6 years ago Closed 6 years ago

Get rid of TabParent::InjectTouchEvent

Categories

(Core :: DOM: Content Processes, defect, P3)

51 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: kats, Assigned: kats)

Details

Attachments

(1 file)

This function has been the source of many problems in the past, because it injects touch events partway into the flow of events, and can leave different parts of the code in inconsistent states, causing problems later in execution. Such problems are not only hard to reproduce but hard to diagnose.

Right now it looks like nobody is using the InjectTouchEvent function, so it's a good time to get rid of it without breaking anything.
Attached patch PatchSplinter Review
Actually it looks like there's still some Gaia code that uses this via the TouchForwarder class. Alex, do you know if a lot of B2G things will break if I land this patch? Note that I'm not removing the sendTouchEvent function entirely, but I'm taking out the APZ injection codepath - so it's possible some actions will break, but hopefully not everything. If that's the case I'm willing to just hide it behind a #ifdef B2G or something, so that people don't start using it on other platforms.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3f8b7d942ea
Attachment #8786751 - Flags: feedback?(lissyx+mozillians)
That's nice to warn us :), although I don't know the impact at all. I do see uses of TouchForwarder class in Gaia. Do you know more about that ?
Flags: needinfo?(bugmail)
Maybe Gabriele and/or Fabrice would have an opinion too
Flags: needinfo?(gsvelto)
Flags: needinfo?(fabrice)
(In reply to Alexandre LISSY :gerard-majax from comment #2)
> That's nice to warn us :), although I don't know the impact at all. I do see
> uses of TouchForwarder class in Gaia. Do you know more about that ?

From what I remember it was used during gesture detection - the JS code would intercept the touch events to do detection, and if the right gesture wasn't detected it would use sendTouchEvent to put the event back into the gecko pipeline so it would go to the content underneath.
Flags: needinfo?(bugmail)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> From what I remember it was used during gesture detection - the JS code
> would intercept the touch events to do detection, and if the right gesture
> wasn't detected it would use sendTouchEvent to put the event back into the
> gecko pipeline so it would go to the content underneath.

Yep. I think this partially broke a long time ago for scrolling, but it might still be working for taps...
But the main issue will probably be JS Errors during if b2gos isn't updated first.
(In reply to Etienne Segonzac (:etienne) from comment #5)

> Yep. I think this partially broke a long time ago for scrolling, but it
> might still be working for taps...
> But the main issue will probably be JS Errors during if b2gos isn't updated
> first.

JS Errors during ... what? :P
Flags: needinfo?(fabrice) → needinfo?(etienne)
During edge gestures :)
Flags: needinfo?(etienne)
Well I'm leaving in the sendTouchEvent codepath that injects touch events into the DOM, so that will get used. I just don't know what effect that will have on the edge gestures - but I don't expect JS errors at least.

Anyhow it sounds like I should be ok to remove this code since it's uncertain if it will break anything - if you guys find something breaks, you can add it back inside an #ifdef B2G.
Attachment #8786751 - Flags: feedback?(lissyx+mozillians) → review?(botond)
Comment on attachment 8786751 [details] [diff] [review]
Patch

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

Please also update [1] as appropriate. 

[2] looks like it's already out of date (what ends up being called is nsIDOMWindowUtils::SendNativeTouchPoint), feel free to update that if you'd like.

[1] http://searchfox.org/mozilla-central/rev/6536590412ea212c291719d1ed226fae65a0f917/gfx/layers/apz/util/APZEventState.cpp#295
[2] http://searchfox.org/mozilla-central/rev/6536590412ea212c291719d1ed226fae65a0f917/testing/marionette/listener.js#628
Attachment #8786751 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #9)
> Please also update [1] as appropriate. 

Good catch. It looks like that shouldn't be needed any more, so I'll replace it with an assertion.

> [2] looks like it's already out of date (what ends up being called is
> nsIDOMWindowUtils::SendNativeTouchPoint), feel free to update that if you'd
> like.

Will do.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c3b580cc9f9
Remove TabParent::InjectTouchEvent as it is unused but error-prone. r=botond
https://hg.mozilla.org/mozilla-central/rev/0c3b580cc9f9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Unfortunately I'm not familiar with how this worked so I can't help.
Flags: needinfo?(gsvelto)
You need to log in before you can comment on or make changes to this bug.