Part of the long-term plan in bug 928833 was to add regions to the EventRegions struct to represent different touch-action behaviours. The APZ code could then use these behaviours to support touch-action directly without having to wait for information from the main thread. This bug is about the APZ-side work needed to implement this, and depends on bug 1101627 to actually add the relevant regions to the EventRegions struct.

I believe there's not a lot of work left to finish this off, so I'm going to try and get it done in the near future.

The patches I have for this get the APZ to look at the event regions data on the layer, and use that if possible. Previously the APZ code was always waiting on the main thread for event regions information. https://treeherder.mozilla.org/#/jobs?repo=try&revision=06d807c48280

The test is failing on Android - unsurprising, since I wrote it with e10s architecture in mind which Fennec doesn't have. I thought about how to make it Fennec-friendly and right now I don't think we have the right APIs to be able to do it. We can add them, but the incremental value of running the test on Fennec in addition to Linux is probably not worth the effort. I'll punt that to a follow-up and just skip the test if on Android.

Well, more specifically, skip the test if on any non-e10s platform.

Review commit: https://reviewboard.mozilla.org/r/55810/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/55810/

Review commit: https://reviewboard.mozilla.org/r/55812/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/55812/

Review commit: https://reviewboard.mozilla.org/r/55814/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/55814/

Review commit: https://reviewboard.mozilla.org/r/55816/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/55816/

Review commit: https://reviewboard.mozilla.org/r/55818/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/55818/

Note that these patches apply on top of those in bug 1275604 (I used push -r foo::bar to push to MozReview, so the diffs are relative to bug 1275604, not master).

Comment on attachment 8757331 [details] MozReview Request: Bug 1101628 - Add additional HitResult types to reflect the different event regions. r=botond https://reviewboard.mozilla.org/r/55810/#review52528 ::: gfx/layers/LayersTypes.h (Diff revision 1) > - { > - mHitRegion.AndWith(aRegion); > - mDispatchToContentHitRegion.AndWith(aRegion); > - } > - > - void Sub(const EventRegions& aMinuend, const nsIntRegion& aSubtrahend) No one seems to use this; let's axe it (I'm not a big fan of the signature anyways).

Comment on attachment 8757332 [details] MozReview Request: Bug 1101628 - Have the APZCTM tell the input queue what the allowed touch behaviours are, if it has the information. r?botond https://reviewboard.mozilla.org/r/55812/#review52530 ::: gfx/layers/apz/src/APZCTreeManager.cpp:898 (Diff revision 1) > +static TouchBehaviorFlags > +ConvertToTouchBehavior(HitTestResult result) > +{ > + switch (result) { > + case HitNothing: > + return AllowedTouchBehavior::NONE; Should we ever get HitNothing as an input to this function? If not, it may be better to assert. ::: gfx/layers/apz/src/APZCTreeManager.cpp:1025 (Diff revision 1) > mApzcForInputBlock->GetGuid(aOutTargetGuid); > result = mInputQueue->ReceiveInputEvent(mApzcForInputBlock, > /* aTargetConfirmed = */ mHitResultForInputBlock != HitDispatchToContentRegion, > aInput, aOutInputBlockId); > + if (!touchBehaviors.IsEmpty()) { > + mInputQueue->SetAllowedTouchBehavior(*aOutInputBlockId, touchBehaviors); What will trigger processing of the input block? ::: gfx/layers/apz/src/InputQueue.cpp:410 (Diff revision 1) > aBlock->SetContentResponse(false); > } else { > waitForMainThread = true; > } > if (aBlock->AsTouchBlock() && gfxPrefs::TouchActionEnabled()) { > // TODO: once bug 1101628 is fixed, waitForMainThread should only be set If this is the complete patch series for bug 1101628, shouldn't one of the patches remove this comment? :)

Comment on attachment 8757333 [details] MozReview Request: Bug 1101628 - Fix function name and documentation to reflect reality. r=botond https://reviewboard.mozilla.org/r/55814/#review52536

Comment on attachment 8757334 [details] MozReview Request: Bug 1101628 - Refactor snapshotting code to allow reusing it across tests. r=botond https://reviewboard.mozilla.org/r/55816/#review52542 r+ assuming the comment is addressed or a non-issue ::: gfx/layers/apz/test/mochitest/apz_test_utils.js:307 (Diff revision 1) > + ctx.drawWindow(topWin, rect.x, rect.y, rect.w, rect.h, 'rgb(255,255,255)', ctx.DRAWWINDOW_DRAW_VIEW | ctx.DRAWWINDOW_USE_WIDGET_LAYERS | ctx.DRAWWINDOW_DRAW_CARET); > + return canvas.toDataURL(); > + }); > + } > + > + if (typeof getSnapshot.chromeHelper == 'undefined') { So this is like a static variable in the function scope? </jsnewbie> ::: gfx/layers/apz/test/mochitest/apz_test_utils.js:310 (Diff revision 1) > + } > + > + if (typeof getSnapshot.chromeHelper == 'undefined') { > + // This is the first time getSnapshot is being called; do initialization > + getSnapshot.chromeHelper = SpecialPowers.loadChromeScript(parentProcessSnapshot); > + SimpleTest.registerCleanupFunction(function() { getSnapshot.chromeHelper.destroy() }); What happens if a subsequent test wants to take a snapshot too? Might the cleanup function run at the end of the first test, but getSnapshot stay alive so the second test doesn't do initialization? (Or does calling destroy() on getSnapshot.chromeHelper cause it to compare equal to 'undefined'?)

Comment on attachment 8757335 [details] MozReview Request: Bug 1101628 - Add a test to ensure that APZ doesn't need to wait for content to provide event region information. r=botond https://reviewboard.mozilla.org/r/55818/#review52550 ::: gfx/layers/apz/test/mochitest/helper_touch_action_regions.html:21 (Diff revision 1) > +function success(e) { > + success.triggered = true; > +} > + > +// This helper function provides a way for the child process to synchronously > +// check how many touch events the chrome process window has seen go by. This When I first read this, I interpeted "window" as "nsIWindow". Tweak the wording to clarify? ::: gfx/layers/apz/test/mochitest/helper_touch_action_regions.html:76 (Diff revision 1) > + } > + > + if (typeof chromeTouchEventCounter.chromeHelper == 'undefined') { > + // This is the first time getSnapshot is being called; do initialization > + chromeTouchEventCounter.chromeHelper = SpecialPowers.loadChromeScript(chromeProcessCounter); > + SimpleTest.registerCleanupFunction(function() { chromeTouchEventCounter.chromeHelper.destroy() }); The question I asked about the previous patch applies here too. ::: gfx/layers/apz/test/mochitest/helper_touch_action_regions.html:87 (Diff revision 1) > +// Simple wrapper that waits until the chrome process has seen |count| instances > +// of the |eventType| event. Returns true on success, and false if 10 seconds > +// go by without the condition being satisfied. > +function waitFor(eventType, count) { > + var start = Date.now(); > + while (JSON.parse(chromeTouchEventCounter('report'))[eventType] != count) { Is this a busy loop? Should we sleep or something? ::: gfx/layers/apz/test/mochitest/helper_touch_action_regions.html:101 (Diff revision 1) > +function* test(testDriver) { > + // The main part of this test should run completely before the child process' > + // main-thread deals with the touch event, so check to make sure that happens. > + document.body.addEventListener('touchstart', failure, { passive: true }); > + > + // What we want here is to synthesize all of touch events (from this code in all of the ::: gfx/layers/apz/test/mochitest/helper_touch_action_regions.html:164 (Diff revision 1) > + // That's what the chromeTouchEventCounter business is all about. The sync > + // polling looks bad but in practice only ends up needing to poll once or > + // twice before the condition is satisfied, and as an extra precaution we add > + // a time guard so it fails after 10s of polling. > + > + // So, here we go... Thanks for the detailed explanation :) That said, I have a nagging feeling that one day, something in this delicate setup will fail or start working differently, and we'll get timeouts and such... ::: gfx/layers/apz/test/mochitest/helper_touch_action_regions.html:201 (Diff revision 1) > + if (i == 1) { > + // The first touchmove is consumed to get us into the panning state, so > + // no actual panning occurs > + ok(lastSnapshot == snapshot, "Snapshot 1 was the same as baseline"); > + } else { > + ok(lastSnapshot != snapshot, "Snapshot " + i + " was different from the previous one"); I feel like snapshots are a bit of a heavyweight mechanism for testing this. Could we have used the APZ test data to log the async scroll offset instead?

