Closed Bug 1122090 Opened 5 years ago Closed 5 years ago

Have widget code send APZCTreeManager::SetAllowedTouchBehavior notifications as needed

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files, 2 obsolete files)

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.
Whiteboard: [gfx-noted]
Attached patch allowed_behavior_ver1.diff (obsolete) — Splinter Review
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.
Attachment #8571944 - Flags: feedback?(bugs)
Attachment #8571944 - Flags: feedback?(bugmail.mozilla)
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
Attachment #8571944 - Flags: feedback?(bugs)
Attachment #8571944 - Flags: feedback?(bugmail.mozilla)
Attachment #8571944 - Flags: feedback-
Attached patch allowed_behavior_ver2.diff (obsolete) — Splinter Review
- Removed previous proposed idea
+ Added calculating behavior through Widget->APZCTreeManager::SetAllowedTouchBehavior
Attachment #8571944 - Attachment is obsolete: true
Attachment #8572660 - Flags: feedback?(bugmail.mozilla)
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.
Attachment #8572660 - Flags: feedback?(bugmail.mozilla) → feedback+
Comment on attachment 8572660 [details] [diff] [review]
allowed_behavior_ver2.diff

dvander should probably look at this too (see also bug 736048)
Attachment #8572660 - Flags: feedback?(dvander)
Comment on attachment 8572660 [details] [diff] [review]
allowed_behavior_ver2.diff

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

Seems reasonable.
Attachment #8572660 - Flags: feedback?(dvander) → feedback+
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.
Attachment #8578771 - Flags: review?(botond)
Assignee: nobody → bugmail.mozilla
Attachment #8572660 - Attachment is obsolete: true
Attachment #8578772 - Flags: review?(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?
Attachment #8578771 - Flags: review?(botond) → review+
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).
Attachment #8578772 - Flags: review?(botond) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/d1f19152d5f8
https://hg.mozilla.org/mozilla-central/rev/01abf38a6a4d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.