Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Description • 11 years ago

On Metro, we have been processing native events in terms of calls to ProcessEvents [1]. Our main event loop is also implemented in terms of a call to ProcessEvents [2]. On Windows 8.1, if you attempt to call ProcessEvents while processing a message as a result of a call to ProcessEvents (i.e. if you attempt to make a nested call to ProcessEvents), you will see an OriginateError. Analyzing the call to RoOriginateError reveals this:

combase!RoOriginateErrorW(HRESULT error = 0x8000ffff,
                          unsigned int cchMax = 0x34,
                          wchar_t * message = 0x088b0916 "Nested calls to ProcessEvents method is not allowed.")

We must change the way we handle native event processing on Metro to eliminate nested calls to ProcessEvents.

[1] https://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroAppShell.cpp?rev=74f983165c02#111
[2] https://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/FrameworkView.cpp?rev=859a06fa227c#141

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 1 • 11 years ago

The attached patch switches our event processing to the more traditional Get/PeekMessage, TranslateMessage, DispatchMessage.

In my limited testing thus far, the only downside I've noticed is that we lose our widget support for edge swipe gestures. I believe that we could (hopefully fairly easily) work around this by implementing our own edge swipe recognition in front-end code.

I might be crazy, but I think also that my local build of metroFx is much more snappy/responsive with this patch applied.

jimm: I'm wondering what you think about this approach. We know that we can't use ProcessEvents for our nested event loops, so maybe we shouldn't use it for our outer event loop either. It seems to degrade our performance noticeably, and so far it seems like we can implement the extra functionality that it gives us (edge swipes) without much difficulty

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

Have we poked through the assembly to see what we're missing out on? Also we need to use WinUtils methods for things like peekmessage and be careful about tsf message handling.

	Jim Mathies [:jimm] Assignee
	Comment 3 • 11 years ago

For reference - 

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsAppShell.cpp#170

	Jim Mathies [:jimm] Assignee
	Comment 4 • 11 years ago

