[Win32] Make Edge only process relevant OS messages #1789 #1797
Conversation
|
@amartya4256 here's the reproducer so you can test locally: vi-eclipse/Eclipse-Platform#223 (comment) |
HeikoKlare
force-pushed
the
issue-1789
branch
from
f2b3f97 to
048182f
Compare
| @@ -510,7 +510,7 @@ private static void processOSMessagesUntil(Supplier<Boolean> condition, Consumer | |||
| // The timer call also wakes up the display to avoid being stuck in display.sleep() | |||
| display.timerExec((int) MAXIMUM_OPERATION_TIME.toMillis(), () -> timeoutOccurred.set(true)); | |||
| while (!display.isDisposed() && !condition.get() && !timeoutOccurred.get()) { | |||
| if (OS.PeekMessage(msg, 0, 0, 0, OS.PM_NOREMOVE)) { | |||
| if (OS.PeekMessage(msg, 0, 0, 0, OS.PM_NOREMOVE | OS.PM_QS_POSTMESSAGE)) { | |||
There was a problem hiding this comment.
Just wondering if we can make it more specific to the edge instance by providing a hwnd? But another concern is, does the filtering mean that the possibility of this falling into display.sleep() is higher?
There was a problem hiding this comment.
Might be possible, but we need to be aware that this will not mean that this message is processed by display.readAndDispatch() then, as there messages are processed by priority. Posted messages have high priority and will be processed before, e.g., input events, so if there is a posted message, it will always be processed before an eventual input event also being in the queue.
This change is only avoiding that we run into display.readAndDispatch() without a message being in the queue as that has already been processed via this PeekMessage call.
The possibility of falling into display.sleep() is of course higher, but that's also by intent. We limit processing to specific messages, so instead of processing other messages we will sleep and wait for the next message instead. But since we will wake from display.sleep() whenever a new message arrives, this is not an issue. Or do you have any further concerns here?
There was a problem hiding this comment.
If it keeps waking up with new messages, it should be fine. The only concern was about, in an imaginary scenario, no messages are received for a long time, it means the display thread will be sleeping for that period - which will freeze the UI, right? But of course we need to test it out practically. For me locally it worked just fine so I would be in the favour of merging this and waiting for the feedback from the users.
There was a problem hiding this comment.
Well, if we don't receive messages (which also means that Edge initialization fails), we run into our timeout. That's what we have the display.timerExec() for, setting the timeout value and breaking the loop execution.
There was a problem hiding this comment.
Alright, then its safe from the infiite sleeping problem here.
There was a problem hiding this comment.
By the way I always wondered why there is no Display.sleep(timeout) is it not supported on the OS level? How does a "real" native app handle this case?
There was a problem hiding this comment.
Good question. If you are simply spinning the queue, waiting for messages and processing them, such a timeout should actually not be necessary, as without any messages arriving from the SO there is something completely broken anyway. In the case we have here, if we simply spinned the whole event loop instead of limiting to processing specific messages, we would not run into that situation either (see #1773).
…1789 When waiting for initialization of the WebView2 component in the Edge implementation, OS messages are processed by peeking the queue and performing a display.readAndDispatch() in case a message is present. The call to OS.PeekMessage() does, however, already perform some processing of the messages. In fact, Edge is not supposed to process all kinds of OS messages at all (like paint or input events) but should rather only process the WebView2 initialization callback message, arriving as a posted OS message. This change thus limits the message peeking inside Edge to only process the message type PM_QS_POSTMESSAGE. Fixes eclipse-platform#1789
HeikoKlare
force-pushed
the
issue-1789
branch
from
048182f to
c8eee7d
Compare
When waiting for initialization of the WebView2 component in the Edge implementation, OS messages are processed by peeking the queue and performing a display.readAndDispatch() in case a message is present. The call to OS.PeekMessage() does, however, already perform some processing of the messages. In fact, Edge is not supposed to process all kinds of OS messages at all (like paint or input events) but should rather only process the WebView2 initialization callback message, arriving as a posted OS message. This change thus limits the message peeking inside Edge to only process the message type PM_QS_POSTMESSAGE.
Fixes #1789