Add support for touch input injection for testing purposes

RESOLVED FIXED in Firefox 28

Status

()

defect
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

26 Branch
mozilla28
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 fixed, firefox29 fixed)

Details

(Whiteboard: [beta28] p=8 [qa-])

Attachments

(4 attachments, 11 obsolete attachments)

9.38 KB, patch
smaug
: review+
Details | Diff | Splinter Review
20.74 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
13.07 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
12.54 KB, patch
Details | Diff | Splinter Review
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.
Depends on: 941324
OS: Windows 7 → Windows 8.1
Blocks: 945765
Posted patch dom window utils v.1 (obsolete) — Splinter Review
Assignee: nobody → jmathies
Posted patch dom window utils v.1 (obsolete) — Splinter Review
Attachment #8344586 - Attachment is obsolete: true
Attachment #8344588 - Attachment is obsolete: true
Posted patch touch inject apis v.1 (obsolete) — Splinter Review
Posted patch touch inject apis v.1 (obsolete) — Splinter Review
Attachment #8344591 - Attachment is obsolete: true
Posted patch basic metro apzc tests v.1 (obsolete) — Splinter Review
Attachment #8344601 - Attachment is obsolete: true
Blocks: metrov1it21
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [beta28] p=0
Whiteboard: [beta28] p=0 → [beta28] p=8
Dom window utils interfaces for injecting touch input, similar to the other native event apis.
Attachment #8344587 - Attachment is obsolete: true
Attachment #8344788 - Flags: review?(bugs)
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.
Attachment #8344590 - Attachment is obsolete: true
Attachment #8344791 - Flags: review?(netzen)
Posted patch widget base changes v.1 (obsolete) — Splinter Review
Widget interface that dom utils calls, stubbed out.
Attachment #8344792 - Flags: review?(roc)
Posted patch touch inject apis v.1 (obsolete) — Splinter Review
Touch injection apis for win32 and winrt widget, implemented in nsWindowBase.
Attachment #8344600 - Attachment is obsolete: true
Attachment #8344793 - Flags: review?(netzen)
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?
> 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.:)
Attachment #8344788 - Flags: review?(bugs) → review+
Blocks: 947146
Comment on attachment 8344791 [details] [diff] [review]
widget scale factor updates v.1

Per IRC new patch is coming
Attachment #8344791 - Flags: review?(netzen)
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
Attachment #8344793 - Flags: review?(netzen)
Cleaned up the float/FLOAT/double mismatches.
Attachment #8344791 - Attachment is obsolete: true
Attachment #8345923 - Flags: review?(netzen)
Posted patch widget base changes v.2 (obsolete) — Splinter Review
Attachment #8344792 - Attachment is obsolete: true
Attachment #8344792 - Flags: review?(roc)
Comment on attachment 8345924 [details] [diff] [review]
widget base changes v.2

Moved long tap logic down into base widget.
Attachment #8345924 - Flags: review?(roc)
win32/winrt widget impl.
Attachment #8344793 - Attachment is obsolete: true
Attachment #8345928 - Flags: review?(netzen)
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
Attachment #8345923 - Flags: review?(netzen) → review+
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 |
Attachment #8345928 - Flags: review?(netzen) → review+
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.
Attachment #8345924 - Flags: review?(roc) → review+
(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.
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.
Attachment #8345924 - Attachment is obsolete: true
> @@ +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 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.
Attachment #8344788 - Flags: approval-mozilla-aurora?
Attachment #8344788 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
These patches include an IID change in nsIWidget.h.  Are IID changes allowed on Aurora?
Flags: needinfo?(lsblakk)
(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.
Flags: needinfo?(lsblakk)
Target Milestone: mozilla29 → mozilla28
Whiteboard: [beta28] p=8 → [beta28] p=8 [qa-]
You need to log in before you can comment on or make changes to this bug.