Bug 58374 – WebKit2: Clients have to know not to call TranslateMessage() on key messages destined for WebKit2 web views

• Component: WebKit2
• Status: RESOLVED INVALID

[Jeff Miller]

As described in bug 56020, Windows clients of WebKit2 have to know not to call TranslateMessage() on key messages destined for WebKit2 web views.  This is because WebKit2 handles messages asynchronously in the web process, and we don't want to call TranslateMessage() if the WM_KEYDOWN was actually handled by WebKit2 so things like pressing tab in the Web Inspector's console cycles through completion options (as opposed to inserting a tab because TranslateMessage() generates a WM_CHAR message from the WM_KEYDOWN).

If the client installs a WKPageUIClient, it's didNotHandleKeyEvent callback gets called in this case, but things like the Web Inspector typically won't have a WKPageUIClient installed.

The fact that clients have to know not to call TranslateMessage() seems prone to error, we should figure out a better solution to this.

[Jeff Miller]

<rdar://problem/9274510>

[Jeff Miller]

One reason we need to fix this is that client apps can't really control whether TranslateMessage() is called in all cases.  For example, when Windows pumps messages during MessageBox() and DialogBox(), TranslateMessage() will always be called.  It's also possible for non-application code (e.g. a third-party DLL) to pump messages in the application's process, which is out of the application's control.

[Jeff Miller]

The net result is that TranslateMessage() could be called multiple times for the same WM_KEYDOWN, which generates duplicate WM_CHAR messages and leads to symptoms like a character being typed twice in an edit field.

[Jeff Miller]

Created attachment 94351 [details]
Patch

[Jeff Miller]

Hmm, I just realized I need to at least make sure that the WM_CHAR message is for the same HWND as the WM_KEYDOWN, so this patch should probably be r-'d.  However, I'm still interested in feedback about this design in general.

[Alexey Proskuryakov]

It is very suspicious that WM_UNICHAR messages aren't handled here. I don't really know when these are dispatched in practice, but they probably are?

[Jeff Miller]

