Closed Bug 978679 Opened 10 years ago Closed 9 years ago

GTK3 touch event support

Categories

(Core :: Widget: Gtk, enhancement)

Product:
Core
Component:
Widget: Gtk
Platform:
x86
Linux
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: m_kato, Assigned: roc)

References

(Depends on 1 open bug,
URL
)

Details

Attachments

(3 files, 8 obsolete files)

Fix v2
17.95 KB, patch
karlt
: review-
Details | Diff | Splinter Review
MozReview Request: Bug 978679 - Implement touch events for GTK3. r=karlt
40 bytes, text/x-review-board-request
Details
MozReview Request: Bug 978679. Convert GDK touch event coordinates properly. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
Attached patch touch event for GTK3 (obsolete) — — Splinter Review 
      No description provided.
Comment on attachment 8384470 [details] [diff] [review]
touch event for GTK3

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

Hey Makoto,

thanks for your patch!

For it to be commited, it needs review first. You need to set an appropriate reviewer yourself, as Bugzilla flag in the details of the attachment.
You find an appropriate reviewer by either looking at the module owners list <https://wiki.mozilla.org/Modules/All> or by looking the commit log of the file you changed <http://hg.mozilla.org/mozilla-central/annotate/4cb766685b73/widget/gtk/nsLookAndFeel.cpp>.
In this case, roc is owner and karlt is peer.
Attachment #8384470 - Flags: review?(karlt)
(In reply to Ben Bucksch (:BenB) from comment #1)
> Comment on attachment 8384470 [details] [diff] [review]
> touch event for GTK3
> 
> Review of attachment 8384470 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey Makoto,
> 
> thanks for your patch!
> 
> For it to be commited, it needs review first. You need to set an appropriate
> reviewer yourself, as Bugzilla flag in the details of the attachment.
> You find an appropriate reviewer by either looking at the module owners list
> <https://wiki.mozilla.org/Modules/All> or by looking the commit log of the
> file you changed
> <http://hg.mozilla.org/mozilla-central/annotate/4cb766685b73/widget/gtk/
> nsLookAndFeel.cpp>.
> In this case, roc is owner and karlt is peer.

Thanks for setting review flag.
Comment on attachment 8384470 [details] [diff] [review]
touch event for GTK3

This fix isn't formatted for diff -U8.  So I will rebase this.
Attachment #8384470 - Flags: review?(karlt)
Attached patch GTK3 touch event support (obsolete) — — Splinter Review 

        Attachment #8384470 -
        Attachment is obsolete: true

        Attachment #8385026 -
        Flags: review?(karlt)

    
    

    
  
Comment on attachment 8385026 [details] [diff] [review]
GTK3 touch event support

>+#if defined(XP_WIN) || (defined(MOZ_WIDGET_GTK) && MOZ_WIDGET_GTK == 3)

Undefined preprocessor symbols evaluate to 0 IIRC and so this can be 

#if defined(XP_WIN) || MOZ_WIDGET_GTK == 3

and similarly in TouchEvent::PrefEnabled().

>+#ifdef MOZ_WIDGET_GTK
>+#ifndef MOZ_WIDGET_GTK2
>+pref("dom.w3c_touch_events.enabled", 2);

#if MOZ_WIDGET_GTK == 3

>+        LayoutDeviceIntPoint point(NSToIntFloor(aEvent->x_root),
>+                                   NSToIntFloor(aEvent->y_root));
>+        touchPoint =
>+            LayoutDevicePixel::ToUntyped(
>+                point -
>+                LayoutDeviceIntPoint::FromUntyped(WidgetToScreenOffset()));

No need to use LayoutDeviceIntPoint as the destination on Touch is nsIntPoint.

>+        new mozilla::dom::Touch((int32_t)(intptr_t)aEvent->sequence,
>+                                touchPoint, nsIntPoint(1,1), 0.0f, 0.0f);

(I don't know why GDK gave us a pointer here.)

>+    // If the event was consumed, return.
>+    if (status == nsEventStatus_eConsumeNoDefault) {
>+        return TRUE;
>+    }
>+
>+    return FALSE;

Returning false here will result in the dispatch of old-style pointer events,
I expect.  Is this the right thing to do here?
The code in widget/windows/nsWindow.cpp doesn't do this, so perhaps
nsEventStateManager or something might already dispatch old-style events.

        Attachment #8385026 -
        Flags: review?(karlt)

        Attachment #8385026 -
        Flags: review-

        Attachment #8385026 -
        Flags: feedback+

    
    

    
  
(In reply to Karl Tomlinson (:karlt, back May 5) from comment #5)
> Comment on attachment 8385026 [details] [diff] [review]
> GTK3 touch event support

> Returning false here will result in the dispatch of old-style pointer events,
> I expect.  Is this the right thing to do here?
> The code in widget/windows/nsWindow.cpp doesn't do this, so perhaps
> nsEventStateManager or something might already dispatch old-style events.

When content has ontouch* event, nsIWidget::RegisterTouchWindow() is called.  So Windows widget handles WM_TOUCH during this.  So it can return TRUE only.  If not using ontouch*, RegisterTouchWindow isn't called, so WM_TOUCH isn't posted from OS.

So we should implement this method on GTK3, then we should be ignore on UnregisterTouchWindow.

    
  
Blocks: 1034064

    
  
No longer blocks: 1034064

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix v2
        — 
          — Splinter Review
      

    


  
Use RegisterTouchWindow and generate touch id per touch event.

        Attachment #8385026 -
        Attachment is obsolete: true

        Attachment #8463253 -
        Flags: review?(karlt)

    
    

    
  
Comment on attachment 8463253 [details] [diff] [review]
Fix v2

I'm not familiar with touch events and so I need answers to a number of
questions in order to fully review this.  For some of these, I think there
should be additional comments to explain in the code.  Others can probably
just be answered in this bug.

What triggers the default actions for touch events, sending mouse and button
events when appropriate?
(AFAIS widget/windows code looks like it sends mouse events as well as touch
events, so I don't know what prevents that, when the default is prevented at
least.)

https://bugzilla.mozilla.org/show_bug.cgi?id=798821#c16 says the auto pref
value is ignoring the possibility of attaching/detaching devices.  There
should be a comment at the code that is affected by this.

IsTouchDeviceSupportPresent() looks only for GDK_SOURCE_TOUCHSCREEN devices.
Why not GDK_SOURCE_TOUCHPAD devices?

Touch events are also received from GDK_SOURCE_TOUCHPAD devices I assume.
Are touch events from the different sources treated differently?

>+NS_IMETHODIMP
>+nsWindow::RegisterTouchWindow()
>+{
>+    mHandleTouchEvent = true;
>+    mTouches.Clear();
>+    return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsWindow::UnregisterTouchWindow()
>+{
>+    mHandleTouchEvent = false;
>+    return NS_OK;
>+}
>+#endif

Is this just an optimization?
Is it necessary?

Does this switch between letting GTK do the mouse/button event emulation and
Gecko doing the emulation (when default actions are not prevented)?

If the optimization is not necessary, then I'd prefer to stay with the Gecko
emulation all the time, so that we don't get different behavior when a
listener is added.

>+static
>+PLDHashOperator
>+EnumTouchToAppend(GdkEventSequence* aKey, nsRefPtr<dom::Touch> aData, void* aArg)

The style in this file is usually to have "static PLDHashOperator" on one
line.

I'd prefer the name |AppendTouchToEvent|, or |AppendToTouchArray| if you pass
&event.touches as the pointer.

>+    if (mTouches.Get(aEvent->sequence, &touch)) {
>+        id = touch->mIdentifier;
>+        mTouches.Remove(aEvent->sequence);

I assume it wouldn't be possible to reuse the allocated Touch and update the
values, because of the other objects holding references to Touch.  If that is
true, then please add a comment to explain so that no one changes the code
(trying to be more efficient).

>+    // for touch event handling
>+    nsDataHashtable<nsPtrHashKey<GdkEventSequence>, nsRefPtr<mozilla::dom::Touch> > mTouches;

I think nsRefPtrHashtable is more commonly used for this situation.  It's
Put() is a little more efficient, transferring ownership instead of more
ref-count manipulation, but perhaps you have a reason to prefer
nsDataHashtable?  (I assume there are not going to be more than 10 concurrent
touch sequences and so an array is probably more efficient, but a hashtable is
fine if that provides simpler code.)

>+    uint32_t            mLastTouchId;

The aIdentifier argument to the Touch constructor is int32_t, so behaviour is
undefined when mLastTouchId passes INT_MAX.  (I don't know why aIdentifier is
signed.)

Also I think it would be better to have global identifiers, or perhaps there
could be issues with Touchs across different nsWindows ending up in the same
PresShell or document with the same id.

On X11, GdkEventTouch::sequence is set from identifiers which are "represented
as unsigned 32-bit values and increase strictly monotonically in value for
each new touch, wrapping back to 0 upon reaching the numerical limit of IDs."
The next sentence makes that one almost meaningless: "The increment between
two touch IDs is indeterminate."  However, I think it should be fine to just
take the lowest 31 bits of GdkEventTouch::sequence.  (Gecko will need to reset
ids to prevent leaking of event counts from one document to another anyway.)

        Attachment #8463253 -
        Flags: review?(karlt) → review-

    
    

    
  
What's the status here? The patch failed review? X-hundred thousand Firefox users on Linux would love to see native touch working.

    
    

    
  
(In reply to Sam Tuke from comment #9)
> What's the status here? The patch failed review? X-hundred thousand Firefox
> users on Linux would love to see native touch working.

is this testeable in some beta or alpha version?

    
    

    
  
kats wrote some wiki notes about Gecko touch input handling.
I'm told that existing Gecko support for touch events is only for touch screens, not touch pads.
URL: http://www.w3.org/TR/touch-events/ → https://wiki.mozilla.org/Platform/Inp...

    
    

    
  
(Touch events are really for touch screens. Touch pad should map to mouse events, though, for multitouch cases... not so sure. Pointer events might work better for multitouch touchpads)

    
    

    
  
Yeah, agreed. For touchpads we should probably go straight to pointer events. If you some sort of absolute-positioned touchpad that maps directly to a screen area then touch events *might* be doable but I would still prefer using pointer events only for that.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        refreshed to tip (obsolete)
        — 
          — Splinter Review
      

    


  
Dunno if it works yet, but it builds again.

    
    

    
  
Botond: something for your to-do list: apply the patch above, changing DispatchEvent to DispatchAPZAwareEvent, enable APZ, and see how it works on a touch-enabled linux device.
Flags: needinfo?(botond)
Assignee: m_kato → roc
Status: NEW → ASSIGNED

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #8)
> What triggers the default actions for touch events, sending mouse and button
> events when appropriate?
> (AFAIS widget/windows code looks like it sends mouse events as well as touch
> events, so I don't know what prevents that, when the default is prevented at
> least.)

APZC will do that, providing we direct events through APZC as kats mentioned in comment #15.

> >+NS_IMETHODIMP
> >+nsWindow::RegisterTouchWindow()
> >+{
> >+    mHandleTouchEvent = true;
> >+    mTouches.Clear();
> >+    return NS_OK;
> >+}
> >+
> >+NS_IMETHODIMP
> >+nsWindow::UnregisterTouchWindow()
> >+{
> >+    mHandleTouchEvent = false;
> >+    return NS_OK;
> >+}
> >+#endif
> 
> Is this just an optimization?
> Is it necessary?

This code exists so that we only listen for platform touch events when APZC is enabled, and dom.w3c_touch_events.enabled or dom.w3c_pointer_events.enabled are true.

> Does this switch between letting GTK do the mouse/button event emulation and
> Gecko doing the emulation (when default actions are not prevented)?

Yes.

> If the optimization is not necessary, then I'd prefer to stay with the Gecko
> emulation all the time, so that we don't get different behavior when a
> listener is added.

Right, but since this requires APZC we need it to be conditional until APZC is unconditionally enabled.

> >+static
> >+PLDHashOperator
> >+EnumTouchToAppend(GdkEventSequence* aKey, nsRefPtr<dom::Touch> aData, void* aArg)
> 
> The style in this file is usually to have "static PLDHashOperator" on one
> line.

Fixed.

> I'd prefer the name |AppendTouchToEvent|, or |AppendToTouchArray| if you pass
> &event.touches as the pointer.

Fixed.

> >+    if (mTouches.Get(aEvent->sequence, &touch)) {
> >+        id = touch->mIdentifier;
> >+        mTouches.Remove(aEvent->sequence);
> 
> I assume it wouldn't be possible to reuse the allocated Touch and update the
> values, because of the other objects holding references to Touch.  If that is
> true, then please add a comment to explain so that no one changes the code
> (trying to be more efficient).

This code needs to switch to using SingleTouchData, in which case they have to be copied into the event's touch array so this issue is moot.

> >+    // for touch event handling
> >+    nsDataHashtable<nsPtrHashKey<GdkEventSequence>, nsRefPtr<mozilla::dom::Touch> > mTouches;
> 
> I think nsRefPtrHashtable is more commonly used for this situation.  It's
> Put() is a little more efficient, transferring ownership instead of more
> ref-count manipulation, but perhaps you have a reason to prefer
> nsDataHashtable?  (I assume there are not going to be more than 10 concurrent
> touch sequences and so an array is probably more efficient, but a hashtable
> is
> fine if that provides simpler code.)

This has to change anyway.

> >+    uint32_t            mLastTouchId;
> 
> The aIdentifier argument to the Touch constructor is int32_t, so behaviour is
> undefined when mLastTouchId passes INT_MAX.  (I don't know why aIdentifier is
> signed.)
> 
> Also I think it would be better to have global identifiers, or perhaps there
> could be issues with Touchs across different nsWindows ending up in the same
> PresShell or document with the same id.

IIUC the touch ids only have to be unique at a given point in time. It seems unlikely you can have touch sequences being received by multiple nsWindows at the same time. But it's easy to make global anyway.

    
    

    
  
Comment on attachment 8598406 [details] [diff] [review]
TabParent should undo its changes to the touch objects in case those objects get used by another event

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

We actually won't need this fixed, I think...

        Attachment #8598406 -
        Flags: review?(bugs)

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> Botond: something for your to-do list: apply the patch above, changing
> DispatchEvent to DispatchAPZAwareEvent, enable APZ, and see how it works on
> a touch-enabled linux device.

I tried it, and on first blush, it's working fairly well! I'll play around with it a bit more and file any bugs I encounter.
Flags: needinfo?(botond)

    
    

    
  
Comment on attachment 8599682 [details] [diff] [review]
Implement touch events for GTK3

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

oops, found a bug

        Attachment #8599682 -
        Flags: review?(karlt)

    
    

    
  
Comment on attachment 8599682 [details] [diff] [review]
Implement touch events for GTK3

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

False alarm; I hit an assertion failure that appears to be a touch-action bug (which I had enabled, but is disabled by default).

        Attachment #8599682 -
        Flags: review?(karlt)

    
    

    
  
Comment on attachment 8599682 [details] [diff] [review]
Implement touch events for GTK3

(In reply to Karl Tomlinson (ni?:karlt) from comment #8)
> The aIdentifier argument to the Touch constructor is int32_t, so behaviour is
> undefined when mLastTouchId passes INT_MAX.  (I don't know why aIdentifier is
> signed.)

Please address this, as gLastTouchID can still be > INT_MAX.

> However, I think it should be fine to just
> take the lowest 31 bits of GdkEventTouch::sequence.

Is there a need to keep gLastTouchID instead of using the lowest 31 bits of
GdkEventTouch::sequence?

># User Robert O'Callahan <robert@ocallahan.org>

Enough of this is similar to m_kato's patch that he should also be listed as
an author.

>+#if GTK_CHECK_VERSION(3,4,0)

configure.in now requires GTK3_VERSION=3.4.0 and I suspect using earlier
version would not be practical, so no need for the version checks.

>     case eIntID_TouchEnabled:
>+#if MOZ_WIDGET_GTK == 3
>+        aResult = mozilla::widget::IsTouchDeviceSupportPresent();

Looks like the media query depends on whether there is a touch device present
at one point in time, while EventDispatcher and nsCoreUtils will only dispatch
events if there was one present at a different point in time, and widget code
will only dispatch touch events if RegisterTouchWindow() has been called.

Still, AFAIK it looks like the widget code is doing the right thing, so I'll
assume any bug is in other code.

>+    virtual void RegisterTouchWindow();

override, please.

        Attachment #8599682 -
        Flags: review?(karlt) → review+

    
    

    
  
Comment on attachment 8599682 [details] [diff] [review]
Implement touch events for GTK3

>+    if (aEvent->window == mGdkWindow) {
>+        touchPoint = LayoutDeviceIntPoint(aEvent->x, aEvent->y);
>+    } else {
>+        touchPoint = LayoutDeviceIntPoint(aEvent->x_root, aEvent->y_root) -
>+                     WidgetToScreenOffset();
>+    }

GdkPointToDevicePixels or similar should be used to convert the GDK coords to LayoutDeviceIntPoint.

(For most events DispatchEvent does the conversion on refPoint, even though that is not quite the right place, particularly with some WidgetToScreenOffset() transformations of root coords.)

    
  
Blocks: 1175029
Severity: normal → enhancement

    
    

    
  
Has this bug gone stale?

    
    

    
  
Yeah I need to update the patch and land it.

    
    

    
  
Hi! We have a pressing urgency to integrate touch event support in our Digital Signage platform, currently comprising several thousand embedded devices running Firefox and rapidly growing.

Is there any ETA for this feature to show up in Nightly builds? Please let us know if there's anything we can test (even unofficial patches). We're eager to try it and deploy it.

My understanding is that the latest patches in this bug are targeted at an older snapshot which doesn't include the GTK3 transition.

What would be the easiest way for us to test the new functionality?

    
    

    
  
(In reply to Robert Millan (Beabloo) from comment #27)
> My understanding is that the latest patches in this bug are targeted at an
> older snapshot which doesn't include the GTK3 transition.

They are not, in fact they require GTK3. A few months ago I successfully applied and used these patches for debugging APZ with touch events on desktop (at the time, I had to manually switch to GTK3 as well; now it's the default). I can rebase the patches to apply to latest trunk and upload them if you'd like.

    
    

    
  
Comment on attachment 8673984 [details]
MozReview Request: Bug 978679 - Implement touch events for GTK3. r=karlt

Rebased patch.

        Attachment #8673984 -
        Attachment description: MozReview Request: Bug 978679 → Implement touch events for GTK3

        Attachment #8599682 -
        Attachment is obsolete: true

        Attachment #8591406 -
        Attachment is obsolete: true

    
    

    
  
MOZ_USE_XINPUT2=1 is currently needed in the environment.
No longer blocks: gtk3
Depends on: 1207700, gtk3

    
    

    
  
Sorry, I didn't rebase that properly, it doesn't build. Will fix.

    
    

    
  
Hi,

It seems to be affected by NS_TOUCH_* rename:

changeset:   262348:641727472a5c
user:        Masayuki Nakano <masayuki@d-toybox.com>
date:        Tue Sep 15 00:14:35 2015 +0900
summary:     Bug 895274 part.244 Rename NS_TOUCH_CANCEL to eTouchCancel r=smaug

changeset:   262347:aa8e1b6f6753
user:        Masayuki Nakano <masayuki@d-toybox.com>
date:        Tue Sep 15 00:14:35 2015 +0900
summary:     Bug 895274 part.243 Rename NS_TOUCH_END to eTouchEnd r=smaug

changeset:   262346:79b8d374b4fd
user:        Masayuki Nakano <masayuki@d-toybox.com>
date:        Tue Sep 15 00:14:35 2015 +0900
summary:     Bug 895274 part.242 Rename NS_TOUCH_MOVE to eTouchMove r=smaug

changeset:   262345:a92cec9902d7
user:        Masayuki Nakano <masayuki@d-toybox.com>
date:        Tue Sep 15 00:14:34 2015 +0900
summary:     Bug 895274 part.241 Rename NS_TOUCH_START to eTouchStart r=smaug

    
    

    
  


  
This fixes the build problem caused by macro rename

    
    

    
  
It seems the "#undef Status" kludges in various places of the Mozilla codebase (e.g. dom/workers/WorkerFeature.h) interact badly with internal "Status" macro in Xlib.

I'm not sure why this pops up now, but I suspect subtle changes in #include order introduced by this patch are uncovering it.

    
    

    
  
Found it. After applying the patch, widget/gtk/nsWindow.h includes mozilla/TouchEvents.h. This new include path is created:

widget/gtk/Unified_cpp_widget_gtk0.cpp -> widget/gtk/IMContextWrapper.cpp -> widget/gtk/nsWindow.h -> mozilla/TouchEvents.h -> mozilla/dom/Touch.h -> mozilla/MouseEvents.h -> mozilla/dom/DataTransfer.h -> mozilla/dom/Promise.h -> dom/workers/bindings/WorkerFeature.h

Then WorkerFeature.h screws up the "Status" macro:

#ifdef Status
/* Xlib headers insist on this for some reason... Nuke it because
   it'll override our member name */
#undef Status
#endif

Shortly after that, <X11/XKBlib.h> is included by the same user:

widget/gtk/Unified_cpp_widget_gtk0.cpp -> widget/gtk/NativeKeyBindings.cpp -> widget/gtk/nsGtkKeyUtils.h -> dist/system_wrappers/X11/XKBlib.h -> X11/XKBlib.h

And the same problem happens over and over thorough the source tree.

So my question is: Is there any QUICK way around this mess?

If we're going to use Firefox for our new touch screens, we need urgent confirmation / verification that this can work.

    
    

    
  
I traced the "#undef Status" back to channgeset 187333:e2b9d289514f. Adding the original author of that commit (Chris Pearce).

Chris if you can provide any advice that would be much appreciated.

    
    

    
  
DataTransfer.h  shouldn't include Promise.h.
It can just forward declare Promise, as far as I see.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        no_promise_include.diff (obsolete)
        — 
          — Splinter Review
      

    


  
Does this help?

    
    

    
  


  
FWIW, I have achieved successful build using this workaround.

    
    

    
  
Sorry Olli, didn't notice your update. I'll make a new build with your patch and report.

    
    

    
  
Comment on attachment 8674209 [details] [diff] [review]
no_promise_include.diff

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

Works for me! Thank you Kato-san

    
    

    
  
So the code finally builds but I can't seem to get it working. I've tried latest Aurora build from [1] and then latest daily build from [2], both without success.

My test steps are:

1. export MOZ_USE_XINPUT2=1 and launch Firefox
2. In about:config, set:
    dom.w3c_touch_events.enabled = 1
    layout.css.touch_action = true
3. Open http://www.paulirish.com/demo/multi
4. Attempt to draw something in the demo site's canvas

I'm doing this test on an Ubuntu V14.04 system. Note that 3-finger touch gestures are working fine in Unity, and applications can also make use of them (e.g. Chromium works on the same demo URL).

Any idea what could be missing?

[1] https://launchpad.net/~ubuntu-mozilla-daily/+archive/ubuntu/firefox-aurora
[2] https://launchpad.net/~ubuntu-mozilla-daily/+archive/ubuntu/ppa

    
    

    
  
Comment on attachment 8674215 [details] [diff] [review]
workaround for "#undef Status" problem

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

Obsoleted by https://bugzilla.mozilla.org/attachment.cgi?id=8674209

    
    

    
  
Very strange, it works now. I'm unsure what changed, but at least we can confirm multitouch is supported!

Many thanks to everyone

    
    

    
  
Thanks, Robert, for providing these fixes!

        Attachment #8673984 -
        Attachment description: Implement touch events for GTK3 → MozReview Request: Bug 978679 - Implement touch events for GTK3. r=karlt

        Attachment #8673984 -
        Flags: review?(karlt)

    
    

    
  
Comment on attachment 8673984 [details]
MozReview Request: Bug 978679 - Implement touch events for GTK3. r=karlt

Bug 978679 - Implement touch events for GTK3. r=karlt

    
    

    
  
The updated patch includes Robert's and Olli's fixes from the past couple of comments.

The only notable change I had to rebase across is the move IsTouchDeviceSupportPresent() into WidgetUtils. I introduced new files widget/gtk/WidgetUtilsGTK.{h,cpp} to house the GTK implementation.

        Attachment #8674178 -
        Attachment is obsolete: true

        Attachment #8674209 -
        Attachment is obsolete: true

        Attachment #8674215 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Botond Ballo [:botond] from comment #49)
> Let's see how this fares on Try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d90951ce067d

Had some Android and static analysis build failures. Fixed:

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

The tests looked pretty green though!

    
    

    
  
Comment on attachment 8673984 [details]
MozReview Request: Bug 978679 - Implement touch events for GTK3. r=karlt

Bug 978679 - Implement touch events for GTK3. r=karlt

    
    

    
  
roc, is there anything else that needs to be done before this patch can land?
Flags: needinfo?(roc)

    
    

    
  
Some of Karl's comments haven't been addressed yet.

(In reply to Karl Tomlinson (ni?:karlt) from comment #23)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #8)
> > The aIdentifier argument to the Touch constructor is int32_t, so behaviour is
> > undefined when mLastTouchId passes INT_MAX.  (I don't know why aIdentifier is
> > signed.)
> 
> Please address this, as gLastTouchID can still be > INT_MAX.

This one...

> > However, I think it should be fine to just
> > take the lowest 31 bits of GdkEventTouch::sequence.
> 
> Is there a need to keep gLastTouchID instead of using the lowest 31 bits of
> GdkEventTouch::sequence?

This one...

> ># User Robert O'Callahan <robert@ocallahan.org>
> 
> Enough of this is similar to m_kato's patch that he should also be listed as
> an author.

This one...

> >+#if GTK_CHECK_VERSION(3,4,0)
> 
> configure.in now requires GTK3_VERSION=3.4.0 and I suspect using earlier
> version would not be practical, so no need for the version checks.

And this one.
Flags: needinfo?(roc)

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #53)
> Some of Karl's comments haven't been addressed yet.
> 
> (In reply to Karl Tomlinson (ni?:karlt) from comment #23)
> > (In reply to Karl Tomlinson (ni?:karlt) from comment #8)
> > > The aIdentifier argument to the Touch constructor is int32_t, so behaviour is
> > > undefined when mLastTouchId passes INT_MAX.  (I don't know why aIdentifier is
> > > signed.)
> > 
> > Please address this, as gLastTouchID can still be > INT_MAX.
> 
> This one...
> 
> > > However, I think it should be fine to just
> > > take the lowest 31 bits of GdkEventTouch::sequence.
> > 
> > Is there a need to keep gLastTouchID instead of using the lowest 31 bits of
> > GdkEventTouch::sequence?

'sequence' is a pointer and to be honest, I don't feel comfortable with exposing its low bits directly to Web content. I worry that Web content might assume these IDs are increasing. Also, I *really* worry that exposing pointer values leaks information about the address space that might contribute to security exploits. So let's stick with the increasing ID counter.

    
    

    
  
Actually it might not really be a pointer, but actually just an integer ID that GTK3 exposes as a pointer for opaqueness, but still, better safe than sorry.

    
    

    
  
sorry had to revert this for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=15845944&repo=mozilla-inbound
Flags: needinfo?(roc)

    
    

    
  
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-asan/1445252555/mozilla-inbound-linux64-asan-bm72-build1-build961.txt.gz

04:10:14     INFO -  In file included from /builds/slave/m-in-l64-asan-0000000000000000/build/src/widget/gtk/nsWindow.cpp:127:
04:10:14     INFO -  /builds/slave/m-in-l64-asan-0000000000000000/build/src/widget/gtk/nsWindow.h:211:37: error: unknown type name 'GdkEventTouch'
04:10:14     INFO -      gboolean           OnTouchEvent(GdkEventTouch* aEvent);
04:10:14     INFO -                                      ^
04:10:14     INFO -  /builds/slave/m-in-l64-asan-0000000000000000/build/src/widget/gtk/nsWindow.h:409:34: error: use of undeclared identifier 'GdkEventSequence'
04:10:14     INFO -      nsDataHashtable<nsPtrHashKey<GdkEventSequence>, RefPtr<mozilla::dom::Touch> > mTouches;

Karl, do you happen to know what version of GTK3 we have on the try builders?
Flags: needinfo?(roc) → needinfo?(karlt)

    
    

    
  
>   12.320 -#if GTK_CHECK_VERSION(3,4,0)
>   12.321 -  // TODO: is this correct? I don't have GTK 3.4+ so I can't check
>   12.322    event.scroll.direction = GDK_SCROLL_SMOOTH;
>   12.323    event.scroll.delta_x = -aDeltaX;
>   12.324    event.scroll.delta_y = -aDeltaY;
>   12.325 -#else
>   12.326    if (aDeltaX < 0) {
>   12.327      event.scroll.direction = GDK_SCROLL_RIGHT;
>   12.328    } else if (aDeltaX > 0) {
>   12.329      event.scroll.direction = GDK_SCROLL_LEFT;
>   12.330    } else if (aDeltaY < 0) {
>   12.331      event.scroll.direction = GDK_SCROLL_DOWN;
>   12.332    } else if (aDeltaY > 0) {
>   12.333      event.scroll.direction = GDK_SCROLL_UP;
>   12.334    } else {
>   12.335      return NS_OK;
>   12.336    }
>   12.337 -#endif

That looks odd - event.scroll.direction will always be overridden. Was this meant to be changed to |#if MOZ_WIDGET_GTK == 3|?

    
    

    
  
Yes, my bad. Good catch!

    
    

    
  
Comment on attachment 8673984 [details]
MozReview Request: Bug 978679 - Implement touch events for GTK3. r=karlt

https://reviewboard.mozilla.org/r/22181/#review20055

::: widget/gtk/nsWindow.h:414
(Diff revision 3)
> +    nsDataHashtable<nsPtrHashKey<GdkEventSequence>, nsRefPtr<mozilla::dom::Touch> > mTouches;
> +    uint32_t            mLastTouchId;

This seems to have reverted from nsRefPtrHashtable and the global sequence id.

Is this based on attachment or an earlier version

::: widget/gtk/nsWindow.cpp:3413
(Diff revision 3)
> +    LayoutDeviceIntPoint touchPoint;
> +    if (aEvent->window == mGdkWindow) {
> +        touchPoint = LayoutDeviceIntPoint(aEvent->x, aEvent->y);
> +    } else {
> +        touchPoint = LayoutDeviceIntPoint(aEvent->x_root, aEvent->y_root) -
> +                     WidgetToScreenOffset();
> +    }

See comment 24.  Use GdkEventCoordsToDevicePixels().

        Attachment #8673984 -
        Flags: review?(karlt)

    
    

    
  
(In reply to Simon Lindholm from comment #60)
> That looks odd - event.scroll.direction will always be overridden.

Thanks.  Yes, sorry.  That was my request, but
some version check is required because there are still GTK2 builds.

It would be nice if we could have a consistent check for not-GTK2, but things
are already not consistent and GTK_CHECK_VERSION(3,4,0) is already used
in related code and so is fine for this patch.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #59)
> Karl, do you happen to know what version of GTK3 we have on the try builders?

It's 3.4 when it is GTK3 but some builds are still against GTK2 due to bugs, perhaps bugs in old versions of GTK3.
Flags: needinfo?(karlt)

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #54)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #53)
> > Some of Karl's comments haven't been addressed yet.
> > 
> > (In reply to Karl Tomlinson (ni?:karlt) from comment #23)
> > > (In reply to Karl Tomlinson (ni?:karlt) from comment #8)
> > > > However, I think it should be fine to just
> > > > take the lowest 31 bits of GdkEventTouch::sequence.
> > > 
> > > Is there a need to keep gLastTouchID instead of using the lowest 31 bits of
> > > GdkEventTouch::sequence?
> 
> 'sequence' is a pointer and to be honest, I don't feel comfortable with
> exposing its low bits directly to Web content. I worry that Web content
> might assume these IDs are increasing. Also, I *really* worry that exposing
> pointer values leaks information about the address space that might
> contribute to security exploits. So let's stick with the increasing ID
> counter.

Yes, pointer bits should not be exposed to content.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #55)
> Actually it might not really be a pointer, but actually just an integer ID
> that GTK3 exposes as a pointer for opaqueness, but still, better safe than
> sorry.

It is not a pointer to memory for either X11 or Wayland.  Perhaps it is
possible that GDK might make it a pointer to memory one day (though I don't
know why or how), but I'm expecting these won't be passed directly on to
content anyway (comment 8).  Gecko needs to prevent leaking event counts from
one document to another, and, if content is expecting sequential event ids, then
Gecko will need to sort that out per document.

We don't need to hold back the patch on removing gLastTouchID, but I think it is
unnecessary code.

    
    

    
  
Botond's patch didn't include changes I made earlier in response to Karl's comments, so I started from that version of my patch and reapplied all the other changes we've been talking about.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/cb6b065f2a23
Status: ASSIGNED → RESOLVED
Closed: 9 years ago

          status-firefox44:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Whiteboard: [Comment 24 not addressed.  See comment 62.]

        Attachment #8677237 -
        Flags: review?(karlt) → review+

    
    

    
  
Comment on attachment 8677237 [details]
MozReview Request: Bug 978679. Convert GDK touch event coordinates properly. r=karlt

https://reviewboard.mozilla.org/r/22907/#review20431

Nice template trick.  Thanks for tidying this up.

::: widget/gtk/nsWindow.cpp:2536
(Diff revision 1)
> +template <typename Event> static LayoutDeviceIntPoint
> +GetRefPoint(nsWindow* aWindow, Event* aEvent)

Perhaps this could be an object method so that the aWindow parameter is implicit.
Whiteboard: [Comment 24 not addressed.  See comment 62.]
Depends on: 1217515

    
    

    
  
Bug 1217515 depends on this because it's not a regression.
Blocks: 1217515
No longer depends on: 1217515

    
    

    
  
(In reply to Karl Tomlinson (ni?:karlt) from comment #74)
> Perhaps this could be an object method so that the aWindow parameter is
> implicit.

I'd prefer not to expose it in the .h file.

    
  
Blocks: 1213342

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: