941774 - Add support for touch input injection for testing purposes

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Jim Mathies [:jimm]]

Microsoft revamped their pointer input apis, and added a nice touch input injection call similar to SendInput -

http://msdn.microsoft.com/en-us/library/windows/desktop/ff657750(v=vs.85).aspx

We should get this hooked up for testing in winrt and win32.

Comment 1

[Jim Mathies [:jimm]]

Attachment: dom window utils v.1

Comment 2

[Jim Mathies [:jimm]]

Attachment: dom window utils v.1

Comment 3

[Jim Mathies [:jimm]]

Attachment: widget scale factor updates v.1

Comment 4

[Jim Mathies [:jimm]]

Attachment: widget scale factor updates v.1

Comment 5

[Jim Mathies [:jimm]]

Attachment: touch inject apis v.1

Comment 6

[Jim Mathies [:jimm]]

Attachment: touch inject apis v.1

Comment 7

[Jim Mathies [:jimm]]

Attachment: basic metro apzc tests v.1

Comment 8

[Jim Mathies [:jimm]]

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=4e39147fe82a

Comment 9

[Jim Mathies [:jimm]]

Attachment: dom window utils v.1

Dom window utils interfaces for injecting touch input, similar to the other native event apis.

Comment 10

[Jim Mathies [:jimm]]

Attachment: widget scale factor updates v.1

Cleaning up some of the dpi/scale info code we have strewn around. I'd like consumers outside of widget going through WinUtils for this info.

Comment 11

[Jim Mathies [:jimm]]

Attachment: widget base changes v.1

Widget interface that dom utils calls, stubbed out.

Comment 12

[Jim Mathies [:jimm]]

Attachment: touch inject apis v.1

Touch injection apis for win32 and winrt widget, implemented in nsWindowBase.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8344792 [details] [diff] [review]
widget base changes v.1

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

::: widget/nsIWidget.h
@@ +1602,5 @@
> +     * ui.click_hold_context_menus.delay pref. (This duration is compatible
> +     * with the default apzc long tap duration.) 
> +     */
> +    virtual nsresult SynthesizeNativeTouchTap(nsIntPoint aPointerScreenPoint,
> +                                              bool aLongTap) = 0;

Why is this pure virtual? Can't this be a helper method somewhere that just calls SynthesizeNativeTouchPoint?

Comment 14

[Jim Mathies [:jimm]]

> Why is this pure virtual? Can't this be a helper method somewhere that just
> calls SynthesizeNativeTouchPoint?

