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.

(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.

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.

(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.

Use RegisterTouchWindow and generate touch id per touch event.

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.)

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.

(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.

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.

(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...

(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.

Comment on attachment 8599682 [details] [diff] [review] Implement touch events for GTK3 Review of attachment 8599682 [details] [diff] [review]: ----------------------------------------------------------------- oops, found a bug

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).

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.

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.)

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.

MOZ_USE_XINPUT2=1 is currently needed in the environment.

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.

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!

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.

Let's see how this fares on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d90951ce067d

(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?

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.

(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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f909d41108b1d3d3b266a75673c82a28e468b26 Bug 978679 - Implement touch events for GTK3. r=karlt

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

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/56ffd8412250

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?

> 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().

(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.

(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.

Looks good on try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6b065f2a23d263fa47cdcf71016cbdf0f8deb6 Bug 978679. Implement touch events for GTK3. r=karlt

Bug 978679. Convert GDK touch event coordinates properly. r=karlt

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.

Bug 1217515 depends on this because it's not a regression.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed4298f0cc674c174d6c3a292f76855ad470149 Bug 978679. Convert GDK touch event coordinates properly. r=karlt

(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.