1122090 - Have widget code send APZCTreeManager::SetAllowedTouchBehavior notifications as needed

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

In order to finish off the touch-action support in APZ, we need to make sure all the "widget" pieces of code to call SetAllowedTouchBehaviour as needed, for the cases where the touch lands in a dispatch-to-content region. There is some documentation at https://github.com/mozilla/gecko-dev/blob/master/gfx/doc/AsyncPanZoom.md#technical-details that describes the expected flow here.

There is already some code on Metro that sort of implements this, see the code in widget/ContentHelper.cpp and the code that uses it. We just need to clean it up, perhaps reorganize it to more easily use it across platforms, test it, and so on.

Comment 1

[Maksim Lebedev]

Attachment: allowed_behavior_ver1.diff

Main idea which I can propose is detect allowed behavior immediately after creating TouchBlockState via AsyncPanZoomController->ChromeProcessController(GeckoContentController)->ContentHelper
(In case ContentHelper correct work with different widgets).
In other case (or for safety work) we can add one more element:
AsyncPanZoomController->ChromeProcessController(GeckoContentController)->
->nsWindow(nsIWidget)->ContentHelper.

Comment 2

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

Comment on attachment 8571944 [details] [diff] [review]
allowed_behavior_ver1.diff

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

There is already a mechanism provided to have the widget code notify the APZ about the AllowedTouchBehavior. Please see comment 0. There is no need to have InputQueue call back into the widget. And in fact, this approach has all sorts of threading problems because the input block is handled on the controller thread which is not the same as the main thread on all platforms, and the ContentHelper code must be run on the main thread. What you should be doing instead is invoke the ContentHelper stuff at [1] and then call APZCTreeManager::SetAllowedTouchBehaviour.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp?rev=b28fd46a1174#1003

Comment 3

[Maksim Lebedev]

Attachment: allowed_behavior_ver2.diff

- Removed previous proposed idea
+ Added calculating behavior through Widget->APZCTreeManager::SetAllowedTouchBehavior

Comment 4

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

Comment on attachment 8572660 [details] [diff] [review]
allowed_behavior_ver2.diff

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

This is much better, thanks.

::: widget/windows/nsWindow.cpp
@@ +6273,5 @@
>        if (result == nsEventStatus_eConsumeNoDefault) {
>          return true;
>        }
>  
> +      static bool touchActionEnabled = Preferences::GetBool("layout.css.touch_action.enabled", false);;

use gfxPrefs::TouchActionEnabled() and inline it into the if condition, since it takes care of caching already.

@@ +7592,5 @@
> +void
> +nsWindow::GetAllowedTouchBehavior(WidgetGUIEvent* aEvent, nsTArray<TouchBehaviorFlags>& aOutBehaviors)
> +{
> +  if (WidgetTouchEvent* touchEvent = aEvent->AsTouchEvent()) {
> +    mAPZC->GetAllowedTouchBehavior(touchEvent, aOutBehaviors);

Remove this call (see bug 1122094). Instead just loop over aEvent->touches[i].Length() and add a behavior for that touch point to aOutBehaviors unconditionally.

Comment 5

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

Comment on attachment 8572660 [details] [diff] [review]
allowed_behavior_ver2.diff

dvander should probably look at this too (see also bug 736048)

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8572660 [details] [diff] [review]
allowed_behavior_ver2.diff

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

Seems reasonable.

Comment 7

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

Attachment: Part 1 - s/nsIntPoint/LayoutDeviceIntPoint/

The Metro code is likely broken in the current state because mRefPoint is already a LayoutDeviceIntPoint, I didn't build it to check that though.

Comment 8

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

Attachment: Part 2 - Send allowed touch behavior notification to APZ when touch-action is enabled

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0397efd25225

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8578771 [details] [diff] [review]
Part 1 - s/nsIntPoint/LayoutDeviceIntPoint/

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

::: widget/ContentHelper.cpp
@@ -68,5 @@
>      }
>    }
>  }
>  
>  ContentHelper::TouchBehaviorFlags

Unrelated, but can we pick one of ContentHelper::TouchBehaviorFlags and the TouchBehaviorFlags typedef in APZUtils.h, and use it in place of both?

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8578772 [details] [diff] [review]
Part 2 - Send allowed touch behavior notification to APZ when touch-action is enabled

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

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +672,5 @@
> +        uint64_t aInputBlockId,
> +        const nsRefPtr<SetAllowedTouchBehaviorCallback>& aCallback)
> +{
> +  nsTArray<TouchBehaviorFlags> flags;
> +  for (uint32_t i = 0; i < aEvent.touches.Length(); i++) {

Note that we can now do:

  for (const auto& touch : aEvent.touches) {
      flags.AppendElement(widget::ContentHelper::GetAllowedTouchBehavior(aWidget, touch->mRefPoint));
  }

(or replace 'auto' with 'nsRefPtr<dom::Touch>', as you prefer).

Comment 12

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

(In reply to Botond Ballo [:botond] from comment #10)
> Unrelated, but can we pick one of ContentHelper::TouchBehaviorFlags and the
> TouchBehaviorFlags typedef in APZUtils.h, and use it in place of both?

Filed bug 1145085 for this.

(In reply to Botond Ballo [:botond] from comment #11)
> Note that we can now do:
> 
>   for (const auto& touch : aEvent.touches) {

I'll keep that in mind for the next time I touch this code (and for future code) but I don't want to do another try push now to make sure it builds everywhere :|

Landed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f19152d5f8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/01abf38a6a4d

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d1f19152d5f8
https://hg.mozilla.org/mozilla-central/rev/01abf38a6a4d