Closed
Bug 1143618
Opened 10 years ago
Closed 10 years ago
Change Window::OnTouch implementation to use MultiTouchInput class
Categories
(Core :: Widget: Win32, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: alessarik, Assigned: alessarik)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•10 years ago
|
||
Attachment #8577990 -
Flags: review?(bugs)
Attachment #8577990 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 2•10 years ago
|
||
Comment 3•10 years ago
|
||
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-
Updated•10 years ago
|
Component: Untriaged → Widget: Win32
Product: Firefox → Core
Updated•10 years ago
|
Assignee | ||
Comment 4•10 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?
Comment 5•10 years ago
|
||
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•10 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
Comment 7•10 years ago
|
||
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•10 years ago
|
||
+ 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)
Assignee | ||
Comment 9•10 years ago
|
||
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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•10 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•10 years ago
|
||
+ 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 14•10 years ago
|
||
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+
Updated•10 years ago
|
Assignee: nobody → alessarik
Assignee | ||
Comment 15•10 years ago
|
||
+ 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)
Assignee | ||
Comment 16•10 years ago
|
||
Comment 17•10 years ago
|
||
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)
Comment 18•10 years ago
|
||
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•10 years ago
|
||
+ Changed InputData constructor
Attachment #8582323 -
Attachment is obsolete: true
Attachment #8583079 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 20•10 years ago
|
||
Comment 21•10 years ago
|
||
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+
Comment 22•10 years ago
|
||
landing |
Comment 23•10 years ago
|
||
Follow-up to fix static analysis build
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f4090384ba4
Comment 24•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dcd2f23cf9bc
https://hg.mozilla.org/mozilla-central/rev/5f4090384ba4
Status: UNCONFIRMED → RESOLVED
Closed: 10 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.
Description
•