(In reply to Jim Mathies [:jimm] from comment #3)
> For reference - 
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsAppShell.
> cpp#170

We don't need some of this stuff, like the hang monitor code, the event starvation stuff at the end and anything associated with plugins.

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 5 • 11 years ago

Thanks for the feedback, jimm!

I've updated the patch to use the correct versions of PeekMessage and GetMessage. Things seem to work about the same, I think.

I'll see what I can learn about ProcessEvents. At this point I don't think we have much choice but to try to avoid using it entirely: The other option is to not use nested loops at all, but gecko seems to rely heavily on the ability to spin up nested loops.

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 6 • 11 years ago

Oops, missing include.

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 7 • 11 years ago

I monitored a couple of passes of execution through CDispatcher::ProcessEvents. Below are the results.

CDispatcher::ProcessEvents
  Precondition testing (call GetCurrentThreadId, compare against a value, check other values for 0, etc)
  CoBeginProcessEvents
    AcquireSRWLockExclusive
    ASTAWaitContext Constructor
      ASTAWaitContext::AdjustDispatchableCallsOnEnterExit
        CComApartment::ASTAGetQueuedCallsNowDispatchable
        CComApartment::ASTAGetDispatchableCallsNowQueued
    ASTAWaitContext::HandlePriorityEvents
    ReleaseSRWLockExclusive
  CDispatcher::ProcessMessage
    PeekMessageW
    CDispatcher::PerformAcceleratorCallback
    TranslateMessage
    DispatchMessageW
    CoHandlePriorityEventsFromMessagePump
  CoEndProcessEvents
    AcquireSRWLockExclusive
    ASTAWaitContext Destructor
      RoRegisterForApartmentShutdown
      ASTAWaitContext::DispatchCallsOnExitNonBlockingProcessEventsIfAppropriate
      ASTAWaitContext::AdjustDispatchableCallsOnEnterExit
        CComApartment::ASTAGetQueuedCallsNowDispatchable
        CComApartment::ASTAGetDispatchableCallsNowQueued
    ReleaseSRWLockExclusive

The middle part (CDispatcher::ProcessMessage) is the more familiar Peek/GetMessage, TranslateMessage, DispatchMessage idiom.

The outer parts (CoBeginProcessEvents, CoEndProcessEvents) seem to be a lot of ASTA-specific [1] processing. I'm not too familiar with ASTA, so it's hard to say at this point whether we can safely ignore that processing.

[1] http://msdn.microsoft.com/en-us/library/windows/apps/hh750290.aspx

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 8 • 11 years ago

Comment on attachment 793186 [details] [diff] [review]
Patch v3 (WIP)

Now that I've tested more, I've discovered additional issues with the current patch applied:
  - The browser fails to handle suspend notifications and close gestures correctly, so it often gets terminated by Windows
  - Windows fails to get our custom "Settings" entries

	Jim Mathies [:jimm] Assignee
	Comment 9 • 11 years ago

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #5)
> I'll see what I can learn about ProcessEvents. At this point I don't think
> we have much choice but to try to avoid using it entirely: The other option
> is to not use nested loops at all, but gecko seems to rely heavily on the
> ability to spin up nested loops.

I can think of a couple cases of this like tab modal prompts, but I don't think we rely on it that heavily. Move of the modal loos we have in win32 are related to showing system dialogs, which we don't use here. What other areas have you found that rely on modal event processing?

	Jim Mathies [:jimm] Assignee
	Comment 10 • 11 years ago

I see a lot of dispatch calls, but very few dispatch loops - 

http://mxr.mozilla.org/mozilla-central/search?string=Services.tm.currentThread&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

http://mxr.mozilla.org/mozilla-central/search?string=processNextEvent%28true%29

I haven't seen the hang stack and I can't reproduce this problem on 8.0. Would you mind posting a tab modal hang stack so we can take a look? Maybe we can find a way around this without changing up existing event processing.

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

Looking at the android code, it looks like they ran into something similar. Looks like they have custom app modal prompts that use a doorhanger class implemented in native code. It also looks like their show call block.

prompt example:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/ContentPermissionPrompt.js
http://mxr.mozilla.org/mozilla-central/search?string=doorhanger
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/DoorHanger.java

cc'ing mfinkle who might be able to shed some light on androids solution to this.

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 12 • 11 years ago

> I can think of a couple cases of this like tab modal prompts, but I don't
> think we rely on it that heavily. Move of the modal loos we have in win32
> are related to showing system dialogs, which we don't use here. What other
> areas have you found that rely on modal event processing?

I haven't tried to dive too deep into which areas of the codebase are going to run into trouble, but in general, any piece of code that eventually calls |ProcessNativeEvents| [3] in MetroAppShell.cpp is going to fail since that function doesn't actually process any events on metro 8.1: It tries to call |ProcessEvents|, which throws the OriginateError and fails to process any events.

This means that any calls to |nsIThread.processNextEvent| and any calls to |NS_ProcessNextEvent| (which calls nsIThread.processNextEvent) are going to fail and potentially hang metroFx. From the looks of it, there are lot of calls that could get us into trouble [4][5].

> I see a lot of dispatch calls, but very few dispatch loops - 

nsThread::Dispatch [1] *is* a dispatch loop if it's called with the |DISPATCH_SYNC| flag [2]. I'm not sure how frequently it is called this way vs how frequently it is called to dispach async events, but even one sync consumer will hang metroFx.

[1] https://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThread.cpp?rev=c4d279b10128#367
[2] https://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThread.cpp?rev=c4d279b10128#398
[3] https://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroAppShell.cpp?rev=74f983165c02#111
[4] https://mxr.mozilla.org/mozilla-central/ident?i=processNextEvent&filter=
[5] https://mxr.mozilla.org/mozilla-central/ident?i=NS_ProcessNextEvent

	Jim Mathies [:jimm] Assignee
	Comment 13 • 11 years ago

An event dispatched to the main thread sync shouldn't be an issue, what we are concerned about is native event callbacks getting processed. If we have a native dialog up we can prevent those from being processed, and thereby prevent calls to the winrt dispatcher when we have the main thread.

One issue with going native is that there isn't good customisation support for native dialogs. Looks like all you can do is add check boxes, no text text entry. We might be able to work around that, not sure.

	Jim Mathies [:jimm] Assignee
	Comment 14 • 11 years ago

Ok this gets around the first problem. If we land this without figuring out how to address the prompts modal processing, we should disable prompts temporarily because I added an abort when we reenter those calls.

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 15 • 11 years ago

Jimm and I spoke at length in #windev, putting us mostly on the same page. For anyone who is following along at home:

FrameworkView::Run
  - is called on the main thread
  - is where our main event loop currently exists in metro
  - enters the main event loop by calling CoreDispatcher::ProcessEvents
  - does not return from CoreDispatcher::ProcessEvents until the app is exiting

MetroAppShell::ProcessNextNativeEvent
  - is used to pump messages while waiting for a blocking condition to be resolved (e.g. in nsIThread.processNextEvent and NS_ProcessNextEvent/NS_ProcessPendingEvents)
  - pumps messages by calling CoreDispatcher::ProcessEvents

We're not allowed to nest calls to CoreDispatcher::ProcessEvents in metro on Windows 8.1, so every time MetroAppShell::ProcessNextNativeEvent is called we see an OriginateError (first-chance excpetion) get thrown, and no event actually gets processed. This happens quite a lot, and is causing performance degradation on 8.1.  Additionally, anywhere in the code where we try to create an event loop by calling one of the functions that eventually calls MetroAppShell::ProcessNextNativeEvent, we end up in an infinite loop since messages never get processed (e.g. bug 905698).

The patch that jimm has attached in attachment 793701 [details] [diff] [review] works by attempting to eliminate the call to CoreDispatcher::ProcessEvents from FrameworkView::Run. Things look like the following with jimm's patch applied.

MetroAppShell::ProcessNextNativeEvent
  - same as before

FrameworkView::Run
  - same as before except enters the main event loop by calling MetroApp::Run, which calls XRE_metroStartup, which calls MetroAppShell::Run, which calls nsBaseAppShell::Run()

nsBaseAppShell::Run
  - uses MessageLoop::current()->Run() to dispatch events
  - cleans up remaining events by calling NS_ProcessPendingEvents

Some things work well with the patch applied. For example, tab-modal alerts generated by scripts work perfectly, as do beforeunload prompts generated by hitting ctrl+w on a tab with a beforeunload handler.

Some things do not work well: We still get OriginateErrors thrown whenever the window is activated, and it is still possible to reproduce bug 905968 by clicking a button that fires an alert, or by clicking the 'back' overlay on a page with a beforeunload handler.

I haven't looked into how MessageLoop::current()->Run() operates, but it may be possible to clear up our remaining issues by tinkering around with MessageLoop. The NS_ProcessPendingEvents call is still a little worrisome, though. In any case, there's more research/experimentation to be done.

	Brian R. Bondy [:bbondy]
	Comment 16 • 11 years ago

Great summary, thanks for posting it.

	Jim Mathies [:jimm] Assignee
	Comment 17 • 11 years ago

> I haven't looked into how MessageLoop::current()->Run() operates, but it may
> be possible to clear up our remaining issues by tinkering around with
> MessageLoop. The NS_ProcessPendingEvents call is still a little worrisome,
> though. In any case, there's more research/experimentation to be done.

I'm interested in hearing ideas, but I don't think this can be solved in chromium's MessageLoop, it's just a glorified main thread run loop that processes gecko thread and chromium ipc events.

We're going to hit this any time you invoke a mozilla modal loop using user input since NativeEventCallback (dispatcher->ProcessEvents) will be on the stack.

We have two worst offenders currently, prompt service and sync's async loop. We can solve the sync problem by taking sync out of the product. Before we do that though we need to chat with rnewman to see if there's any way we can get rid of sync's async loop or insure it never gets called while dispatcher->ProcessEvents is on the stack (may be possible?).

Prompts are another matter, we obviously need those. Most of these though (could) be replace with stock, native winrt modal prompts (CoreWindowDialog or some variant) or some other winrt trickery.  I was looking at the stack of ie for alert() and for auth prompts.. for alerts they have a custom winrt dialog class that launches an async winrt dialog while also holding the execution thread to prevent recursion. For auth prompts it looks like wininet has built-in support for something similar.

The main problem here is that once a prompt is generated and if dispatcher->ProcessEvents is on the stack, we can't fall back into our normal processing. I don't think there's any way to unwind below the prompt service (which would mean returning from the alert or whatever call and falling back through whatever generated that call).

	neil@parkwaycc.co.uk
	Comment 18 • 11 years ago

OK, so I don't know the first thing about Metro, so apologies if I have misunderstood the problem. From reading the bug it seems that you make an API call to get Metro events. This API call then calls you back. The problem arises when synchronous processing of the callback reaches a point where you need to get more Metro events.

Is it perhaps possible to queue the processing somehow? I'll try to write some pseudocode to show what I mean:

var inputting;

function input(callback) { // provided by OS
  if (inputting)
    throw "illegal recursion";
  inputting = read();
  callback(inputting);
  inputting = null;
}

function process(event) { // provided by MetroFox
  switch (event.type) {
    // etc.
  }
}

Old style:

function loop() {
  while (waiting())
    input(process);
}

New style:

var global;

function save(event) {
  global = event;
}

function loop() {
  while (waiting()) {
    input(save);
    process(global);
  }
}

This makes it safe for process() to recursively invoke eventloop(), because the call to input will no longer be on the stack.

	Jim Mathies [:jimm] Assignee
	Comment 19 • 11 years ago

This problem surrounds getting to the windows message dispatch loop we have down in app shell. The two Windows app shells we have are very similar - 

desktop:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsAppShell.cpp#159

metro:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroAppShell.cpp#130

base app shell handles calling into ProcessNextNativeEvent(bool mayWait) when it's ready to dispatch Windows events. Obviously we have to call into this regularly or the ui freeze due to Windows event queue starvation. While we're running normally in metrofx, everything works as expected, base app shell calls into ProcessNextNativeEvent on a regular basis and that in turn dispatches Windows events that get delivered to widget, which result in gecko ui events being fired. Normal widget stuff.

The problem arises when we drop into a geck created modal dispatch in js or cpp to wait on some event to occur while preventing the stack from unwinding. An alert() call is a perfect example, we don't want the js to unwind until we have the result of the alert click, and to get the alert clicked we have to process Windows message user input. 

In win32 this isn't an issue because we can have multiple calls to ProcessNextNativeEvent on the stack. In metro  / 8.1, microsoft expressly prohibits this, if we have ProcessNextNativeEvent on the stack and call it again, the winrt call we make there [1] will throw.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroAppShell.cpp#111

So we can't recursively call ProcessNextNativeEvent in metrofx. :/ Which doesn't break in a lot of places since we don't do modal gecko thread loops waiting for user input much, but it does break in a few key places, prompts and sync being perfect examples - 

http://mxr.mozilla.org/mozilla-central/source/services/common/async.js#99
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/nsPrompter.js#420

There are a few more of these spread around but most can be avoided / fixed.

Note to get ProcessNextNativeEvent on the stack twice, the first action that triggers the recursive call needs to be from user input (a button click for example) since that's when we're going to drop down into ProcessNextNativeEvent the first time to deliver a Windows event to widget, which heads up into gecko to 'do something' that results in a main thread wait loop that calls processNextEvent on the gecko thread object, which gets us back into baser app shell and a potential call to ProcessNextNativeEvent -

http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsBaseAppShell.cpp#219

	Jim Mathies [:jimm] Assignee
	Comment 20 • 11 years ago

One thing that I'm curious about here is that it is possible to drop into a modal loop and dispatch windowing event. The winrt common dialog calls do this, and ie apparently has done this in a custom prompt dialog they've created. So if we can figure out what they do there that avoids this throw in the dispatcher, we can fix this.

	Jim Mathies [:jimm] Assignee
	Comment 21 • 11 years ago

Maybe they just drop into a standard Windows dispatch loop? Since this would be outside of the winrt dispatcher, no winrt async events would fire which is fine.

	neil@parkwaycc.co.uk
	Comment 22 • 11 years ago

(In reply to Jim Mathies from comment #19)
> Note to get ProcessNextNativeEvent on the stack twice, the first action that
> triggers the recursive call needs to be from user input
My point exactly. Is it not possible to defer the user input event somehow, e.g. instead of directly invoking an nsEvent, schedule a runnable to invoke it? Then the runnable will not have ProcessNextNativeEvent on its stack.

	Jim Mathies [:jimm] Assignee
	Comment 23 • 11 years ago

Tim, mind taking this for a spin on win 8.1?

	Jim Mathies [:jimm] Assignee
	Comment 24 • 11 years ago

(In reply to neil@parkwaycc.co.uk from comment #22)
> (In reply to Jim Mathies from comment #19)
> > Note to get ProcessNextNativeEvent on the stack twice, the first action that
> > triggers the recursive call needs to be from user input
> My point exactly. Is it not possible to defer the user input event somehow,
> e.g. instead of directly invoking an nsEvent, schedule a runnable to invoke
> it? Then the runnable will not have ProcessNextNativeEvent on its stack.

I don't think this works around the problem because we want to avoid calls to dispatcher->ProcessEvents, which delivers windows events to widget which in turn trigger gecko ui events. Trapping in the windows procedure after a call to dispatch is too late. Is that what you're suggesting?

	neil@parkwaycc.co.uk
	Comment 25 • 11 years ago

(In reply to Jim Mathies from comment #24)
> I don't think this works around the problem because we want to avoid calls
> to dispatcher->ProcessEvents, which delivers windows events to widget which
> in turn trigger gecko ui events. Trapping in the windows procedure after a
> call to dispatch is too late. Is that what you're suggesting?

I don't think it is because I don't understand what you mean by trapping in the windows procedure. Let me try another way:

Gecko Event Loop -> Process Events -> Window Procedure -> Gecko Event

The problem here is that the Gecko event might attempt to re-enter the Gecko event loop. So the idea is to post a runnable instead.

Gecko Event Loop -> Process Events -> Window Procedure -> Runnable Posted
Gecko Event Loop -> Runnable Processed -> Gecko Event

	Jim Mathies [:jimm] Assignee
	Comment 26 • 11 years ago

(In reply to neil@parkwaycc.co.uk from comment #25)
> (In reply to Jim Mathies from comment #24)
> > I don't think this works around the problem because we want to avoid calls
> > to dispatcher->ProcessEvents, which delivers windows events to widget which
> > in turn trigger gecko ui events. Trapping in the windows procedure after a
> > call to dispatch is too late. Is that what you're suggesting?
> 
> I don't think it is because I don't understand what you mean by trapping in
> the windows procedure. Let me try another way:
> 
> Gecko Event Loop -> Process Events -> Window Procedure -> Gecko Event
> 
> The problem here is that the Gecko event might attempt to re-enter the Gecko
> event loop. So the idea is to post a runnable instead.
> 
> Gecko Event Loop -> Process Events -> Window Procedure -> Runnable Posted
> Gecko Event Loop -> Runnable Processed -> Gecko Event

I see, you're suggesting we post a runnable for every input event. Yes that's a possible fix. I wonder what the added overhead would be.

	Jim Mathies [:jimm] Assignee
	Comment 27 • 11 years ago

The complexity of dealing with events coming into the window procedure might be interesting. We do something similar to this in the ipc code. Biggest issue I think would be with events that carry data that is volatile.

http://mxr.mozilla.org/mozilla-central/source/ipc/glue/WindowsMessageLoop.cpp#164

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 28 • 11 years ago

I really like the approach being discussed (posting a runnable for each Windows event): It fixes all consumers of MetroAppShell::ProcessNextNativeEvent in one swoop, and it brings metroFx more in line with the ipc way of doing things.

I think one reason that Chrome hasn't had issues with nested calls to ProcessEvents is that they implicitly - checkout line 417 (ChromeAppViewAsh::Run()) at [1].

[1] http://src.chromium.org/viewvc/chrome/trunk/src/win8/metro_driver/chrome_app_view_ash.cc?revision=217155

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 29 • 11 years ago

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #28)
> I think one reason that Chrome hasn't had issues with nested calls to
> ProcessEvents is that they implicitly [...]

This should read more like "I think one reason that Chrome hasn't had issues with nested calls to ProcessEvents is that they already post runnables for all of their input, since they're sending all input across ipc."

	Jim Mathies [:jimm] Assignee
	Comment 30 • 11 years ago

Found another rather ugly version of one of these dispatch loops via bug 909665. We apparently drop into a modal dispatch loop in nsXMLHttpRequest::Send().

	Jim Mathies [:jimm] Assignee
	Comment 38 • 11 years ago

I still have win32 module use to deal with in MetroWidget, and some resorting / cleanup / testing. But generally based on some testing with the alert test cases tabraldes pointed me at this appears to be working well. I'm also not seeing any perceptible increase in lag which is good, we'll just have to see how it goes once we get this landed.

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 39 • 11 years ago

This looks really great, Jim!

For the gestures (magnification gesture, rotation gesture), I think we're going to be switching to APZC's recognizer instead of using Windows' GestureRecognizer, so don't work too hard getting those ones working.

For touch input, you can use the attached html page as a test (just use your fingers to draw on the page).

	Jim Mathies [:jimm] Assignee
	Comment 40 • 11 years ago

There's a problem with part 1 of this work. In the existing code base when the browser is idle the metro main thread falls all the way down to the base call to ProcessEvents - 

http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/FrameworkView.cpp#141

In the new code, we've taken more control over event dispatches, so in stead we end up in the WaitForMessage call in MetroAppShell's ProcessNextNativeEvent. 

Unfortunately this wait doesn't pick up on certain events that might occur, specifically edge gestures, which don't come in as normal input. 

I've mucked around quite a bit with this wait logic to try and pick these events up but so far haven't come up with anything. I'm not even sure these come in as events, they may be wait handles that the dispatcher knows about. Still trying to sort this out. 

It's unfortunate that the dispatcher doesn't give us a "wait for one event and dispatch" option, which could replace our wait logic.

	Jim Mathies [:jimm] Assignee
	Comment 41 • 11 years ago

One somewhat radical approach to this might be extend the message relay work here to every event we receive from winrt / the core window subclass. Then we could split the winrt main thread from a new gecko main thread we would create at startup.

	Jim Mathies [:jimm] Assignee
	Comment 42 • 11 years ago

(In reply to Jim Mathies [:jimm] from comment #41)
> One somewhat radical approach to this might be extend the message relay work
> here to every event we receive from winrt / the core window subclass. Then
> we could split the winrt main thread from a new gecko main thread we would
> create at startup.

Tricky part of this is the main widget. We'd need a widget proxy on the gecko side that forwards method calls on the widget over to the metro main thread window. :/ Not insurmountable since we don't do much to widget. I think the gecko event work I've already done is reusable, we could set the widget pointer after the event was forwarded over to the gecko side.

http://mxr.mozilla.org/mozilla-central/search?find=%2Fwidget%2Fwindows%2Fwinrt%2F&string=mWidget

	Jim Mathies [:jimm] Assignee
	Comment 43 • 11 years ago

Before diving off the deep end on full thread separation I did a bit more debugging of the winrt dispatch loop. A couple of interesting notes - 

1) The dispatcher polls CoHandlePriorityEventsFromMessagePump() [1] which appears to handle inter-thread com dispatching. This handler is responsible for edge gesture events in the ui.
2) The dispatcher uses MsgWaitForMultipleObjectsEx for queue status, but it never calls it with dwMilliseconds set to INFINITE, instead it relies on a different delay mechanism for setting up calls to MsgWaitForMultipleObjectsEx that basically polls this routine. The poll period seemed to be around ~250ms or so. This polling also insures CoHandlePriorityEventsFromMessagePump gets polled regularly.
3) In calling MsgWaitForMultipleObjectsEx  the dispatcher passed some new undocumented win8 QS values, specifically QS_TOUCH and QS_POINTER.

Based on all of this, I put together a new rev of ProcessNextNativeEvent that mimics all of this behavior. With these changes everything seems to be working correctly include edge gesture events.

Long term, I think thread separation is the way to go. This would allow us to use the winrt dispatcher rather than try to mimic it's logic, and keep using our standard gecko event processing without having to process input events from winrt. I think this would also help us with apz performance. *But* I'm not willing to make these kinds of changes right now, so I think we should go with the modified ProcessNextNativeEvent approach for uplift and work on thread separation in v2.

[1] http://msdn.microsoft.com/en-us/library/windows/desktop/hh404140%28v=vs.85%29.aspx

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 44 • 11 years ago

Thanks for the write-up, Jim!

It's exciting that we can mimic the functionality of ProcessEvents well enough to get ProcessNextNativeEvent working. I agree that this seems like the best approach for v1, and we can look at async-ifying our Window message handling for v2.

	Jim Mathies [:jimm] Assignee
	Comment 46 • 11 years ago

One shortcoming of this keyboard handling is we ignore prevent default calls on keydown events. Not sure if that needs addressing here, I might file it as a follow up.

	Jim Mathies [:jimm] Assignee
	Comment 49 • 11 years ago

Note the removal of FrameworkViewGfx. This code wasn't in use much aside from an initial call to Render() on startup. Layout doesn't invalidate this widget so the render code is called maybe once and that's it.  UpdateRenderMode() isn't really in use either, which doesn't seem to be an issue. I need to dig up that bug on it and figure out why we should be calling it and when we should call it based on all the gfx changes we've made.

	Jim Mathies [:jimm] Assignee
	Comment 50 • 11 years ago

This param was not in use, so I just got rid of it to get it out of the way for future changes.

	Jim Mathies [:jimm] Assignee
	Comment 51 • 11 years ago

Note I do MetroInput.h header cleanup in the final touch patch. In this patch I'm just tossing stuff in there.

	Jim Mathies [:jimm] Assignee
	Comment 56 • 11 years ago

Noticed I was appending mTouches to touchend events which isn't needed, since we add one touch point in the event handler.

	Jim Mathies [:jimm] Assignee
	Comment 57 • 11 years ago

Some additional code reshuffling to make these easier to read.

	Jim Mathies [:jimm] Assignee
	Comment 59 • 11 years ago

Left this comment as a note to myself to test this. It's working fine.

	Jim Mathies [:jimm] Assignee
	Comment 60 • 11 years ago

Without the DEBUG_delay_start_metro.

	Jim Mathies [:jimm] Assignee
	Comment 61 • 11 years ago

with the right patch file. :)

	Jim Mathies [:jimm] Assignee
	Comment 62 • 11 years ago

Now that we're using ipc's MessagePump as our run loop we can clean out some of the stuff we were doing in MetroAppShell to handle shutdown. Base app shell now receives the Exit call which shuts down MessagePump and returns the thread to us from nsBaseAppShell::Run(). We then shutdown xpcom and free the thread. Winrt returns the application thread to us after that.

	Jim Mathies [:jimm] Assignee
	Comment 63 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=fda0ceaeb162

	Brian R. Bondy [:bbondy]
	Comment 64 • 11 years ago

Comment on attachment 797815 [details] [diff] [review]
part 8 - activation

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

::: widget/windows/winrt/FrameworkView.cpp
@@ -343,5 @@
>                             IActivatedEventArgs* aArgs)
>  {
>    LogFunction();
>  
> -  // XXX need to be sure this happen late enough

I think you may not have what you meant to have in this patch. This comment removal is the only thing.

	Brian R. Bondy [:bbondy]
	Comment 65 • 11 years ago

Comment on attachment 797838 [details] [diff] [review]
part 9 - shutdown

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

::: widget/windows/winrt/FrameworkView.h
@@ -82,5 @@
>  
>    // Public apis for MetroWidget
>    void ShutdownXPCOM();
> -  bool Render();
> -  bool Render(const nsIntRegion& aInvalidRegion);

I think this removal was meant to be in another patch

	Jim Mathies [:jimm] Assignee
	Comment 66 • 11 years ago

(In reply to Brian R. Bondy [:bbondy] from comment #64)
> Comment on attachment 797815 [details] [diff] [review]
> part 8 - activation
> 
> Review of attachment 797815 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/winrt/FrameworkView.cpp
> @@ -343,5 @@
> >                             IActivatedEventArgs* aArgs)
> >  {
> >    LogFunction();
> >  
> > -  // XXX need to be sure this happen late enough
> 
> I think you may not have what you meant to have in this patch. This comment
> removal is the only thing.

Yeah that was the right patch. :) I just wanted to let you know I got around to testing this (which was added in the base patch.)

	Jim Mathies [:jimm] Assignee
	Comment 67 • 11 years ago

(In reply to Brian R. Bondy [:bbondy] from comment #65)
> Comment on attachment 797838 [details] [diff] [review]
> part 9 - shutdown
> 
> Review of attachment 797838 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/winrt/FrameworkView.h
> @@ -82,5 @@
> >  
> >    // Public apis for MetroWidget
> >    void ShutdownXPCOM();
> > -  bool Render();
> > -  bool Render(const nsIntRegion& aInvalidRegion);
> 
> I think this removal was meant to be in another patch

Found those while cleaning up, they belong in the base patch.

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 69 • 11 years ago

Comment on attachment 797783 [details] [diff] [review]
part 3 - mouse input plus base event handling

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

Looks good!

::: widget/windows/winrt/MetroInput.cpp
@@ +224,3 @@
>    DispatchEventIgnoreStatus(&geckoEvent);
>    return S_OK;
>  }

nit: This comment won't make sense to us (or new contributors) later. There are 2 other occurrences of this. Maybe we could make it say something like "We don't need to dispatch this event async. See MetroAppShell::ProcessNextNativeEvent" for more details" (of course, we'd need to add a comment there with more details). I'm also OK with just deleting this comment (and the other 2).

@@ -1013,5 @@
>  
> -// This function allows us to call MetroWidget's DispatchEvent function
> -// without passing in a status.  It uses a static nsEventStatus whose value
> -// is never read.  This allows us to avoid the (admittedly small) overhead
> -// of creating a new nsEventStatus every time we dispatch an event.

nit: Was this comment block deleted on purpose?

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 70 • 11 years ago

Comment on attachment 797784 [details] [diff] [review]
part 4 - gesture input

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

Looks good!

::: widget/windows/winrt/MetroInput.cpp
@@ +956,5 @@
>  #ifdef DEBUG_INPUT
>    LogFunction();
>  #endif
>  
> +  // send mousemove

nit: Let's make these 3 comments match. Either "send the mousemove, send the mousedown, send the mouseup" or "send mousemove, send mousedown, send mouseup"

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 71 • 11 years ago

Comment on attachment 797797 [details] [diff] [review]
part 5 - touch input

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

I applied the rollup patch and tested using attachment 796922 [details], and I found that multi-touch doesn't work. I think this is due to us always sending NS_TOUCH_MOVE when sometimes we need to send NS_TOUCH_START. r- for now due to that issue.

There are a couple other minor issues, but nothing that should stop us from landing this.

::: widget/windows/winrt/MetroInput.cpp
@@ +414,1 @@
>    }

It looks like we're using CompleteGesture in a way that isn't really intended. In our handling of PointerPressed, we call mGestureRecognizer->ProcessDownEvent unconditionally, and then in our callback we call mGestureRecognizer->CompleteGesture if preventDefault was called. I can't think of a better way to deal with this issue, but I think we should point out (with a more descriptive comment) why we're using CompleteGesture this way.

@@ +456,1 @@
>    }

We don't always want to dispatch a NS_TOUCH_MOVE here. We might have a touchstart pending, in which case we'd want to send a NS_TOUCH_START.

Two solutions come to mind:
  1. Keep track of the event type that we should send (NS_TOUCH_MOVE or NS_TOUCH_START) in a member variable
  2. Dispatch touchstart events as soon as they arrive (that way there will never be a touchstart pending when we get to this piece of code)

@@ +529,1 @@
>    }

Same as above; we might need to send NS_TOUCH_START instead of NS_TOUCH_MOVE here.

@@ +536,5 @@
>    // and store our mTouchMoveDefaultPrevented value
>    if (mIsFirstTouchMove) {
> +    nsTouchEvent* touchEvent =
> +      new nsTouchEvent(true, NS_TOUCH_MOVE, mWidget.Get());
> +    DispatchAsyncTouchEventWithCallback(touchEvent, &MetroInput::OnFirstPointerMoveCallback);

Same as above; we might need to send NS_TOUCH_START instead of NS_TOUCH_MOVE here.

@@ +561,1 @@
>    }

This breaks the mTouchMoveDefaultPrevented/mTouchStartDefaultPrevented distinction. The way things were, if a page called preventDefault on the first touchmove event, the sequence of touch input could still be interpreted as a tap, so MetroInput::OnTapped would be called and we would send mousedown/mousemove/mouseup. The way things are with this patch applied, calling preventDefault on the first touchmove causes mGestureRecognizer->CompleteGesture to be called, which means that MetroInput::OnTapped will not be called and we won't send the mouse events.

It's not the worst thing in the world, given how broken touch events are in general, but let's file a followup bug to get preventDefault working again.

@@ +1064,5 @@
>  }
>  
> +nsEventStatus
> +MetroInput::DispatchNextQueuedEvent()
> +{

I don't see where this function is used. Is this supposed to be part of this patch?

@@ +1076,5 @@
> +
> +void
> +MetroInput::DispatchAsyncTouchEventIgnoreStatus(nsTouchEvent* aEvent, bool aAddTouches)
> +{
> +  if (aAddTouches) {

nit: I'm not a fan of passing bool args that change the behavior of a function: It makes it harder to see what the callers are actually trying to accomplish. I'd prefer to see this split into MetroInput::InitTouchEventTouchList(nsTouchEvent*) and MetroInput::DispatchAsyncTouchEventIgnoreStatus(nsTouchEvent*)

Any consumers that would have called this function with aAddTouches==true will call both of the above functions, and any other consumer will just call the second function

@@ +1100,5 @@
> +  mWidget->DispatchEvent(event, status);
> +  if (status != nsEventStatus_eConsumeNoDefault && MetroWidget::sAPZC) {
> +    MultiTouchInput inputData(*event);
> +    MetroWidget::sAPZC->ReceiveInputEvent(inputData);
> +  }

I love how much cleaner our APZC gesture recognition is than our GestureRecognizer gesture recognition. Some day I hope we'll just have one gesture recognizer :)

::: widget/windows/winrt/MetroInput.h
@@ +270,5 @@
> +  void OnPointerPressedCallback();
> +  void OnFirstPointerMoveCallback();
> +
> +  // Sync event dispatching
> +  void DispatchPendingTouchEvent(nsTouchEvent& aEvent, nsEventStatus& status);

I think the implementation for this got removed. We should remove the declaration too

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

Comment on attachment 797801 [details] [diff] [review]
part 6 - scroll input

>diff -r 557a27078272 widget/windows/winrt/MetroWidget.cpp
>+DispatchMsg*
>+MetroWidget::CreateDispatchMsg(UINT aMsg, WPARAM aWParam, LPARAM aLParam)
>+{
>+  switch (aMsg) {
>+    case WM_SETTINGCHANGE:
>+    case WM_MOUSEWHEEL:
>+    case WM_MOUSEHWHEEL:
>+    case WM_HSCROLL:
>+    case WM_VSCROLL:
>+    case MOZ_WM_HSCROLL:
>+    case MOZ_WM_VSCROLL:
>+    case WM_KEYDOWN:
>+    case WM_KEYUP:
>+    // MOZ_WM events are plugin specific, we keep them for completness
>+    case MOZ_WM_MOUSEVWHEEL:
>+    case MOZ_WM_MOUSEHWHEEL:
>+      return new DispatchMsg(aMsg, aWParam, aLParam);
>+      break;

nit: you don't need the |break;|

>+    default:
>+      NS_RUNTIMEABORT("Unknown event being passed to CreateDispatchMsg.");
>+      return nullptr;
>+  }

Just my curious, why don't you use MOZ_CRASH()?

>+void
>+MetroWidget::DeliverNextScrollEvent()
>+{
>+  DispatchMsg* msg = static_cast<DispatchMsg*>(mInputEventQueue.PopFront());
>+  MOZ_ASSERT(msg);
>+  LRESULT dummyResult;
>+  MSGResult msgResult(&dummyResult);

You can write this line must |MSGResult msgResult;|. When the argument is omitted or nullptr, its internal variable is used. So, you can remove the previous line.

>+  MouseScrollHandler::ProcessMessage(this, msg->mMsg, msg->mWParam, msg->mLParam, msgResult);
>+  delete msg;
>+}
>+
> // static
> LRESULT CALLBACK
> MetroWidget::StaticWindowProcedure(HWND aWnd, UINT aMsg, WPARAM aWParam, LPARAM aLParam)
> {
>   MetroWidget* self = reinterpret_cast<MetroWidget*>(
>     GetProp(aWnd, kMetroSubclassThisProp));
>   if (!self) {
>     NS_NOTREACHED("Missing 'this' prop on subclassed metro window, this is bad.");
>@@ -584,20 +649,24 @@
> 
>   // Indicates if we should hand messages to the default windows
>   // procedure for processing.
>   bool processDefault = true;
> 
>   // The result returned if we do not do default processing.
>   LRESULT processResult = 0;
> 
>-  MSGResult msgResult(&processResult);
>-  MouseScrollHandler::ProcessMessage(this, aMsg, aWParam, aLParam, msgResult);
>-  if (msgResult.mConsumed) {
>-    return processResult;
>+  // We ignore return results from the scroll module and pass everything
>+  // to mMetroWndProc. These fall through to winrt handlers that generate
>+  // input events in MetroInput. Since we have no listeners for scroll
>+  // events no processing should occur. For now processDefault must be left
>+  // true since the mouse module cinsumes non-mouse wheel related events.

nit: consumes?

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

Comment on attachment 797802 [details] [diff] [review]
part 7 - keyboard input

Hmm, this completely breaks the relation of keydown and keypress events. When keydown event's preventDefault() is called, following keypress event(s) MUST NOT be dispatched.

If MetroWidget always need to dispatch events asynchronously, DeliverNextKeyboardEvent() should remove following keypress events from the queue when keydown event is consumed.

	Jim Mathies [:jimm] Assignee
	Comment 74 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #73)
> Comment on attachment 797802 [details] [diff] [review]
> part 7 - keyboard input
> 
> Hmm, this completely breaks the relation of keydown and keypress events.
> When keydown event's preventDefault() is called, following keypress event(s)
> MUST NOT be dispatched.
> 
> If MetroWidget always need to dispatch events asynchronously,
> DeliverNextKeyboardEvent() should remove following keypress events from the
> queue when keydown event is consumed.

I don't think we can do this in the keyboard code since the key events may be processed and pending in mInputEventQueue before the first keydown event is delivered. If you feel we need to solve this here, I'll have to link these events up in the queue somehow so that they can be removed. (Was hoping to do this in follow up actually.)

	Jim Mathies [:jimm] Assignee
	Comment 75 • 11 years ago

(In reply to Jim Mathies [:jimm] from comment #74)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #73)
> > Comment on attachment 797802 [details] [diff] [review]
> > part 7 - keyboard input
> > 
> > Hmm, this completely breaks the relation of keydown and keypress events.
> > When keydown event's preventDefault() is called, following keypress event(s)
> > MUST NOT be dispatched.
> > 
> > If MetroWidget always need to dispatch events asynchronously,
> > DeliverNextKeyboardEvent() should remove following keypress events from the
> > queue when keydown event is consumed.
> 
> I don't think we can do this in the keyboard code since the key events may
> be processed and pending in mInputEventQueue before the first keydown event
> is delivered. If you feel we need to solve this here, I'll have to link
> these events up in the queue somehow so that they can be removed. (Was
> hoping to do this in follow up actually.)

http://www.w3.org/TR/DOM-Level-3-Events/#keys-cancelable_keys

I guess I can take a shot at this here. If I can match these up in the keyboard code using a unique id stored in the event, I can search the queue for like events and dump them.

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

(In reply to Jim Mathies [:jimm] from comment #74)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #73)
> > Comment on attachment 797802 [details] [diff] [review]
> > part 7 - keyboard input
> > 
> > Hmm, this completely breaks the relation of keydown and keypress events.
> > When keydown event's preventDefault() is called, following keypress event(s)
> > MUST NOT be dispatched.
> > 
> > If MetroWidget always need to dispatch events asynchronously,
> > DeliverNextKeyboardEvent() should remove following keypress events from the
> > queue when keydown event is consumed.
> 
> I don't think we can do this in the keyboard code since the key events may
> be processed and pending in mInputEventQueue before the first keydown event
> is delivered. If you feel we need to solve this here, I'll have to link
> these events up in the queue somehow so that they can be removed. (Was
> hoping to do this in follow up actually.)

If the follow up will be fixed by next merge, it's okay to do it in another bug.

Then, request review again, then, I'll review it again.

	Jim Mathies [:jimm] Assignee
	Comment 77 • 11 years ago

nits addressed.

	Jim Mathies [:jimm] Assignee
	Comment 79 • 11 years ago

Updated to support blocking keypress events if preventDefault is called on keydown.

	Jim Mathies [:jimm] Assignee
	Comment 80 • 11 years ago

Comment on attachment 799040 [details] [diff] [review]
part 7 - add a remove object call to nsDeque

frack, sorry those two got intermixed.

	Jim Mathies [:jimm] Assignee
	Comment 83 • 11 years ago

I had to split the two que objects up since they were holding different object types. I'll work on something more uniform in bug 911133.

	Jim Mathies [:jimm] Assignee
	Comment 84 • 11 years ago

Comment on attachment 797797 [details] [diff] [review]
part 5 - touch input

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

::: widget/windows/winrt/MetroInput.cpp
@@ +414,1 @@
>    }

It's not well named but it seems to do what I want it too - clear and reset the state of the recognizer without firing any gesture events.

@@ +456,1 @@
>    }

Ah, ok. I didn't realize OnPointerPressed only posted the first touch start. Will rework this a bit.

@@ +1064,5 @@
>  }
>  
> +nsEventStatus
> +MetroInput::DispatchNextQueuedEvent()
> +{

Removed this, it was unused.

@@ +1076,5 @@
> +
> +void
> +MetroInput::DispatchAsyncTouchEventIgnoreStatus(nsTouchEvent* aEvent, bool aAddTouches)
> +{
> +  if (aAddTouches) {

No problem, I'll split them up.

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 87 • 11 years ago

Comment on attachment 799530 [details] [diff] [review]
part 5 - touch input

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

This is almost there, but it causes a perf regression that we should fix before landing.

::: widget/windows/winrt/MetroInput.cpp
@@ +488,5 @@
> +    DispatchAsyncTouchEventWithCallback(touchEvent, &MetroInput::OnFirstPointerMoveCallback);
> +    mIsFirstTouchMove = false;
> +  } else {
> +    InitTouchEventTouchList(touchEvent);
> +    DispatchAsyncTouchEventIgnoreStatus(touchEvent);

It looks like we're sending a touchmove event every time we get a PointerMoved event, which means we're going to run into the same performance issues with multi-touch that we had in the early days of our touch input implementation. To verify the performance issue, use the attached test page and drag a single finger across the screen, adding fingers as you go. Note how quickly performance degrades as you add fingers.

We should send a touchmove if one of the following is true:
  - This is the first touchmove of a touch session (so we can see if preventDefault is called)
  - This touch already has its mChanged member set to true

Otherwise we should create the new touch point, put in in mTouches, and set its mChanged to true, but we should not dispatch a touch event. This allows us to send a bunch of touchmove information all at once, reducing overhead.

::: widget/windows/winrt/MetroInput.h
@@ +217,5 @@
>    // upon receiving PointerPressed or PointerMoved.  Instead, we store
>    // the updated touchpoint info and record the fact that the touchpoint
>    // has changed.  If ever we try to update a touchpoint has already
>    // changed, we dispatch a touch event containing all the changed touches.
> +  void InitTouchEventTouchList(nsTouchEvent* aEvent);

nit: The long comment here is meant to explain why we keep an event around and dispatch it only when we really have to. It doesn't really apply to InitTouchEventTouchList. Please move it somewhere that makes more sense (maybe into the cpp file where we actually send touchmove events?)

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

Comment on attachment 799054 [details] [diff] [review]
part 7.2 - keyboard input

>diff -r a6b1b9abff80 widget/nsGUIEvent.h
>--- a/widget/nsGUIEvent.h	Tue Sep 03 14:35:06 2013 -0500
>+++ b/widget/nsGUIEvent.h	Tue Sep 03 14:36:20 2013 -0500
>@@ -1091,16 +1091,21 @@
>   // handling. The handlers will try from first character to last character.
>   nsTArray<nsAlternativeCharCode> alternativeCharCodes;
>   // indicates whether the event signifies a printable character
>   bool            isChar;
>   // DOM KeyboardEvent.key
>   mozilla::widget::KeyNameIndex mKeyNameIndex;
>   // OS-specific native event can optionally be preserved
>   void*           mNativeKeyEvent;
>+  // Unique id associated with a keydown / keypress event. Used in identifing
>+  // keypress events for removal from async event dispatch queue in metrofx
>+  // after preventDefault is called on keydown events. It's ok if this wraps
>+  // over long periods.
>+  uint32_t        uniqueId;

Could you initialize this in constructor for other platforms?

>diff -r a6b1b9abff80 widget/nsGUIEventIPC.h
>--- a/widget/nsGUIEventIPC.h	Tue Sep 03 14:35:06 2013 -0500
>+++ b/widget/nsGUIEventIPC.h	Tue Sep 03 14:36:20 2013 -0500

I'm not sure if these changes in this file is necessary. But let's take them.

>diff -r a6b1b9abff80 widget/windows/winrt/MetroWidget.cpp
>--- a/widget/windows/winrt/MetroWidget.cpp	Tue Sep 03 14:35:06 2013 -0500
>+++ b/widget/windows/winrt/MetroWidget.cpp	Tue Sep 03 14:36:20 2013 -0500
>@@ -620,16 +620,73 @@
> {
>   DispatchMsg* msg = static_cast<DispatchMsg*>(mMsgEventQueue.PopFront());
>   MOZ_ASSERT(msg);
>   MSGResult msgResult;
>   MouseScrollHandler::ProcessMessage(this, msg->mMsg, msg->mWParam, msg->mLParam, msgResult);
>   delete msg;
> }
> 
>+// defined in nsWiondowBase, called from shared module KeyboardLayout.
>+bool
>+MetroWidget::DispatchKeyboardEvent(nsGUIEvent* aEvent)
>+{
>+  MOZ_ASSERT(aEvent);
>+  nsKeyEvent* event = new nsKeyEvent(*(static_cast<nsKeyEvent*>(aEvent)));

I think this is wrong because there is nsCOMPtr<nsIWidget>. I think that you should do:

nsKeyEvent* oldKeyEvent = static_cast<nsKeyEvent*>(aEvent);
nsKeyEvent* newKeyEvent = new nsKeyEvent(oldKeyEvent->mFlags.mIsTrusted,
                                         oldKeyEvent->message,
                                         oldKeyEvent->widget);
newKeyEvent->AssignKeyEventData(*oldKeyEvent, true);
// pluginEvent references a pointer of NPEvent created on the stack.
// So, it has to be copied to new instance.
if (oldEvent->pluginEvent) {
  // XXX need to release the new instance at destructing the newKeyEvent
  newKeyEvent->pluginEvent = new NPEvent(oldEvent->pluginEvent);
}

>+void
>+MetroWidget::DeliverNextKeyboardEvent()
>+{
>+  nsKeyEvent* event = static_cast<nsKeyEvent*>(mKeyEventQueue.PopFront());
>+  if (!event) {
>+    // This can happen if a keypress was previously removed.
>+    return;
>+  }
>+  
>+  if (DispatchWindowEvent(event) && event->message == NS_KEY_DOWN) {
>+    // keydown event are followed by keypress events which shouldn't
>+    // be sent if preventDefault is called on keydown.
>+    uint32_t id = event->uniqueId;
>+    KeyQueueIdQuery query(id);
>+    nsKeyEvent* found =
>+      static_cast<nsKeyEvent*>(const_cast<void*>(mKeyEventQueue.FirstThat(query)));
>+    if (found) {
>+      MOZ_ASSERT(found->message == NS_KEY_PRESS);
>+      mKeyEventQueue.RemoveObject(static_cast<void*>(found));
>+    }

You need to check second or third keypress events too. One keydown event may cause two or more keypress events with dead key or non-BMP character input.
http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#1282

Even if you fix this issue, but it doesn't work fine when you type same dead key twice (e.g., "^^" with Spanish keyboard layout), the reason is why second character is not handled in NativeKey::HandleKeyDownMessage(). Instead of that, following WM_CHAR message is handled as an independent WM_CHAR message. If you see this problem, let me know it. I'll fix it in NativeKey::HandleKeyDownMessage().

	Jim Mathies [:jimm] Assignee
	Comment 89 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #88)
> >+  nsKeyEvent* event = new nsKeyEvent(*(static_cast<nsKeyEvent*>(aEvent)));
> 
> I think this is wrong because there is nsCOMPtr<nsIWidget>. I think that you
> should do:

The assignment operator handled this in testing. I can check again but everything, including the widget were getting copied over properly.

	Jim Mathies [:jimm] Assignee
	Comment 90 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #88)
> Comment on attachment 799054 [details] [diff] [review]
> part 7.2 - keyboard input
> 
> >+    // keydown event are followed by keypress events which shouldn't
> >+    // be sent if preventDefault is called on keydown.
> >+    uint32_t id = event->uniqueId;
> >+    KeyQueueIdQuery query(id);
> >+    nsKeyEvent* found =
> >+      static_cast<nsKeyEvent*>(const_cast<void*>(mKeyEventQueue.FirstThat(query)));
> >+    if (found) {
> >+      MOZ_ASSERT(found->message == NS_KEY_PRESS);
> >+      mKeyEventQueue.RemoveObject(static_cast<void*>(found));
> >+    }
> 
> You need to check second or third keypress events too. One keydown event may
> cause two or more keypress events with dead key or non-BMP character input.
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.
> cpp#1282

Hmm, rats, I was hoping I wouldn't have to do this since nsDeque has sucky support for removing elements. :/ Will update.

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

(In reply to Jim Mathies [:jimm] from comment #89)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #88)
> > >+  nsKeyEvent* event = new nsKeyEvent(*(static_cast<nsKeyEvent*>(aEvent)));
> > 
> > I think this is wrong because there is nsCOMPtr<nsIWidget>. I think that you
> > should do:
> 
> The assignment operator handled this in testing. I can check again but
> everything, including the widget were getting copied over properly.

Check if the ref count is increased. And anyway, you need to make new instance for pluginEvent if it's necessary for Metrofox. If it's not necessary, set nullptr.

	Jim Mathies [:jimm] Assignee
	Comment 92 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #91)
> (In reply to Jim Mathies [:jimm] from comment #89)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #88)
> > > >+  nsKeyEvent* event = new nsKeyEvent(*(static_cast<nsKeyEvent*>(aEvent)));
> > > 
> > > I think this is wrong because there is nsCOMPtr<nsIWidget>. I think that you
> > > should do:
> > 
> > The assignment operator handled this in testing. I can check again but
> > everything, including the widget were getting copied over properly.
> 
> Check if the ref count is increased. And anyway, you need to make new
> instance for pluginEvent if it's necessary for Metrofox. If it's not
> necessary, set nullptr.

No plugin support in metro so this shouldn't be an issue, and I think it was being set to nullptr with the assignment.

I considered disabling the plugin event code for these events but didn't want to mess around in keyboard code, and I don't think there's much overhead there anyway.

	Jim Mathies [:jimm] Assignee
	Comment 93 • 11 years ago

Hmm, strange, I don't see why we have to do this - 

http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsDOMEvent.cpp#531

*but* seeing as how dom event has this behavior, I'll duplicate it here. No harm.

	Jim Mathies [:jimm] Assignee
	Comment 94 • 11 years ago

Built on top of the original patch.

	Jim Mathies [:jimm] Assignee
	Comment 95 • 11 years ago

Comment on attachment 800066 [details] [diff] [review]
part 7.3 - keyboard input part 2

I need to copy uniqueId as well.

	Jim Mathies [:jimm] Assignee
	Comment 98 • 11 years ago

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #87)
> Comment on attachment 799530 [details] [diff] [review]
> 
> It looks like we're sending a touchmove event every time we get a
> PointerMoved event, which means we're going to run into the same performance
> issues with multi-touch that we had in the early days of our touch input
> implementation. To verify the performance issue, use the attached test page
> and drag a single finger across the screen, adding fingers as you go. Note
> how quickly performance degrades as you add fingers.
> 
> We should send a touchmove if one of the following is true:
>   - This is the first touchmove of a touch session (so we can see if
> preventDefault is called)
>   - This touch already has its mChanged member set to true
> 
> Otherwise we should create the new touch point, put in in mTouches, and set
> its mChanged to true, but we should not dispatch a touch event. This allows
> us to send a bunch of touchmove information all at once, reducing overhead.

If we add the touch event and set mChanged to true, and send when we receive a move associated with a touch event that has true for mChanged, aren't we just buffering one event? Do we receive a batch of touch pointers such that the next move for the first pointer in the batch triggers a send of the previous batch?

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

Comment on attachment 800069 [details] [diff] [review]
part 7.3 - keyboard input part 2

Great!

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

Comment on attachment 800069 [details] [diff] [review]
part 7.3 - keyboard input part 2

> uniqueId

Er, sorry. Could you rename this as mUniqueID before landing? We've discussed the naming rule of ns*Event's member. Then, we decided that we should use m prefix at adding new member like other classes.

	Jim Mathies [:jimm] Assignee
	Comment 102 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #100)
> Comment on attachment 800069 [details] [diff] [review]
> part 7.3 - keyboard input part 2
> 
> > uniqueId
> 
> Er, sorry. Could you rename this as mUniqueID before landing? We've
> discussed the naming rule of ns*Event's member. Then, we decided that we
> should use m prefix at adding new member like other classes.

sure, np. updated locally.

	Benjamin Smedberg
	Comment 103 • 11 years ago

Comment on attachment 799052 [details] [diff] [review]
part 7.1 - add a remove object call to nsDeque

This method does a linear search *and* a linear resize of the queue. If event processing is backed up at all, this could cause perf slowdowns. Given the use case, I think it would be better to cancel the event (add a canceled flag to nsKeyEvent if necessary) rather than remove it.

	Jim Mathies [:jimm] Assignee
	Comment 104 • 11 years ago

I liked bsmedberg's suggestion - rather than remove the object simply tag and ignore it when it's delivered.

	Jim Mathies [:jimm] Assignee
	Comment 105 • 11 years ago

Missed the assignments in KeyboardLayout.cpp.

	Jim Mathies [:jimm] Assignee
	Comment 106 • 11 years ago

one more try, this time with the right patch file.

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 107 • 11 years ago

(In reply to Jim Mathies [:jimm] from comment #98)
> If we add the touch event and set mChanged to true, and send when we receive
> a move associated with a touch event that has true for mChanged, aren't we
> just buffering one event? Do we receive a batch of touch pointers such that
> the next move for the first pointer in the batch triggers a send of the
> previous batch?

We usually (always?) see a PointerPressed event for each active touch point before we see any repeats. For example, if there are 3 active touchpoints, we would see the following sequence of events:
  PointerMoved for touchpoint 1
  PointerMoved for touchpoint 2
  PointerMoved for touchpoint 3
  PointerMoved for touchPoint 1
  PointerMoved for touchpoint 2
  PointerMoved for touchpoint 3
  etc.

I think what's happening under the hood is that the default WndProc is doing something like this:
  case WM_TOUCH:
    SendPointerPressedEventForEachActiveTouchPoint(lParam, wParam)
    break;

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 108 • 11 years ago

Comment on attachment 800082 [details] [diff] [review]
part 5.1 - touch input part 2

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

Multi-touch perf is muuuuuuuuuuuch better with this patch applied

	Tim Abraldes [:TimAbraldes] [:tabraldes] Reporter
	Comment 109 • 11 years ago

Comment on attachment 799530 [details] [diff] [review]
part 5 - touch input

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

As long as the follow-up touch input patch is applied, this patch works well.

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

Comment on attachment 800342 [details] [diff] [review]
part 7.3 - keyboard input part 2

Hmm, I don't like to add mCancelled. It's really strange member for DOM event staff. I think that you can use nsEvent::mFlags::mPropagationStopped. This becomes true only when DOMEvent.stopPropagation() is called. So, in widget level, this is always false before dispatching the event. And its name isn't so strange for this use case.

	Jim Mathies [:jimm] Assignee
	Comment 111 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #110)
> Comment on attachment 800342 [details] [diff] [review]
> part 7.3 - keyboard input part 2
> 
> Hmm, I don't like to add mCancelled. It's really strange member for DOM
> event staff. I think that you can use nsEvent::mFlags::mPropagationStopped.
> This becomes true only when DOMEvent.stopPropagation() is called. So, in
> widget level, this is always false before dispatching the event. And its
> name isn't so strange for this use case.

Ah nice. I guess I could have created my own flag here if needed rather than the bool too.

	Jim Mathies [:jimm] Assignee
	Comment 113 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=3bc84f5ec163

	Jim Mathies [:jimm] Assignee
	Comment 114 • 11 years ago

remote:   https://hg.mozilla.org/integration/fx-team/rev/b013eb44b2f5
remote:   https://hg.mozilla.org/integration/fx-team/rev/461e637d51c9
remote:   https://hg.mozilla.org/integration/fx-team/rev/c5935a71fc67
remote:   https://hg.mozilla.org/integration/fx-team/rev/3eae40024435
remote:   https://hg.mozilla.org/integration/fx-team/rev/14a18873de49
remote:   https://hg.mozilla.org/integration/fx-team/rev/3fd442a09026
remote:   https://hg.mozilla.org/integration/fx-team/rev/26e59fb638d7
remote:   https://hg.mozilla.org/integration/fx-team/rev/eaad370c10c1
remote:   https://hg.mozilla.org/integration/fx-team/rev/1e80d8e17841

	Ryan VanderMeulen [:RyanVM]
	Comment 115 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/b013eb44b2f5
https://hg.mozilla.org/mozilla-central/rev/461e637d51c9
https://hg.mozilla.org/mozilla-central/rev/c5935a71fc67
https://hg.mozilla.org/mozilla-central/rev/3eae40024435
https://hg.mozilla.org/mozilla-central/rev/14a18873de49
https://hg.mozilla.org/mozilla-central/rev/3fd442a09026
https://hg.mozilla.org/mozilla-central/rev/26e59fb638d7
https://hg.mozilla.org/mozilla-central/rev/eaad370c10c1
https://hg.mozilla.org/mozilla-central/rev/1e80d8e17841