Either way would work I suppose. I implemented this for Windows using windows timer events. I could do something down in base widget using xpcom timers too I think. I could argue that Windows is probably the only platform this will ever support so a Windows specific implementation should suffice. (But that's probably just me trying to avoid reworking this in base widget.:)

Comment 15

[Brian R. Bondy [:bbondy]]

Comment on attachment 8344791 [details] [diff] [review]
widget scale factor updates v.1

Per IRC new patch is coming

Comment 16

[Brian R. Bondy [:bbondy]]

Comment on attachment 8344793 [details] [diff] [review]
touch inject apis v.1

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

I think a new patch is coming here too, just re-request if not though

Comment 17

[Jim Mathies [:jimm]]

Attachment: widget scale factor updates v.2

Cleaned up the float/FLOAT/double mismatches.

Comment 18

[Jim Mathies [:jimm]]

Attachment: widget base changes v.2

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 8345924 [details] [diff] [review]
widget base changes v.2

Moved long tap logic down into base widget.

Comment 20

[Jim Mathies [:jimm]]

Attachment: windows touch inject apis v.2

win32/winrt widget impl.

Comment 21

[Brian R. Bondy [:bbondy]]

Comment on attachment 8345923 [details] [diff] [review]
widget scale factor updates v.2

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

::: widget/windows/WinUtils.cpp
@@ +205,5 @@
> +double
> +WinUtils::PhysToLogFactor()
> +{
> +  // 1.0 / (dpi / 96.0)
> +  return 1.0f / LogToPhysFactor();

nit: just 1.0

::: widget/windows/winrt/MetroUtils.cpp
@@ +64,5 @@
> +
> +double
> +MetroUtils::PhysToLogFactor()
> +{
> +  return 1.0f / LogToPhysFactor();

nit: Just 1.0

@@ +96,2 @@
>  
> +  return 1.0f;

nit: no f

Comment 22

[Brian R. Bondy [:bbondy]]

Comment on attachment 8345928 [details] [diff] [review]
windows touch inject apis v.2

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

::: widget/windows/nsWindowBase.cpp
@@ +13,5 @@
>  using namespace mozilla;
> +using namespace mozilla::widget;
> +
> +static const wchar_t kUser32LibName[] =  L"user32.dll";
> +bool nsWindowBase::sTouchInjectInitialized;

nit: I think this will be initialized to false automatically but might as well put it explicitly.

@@ +39,5 @@
> +bool
> +nsWindowBase::InitTouchInjection()
> +{
> +  if (!sTouchInjectInitialized) {
> +    // Initialize touch injection on the first call 

nit: trailing space here and elsewhere

@@ +40,5 @@
> +nsWindowBase::InitTouchInjection()
> +{
> +  if (!sTouchInjectInitialized) {
> +    // Initialize touch injection on the first call 
> +    HMODULE hMod = LoadLibraryW(kUser32LibName);

nit: unmatched FreeLibrary even know it's only called once.

@@ +77,5 @@
> +  POINTER_TOUCH_INFO info;
> +  memset(&info, 0, sizeof(POINTER_TOUCH_INFO));
> +
> +  info.touchFlags = TOUCH_FLAG_NONE; 
> +  info.touchMask = TOUCH_MASK_CONTACTAREA|TOUCH_MASK_ORIENTATION|TOUCH_MASK_PRESSURE; 

nit: space before and after |

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8345924 [details] [diff] [review]
widget base changes v.2

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

::: widget/nsIWidget.h
@@ +1602,5 @@
> +     * ui.click_hold_context_menus.delay. This pref is compatible with the
> +     * apzc long tap duration. Defaults to 1.5 seconds.
> +     */
> +    virtual nsresult SynthesizeNativeTouchTap(nsIntPoint aPointerScreenPoint,
> +                                              bool aLongTap) = 0;

Make this nonvirtual and just move the implementation to nsIWidget.

::: widget/xpwidgets/nsBaseWidget.cpp
@@ +1620,5 @@
> +  if (!mLongTapTimer) {
> +    return NS_OK;
> +  }
> +  mLongTapTimer->Cancel();
> +  mLongTapTimer = nullptr;

Please add nsIWidget's destructor something that cancels mLongTapTimer.

Comment 24

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> Comment on attachment 8345924 [details] [diff] [review]
> widget base changes v.2
> 
> Review of attachment 8345924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/nsIWidget.h
> @@ +1602,5 @@
> > +     * ui.click_hold_context_menus.delay. This pref is compatible with the
> > +     * apzc long tap duration. Defaults to 1.5 seconds.
> > +     */
> > +    virtual nsresult SynthesizeNativeTouchTap(nsIntPoint aPointerScreenPoint,
> > +                                              bool aLongTap) = 0;
> 
> Make this nonvirtual and just move the implementation to nsIWidget.

Is there a reason why you want this in nsIWidget vs. nsBaseWidget? Seems odd that we would put functionality in the interface. Plus it means we have to add a bunch of clutter to nsIWidget.h, which gets included everywhere.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We don't need to put it inline in nsIWidget.h file. Leave it in nsBaseWidget.cpp.

In an ideal world we'd merge the nsBaseWidget and nsIWidget classes.

Comment 26

[Jim Mathies [:jimm]]

Attachment: widget base changes v.3

Comment 27

[Jim Mathies [:jimm]]

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=bb51fd1c808e

Comment 28

[Jim Mathies [:jimm]]

> @@ +40,5 @@
> > +nsWindowBase::InitTouchInjection()
> > +{
> > +  if (!sTouchInjectInitialized) {
> > +    // Initialize touch injection on the first call 
> > +    HMODULE hMod = LoadLibraryW(kUser32LibName);
> 
> nit: unmatched FreeLibrary even know it's only called once.

Not worried about this, it's user32. Windows ignores the library ref count when the process terminates.

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f861bf92cb7d
https://hg.mozilla.org/mozilla-central/rev/20bc4777d26a
https://hg.mozilla.org/mozilla-central/rev/bd9d5c318301
https://hg.mozilla.org/mozilla-central/rev/f6cf0e5d1573

Comment 30

[Jim Mathies [:jimm]]

Comment on attachment 8344788 [details] [diff] [review]
dom window utils v.1

[Approval Request Comment]

Bug caused by (feature/regressing bug #):

There's no bug associated with this work. Patches provide a way to simulate touch input for apzc related tests. We want these running on aurora.

User impact if declined: 

Missing test coverage on Aurora, Beta, and Release.

Testing completed (on m-c, etc.): 

Tests are green on mc.

Risk to taking this patch (and alternatives if risky): 

Low.

String or IDL/UUID changes made by this patch:

None


This request applies to the four patches in this bug.

Comment 31

[Matt Brubeck (:mbrubeck)]

These patches include an IID change in nsIWidget.h.  Are IID changes allowed on Aurora?

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Matt Brubeck (:mbrubeck) from comment #31)
> These patches include an IID change in nsIWidget.h.  Are IID changes allowed
> on Aurora?

Yes, we can take IID changes on Aurora, not on Beta.

Comment 33

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6886eaa42cac
https://hg.mozilla.org/releases/mozilla-aurora/rev/d4f6385acfff
https://hg.mozilla.org/releases/mozilla-aurora/rev/ff02fb4d8474
https://hg.mozilla.org/releases/mozilla-aurora/rev/f4bc4144c7e4