745381 - We send a lot of touch events during panning of http://people.mozilla.org/~jrosevear/color-test.html

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

This means we end up calling GetFrameForPoint which can be expensive on complex pages.

Comment 1

[Wesley Johnston (:wesj)]

One really easy optimization here, we don't actually need to refind target's for touch events. They're always dispatched to the original target. We can probably just skip GetFrameForPoint for anything but touchstart events.

Comment 2

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

Also something to consider: https://bugzilla.mozilla.org/show_bug.cgi?id=743736#c4

Comment 3

[Wesley Johnston (:wesj)]

There's also a bug(?) in:

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2169

which causes us to use the entire displayport when building a list for hit testing in Fennec (note that should be true for mouse events as well, since I assume elementFromPoint and nodesFromRect take this same code path).

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Brian - Is this optimization the same as bug 743736? If not, let's impl it. If it is, let's dupe it.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #3)
> There's also a bug(?) in:
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/
> nsGfxScrollFrame.cpp#2169
> 
> which causes us to use the entire displayport when building a list for hit
> testing in Fennec (note that should be true for mouse events as well, since
> I assume elementFromPoint and nodesFromRect take this same code path).

Should this be a separate bug?

Comment 6

[Wesley Johnston (:wesj)]

Yep. Filed bug 745936.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch

Sorry to steal this back. I'm digging through all this code anyway. Easy fix.

I'm not sure this will fix this bug. We still send tons of touch events. Just faster. Fingers jitter on the screen, and in addition we send them if your pressure or the radius of the touch changes.

If this isn't enough, we could try smoothing some of that data and only sending events from java if there is a significant delta between the last one. But I wanted to avoid that and give pages access to as close to the raw data as we can (our data is already filtered by android to some extent).

Comment 8

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

Attachment: Don't send touch events if the page isn't listening for them

This patch is a bigger hammer to fix this nail; it will just not send the touch events at all if the page isn't listening for them. From what I can tell, the dom-touch-listener-added will always get called if a page registered a touch listener, so this should always be correct. It may be the case that the page unregisters its touch listeners and there we will keep firing events. (Also if this patch lands then bug 744518 should be marked as WONTFIX because that will render this patch moot by adding a touchstart listener on every page, effectively).

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 615513 [details] [diff] [review]
Patch

So where do we get the right target?

Comment 10

[Wesley Johnston (:wesj)]

For anything that's not a touch move, we are caching the target (on a per touch basis) and redispatching to it here:

http://mxr.mozilla.org/mozilla-beta/source/layout/base/nsPresShell.cpp#6725

This code (the code we're skipping around with this patch) currently fails for touchmove/touchend/touchcancel because the refpoint for those points is (0,0) (inherited from nsEvent). We special cased TOUCH_DOWN slightly above this so that it returns the "correct" refpoint.

Comment 11

[Wesley Johnston (:wesj)]

After much hg fail, pushed with the wrong bug number (argh. I'm done with inbound for the day I think):

https://hg.mozilla.org/integration/mozilla-inbound/rev/d07b72d4865d

Comment 12

[Wesley Johnston (:wesj)]

Marking mtd for the inbound people. I still need to look at kat's patch here. Truthfully, I don't want to do that, and would like to see where we are after this before we take a bigger hammer to it.

If we want to clear the blocker, we can move that to a different bug?

Comment 13

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

Feel free to r- the patch and if we need it later we can file a new bug and grab the patch from here.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Kartikaya Gupta (:kats) from comment #13)
> Feel free to r- the patch and if we need it later we can file a new bug and
> grab the patch from here.

Let's file a new bug and move the patch, especially if the risks are different.

Comment 15

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/mozilla-central/rev/d07b72d4865d