(In reply to Botond Ballo [:botond] from comment #15) > ::: gfx/layers/apz/test/mochitest/helper_touch_action_regions.html:87 > (Diff revision 1) > > +// Simple wrapper that waits until the chrome process has seen |count| instances > > +// of the |eventType| event. Returns true on success, and false if 10 seconds > > +// go by without the condition being satisfied. > > +function waitFor(eventType, count) { > > + var start = Date.now(); > > + while (JSON.parse(chromeTouchEventCounter('report'))[eventType] != count) { > > Is this a busy loop? Should we sleep or something? (I understand that we don't want to advance the child process event loop. But perhaps there's a way to sleep that allows the OS to do other things with the CPU, without advancing the Gecko event loop?)

(In reply to Botond Ballo [:botond] from comment #11) > > - void Sub(const EventRegions& aMinuend, const nsIntRegion& aSubtrahend) > > No one seems to use this; let's axe it (I'm not a big fan of the signature > anyways). The patch *is* deleting these functions. Am I misunderstanding your comment? (In reply to Botond Ballo [:botond] from comment #12) > > + return AllowedTouchBehavior::NONE; > > Should we ever get HitNothing as an input to this function? If not, it may > be better to assert. I think it is possible to get HitNothing as an input, if one of the calls to GetTargetAPZC in GetTouchInputBlockAPZC lands on nothing. GetTouchInputBlockAPZC and its call sites all handle this scenario. > ::: gfx/layers/apz/src/APZCTreeManager.cpp:1025 > > + if (!touchBehaviors.IsEmpty()) { > > + mInputQueue->SetAllowedTouchBehavior(*aOutInputBlockId, touchBehaviors); > > What will trigger processing of the input block? This call to SetAllowedTouchBehavior should trigger processing at [1], assuming there's nothing else the input block is waiting on. [1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/InputQueue.cpp?rev=db473770c2eb#675 > ::: gfx/layers/apz/src/InputQueue.cpp:410 > > if (aBlock->AsTouchBlock() && gfxPrefs::TouchActionEnabled()) { > > // TODO: once bug 1101628 is fixed, waitForMainThread should only be set > > If this is the complete patch series for bug 1101628, shouldn't one of the > patches remove this comment? :) Good point, I'll update that comment in part 2. (In reply to Botond Ballo [:botond] from comment #14) > > + if (typeof getSnapshot.chromeHelper == 'undefined') { > > So this is like a static variable in the function scope? </jsnewbie> Basically, yeah. > ::: gfx/layers/apz/test/mochitest/apz_test_utils.js:310 > > + getSnapshot.chromeHelper = SpecialPowers.loadChromeScript(parentProcessSnapshot); > > + SimpleTest.registerCleanupFunction(function() { getSnapshot.chromeHelper.destroy() }); > > What happens if a subsequent test wants to take a snapshot too? Might the > cleanup function run at the end of the first test, but getSnapshot stay > alive so the second test doesn't do initialization? (Or does calling > destroy() on getSnapshot.chromeHelper cause it to compare equal to > 'undefined'?) getSnapshot itself exists on the global object for the test, so it is replaced with a fresh getSnapshot when the next test starts running. i.e. say we have two tests, test1.html and test2.html. When the mochitest harness loads test1.html, it has a <script src="apz_test_utils.js"> which installs getSnapshot into test1.html's global object. It can then call getSnapshot() and do stuff. When test1.html is done, SimpleTest runs the destroy() function to clean up the chrome helper, and test1.html's global is destroyed, which includes that copy getSnapshot. test2.html starts with a new global object and gets its own copy of getSnapshot. Similarly, if we have two *subtests* inside one of the test_group_* meta-tests, each subtest gets its own copy of getSnapshot. There is one difference in this case, that the "cleanup function" won't get run until all of the subtests are done (because they are all part of a single SimpleTest). So if multiple subtests use getSnapshot, there will be multiple copies of the chromeHelper in memory (still in different global objects) until the meta-test is done, at which point they all get cleaned up. They different chromeHelpers should not interfere with each other, because the 'snapshot' message is sent to the specific chromeHelper attached to the getSnapshot function on the current global. (In reply to Botond Ballo [:botond] from comment #15) > > +// This helper function provides a way for the child process to synchronously > > +// check how many touch events the chrome process window has seen go by. This > > When I first read this, I interpeted "window" as "nsIWindow". Tweak the > wording to clarify? I did s/window has seen go by/main-thread has processed/ > ::: gfx/layers/apz/test/mochitest/helper_touch_action_regions.html:76 > (Diff revision 1) > > + } > > + > > + if (typeof chromeTouchEventCounter.chromeHelper == 'undefined') { > > + // This is the first time getSnapshot is being called; do initialization > > + chromeTouchEventCounter.chromeHelper = SpecialPowers.loadChromeScript(chromeProcessCounter); > > + SimpleTest.registerCleanupFunction(function() { chromeTouchEventCounter.chromeHelper.destroy() }); > > The question I asked about the previous patch applies here too. Ditto. The chromeTouchEventCounter function exists as a property of the global object for helper_touch_action_regions.html, so if a different test wants to do something similar it will happen on a different global object and should be independent. > ::: gfx/layers/apz/test/mochitest/helper_touch_action_regions.html:87 > > + var start = Date.now(); > > + while (JSON.parse(chromeTouchEventCounter('report'))[eventType] != count) { > > Is this a busy loop? Should we sleep or something? > (In reply to Botond Ballo [:botond] from comment #16) > (I understand that we don't want to advance the child process event loop. > But perhaps there's a way to sleep that allows the OS to do other things > with the CPU, without advancing the Gecko event loop?) It is a busy loop, yeah. AFAIK there's no way to sleep in JS without spinning the event loop, which we don't want to do. If there is then we could definitely use it here. > ::: gfx/layers/apz/test/mochitest/helper_touch_action_regions.html:101 > > + // What we want here is to synthesize all of touch events (from this code in > > all of the Fixed. > ::: gfx/layers/apz/test/mochitest/helper_touch_action_regions.html:164 > > + // So, here we go... > > Thanks for the detailed explanation :) > > That said, I have a nagging feeling that one day, something in this delicate > setup will fail or start working differently, and we'll get timeouts and > such... I wouldn't be surprised :/ But we can cross that bridge when we get to it, even if it's just by disabling the test. > ::: gfx/layers/apz/test/mochitest/helper_touch_action_regions.html:201 > (Diff revision 1) > > + if (i == 1) { > > + // The first touchmove is consumed to get us into the panning state, so > > + // no actual panning occurs > > + ok(lastSnapshot == snapshot, "Snapshot 1 was the same as baseline"); > > + } else { > > + ok(lastSnapshot != snapshot, "Snapshot " + i + " was different from the previous one"); > > I feel like snapshots are a bit of a heavyweight mechanism for testing this. > Could we have used the APZ test data to log the async scroll offset instead? Hm, that's an interesting question. I hadn't considered it originally - looking through the code now though I'm not sure it's that easy to do. It looks like the compositor-side APZ test data is logged when we process a layers update message and rebuild the APZC tree. What we need in this test is to ensure that the async scroll offsets are changing over time, and that will happen without layers updates in between. So we'd have to expand the APZ test data mechanism to also log data per-composite in order to use that for this test. It seems like a useful thing to do, and I'll keep it in mind for the next test I write that might need to do something like this. I agree it would be "lighter" than grabbing snapshots.

Comment on attachment 8757331 [details] MozReview Request: Bug 1101628 - Add additional HitResult types to reflect the different event regions. r=botond Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55810/diff/1-2/

Comment on attachment 8757332 [details] MozReview Request: Bug 1101628 - Have the APZCTM tell the input queue what the allowed touch behaviours are, if it has the information. r?botond Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55812/diff/1-2/

Comment on attachment 8757333 [details] MozReview Request: Bug 1101628 - Fix function name and documentation to reflect reality. r=botond Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55814/diff/1-2/

Comment on attachment 8757334 [details] MozReview Request: Bug 1101628 - Refactor snapshotting code to allow reusing it across tests. r=botond Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55816/diff/1-2/

Comment on attachment 8757335 [details] MozReview Request: Bug 1101628 - Add a test to ensure that APZ doesn't need to wait for content to provide event region information. r=botond Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55818/diff/1-2/

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17) > (In reply to Botond Ballo [:botond] from comment #11) > > > - void Sub(const EventRegions& aMinuend, const nsIntRegion& aSubtrahend) > > > > No one seems to use this; let's axe it (I'm not a big fan of the signature > > anyways). > > The patch *is* deleting these functions. Am I misunderstanding your comment? No; apparently I just can't read diffs :p Sorry for the noise. > (In reply to Botond Ballo [:botond] from comment #12) > > > + return AllowedTouchBehavior::NONE; > > > > Should we ever get HitNothing as an input to this function? If not, it may > > be better to assert. > > I think it is possible to get HitNothing as an input, if one of the calls to > GetTargetAPZC in GetTouchInputBlockAPZC lands on nothing. > GetTouchInputBlockAPZC and its call sites all handle this scenario. You're right, I was confused. It's fine as it is. > > ::: gfx/layers/apz/test/mochitest/apz_test_utils.js:310 > > > + getSnapshot.chromeHelper = SpecialPowers.loadChromeScript(parentProcessSnapshot); > > > + SimpleTest.registerCleanupFunction(function() { getSnapshot.chromeHelper.destroy() }); > > > > What happens if a subsequent test wants to take a snapshot too? Might the > > cleanup function run at the end of the first test, but getSnapshot stay > > alive so the second test doesn't do initialization? (Or does calling > > destroy() on getSnapshot.chromeHelper cause it to compare equal to > > 'undefined'?) > > getSnapshot itself exists on the global object for the test, so it is > replaced with a fresh getSnapshot when the next test starts running. i.e. > say we have two tests, test1.html and test2.html. When the mochitest harness > loads test1.html, it has a <script src="apz_test_utils.js"> which installs > getSnapshot into test1.html's global object. It can then call getSnapshot() > and do stuff. When test1.html is done, SimpleTest runs the destroy() > function to clean up the chrome helper, and test1.html's global is > destroyed, which includes that copy getSnapshot. test2.html starts with a > new global object and gets its own copy of getSnapshot. > > Similarly, if we have two *subtests* inside one of the test_group_* > meta-tests, each subtest gets its own copy of getSnapshot. There is one > difference in this case, that the "cleanup function" won't get run until all > of the subtests are done (because they are all part of a single SimpleTest). > So if multiple subtests use getSnapshot, there will be multiple copies of > the chromeHelper in memory (still in different global objects) until the > meta-test is done, at which point they all get cleaned up. They different > chromeHelpers should not interfere with each other, because the 'snapshot' > message is sent to the specific chromeHelper attached to the getSnapshot > function on the current global. Sounds good - thanks for explaining!

Comment on attachment 8757332 [details] MozReview Request: Bug 1101628 - Have the APZCTM tell the input queue what the allowed touch behaviours are, if it has the information. r?botond https://reviewboard.mozilla.org/r/55812/#review53642

Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2e253d272ceb Add additional HitResult types to reflect the different event regions. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/c8011fc5c80b Have the APZCTM tell the input queue what the allowed touch behaviours are, if it has the information. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/274c6d39f963 Fix function name and documentation to reflect reality. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/f06e99369898 Refactor snapshotting code to allow reusing it across tests. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/30cffc598207 Add a test to ensure that APZ doesn't need to wait for content to provide event region information. r=botond

Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f96ca7f043 Follow-up to fix crash in some gtest scenarios. r=me

https://hg.mozilla.org/mozilla-central/rev/2e253d272ceb https://hg.mozilla.org/mozilla-central/rev/c8011fc5c80b https://hg.mozilla.org/mozilla-central/rev/274c6d39f963 https://hg.mozilla.org/mozilla-central/rev/f06e99369898 https://hg.mozilla.org/mozilla-central/rev/30cffc598207 https://hg.mozilla.org/mozilla-central/rev/f9f96ca7f043

Hi Kats! Can this feature be manually verified by QA team? If so, could you please detail on what we should focus our testing? Thank you!

There's not much to test manually in this bug specifically. We'll probably want some manual testing of the touch-action property later, once it's turned on by default. I plan to write some more automated tests for it before I turn it on though.