930177 - Consolidate async scroll event handling with keyboard handling

Status: RESOLVED FIXED

(Core Graveyard :: Widget: WinRT, defect)

Description

[Jim Mathies [:jimm]]

Currently scroll handling queues win32 events, I'd prefer it queue gecko events, similar to keyboard event handling.

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 821246 [details] [diff] [review]
patch v.1

I should have consolidated these after I implemented the keyboard event handling. Finally got around to it with some other work. Basically this just switches to queing up gecko events rather than msgs.

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 821246 [details] [diff] [review]
patch v.1

I think tim might be on pto.

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 821246 [details] [diff] [review]
patch v.1

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

I'm around! Sorry for the delay. This looks good, though the As*Event functions throw me off a little.

::: widget/windows/winrt/MetroWidget.cpp
@@ +572,5 @@
> +  WidgetGUIEvent* newEvent = nullptr;
> +  switch(aEvent->eventStructType) {
> +    case NS_WHEEL_EVENT:
> +    {
> +      WidgetWheelEvent* oldEvent = aEvent->AsWheelEvent();

It feels like all these As*Event functions could be replaced with |static_cast|s, but that's an idea to ponder another day.

Along those lines; should we be checking for a nullptr result from this function?

@@ +581,5 @@
> +    }
> +    break;
> +    case NS_CONTENT_COMMAND_EVENT:
> +    {
> +      WidgetContentCommandEvent* oldEvent = aEvent->AsContentCommandEvent();

same comment

Comment 5

[Brian R. Bondy [:bbondy]]

Thanks for grabbing the review Tim, I have a bunch of updater ones to get at today :)

Comment 6

[Jim Mathies [:jimm]]

> ::: widget/windows/winrt/MetroWidget.cpp
> @@ +572,5 @@
> > +  WidgetGUIEvent* newEvent = nullptr;
> > +  switch(aEvent->eventStructType) {
> > +    case NS_WHEEL_EVENT:
> > +    {
> > +      WidgetWheelEvent* oldEvent = aEvent->AsWheelEvent();
> 
> It feels like all these As*Event functions could be replaced with
> |static_cast|s, but that's an idea to ponder another day.

We're not supposed to use that anymore -

https://groups.google.com/forum/#!search/nsGUIEvent.h/mozilla.dev.platform/cHg_ExqEC_U/4ZEozp5f4nwJ

> Along those lines; should we be checking for a nullptr result from this
> function?

I was curious about that as well. Technically these events should not get out of sync like that, and in the replacement As* calls that landed recently we don't. If they do get out of sync, it's a bug that needs to be fixed, in which case we would probably prefer a crash.

Comment 7

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/c74facfb70ab

Thanks!

Comment 8

[Carsten Book [:Tomcat]]

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