[IME] Fix s_suppressNextKeypressEvent logic in WebViewImpl::setComposition().

Source/web/WebViewImpl.cpp

 /*
  * Copyright (C) 2011, 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 2286 matching lines @@
     // editable because JavaScript may delete a parent node of the composition
     // node. In this case, WebKit crashes while deleting texts from the parent
     // node, which doesn't exist any longer.
     RefPtrWillBeRawPtr<Range> range = inputMethodController.compositionRange();
     if (range) {
         Node* node = range->startContainer();
         if (!node || !node->isContentEditable())
             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) {
-        // 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
-        // composition string with an empty string and complete it.
-        String emptyString;
-        Vector<CompositionUnderline> emptyUnderlines;
-        inputMethodController.setComposition(emptyString, emptyUnderlines, 0, 0);
+    // A keypress event is canceled. If an ongoing composition exists, then the
+    // keydown event should have arisen from a handled key (e.g., backspace), so we
+    // ignore the cancellation and continue. Otherwise (no ongoing composition) we
+    // exit and signal success only for attempts to clear the composition.
+    if (m_suppressNextKeypressEvent && !inputMethodController.hasComposition()) {

[aelias_OOO_until_Jul13, 2015/03/02 08:48:57]

nit: no braces here

         return text.isEmpty();
     }
+    // An attempt to clear the composition, possibly an attempt to cancel the
+    // composition by an IPC message from a browser process.
+    if (text.isEmpty()) {

[aelias_OOO_until_Jul13, 2015/03/02 08:48:57]

How about deleting this entire block?  The setComposition here does the same
thing as the setComposition just below anyway, and the "hasComposition()" early
out is not needed.  So the only actual behavior change caused by this block is
the "return true;".  So I propose deleting this and at the bottom of this method
right above the "return inputMethodController.hasComposition();", putting:

"if (text.isEmpty())
     return true;"

[huangs, 2015/03/02 16:30:46]

We pass different values for underlines, selectionStart, and selectionEnd
though?

[aelias_OOO_until_Jul13, 2015/03/02 17:55:37]

Yes, but if you look at the implementation in InputMethodController, that has no
effect.

[huangs, 2015/03/02 19:11:15]

There's still a code path where |underline| gets used but text.isEmpty() is
true.  And even if not, we would be using the knowledge that a parameter would
be unused based on InputMethodController::setComposition()'s implementation
details -- I think that would make code fragile and harder to maintain?

[aelias_OOO_until_Jul13, 2015/03/02 19:34:07]

I don't think it's some implementation detail that if we pass in an empty text
string, then the composition is deleted and the other parameters are ignored. 
That's a basic part of the contract that we're already relying on.

As far as |underlines| goes, do you have a reason to believe passing in the
actual data the browser gave us would be harmful, to the extent we should go out
of our way to suppress it with additional complexity here?  If that data needs
to be cleaned up (which it likely doesn't), it's usually better to do that at
the sending side, not at some intermediate plumbing stage.

As far as I can see, this codepath existed before only because some synthetic
stuff needed to be created for the m_suppressNextKeypressEvent case where the
string could be non-empty.  Now that we're getting rid of that and the
parameters are a good mapping, we should use straight-line code that plumbs the
arguments cleanly through.

[huangs, 2015/03/02 20:09:18]

Done.

+        if (inputMethodController.hasComposition()) {
+            // If an ongoing composition exists, replacing it with an empty string.
+            String emptyString;
+            Vector<CompositionUnderline> emptyUnderlines;
+            inputMethodController.setComposition(emptyString, emptyUnderlines, 0, 0);
+        }
+        return true;
+    }
 
     // When the range of composition underlines overlap with the range between
     // selectionStart and selectionEnd, WebKit somehow won't paint the selection
     // at all (see InlineTextBox::paint() function in InlineTextBox.cpp).
     // But the selection range actually takes effect.
     inputMethodController.setComposition(String(text),
                            CompositionUnderlineVectorBuilder(underlines),
                            selectionStart, selectionEnd);
 
     return inputMethodController.hasComposition();
@@ skipping 2342 matching lines @@
     return mainFrameImpl()->frameView()->layoutSize().width() == m_size.width
         || (constraints.minimumScale == constraints.maximumScale && constraints.minimumScale != -1);
 }
 
 void WebViewImpl::forceNextWebGLContextCreationToFail()
 {
     WebGLRenderingContext::forceNextWebGLContextCreationToFail();
 }
 
 } // namespace blink