[IME] Differentiate s_suppressNextKeypressEvent in WebViewImpl.

Source/web/WebViewImpl.cpp

Index: Source/web/WebViewImpl.cpp
diff --git a/Source/web/WebViewImpl.cpp b/Source/web/WebViewImpl.cpp
index a0bd095a749a7bec78814557274cc5ca80085d98..e5d7bdd12db36d779f9db9ac61b379c5d11f2efb 100644
--- a/Source/web/WebViewImpl.cpp
+++ b/Source/web/WebViewImpl.cpp
@@ -389,7 +389,7 @@ WebViewImpl::WebViewImpl(WebViewClient* client)
     , m_ignoreInputEvents(false)
     , m_compositorDeviceScaleFactorOverride(0)
     , m_rootLayerScale(1)
-    , m_suppressNextKeypressEvent(false)
+    , m_suppressNextKeypressEvent(Suppress_None)
     , m_imeAcceptEvents(true)
     , m_operationsAllowed(WebDragOperationNone)
     , m_dragOperation(WebDragOperationNone)
@@ -993,9 +993,9 @@ bool WebViewImpl::handleKeyEvent(const WebKeyboardEvent& event)
     // member.
     // The m_suppressNextKeypressEvent is set if the KeyDown is handled by
     // Webkit. A keyDown event is typically associated with a keyPress(char)
-    // event and a keyUp event. We reset this flag here as this is a new keyDown
+    // event and a keyUp event. We reset this state here as this is a new keyDown
     // event.
-    m_suppressNextKeypressEvent = false;
+    m_suppressNextKeypressEvent = Suppress_None;
 
     // If there is a select popup, it should be the one processing the event,
     // not the page.
@@ -1006,7 +1006,7 @@ bool WebViewImpl::handleKeyEvent(const WebKeyboardEvent& event)
         // We need to ignore the next Char event after this otherwise pressing
         // enter when selecting an item in the popup will go to the page.
         if (WebInputEvent::RawKeyDown == event.type)
-            m_suppressNextKeypressEvent = true;
+            m_suppressNextKeypressEvent = Suppress_KeyEventCanceled;

[James Su, 2015/02/27 01:46:18]

Why it's Canceled rather than Handled?

         return true;
     }
 
@@ -1030,7 +1030,7 @@ bool WebViewImpl::handleKeyEvent(const WebKeyboardEvent& event)
             // (Flash needs these keypress events to handle non-US keyboards.)
             Element* element = focusedElement();
             if (!element || !element->renderer() || !element->renderer()->isEmbeddedObject())
-                m_suppressNextKeypressEvent = true;
+                m_suppressNextKeypressEvent = Suppress_KeyEventHandled;

[James Su, 2015/02/27 01:46:18]

Why it's Handled?

eventHandler().keyEvent(evt) returns true whenever event.preventDefault() or
event.stopPropagation() gets called. And IMHO, event.preventDefault() should be
interpreted as Canceled, while event.stopPropagation() should be interpreted as
Handled.
So simply assuming Handled here doesn't make sense.

         }
         return true;
     }
@@ -1063,8 +1063,8 @@ bool WebViewImpl::handleCharEvent(const WebKeyboardEvent& event)
     // handled by Webkit. A keyDown event is typically associated with a
     // keyPress(char) event and a keyUp event. We reset this flag here as it
     // only applies to the current keyPress event.
-    bool suppress = m_suppressNextKeypressEvent;
-    m_suppressNextKeypressEvent = false;
+    bool suppress = (m_suppressNextKeypressEvent != Suppress_None);
+    m_suppressNextKeypressEvent = Suppress_None;
 
     // If there is a select popup, it should be the one processing the event,
     // not the page.
@@ -2268,9 +2268,8 @@ bool WebViewImpl::setComposition(
             return false;
     }
 
-    // If we're not going to fire a keypress event, then the keydown event was
-    // canceled.  In that case, cancel any existing composition.
-    if (text.isEmpty() || m_suppressNextKeypressEvent) {
+    // The keydown event was canceled, so cancel any existing composition.
+    if (text.isEmpty() || m_suppressNextKeypressEvent == Suppress_KeyEventCanceled) {

[James Su, 2015/02/27 01:46:18]

Based on this change, the composition events will never be prevented when
calling corresponding keydown event's preventDefault() method. It looks not
quite right to me.

I'd suggest to do the change in this way:

if (!inputMethodController.hasComposition() && m_suppressNextKeypressEvent) {
    return text.isEmpty();
}

if (text.isEmpty()) {
    if (inputMethodController.hasComposition()) {
        // clear composition
    }
    return true;
}

// update composition

return inputMethodController.hasComposition();


No need to change m_suppressNextKeypressEvent into an enum.

With this change, if there is no composition text, then the call to keydown's
preventDefault() will prevent the following composition event as expected.
But if there is composition text, then the call to preventDefault() will do
nothing.

[huangs, 2015/02/27 03:43:58]

Could you create your own CL and offer it as an alternative to this? It's
inconvenient for me to implement and test this right now. Please feel free to
copy anything you might find useful from this CL (e.g., the unit test). Thanks!

         // A browser process sent an IPC message which does not contain a valid
         // string, which means an ongoing composition has been canceled.
         // If the ongoing composition has been canceled, replace the ongoing