Closed Bug 930177 Opened 6 years ago Closed 6 years ago

Consolidate async scroll event handling with keyboard handling

Categories

(Core Graveyard :: Widget: WinRT, defect)

26 Branch
x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla27

People

(Reporter: jimm, Assigned: jimm)

Details

Attachments

(1 file)

Currently scroll handling queues win32 events, I'd prefer it queue gecko events, similar to keyboard event handling.
Attached patch patch v.1Splinter Review
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.
Attachment #821246 - Flags: review?(tabraldes)
Comment on attachment 821246 [details] [diff] [review]
patch v.1

I think tim might be on pto.
Attachment #821246 - Flags: review?(tabraldes) → review?(netzen)
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
Attachment #821246 - Flags: review?(netzen) → review+
Thanks for grabbing the review Tim, I have a bunch of updater ones to get at today :)
> ::: 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.
https://hg.mozilla.org/mozilla-central/rev/c74facfb70ab
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
OS: Windows 8 Metro → Windows 8.1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.