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

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

This implements suzhe@'s suggested fix, as alternative to the attempt in https://codereview.chromium.org/956133002/ .

Also ported the new test over.

BUG=453499
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191167

[huangs, 2015-03-02 03:14:10]

huangs@chromium.org changed reviewers:
+ aelias@chromium.org, kbr@chromium.org, suzhe@chromium.org

[huangs, 2015-03-02 03:15:40]

PTAL.

[James Su, 2015-03-02 03:38:50]

The change LGTM.

Does it solve the original problem?

[huangs, 2015-03-02 04:00:44]

Yes, now backspace does not cancel composition, regardless of autocomplete.

[James Su, 2015-03-02 05:25:14]

On 2015/03/02 04:00:44, huangs wrote:
> Yes, now backspace does not cancel composition, regardless of autocomplete.

Great to hear it. Let's wait for other reviewer's feedback.

[Ken Russell (switch to Gerrit), 2015-03-02 05:37:12]

kbr@chromium.org changed reviewers:
+ boliu@chromium.org, tkent@chromium.org

[Ken Russell (switch to Gerrit), 2015-03-02 05:37:12]

I'd like to defer my review to aelias@, boliu@, tkent@, or other reviewers they
may suggest. I'll be happy to rubber-stamp the fix afterward if necessary.

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

https://codereview.chromium.org/970743002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/970743002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:2311: if (m_suppressNextKeypressEvent &&
!inputMethodController.hasComposition()) {
nit: no braces here

https://codereview.chromium.org/970743002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:2316: if (text.isEmpty()) {
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]

Updated, but didn't remove the code block yet. PTAL.

https://codereview.chromium.org/970743002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/970743002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:2316: if (text.isEmpty()) {
We pass different values for underlines, selectionStart, and selectionEnd
though?

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

https://codereview.chromium.org/970743002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/970743002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:2316: if (text.isEmpty()) {
On 2015/03/02 16:30:46, huangs wrote:
> We pass different values for underlines, selectionStart, and selectionEnd
> though?

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

[huangs, 2015-03-02 19:11:16]

https://codereview.chromium.org/970743002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/970743002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:2316: if (text.isEmpty()) {
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]

https://codereview.chromium.org/970743002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/970743002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:2316: if (text.isEmpty()) {
On 2015/03/02 19:11:15, huangs wrote:
> 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?

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 19:37:00]

Okay I'll remove the block and test.

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

Updated.  Unittest and manual testing looks okay.  However, I think I found a
new bug (also in existing Chrome, so likely unrelated to this CL): when
backspace deletes the last character of a composition CompositionEnd fires
(okay) with event.data assigned to the composition *before* the final backspace.
 On Windows Chrome, I'd get event.data == "".

https://codereview.chromium.org/970743002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/970743002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:2316: if (text.isEmpty()) {
On 2015/03/02 19:34:07, aelias wrote:
> On 2015/03/02 19:11:15, huangs wrote:
> > 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?
> 
> 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.

Done.

[aelias_OOO_until_Jul13, 2015-03-02 20:22:55]

lgtm as far as this goes.

On 2015/03/02 at 20:09:18, huangs wrote:
> Updated.  Unittest and manual testing looks okay.  However, I think I found a
new bug (also in existing Chrome, so likely unrelated to this CL): when
backspace deletes the last character of a composition CompositionEnd fires
(okay) with event.data assigned to the composition *before* the final backspace.

OK, it sounds like that would be a fix on the browser side?

[huangs, 2015-03-02 20:54:53]

Looking into CompositionEnd specs in
https://developer.mozilla.org/en-US/docs/Web/Events/compositionend , it states
that event.data is
"he original string being edited, otherwise the empty string. Read only."

So the Android behavior might be understandable??  Meanwhile, perhaps the
Windows behavior of empty string is buggy, and could be related to
crbug.com/431735 .  Anyway, these are irrelevant to our present CL.

Meanwhile I want to hear suzhe@ again before submitting.

[James Su, 2015-03-03 00:13:33]

LGTM

[huangs, 2015-03-03 02:32:05]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2015-03-03 02:33:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/970743002/40001

[huangs, 2015-03-03 02:34:06]

Thanks, submitting!

Please note that I advised torne@ that we should try to merge this into m40
WebView to fix the stable blocker.

[commit-bot: I haz the power, 2015-03-03 05:42:40]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191167