Markus Stange [:mstange] Reporter
	Description • 9 years ago

At the moment, nsLayoutUtils::HasApzAwareListeners just lumps all kinds of event handlers into one, and makes APZ wait for a content response if there's a listener for any of those event types, regardless of what kind of event is being processed by APZ.

There are sites (e.g. imgur) which have touch event listeners but no wheel event listeners. Those sites shouldn't be allowed to slow down wheel event processing.

I think there are two options here:
 - Either create a separate dispatch-to-content region per event type, or
 - check which event types are supported by APZ on the current platform, and don't
   add listeners for unsupported events to the dispatch-to-content region.

The latter options sounds more attractive, until we start supporting both wheel and touch events on the same platform.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 1 • 9 years ago

Doing this might also help reduce the performance impact of event regions.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 2 • 8 years ago

Not important enough to uplift.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 3 • 8 years ago

I have patches for this, but the test needs bug 1274284 fixed first.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 4 • 8 years ago

Try push with bug 1273654 + bug 1274284 + this bug, for verification:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aca30e71385

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 5 • 8 years ago

I canceled the above try push because it had some bad patches from bug 1274284 included. Since then I rewrote the test to not require bug 1274284. New try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=211c68f276dc&group_state=expanded confirms it's good (the many starred windows failures are a pre-existing issue).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 9 • 8 years ago

Note that part 3 depends upon the patches in bug 1273654, although it can be rebased to not use those patches.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 8 years ago

Comment on attachment 8755094 [details] [diff] [review]
Part 3 - Test

Don't you need some todo() or ok() in case isApzEnabled() returns false so that the test doesn't complain nothing was tested.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 11 • 8 years ago

(In reply to Olli Pettay [:smaug] from comment #10)
> Don't you need some todo() or ok() in case isApzEnabled() returns false so
> that the test doesn't complain nothing was tested.

The isApzEnabled() function implementation does that internally. I figured it was nicer to do it that way so I didn't have to do it at every use-site.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 12 • 8 years ago

I rebased the test to not depend on bug 1273654, so I can get it landed. I'll update it as part of the refactorings in bug 1273654 eventually.

	Pulsebot
	Comment 13 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ea3b7f6bac
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c1effae7146
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d77ceeacfa6

	Botond Ballo [:botond]
	Comment 14 • 8 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> I rebased the test to not depend on bug 1273654, so I can get it landed.
> I'll update it as part of the refactorings in bug 1273654 eventually.

Sorry for being away for so long and causing you to do things like this :/

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 15 • 8 years ago

No worries, it wasn't very hard to do. I do look forward to landing those refactoring patches though :)

	Carsten Book [:Tomcat]
	Comment 16 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d4ea3b7f6bac
https://hg.mozilla.org/mozilla-central/rev/1c1effae7146
https://hg.mozilla.org/mozilla-central/rev/7d77ceeacfa6

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 17 • 8 years ago

Comment on attachment 8755092 [details] [diff] [review]
Part 1 - Add a pref cache

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: Pages with touch listeners can slow down wheel scrolling even on devices where touch events aren't supported
[Describe test coverage new/current, TreeHerder]: patches include a test
[Risks and why]: pretty low risk, code has been on m-c for a bit and it's a fairly simple optimization
[String/UUID change made/needed]: none

	Sylvestre Ledru [:Sylvestre]
	Comment 18 • 8 years ago

Comment on attachment 8755092 [details] [diff] [review]
Part 1 - Add a pref cache

Improve a feature, taking it.

	Carsten Book [:Tomcat]
	Comment 19 • 8 years ago

part 2 has problems to apply to beta :

grafting 346425:1c1effae7146 "Bug 1203140 - Don't add touch listener areas to dispatch-to-content regions unless touch events are enabled. r=smaug"
merging dom/events/EventListenerManager.cpp
warning: conflicts while merging dom/events/EventListenerManager.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

can you take a look ?

	Carsten Book [:Tomcat]
	Comment 20 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/7366f3c669b3

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 21 • 8 years ago

Parts 2 and 3:

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/4cec5ef7ea55
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/2c5721ac42d7

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 22 • 8 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/8a2842b342b5

I disabled the test on beta because it was permafailing. It requires passive event listener support to pass, and passive event listener support is only in 49 and up. The actual patches in this bug don't need passive event listener support so they are find to stay in 48.