(In reply to comment #6)
> It is very suspicious that WM_UNICHAR messages aren't handled here. I don't really know when these are dispatched in practice, but they probably are?

<http://msdn.microsoft.com/en-us/library/ms646288(v=vs.85).aspx> says that WM_UNICHAR messages can be posted by TranslateMessage().  However, note that this code deals with WebEvents, not Windows messages.  Specifically, WebEvent::Char events are generated from WM_CHAR, WM_SYSCHAR, and WM_IME_CHAR messages by WebEventFactory.  I don't know if the fact that WebEventFactory doesn't handle WM_UNICHAR is a bug or not, but that's a separate issue.  Since this code deals with WebEvents, and Windows message that's encapsulated by a WebEvent::Char event will be handled.

[Jeff Miller]

(In reply to comment #7)
> (In reply to comment #6)
> > It is very suspicious that WM_UNICHAR messages aren't handled here. I don't really know when these are dispatched in practice, but they probably are?
> 
> <http://msdn.microsoft.com/en-us/library/ms646288(v=vs.85).aspx> says that WM_UNICHAR messages can be posted by TranslateMessage().  However, note that this code deals with WebEvents, not Windows messages.  Specifically, WebEvent::Char events are generated from WM_CHAR, WM_SYSCHAR, and WM_IME_CHAR messages by WebEventFactory.  I don't know if the fact that WebEventFactory doesn't handle WM_UNICHAR is a bug or not, but that's a separate issue.  Since this code deals with WebEvents, and Windows message that's encapsulated by a WebEvent::Char event will be handled.

Now that I look more closely, it looks like WebView::wndProc() doesn't handle WM_UNICHAR messages, which means there is no infrastructure in WK2 for notifying the web process about these.  There should probably be a separate bug on this.

[Adam Roben (:aroben)]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > It is very suspicious that WM_UNICHAR messages aren't handled here. I don't really know when these are dispatched in practice, but they probably are?
> > 
> > <http://msdn.microsoft.com/en-us/library/ms646288(v=vs.85).aspx> says that WM_UNICHAR messages can be posted by TranslateMessage().  However, note that this code deals with WebEvents, not Windows messages.  Specifically, WebEvent::Char events are generated from WM_CHAR, WM_SYSCHAR, and WM_IME_CHAR messages by WebEventFactory.  I don't know if the fact that WebEventFactory doesn't handle WM_UNICHAR is a bug or not, but that's a separate issue.  Since this code deals with WebEvents, and Windows message that's encapsulated by a WebEvent::Char event will be handled.
> 
> Now that I look more closely, it looks like WebView::wndProc() doesn't handle WM_UNICHAR messages, which means there is no infrastructure in WK2 for notifying the web process about these.  There should probably be a separate bug on this.

Some pages from a Google search for "WM_UNICHAR" make me think that WM_UNICHAR is never sent to windows created with ::CreateWindowExW. These windows are supposedly already Unicode-aware so just get Unicode characters through WM_CHAR.

[Adam Roben (:aroben)]

Comment on attachment 94351 [details]
Patch

The general approach here seems fine. It took me a little while to understand the queue-of-queues, perhaps partly because m_unsentCharEventQueue sounds like it is itself a queue of Char events (when instead it is a queue of UnsentCharEvents).

We could perhaps make this simpler by assigning a unique ID to every WebKeyboardEvent and recording which RawKeyDown event ID each Char event corresponds with. Then you'd only need a single queue that held all Char events. When you get a didReceiveEvent for a RawKeyDown event you would just dequeue Char events until they stop matching the RawKeyDown event's ID.

[Jeff Miller]

(In reply to comment #10)
> (From update of attachment 94351 [details])
> The general approach here seems fine. It took me a little while to understand the queue-of-queues, perhaps partly because m_unsentCharEventQueue sounds like it is itself a queue of Char events (when instead it is a queue of UnsentCharEvents).
> 
> We could perhaps make this simpler by assigning a unique ID to every WebKeyboardEvent and recording which RawKeyDown event ID each Char event corresponds with. Then you'd only need a single queue that held all Char events. When you get a didReceiveEvent for a RawKeyDown event you would just dequeue Char events until they stop matching the RawKeyDown event's ID.

Good idea, I'll see if I can do this.

[Adam Roben (:aroben)]

(In reply to comment #10)
> (From update of attachment 94351 [details])
> We could perhaps make this simpler by assigning a unique ID to every WebKeyboardEvent and recording which RawKeyDown event ID each Char event corresponds with. Then you'd only need a single queue that held all Char events. When you get a didReceiveEvent for a RawKeyDown event you would just dequeue Char events until they stop matching the RawKeyDown event's ID.

An even simpler idea is to store the RawKeyDown events in the queue, too. Then you'd just dequeue until you hit a RawKeyDown.

[Jeff Miller]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 94351 [details] [details])
> > We could perhaps make this simpler by assigning a unique ID to every WebKeyboardEvent and recording which RawKeyDown event ID each Char event corresponds with. Then you'd only need a single queue that held all Char events. When you get a didReceiveEvent for a RawKeyDown event you would just dequeue Char events until they stop matching the RawKeyDown event's ID.
> 
> An even simpler idea is to store the RawKeyDown events in the queue, too. Then you'd just dequeue until you hit a RawKeyDown.

This assumes an explicit ordering of RawKeyDown and Char events, but since there's no other way to associate a Char event with a RawKeyDown that's probably OK.

We still need to keep track of whether the last RawKeyDown was handled when the queue was exhausted, since we could get called back from the web process in didReceiveKeyDown() for a RawKeyDown before handleKeyboardEvent() has been called for all the Char messages that correspond to the RawKeyDown.

[Jeff Miller]

(In reply to comment #5)
> Hmm, I just realized I need to at least make sure that the WM_CHAR message is for the same HWND as the WM_KEYDOWN, so this patch should probably be r-'d.  However, I'm still interested in feedback about this design in general.

Strike that, since the WebPageProxy is tied to a specific HWND, any Windows messages it processes are always going to be for the same window.

[Jeff Miller]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > It is very suspicious that WM_UNICHAR messages aren't handled here. I don't really know when these are dispatched in practice, but they probably are?
> > > 
> > > <http://msdn.microsoft.com/en-us/library/ms646288(v=vs.85).aspx> says that WM_UNICHAR messages can be posted by TranslateMessage().  However, note that this code deals with WebEvents, not Windows messages.  Specifically, WebEvent::Char events are generated from WM_CHAR, WM_SYSCHAR, and WM_IME_CHAR messages by WebEventFactory.  I don't know if the fact that WebEventFactory doesn't handle WM_UNICHAR is a bug or not, but that's a separate issue.  Since this code deals with WebEvents, and Windows message that's encapsulated by a WebEvent::Char event will be handled.
> > 
> > Now that I look more closely, it looks like WebView::wndProc() doesn't handle WM_UNICHAR messages, which means there is no infrastructure in WK2 for notifying the web process about these.  There should probably be a separate bug on this.
> 
> Some pages from a Google search for "WM_UNICHAR" make me think that WM_UNICHAR is never sent to windows created with ::CreateWindowExW. These windows are supposedly already Unicode-aware so just get Unicode characters through WM_CHAR.

I tested my patch using a Chinese input method to fill in a form field, which works, but interestingly WebPageProxy::handleKeyboardEvent() is never called with a Char event when the IME composition is committed.  I'm not sure how this is working.

[Alexey Proskuryakov]

Besides supporting ANSI windows, WM_UNICHAR is documented to use UTF-32. So,
(1) WM_CHAR cannot always be used even in Unicode applications, since it's limited to 16 bits,
and (2) you'd need a keyboard layout that generates characters from Unicode Supplementary Plane to test WM_UNICHAR.

Inline input uses a different set of messages (composition ones), so it's expected that WM_CHAR wasn't sent for Chinese.

[Alexey Proskuryakov]

This looks like a must read document (which I didn't see before): <http://seit.unsw.adfa.edu.au/staff/sites/hrp/personal/Sanskrit-External/Unicode-KbdsonWindows.pdf>.

[Jeff Miller]

(In reply to comment #16)
> Besides supporting ANSI windows, WM_UNICHAR is documented to use UTF-32. So,
> (1) WM_CHAR cannot always be used even in Unicode applications, since it's limited to 16 bits,
> and (2) you'd need a keyboard layout that generates characters from Unicode Supplementary Plane to test WM_UNICHAR.
> 
> Inline input uses a different set of messages (composition ones), so it's expected that WM_CHAR wasn't sent for Chinese.

D'oh, I knew that.  Yes, IME composition is being handled by the WebView itself, e.g. WebView::onIMEEndComposition().

Thanks for the additional information on WM_UNICHAR.  Based on a FIXME comment in the WK1 version of WebView::WebViewWndProc() (which unfortunately doesn't mention a bug number), it appears that WM_UNICHAR has never been handled, even in WK1.

[Jeff Miller]

I wrote up bug 61588 to cover adding a key handling logging channel, which will help with this bug.

[Jeff Miller]

Created attachment 95937 [details]
Queue key events until we know whether the RawKeyDown was processed by WebKit

[Jeff Miller]

Created attachment 95938 [details]
Remove old key handling test and add a new one, and update MiniBrowser to always call TranslateMessage() again

[WebKit Review Bot]

Attachment 95937 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/win/WebPageProxyWin.cpp:47:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeff Miller]

(In reply to comment #22)
> Attachment 95937 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> 
> Source/WebKit2/UIProcess/win/WebPageProxyWin.cpp:47:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
> Total errors found: 1 in 4 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I'll fix this before landing.

[Adam Roben (:aroben)]

Comment on attachment 95937 [details]
Queue key events until we know whether the RawKeyDown was processed by WebKit

View in context: https://bugs.webkit.org/attachment.cgi?id=95937&action=review

I think we might be able to simplify some of the logic. Here's one possibility:

UI process receives a key event: add it to the queue, then call a function to send events from the queue to the web process if needed
UI process is told web process is finished with an event: remove events from the queue as needed, then call the same send-events function

Does that seem like it would work? The send-events and remove-events functions would need a little bit of platform-specific logic, but the higher-level logic could all be shared between all platforms. This would result in non-Windows platforms queueing events and then immediately dequeueing them, but that would likely be OK. I'm not sure if this is the right trade-off, but it might help clean things up.

> Source/WebKit2/ChangeLog:24
> +        Our current design of making WebKit clients on Windows wait to call TranslateMessage() until they know that
> +        WebKit didn't handle the key down is untenable, since clients don't always have control over when TranslateMessage()
> +        is called (e.g. when Windows pumps messages in DialogBox() or MessageBox()). Instead, WebKit now assumes that clients
> +        always call TranslateMessage() immediately.
> +        
> +        This means that WebPageProxy in the UI process needs to queue up key events that follow a RawKeyDown event
> +        (the equivalent of a WM_KEYDOWN message) until the web process informs it whether the RawKeyDown event was handled.
> +        If WebKit handled the event, WebPageProxy drops any Char events that were received after the RawKeyDown.
> +        If WebKit didn't handle the event, WebPageProxy sends any queued Char events for the RawKeyDown to the web process.
> +        In either case, it always sends any KeyUp events. Finally, if an additional RawKeyDown event was received while the current
> +        RawKeyDown was in process, it sends that event as well.
> +        
> +        Unfortunately, there's no way to determine that a WM_CHAR message was generated from calling TranslateMessage() with a
> +        specific WM_KEYDOWN message.  We just associate any Char event that's received with the last unprocessed RawKeyDown we've seen.
> +        Generally, any RawKeyDown event should eventually be followed by a KeyUp event, but while we can use this as a hint that
> +        the transaction is complete, there's no guarantee that a KeyUp will always be sent.

This explanation is great!

> Source/WebKit2/ChangeLog:28
> +        In theory, someone could artificially send a WM_CHAR message to the web view without a WM_KEYDOWN, so I tried to account for this
> +        possibility as well by limiting the amount of time we swallow Char events when the last RawKeyDown was handled by WebKit to 1 second.
> +        If we get a KeyUp event, which is what typically happens, this timeout isn't necessary.

Is this just trying to account for artificially-generated events, or do we think this situation can arise in actual use? If it's just the former, I'm not sure how necessary it is. (We could always add it later if we found it really was necessary.) Even if we do really want the timeout now, it might be clearer to add it in a separate patch.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2773
>          if (m_uiClient.implementsDidNotHandleKeyEvent())
>              m_uiClient.didNotHandleKeyEvent(this, event);

Do we still need didNotHandleKeyEvent?

> Source/WebKit2/UIProcess/WebPageProxy.h:663
> +    void doneWithKeyEvent(const NativeWebKeyboardEvent&, bool);
> +    void processKeyEvents(bool);

Should we use enums instead of booleans?

[Adam Roben (:aroben)]

(In reply to comment #24)
> (From update of attachment 95937 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95937&action=review
> 
> I think we might be able to simplify some of the logic. Here's one possibility:
> 
> UI process receives a key event: add it to the queue, then call a function to send events from the queue to the web process if needed
> UI process is told web process is finished with an event: remove events from the queue as needed, then call the same send-events function
> 
> Does that seem like it would work? The send-events and remove-events functions would need a little bit of platform-specific logic, but the higher-level logic could all be shared between all platforms. This would result in non-Windows platforms queueing events and then immediately dequeueing them, but that would likely be OK. I'm not sure if this is the right trade-off, but it might help clean things up.

Here's a better way of phrasing this:

On all platforms, key event dispatch follow this pattern:
1. Key event enters the UI process
2. The UI process queues the event
3. The UI process sends all appropriate queued events to the web process
4. The web process tells the UI process it is finished with an event
5. The UI process sends all appropriate queued events to the web process

The only Windows-specific bits would be in steps 2 and 5. In step 2, on Windows we'd sometimes decide not to send certain events to the web process yet. In step 5, we'd sometimes decide to remove some events from the queue without sending them to the web process.

[Jeff Miller]

Comment on attachment 95937 [details]
Queue key events until we know whether the RawKeyDown was processed by WebKit

View in context: https://bugs.webkit.org/attachment.cgi?id=95937&action=review

>> Source/WebKit2/ChangeLog:28
>> +        If we get a KeyUp event, which is what typically happens, this timeout isn't necessary.
> 
> Is this just trying to account for artificially-generated events, or do we think this situation can arise in actual use? If it's just the former, I'm not sure how necessary it is. (We could always add it later if we found it really was necessary.) Even if we do really want the timeout now, it might be clearer to add it in a separate patch.

It is the former, certainly with Safari as a client this isn't necessary. I will remove this (along with the test I added to simulate this situation).  As you said, we can always add it later if it turns out to be necessary.

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:2773
>>              m_uiClient.didNotHandleKeyEvent(this, event);
> 
> Do we still need didNotHandleKeyEvent?

No, we don't. I was concerned about compatibility with Safari 5.1 if I removed this, but after talking to you and Anders it appears there is a way to do this while continuing to allow Safari 5.1 to work with WebKit nightlies. In any case, that change merits a separate bug, which I'll file after landing this patch.

>> Source/WebKit2/UIProcess/WebPageProxy.h:663
>> +    void processKeyEvents(bool);
> 
> Should we use enums instead of booleans?

Good point, I will change this.

>> Source/WebKit2/UIProcess/win/WebPageProxyWin.cpp:47
>> +// Normally, a Char event will be generated from a RawKeyDown event, and will arrive close to the RawKeyDown event.  However, just in case
> 
> Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]

Fixed.

[Jeff Miller]

(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 95937 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=95937&action=review
> > 
> > I think we might be able to simplify some of the logic. Here's one possibility:
> > 
> > UI process receives a key event: add it to the queue, then call a function to send events from the queue to the web process if needed
> > UI process is told web process is finished with an event: remove events from the queue as needed, then call the same send-events function
> > 
> > Does that seem like it would work? The send-events and remove-events functions would need a little bit of platform-specific logic, but the higher-level logic could all be shared between all platforms. This would result in non-Windows platforms queueing events and then immediately dequeueing them, but that would likely be OK. I'm not sure if this is the right trade-off, but it might help clean things up.
> 
> Here's a better way of phrasing this:
> 
> On all platforms, key event dispatch follow this pattern:
> 1. Key event enters the UI process
> 2. The UI process queues the event
> 3. The UI process sends all appropriate queued events to the web process
> 4. The web process tells the UI process it is finished with an event
> 5. The UI process sends all appropriate queued events to the web process
> 
> The only Windows-specific bits would be in steps 2 and 5. In step 2, on Windows we'd sometimes decide not to send certain events to the web process yet. In step 5, we'd sometimes decide to remove some events from the queue without sending them to the web process.

Adam and I discussed this offline, and I think I understand his proposal.  I will submit a new patch, along with the other changes noted in my last comment.

[Alexey Proskuryakov]

Can TranslateMessage change application state, or does it only dispatch a translated message? I'm worried about state such as dead keys being changed even if keydown event is canceled by DOM.

[Adam Roben (:aroben)]

(In reply to comment #28)
> Can TranslateMessage change application state, or does it only dispatch a translated message? I'm worried about state such as dead keys being changed even if keydown event is canceled by DOM.

I think it only dispatches a translated message. I believe key state is handled at a lower level that is not affected by TranslateMessage. But we should definitely test dead keys and anything else we can think of after this change.

[Jeff Miller]

Created attachment 101790 [details]
Take 2: Queue key events until we know whether the RawKeyDown was processed by WebKit

[Jeff Miller]

Created attachment 101792 [details]
Take 2: Remove old key handling test and add a new one, and update MiniBrowser to always call TranslateMessage() again

[WebKit Review Bot]

Attachment 101790 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/WebPageProxy.h:710:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeff Miller]

(In reply to comment #32)
> Attachment 101790 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> 
> Source/WebKit2/UIProcess/WebPageProxy.h:710:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Total errors found: 1 in 5 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Fixed.

[Adam Roben (:aroben)]

Comment on attachment 101790 [details]
Take 2: Queue key events until we know whether the RawKeyDown was processed by WebKit

View in context: https://bugs.webkit.org/attachment.cgi?id=101790&action=review

> Source/WebKit2/ChangeLog:36
> +        (WebKit::WebPageProxy::didReceiveEvent): Move some code to doneWithKeyEvent().
> +        (WebKit::WebPageProxy::doneWithKeyEvent): Added.

Might be worth mentioning that the ::TranslateMessage() call was intentionally removed as part of this bug fix.

Do we need to keep calling ::TranslateMessage() for Safari 5.1? Does it have any WKPageUIClients which don't implement didNotHandleKeyEvent?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:988
> +#if !LOG_DISABLED
> +    if (event.type() == WebEvent::Char) {
> +#if PLATFORM(WIN)
> +        LOG(KeyHandling, "WebPageProxy::sendKeyEventToWebProcess: sending Char (%c)", event.nativeEvent()->wParam);
> +#else
> +        LOG(KeyHandling, "WebPageProxy::sendKeyEventToWebProcess: sending Char");
> +#endif
> +    } else
> +        LOG(KeyHandling, "WebPageProxy::sendKeyEventToWebProcess: sending %s", webKeyboardEventTypeString(event.type()));
> +#endif // !LOG_DISABLED

You could enhance webKeyboardEventTypeString to include the character code, which would simplify this code quite a bit. (You'd probably want to rename that function, too.)

> Source/WebKit2/UIProcess/WebPageProxy.h:907
> +    bool m_assumeLastRawKeyDownEventWasHandledByWebKit;

Maybe "WebPage" would be clearer than "WebKit". This code is part of WebKit, too, after all.

Do we really need "assume" in the name? We know the answer for sure, right? We don't have to assume anything: m_webPageHandledLastRawKeyDown

> Source/WebKit2/UIProcess/win/WebPageProxyWin.cpp:88
> +    ASSERT(event.type() != WebEvent::KeyDown);

Could use ASSERT_ARG here.

> Source/WebKit2/UIProcess/win/WebPageProxyWin.cpp:92
> +    // We know that a RawKeyDown is in-flight to the web process if there are any unsent
> +    // Char and KeyUp events, or if the last key event we sent to the web process was a RawKeyDown.
> +    bool keyDownIsInFlight = !m_unsentKeyEventQueue.isEmpty() || (!m_sentKeyEventQueue.isEmpty() && (m_sentKeyEventQueue.last().type() == WebEvent::RawKeyDown));

I'm not sure the !m_unsetKeyEventQueue.isEmpty() check is needed. Whenever the unsent queue is non-empty, the last sent event *must* be a RawKeyDown, right? So the second check should be sufficient (and the first could just become an assertion).

It seems like the code below this would be quite a bit simpler if it just did something like:

if (keyDownIsInFlight) {
    LOG(something);
    m_unsentKeyEventQueue.append(event);
    return false;
}

That way all the queueing would happen in one place, instead of in a couple.

> Source/WebKit2/UIProcess/win/WebPageProxyWin.cpp:109
> +    if ((event.type() != WebEvent::Char) && (event.type() != WebEvent::KeyUp)) {
> +        ASSERT_NOT_REACHED();
> +        return true;
> +    }

We normally leave out the extra parentheses in cases like this, but I don't feel too strongly about it. It would be slightly nicer to put the condition in the assertion too. The assertion might not really be appropriate, though, given that any app on the system can send random messages to us. You could just log something (LOG_ERROR maybe) and bail.

> Source/WebKit2/UIProcess/win/WebPageProxyWin.cpp:112
> +    // Queue up any Char or KeyUp events we receive before we know whether the web process has handled the
> +    // last RawKeyDown the we saw.

I'd change "Queue up" to just "Queue" to avoid confusion with "KeyUp" :-)

> Source/WebKit2/UIProcess/win/WebPageProxyWin.cpp:119
> +#if !LOG_DISABLED
> +        if (event.type() == WebEvent::Char)
> +            LOG(KeyHandling, "WebPageProxy::shouldSendKeyEventToWebProcess: queueing Char (%c)", event.nativeEvent()->wParam);
> +        else
> +            LOG(KeyHandling, "WebPageProxy::shouldSendKeyEventToWebProcess: queueing KeyUp");
> +#endif

Again, this could be simplified by enhancing webKeyboardEventTypeString.

> Source/WebKit2/UIProcess/win/WebPageProxyWin.cpp:141
> +    // If we get a Char event after WebKit has processed all outstanding RawKeyDown events, we need to
> +    // drop it on the floor if WebKit handled the last RawKeyDown. In practice, this rarely happens.
> +    // Usually, we'll get an intervening KeyUp which will invalidate m_assumeLastRawKeyDownEventWasHandledByWebKit.
> +    if (!m_assumeLastRawKeyDownEventWasHandledByWebKit) {
> +        // There's no outstanding RawKeyDown, and WebKit didn't handle the last RawKeyDown, so it's OK to send the event.
> +#if !LOG_DISABLED
> +        // It's extremely unusual to receive a Char event without an intervening RawKeyDown, so log this.
> +        if (m_assumeLastRawKeyDownEventWasHandledByWebKit)
> +            LOG(KeyHandling, "WebPageProxy::shouldSendKeyEventToWebProcess: received Char for last handled RawKeyDown");
> +#endif
> +
> +        m_assumeLastRawKeyDownEventWasHandledByWebKit = false;
> +        return true;
> +    }

It seems like this code just collapses to:

if (!m_assumeLastRawKeyDownEventWasHandledByWebKit)
    return true;

> Source/WebKit2/UIProcess/win/WebPageProxyWin.cpp:174
> +    if (event.type() != WebEvent::RawKeyDown) {
> +        // As soon as we get a key event that isn't a RawKeyDown or Char, we can reset m_assumeLastRawKeyDownEventWasHandledByWebKit.
> +        // In practice, the only other key event we should be generating on Windows is KeyUp.
> +        ASSERT(event.type() == WebEvent::KeyUp);
> +        m_assumeLastRawKeyDownEventWasHandledByWebKit = false;
> +        return;
> +    }
> +    
> +    if (handled) {
> +        LOG(KeyHandling, "WebPageProxy::platformDoneWithKeyEvent: RawKeyDown handled");
> +        
> +        // Drop any Char events for this RawKeyDown on the floor (but send any KeyUp events).
> +        sendQueuedKeyEventsToWebProcess(DoNotSendQueuedCharEvents);
> +
> +        // If we get further Char events, we want to drop them on the floor as well. 
> +        m_assumeLastRawKeyDownEventWasHandledByWebKit = m_unsentKeyEventQueue.isEmpty() ? true : false;
> +        return;
> +    }
> +
> +    // We didn't handle the RawKeyDown event, so send any Char and KeyUp events we received after the RawKeyDown.
> +    LOG(KeyHandling, "WebPageProxy::platformDoneWithKeyEvent: RawKeyDown not handled, sending queued key events");
> +    sendQueuedKeyEventsToWebProcess(SendQueuedCharEvents);
> +    m_assumeLastRawKeyDownEventWasHandledByWebKit = false;

As we talked about in person, the setting of m_assumeLastRawKeyDownEventWasHandledByWebKit here isn't quite right. Instead we should set it to true as soon as we find out a RawKeyDown was handled, and set it to false whenever a RawKeyDown is sent to the web process.

> Source/WebKit2/UIProcess/win/WebPageProxyWin.cpp:183
> +        NativeWebKeyboardEvent event = m_unsentKeyEventQueue.first();
> +
> +        ASSERT((event.type() == WebEvent::RawKeyDown) || (event.type() == WebEvent::Char) || (event.type() == WebEvent::KeyUp));
> +        m_unsentKeyEventQueue.removeFirst();

You can simplify this by using Deque::takeFirst().

> Source/WebKit2/UIProcess/win/WebPageProxyWin.cpp:185
> +        if ((shouldSendQueuedCharEvents == SendQueuedCharEvents) || (event.type() != WebEvent::Char)) {

This would be a bit clearer as an early-continue. But if you add code to platformDoneWithKeyEvent to pull out the Char events, you won't need this at all.

[Adam Roben (:aroben)]

Comment on attachment 101792 [details]
Take 2: Remove old key handling test and add a new one, and update MiniBrowser to always call TranslateMessage() again

View in context: https://bugs.webkit.org/attachment.cgi?id=101792&action=review

> Tools/TestWebKitAPI/Tests/WebKit2/win/key-event-handling.html:38
> +    function loaded()
> +    {
> +        setFocusToField1();
> +    }
> +
> +    addEventListener("load", loaded);

You might be able to use the autofocus attribute instead.

> Tools/TestWebKitAPI/win/PlatformUtilitiesWin.cpp:59
> +void runUntil(double timeoutInMS)

Usually we use double to represent time in seconds. (This is inherited from NS/CFTimeInterval/AbsoluteTime.) I think it would be good to do that here, too.

> Tools/TestWebKitAPI/win/PlatformUtilitiesWin.cpp:68
> +        DWORD result = ::MsgWaitForMultipleObjectsEx(0, 0, endTime-now, QS_ALLINPUT, MWMO_INPUTAVAILABLE);

Missing spaces around '-'.