Matt Brubeck (:mbrubeck) Assignee
	Description • 11 years ago

AsyncPanZoomController and GestureEventListener pass "tap" events to the GeckoContentController, which uses them to generate mouse events.  However, these events do not include the appropriate modifier flags when modifier keys (e.g. ctrl, alt, shift) are pressed.

In Metro Firefox, bug 941324 switched from generating mouse events in widget code to using APZC's GestureEventListener.  This broke things like control-clictap to open a link in a new window.

This patch adds modifier info to the tap events produced by APZC, and makes the Metro front-end use this info when generating mouse events.  (We'll need to do the same for B2G in a follow-up patch, though this isn't critical until B2G devices have hardware keyboards.)

Requesting review from jimm for the Windows parts, roc for the xpwidgets parts, and botond for the gfx parts.  One question in particular:  Is there a better way or place to handle the conversion from the widget mozilla::Modifiers to the nsIDOMWindowUtils modifier flags?

	Matt Brubeck (:mbrubeck) Assignee
	Comment 1 • 11 years ago

Sorry, found one line that I missed in the previous version.

	Jim Mathies [:jimm]
	Comment 2 • 11 years ago

Comment on attachment 8337529 [details] [diff] [review]
patch v2

nice!

For the mod flag conversions, you might flag masayuki, he's the expert on that down in widget.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 3 • 11 years ago

This Try push failed on B2G because of a build error in ContentProcessController:
https://tbpl.mozilla.org/?tree=Try&rev=d91c9ad09ca2

Pushed again with a trivial fix:
https://tbpl.mozilla.org/?tree=Try&rev=358cb673e158

	Matt Brubeck (:mbrubeck) Assignee
	Comment 4 • 11 years ago

Comment on attachment 8337529 [details] [diff] [review]
patch v2

Masayuki, is this a reasonable way to convert the widget "Modifiers" flags to ones that can be used with nsIDOMWindowUtils?  Is there a better solution?

	Botond Ballo [:botond]
	Comment 5 • 11 years ago

Comment on attachment 8337529 [details] [diff] [review]
patch v2

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

We also have GeckoContentController implementations in http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidBridge.h and http://mxr.mozilla.org/mozilla-central/source/widget/gonk/ParentProcessController.h.

The Android one isn't currently used, but the file is still compiled, and the overridden methods are marked MOZ_OVERRIDE, so the signatures of Handle*Tap need to be updated to not break Fennec builds. The implementations can ignore the modifiers for now.

The parent process one just has no ops for the Handle*Tap functions, but again the signatures need to be updated.

r=me with these changes.

::: dom/ipc/TabParent.cpp
@@ +498,3 @@
>  {
>    if (!mIsDestroyed) {
>      unused << SendHandleDoubleTap(aPoint);

Is propagating the modifiers to TabChild the part that you said will be done in a follow-up patch? If so, can you add a TODO comment in these functions?

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +114,5 @@
> +  return result;
> +}
> +
> +}
> +

If there is a more general place in the code than AsyncPanZoomController.cpp to place this function, I think we should do that. If not then there is fine.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 6 • 11 years ago

(In reply to Botond Ballo [:botond] from comment #5)
> The Android one isn't currently used, but the file is still compiled, and
> the overridden methods are marked MOZ_OVERRIDE, so the signatures of
> Handle*Tap need to be updated to not break Fennec builds. The
> implementations can ignore the modifiers for now.

Pushed to Try again with build fixes:
https://tbpl.mozilla.org/?tree=Try&rev=19e70444723b

> ::: dom/ipc/TabParent.cpp
> Is propagating the modifiers to TabChild the part that you said will be done
> in a follow-up patch? If so, can you add a TODO comment in these functions?

Yes -- I'll file the follow-up bug once this lands, and I can write a patch too.

> If there is a more general place in the code than AsyncPanZoomController.cpp
> to place this function, I think we should do that. If not then there is fine.

I'm open to suggestions here.  I considered putting it in nsDOMWindowUtils, but I'd need to add it as a public API rather than a static function...

	Matt Brubeck (:mbrubeck) Assignee
	Comment 7 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3110e9acf10

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 8 • 11 years ago

Comment on attachment 8337529 [details] [diff] [review]
patch v2

Hmm, IIRC, nsDOMWindowUtils.cpp already has same method. Could you make it as static and use it from the AsyncPanZoomController.cpp?

	Carsten Book [:Tomcat]
	Comment 9 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/c3110e9acf10

	Matt Brubeck (:mbrubeck) Assignee
	Comment 10 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8)
> Hmm, IIRC, nsDOMWindowUtils.cpp already has same method. Could you make it
> as static and use it from the AsyncPanZoomController.cpp?

nsDOMWindowUtils only has a method to do the reverse conversion (from nsIDOMWindowUtils modifiers to mozilla::Modifiers).

Also, I think we can't use static nsDOMWindowUtils methods from AsyncPanZoomController, because nsDOMWindowUtils.h is not exported.

Should we move this new method to nsDOMWindowUtils, and add that header to EXPORTS so we can use it from gfx?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 11 • 11 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #10)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8)
> > Hmm, IIRC, nsDOMWindowUtils.cpp already has same method. Could you make it
> > as static and use it from the AsyncPanZoomController.cpp?
> 
> nsDOMWindowUtils only has a method to do the reverse conversion (from
> nsIDOMWindowUtils modifiers to mozilla::Modifiers).

Ah...

> Also, I think we can't use static nsDOMWindowUtils methods from
> AsyncPanZoomController, because nsDOMWindowUtils.h is not exported.
> 
> Should we move this new method to nsDOMWindowUtils, and add that header to
> EXPORTS so we can use it from gfx?

Hmm, then, I think that WidgetInputEvent should have two static methods for converting modifier flags. If it's implemented in WidgetEventsImpl.cpp, it doesn't harm include hell. I'll file a bug for this issue.