900990 - Do not attach [mouse|touch]mode events if events target is not a scrollable area

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

The scrolling in the homescreen is an app based custom panning but some platform code is still running and consume a few fps.

Calling evt.preventDefault() on the touchstart event in grid.js should shut up the platform code. Sadly this have some side effects like preventing the context menu event to fire for example, or killing focus switch :)

So someone needs to dig into the code and fix the possible regressions.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug900990.wip.patch

This does ask the platform to be quiet. And it also creates regressions! \o/

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Asking koi? for performance bug on the homescreen

Comment 3

[Cristian Rodriguez (:crdlc)]

I would like to know what that patch does, I mean, we are sure that it disables contextmenu and click events. Will it land on master?

Comment 4

[Gregor Wagner [:gwagner]]

We shouldn't block on performance bugs. Lets go through the approval process once it's done.

Comment 5

[Jason Smith [:jsmith]]

(In reply to Gregor Wagner [:gwagner] from comment #4)
> We shouldn't block on performance bugs. Lets go through the approval process
> once it's done.

Well, we shouldn't block on general perf bugs, but we should block on perf regressions impacting us maintaining target certification performance metrics. Agreed on this bug not blocking though.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Cristian Rodriguez (:crdlc) from comment #3)
> I would like to know what that patch does, I mean, we are sure that it
> disables contextmenu and click events. Will it land on master?

Cristian this patch is just a work in progress right now. Feel free to finish it. Basically this patch prevent the platform to intercept some of the touch events. Even if the platform does nothing with them in our case there is still some code triggers all the time that is useless here.

The relevant code is http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js#68

Comment 7

[Cristian Rodriguez (:crdlc)]

I feel free but I don't have more time to take more bugs and reviews :( Sorry

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: no.move.event.listener.patch

Fabrice, I fix the issue directly in BrowserElementPanning.js by not listening [mouse|touch[ if those are not needed (if the area is not scrollable). I also removed some dead code since subframes are directly handle by APZC now.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 819507 [details] [diff] [review]
no.move.event.listener.patch

Asking an additional review for the removal of the relevant TabChild.cpp code.

Comment 10

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

Comment on attachment 819507 [details] [diff] [review]
no.move.event.listener.patch

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

It would be good to push this to try and do a bunch of Marionette runs because my previous attempt to get rid of this code (along with other stuff) in bug 906877 was backed out for that reason.

Comment 11

[[:fabrice] Fabrice Desré]

Vivien, do you have a test app with a subframes? Also please push to try.

Comment 12

[[:fabrice] Fabrice Desré]

Do we still need that Vivien?

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabrice Desré [:fabrice] from comment #12)
> Do we still need that Vivien?

I need to double check with APZC. I guess that it will be useless once we will be able to pan the homescreen with APZC directly. Also I plan to move some of the BrowserElementPanning.js code into TabChild.cpp so maybe this bug does not worth working too much on it yet.