[webkit-reviews] review denied: [Bug 58374] WebKit2: Clients have to know not to call TranslateMessage() on key messages destined for WebKit2 web views : [Attachment 101790] Take 2: Queue key events until we know whether the RawKeyDown was processed by WebKit

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Jeff Miller
<jeffm at apple.com>'s request for review:
Bug 58374: WebKit2: Clients have to know not to call TranslateMessage() on key
messages destined for WebKit2 web views
https://bugs.webkit.org/show_bug.cgi?id=58374

Attachment 101790: Take 2: Queue key events until we know whether the
RawKeyDown was processed by WebKit
https://bugs.webkit.org/attachment.cgi?id=101790&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
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.