Change Window::OnTouch implementation to use MultiTouchInput class

RESOLVED FIXED in Firefox 39

Status

()

Core
Widget: Win32
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Maksim Lebedev, Assigned: Maksim Lebedev)

Tracking

Trunk
mozilla39
x86_64
Windows 8
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 736048
(Assignee)

Comment 1

3 years ago
Created attachment 8577990 [details] [diff] [review]
ontouch_implementation_ver1.diff
Attachment #8577990 - Flags: review?(bugs)
Attachment #8577990 - Flags: review?(bugmail.mozilla)
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.
Attachment #8577990 - Flags: review?(bugs)
Attachment #8577990 - Flags: review?(bugmail.mozilla)
Attachment #8577990 - Flags: review-
Component: Untriaged → Widget: Win32
Product: Firefox → Core
(Assignee)

Comment 4

3 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
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.
(Assignee)

Comment 6

3 years ago
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
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.
(Assignee)

Comment 8

3 years ago
Created attachment 8578604 [details] [diff] [review]
ontouch_implementation_ver2.diff

+ Changes: Keeps ability to dispatch more than one events
Attachment #8577990 - Attachment is obsolete: true
Attachment #8578604 - Flags: review?(bugs)
Attachment #8578604 - Flags: review?(bugmail.mozilla)
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.
Attachment #8578604 - Flags: review?(bugs)
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.
Attachment #8578604 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 12

3 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(Assignee)

Comment 13

3 years ago
Created attachment 8581550 [details] [diff] [review]
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
Attachment #8578604 - Attachment is obsolete: true
Attachment #8581550 - Flags: review?(bugmail.mozilla)
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
Attachment #8581550 - Flags: review?(bugmail.mozilla) → review+
Assignee: nobody → alessarik
(Assignee)

Comment 15

3 years ago
Created attachment 8582323 [details] [diff] [review]
ontouch_implementation_ver4.diff

+ Moved comment to "if" condition
+ Moved MULTITOUCH_INPUT type initialization into c-tor MultiTouchInput
Attachment #8581550 - Attachment is obsolete: true
Attachment #8582323 - Flags: review?(bugmail.mozilla)
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.
Attachment #8582323 - Flags: review?(bugmail.mozilla)
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
(Assignee)

Comment 19

3 years ago
Created attachment 8583079 [details] [diff] [review]
ontouch_implementation_ver5.diff

+ Changed InputData constructor
Attachment #8582323 - Attachment is obsolete: true
Attachment #8583079 - Flags: review?(bugmail.mozilla)
Comment on attachment 8583079 [details] [diff] [review]
ontouch_implementation_ver5.diff

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

Thanks.
Attachment #8583079 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/dcd2f23cf9bc
https://hg.mozilla.org/mozilla-central/rev/5f4090384ba4
Status: UNCONFIRMED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.