1143618 - Change Window::OnTouch implementation to use MultiTouchInput class

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Maksim Lebedev]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150305021524




Expected results:

Using MultiTouchInput is more simplier than using WidgetTouchEvent.
Implementation does not contain "new" and "delete" operators.

Comment 1

[Maksim Lebedev]

Attachment: ontouch_implementation_ver1.diff

Comment 2

[Maksim Lebedev]

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

Comment 3

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

Comment on attachment 8577990 [details] [diff] [review]
ontouch_implementation_ver1.diff

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

So based on my reading of the original code it's possible for a single call to OnTouch to send *both* a start/move event and an up event because some of the pInputs[i] might have the TOUCHEVENTF_MOVE flag set and some might have the TOUCHEVENTF_UP flag set. Your modifications to this code don't deal with that case - all of the touch points will get added to the same MultiTouchInput regardless of what kind they are, and the type of the last touch point encountered will determine the type of the MultiTouchInput.

Comment 4

[Maksim Lebedev]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> So based on my reading of the original code it's possible for a single call
> to OnTouch to send *both* a start/move event and an up event because some of
> the pInputs[i] might have the TOUCHEVENTF_MOVE flag set and some might have
> the TOUCHEVENTF_UP flag set.
Is it realy possible?

Comment 5

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

I don't know, but that's what it looks like from the code. You can check the API documentation or look at the code history to figure it out.

Comment 6

[Maksim Lebedev]

https://msdn.microsoft.com/en-us/library/windows/desktop/dd317334(v=vs.85).aspx
> TOUCHEVENTF_DOWN - Cannot be combined with TOUCHEVENTF_MOVE or TOUCHEVENTF_UP.
> TOUCHEVENTF_MOVE - Cannot be combined with TOUCHEVENTF_DOWN.
This info about TOUCHINPUT structure

Comment 7

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

So you can still have TOUCHEVENTF_MOVE | TOUCHEVENTF_UP. Also it looks like the documentation you quoted applies to a single touchevent, but we get notifications regarding multiple touch events in a single OnTouch call - what if one touch event is TOUCHEVENTF_MOVE and another is TOUCHEVENTF_UP?

That being said, maybe we can hold off on making this change. dvander is cleaning up some of the APIs we use to talk to the APZ in bug 1143567 and right now all of his code is using Widget*Event. Eventually I do want to transition it to InputData subclasses but we can do that later too, since it's not particularly urgent.

Comment 8

[Maksim Lebedev]

Attachment: ontouch_implementation_ver2.diff

+ Changes: Keeps ability to dispatch more than one events

Comment 9

[Maksim Lebedev]

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

Comment 10

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

Comment on attachment 8578604 [details] [diff] [review]
ontouch_implementation_ver2.diff

I think I'd prefer if kats says when we should actually do this change, given all the work happening with APZ.

Comment 11

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

Comment on attachment 8578604 [details] [diff] [review]
ontouch_implementation_ver2.diff

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

I'm fine with making this change in theory (since it's something we'll want to do eventually, and shouldn't hurt to do it now) but it's buggy. Maksim, please keep in mind also that you probably don't need this change any more to continue working on pointer-events. The patches I've posted in other bugs should be sufficient.

::: widget/windows/nsWindow.cpp
@@ +6161,3 @@
>          }
> +        addToNormalEvent = false;
> +      } else if (pInputs[i].dwFlags & TOUCHEVENTF_DOWN | TOUCHEVENTF_MOVE) {

So if you have an input event with a single touch point that has TOUCHEVENTF_UP and TOUCHEVENTF_MOVE both set, it will get treated as MULTITOUCH_END, and the move will get ignored. And regardless, this check is wrong because & takes higher precedence over |

@@ +6170,5 @@
> +          // Pres shell expects this event to be a NS_TOUCH_START
> +          // if new contact points have been added since the last event sent.
> +          if(pInputs[i].dwFlags & TOUCHEVENTF_DOWN) {
> +            touchInput.mType = MultiTouchInput::MULTITOUCH_START;
> +          }

This if condition being inside the mTimeStamp.IsNull() check means that the input will only end up as MULTITOUCH_START if the *first* touch point is a DOWN. Instead it should be a MULTITOUCH_START if *any* touch point is a DOWN.

Comment 12

[Maksim Lebedev]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> Maksim, please keep in mind also that you probably don't need this change any more
> to continue working on pointer-events.
Our internal build passed the most part of tests.
All that I want is to provide correct patches with suggestion and comments from community.

Comment 13

[Maksim Lebedev]

Attachment: ontouch_implementation_ver3.diff

+ Changes: In case combined MOVE and UP we send two events
+ Changes: In case any new point we send START event
- Changes: Removed errors according with comments

Comment 14

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

Comment on attachment 8581550 [details] [diff] [review]
ontouch_implementation_ver3.diff

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

r=me with comment addressed. Please also make sure you have a green try run.

::: widget/windows/nsWindow.cpp
@@ +6135,5 @@
>  
> +// N.B.: According with MS documentation
> +// https://msdn.microsoft.com/en-us/library/windows/desktop/dd317334(v=vs.85).aspx
> +// TOUCHEVENTF_DOWN cannot be combined with TOUCHEVENTF_MOVE or TOUCHEVENTF_UP.
> +// Possibly, it means that TOUCHEVENTF_MOVE and TOUCHEVENTF_UP can be combined together.

Move this comment down to above the if statement where you check for _DOWN | _MOVE

Comment 15

[Maksim Lebedev]

Attachment: ontouch_implementation_ver4.diff

+ Moved comment to "if" condition
+ Moved MULTITOUCH_INPUT type initialization into c-tor MultiTouchInput

Comment 16

[Maksim Lebedev]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68ddedd7d8e8

Comment 17

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

Comment on attachment 8582323 [details] [diff] [review]
ontouch_implementation_ver4.diff

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

::: widget/InputData.h
@@ +81,5 @@
>    INPUTDATA_AS_CHILD_TYPE(ScrollWheelInput, SCROLLWHEEL_INPUT)
>  
>    InputData()
> +    : mTime(0),
> +      modifiers(0)

Hm, I'd rather do this:

-  InputData()
+  InputData(InputType aInputType)
+    : mInputType(aInputType),
+      mTime(0),
+      modifiers(0)

and change the MultiTouchInput() constructor to be like this:

   MultiTouchInput()
+    : InputData(MULTITOUCH_INPUT)
   {
   }

I don't think we ever use the InputData() constructor directly anywhere and it doesn't even make sense to have an InputData object with no mInputType.

Comment 18

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

If you make the above change please include a try run with builds across all platforms. Something like this would be good:

try: -b do -p all -u all[Windows 8,x64,b2g] -t none

Comment 19

[Maksim Lebedev]

Attachment: ontouch_implementation_ver5.diff

+ Changed InputData constructor

Comment 20

[Maksim Lebedev]

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

Comment 21

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

Comment on attachment 8583079 [details] [diff] [review]
ontouch_implementation_ver5.diff

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

Thanks.

Comment 22

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

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

Comment 23

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

Follow-up to fix static analysis build

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f4090384ba4

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dcd2f23cf9bc
https://hg.mozilla.org/mozilla-central/rev/5f4090384ba4