Bug 51693 – [chromium] editing/input/ime-composition-clearpreedit.html fails in chromium.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[James Su]

See http://crbug.com/67960

[James Su]

Created attachment 77593 [details]
Patch to fix the test in chromium.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/chromium/test_expectations.txt', u'WebKit/chromium/ChangeLog', u'WebKit/chromium/src/WebFrameImpl.cpp']" exit_code: 1
LayoutTests/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


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

[James Su]

Created attachment 77594 [details]
Remove tab character in ChangeLog

[Eric Seidel (no email)]

It's unclear what this patch is doing or why removing that line is OK.  Please update your ChangeLog to explain.

[Eric Seidel (no email)]

Comment on attachment 77594 [details]
Remove tab character in ChangeLog

Please explain what you're doing and why in the ChagneLog.

[James Su]

Created attachment 77599 [details]
Update ChangeLog

[Eric Seidel (no email)]

CCing the author and reviewer of the original test.

[Jan Erik Hanssen]

Seems reasonable to me.

My understanding is that setMarkedText() should just replace the selected text and then update the selection, not confirm the text. This is also what other ports (at least Mac and Qt) are doing from what I can tell.

[Eric Seidel (no email)]

Comment on attachment 77599 [details]
Update ChangeLog

OK.

[WebKit Commit Bot]

Comment on attachment 77599 [details]
Update ChangeLog

Clearing flags on attachment: 77599

Committed r74735: <http://trac.webkit.org/changeset/74735>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mihai Parparita]

Re-opening since I rolled out this patch with http://trac.webkit.org/changeset/74749

It causing assert failures on fast/forms/input-maxlength-ime-completed.html (http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fforms%2Finput-maxlength-ime-completed.html&master=ChromiumWebkit):

ASSERTION FAILED: m_offset + m_count <= m_node->length()
(third_party/WebKit/WebCore/editing/DeleteFromTextNodeCommand.cpp:42 WebCore::DeleteFromTextNodeCommand::DeleteFromTextNodeCommand(WTF::PassRefPtr<WebCore::Text>, unsigned int, unsigned int))
[28117:28117:1434569528609:ERROR:process_util_posix.cc(106)] Received signal 11

Even before then, it needed an expectations update for another layout test that I did with http://trac.webkit.org/changeset/74748 (I'll be rolling that out too).

[James Su]

(In reply to comment #12)
> Re-opening since I rolled out this patch with http://trac.webkit.org/changeset/74749
> 
> It causing assert failures on fast/forms/input-maxlength-ime-completed.html (http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fforms%2Finput-maxlength-ime-completed.html&master=ChromiumWebkit):
> 
> ASSERTION FAILED: m_offset + m_count <= m_node->length()
> (third_party/WebKit/WebCore/editing/DeleteFromTextNodeCommand.cpp:42 WebCore::DeleteFromTextNodeCommand::DeleteFromTextNodeCommand(WTF::PassRefPtr<WebCore::Text>, unsigned int, unsigned int))
> [28117:28117:1434569528609:ERROR:process_util_posix.cc(106)] Received signal 11
> 
> Even before then, it needed an expectations update for another layout test that I did with http://trac.webkit.org/changeset/74748 (I'll be rolling that out too).

This failure is caused by incorrect implementation of test_shell's TextInputController::insertText() rather than this patch.

And in order to fix WebKit/LayoutTests/fast/events/ime-composition-events-001.html in chromium, we need to modify both WebWidget API in WebKit/chromium and the corresponding part in chromium. I'll post a separated patch for that part as well as the test_shell fix.

[Ryosuke Niwa]

(In reply to comment #12)
> ASSERTION FAILED: m_offset + m_count <= m_node->length()
> (third_party/WebKit/WebCore/editing/DeleteFromTextNodeCommand.cpp:42 WebCore::DeleteFromTextNodeCommand::DeleteFromTextNodeCommand(WTF::PassRefPtr<WebCore::Text>, unsigned int, unsigned int))
> [28117:28117:1434569528609:ERROR:process_util_posix.cc(106)] Received signal 11

This sounds like the bug 51389.  Maybe this bug should depend on the bug 51389 if fixing this bug causes the assertion failure to appear.

[Ryosuke Niwa]

(In reply to comment #13)
> This failure is caused by incorrect implementation of test_shell's TextInputController::insertText() rather than this patch.
> 
> And in order to fix WebKit/LayoutTests/fast/events/ime-composition-events-001.html in chromium, we need to modify both WebWidget API in WebKit/chromium and the corresponding part in chromium. I'll post a separated patch for that part as well as the test_shell fix.

I don't think we use test_shell anymore.  You should just fix DRT.

[James Su]

(In reply to comment #14)
> (In reply to comment #12)
> > ASSERTION FAILED: m_offset + m_count <= m_node->length()
> > (third_party/WebKit/WebCore/editing/DeleteFromTextNodeCommand.cpp:42 WebCore::DeleteFromTextNodeCommand::DeleteFromTextNodeCommand(WTF::PassRefPtr<WebCore::Text>, unsigned int, unsigned int))
> > [28117:28117:1434569528609:ERROR:process_util_posix.cc(106)] Received signal 11
> 
> This sounds like the bug 51389.  Maybe this bug should depend on the bug 51389 if fixing this bug causes the assertion failure to appear.

They are probably related. But the assertion disappears after fixing TextInputController::insertText().

[James Su]

(In reply to comment #15)
> (In reply to comment #13)
> > This failure is caused by incorrect implementation of test_shell's TextInputController::insertText() rather than this patch.
> > 
> > And in order to fix WebKit/LayoutTests/fast/events/ime-composition-events-001.html in chromium, we need to modify both WebWidget API in WebKit/chromium and the corresponding part in chromium. I'll post a separated patch for that part as well as the test_shell fix.
> 
> I don't think we use test_shell anymore.  You should just fix DRT.

Then, I can fix all of them in one patch.

[James Su]

Created attachment 77656 [details]
Patch

[James Su]

(In reply to comment #18)
> Created an attachment (id=77656) [details]
> Patch

Can anybody help me test this patch, especially on Qt?

[Jan Erik Hanssen]

(In reply to comment #19)
> (In reply to comment #18)
> > Created an attachment (id=77656) [details] [details]
> > Patch
> 
> Can anybody help me test this patch, especially on Qt?

The Qt expected file seems to have the result duplicated after applying this patch, this is the expected result of run-webkit-tests:

http://pastebin.ca/2034153

and this is the actual output of the Qt DRT (on OS X):

http://pastebin.ca/2034154

[James Su]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > Created an attachment (id=77656) [details] [details] [details]
> > > Patch
> > 
> > Can anybody help me test this patch, especially on Qt?
> 
> The Qt expected file seems to have the result duplicated after applying this patch, this is the expected result of run-webkit-tests:
> 
> http://pastebin.ca/2034153
> 
> and this is the actual output of the Qt DRT (on OS X):
> 
> http://pastebin.ca/2034154

I can't see any problem from the patch itself. Please make sure that there is no LayoutTests/platform/qt/fast/events/ime-composition-events-001-expected.txt file before applying the patch.

[Jan Erik Hanssen]

(In reply to comment #21)
 > I can't see any problem from the patch itself. Please make sure that there is no LayoutTests/platform/qt/fast/events/ime-composition-events-001-expected.txt file before applying the patch.

If I manually remove the Qt expected file before applying your patch then the test fails with the following diff:

--- /tmp/layout-test-results/fast/events/ime-composition-events-001-expected.txt	2010-12-31 02:45:29.000000000 +0100
+++ /tmp/layout-test-results/fast/events/ime-composition-events-001-actual.txt	2010-12-31 02:45:29.000000000 +0100
@@ -12,4 +12,5 @@
 SUCCESS: INPUT - compositionupdate - "6"
 SUCCESS: INPUT - compositionupdate - "7"
 SUCCESS: INPUT - compositionend - ""
+SUCCESS: INPUT - compositionend - "8"
 SUCCESS: INPUT - textInput - "8"

[James Su]

(In reply to comment #22)
> (In reply to comment #21)
>  > I can't see any problem from the patch itself. Please make sure that there is no LayoutTests/platform/qt/fast/events/ime-composition-events-001-expected.txt file before applying the patch.
> 
> If I manually remove the Qt expected file before applying your patch then the test fails with the following diff:
> 
> --- /tmp/layout-test-results/fast/events/ime-composition-events-001-expected.txt    2010-12-31 02:45:29.000000000 +0100
> +++ /tmp/layout-test-results/fast/events/ime-composition-events-001-actual.txt    2010-12-31 02:45:29.000000000 +0100
> @@ -12,4 +12,5 @@
>  SUCCESS: INPUT - compositionupdate - "6"
>  SUCCESS: INPUT - compositionupdate - "7"
>  SUCCESS: INPUT - compositionend - ""
> +SUCCESS: INPUT - compositionend - "8"
>  SUCCESS: INPUT - textInput - "8"

It's an issue of Qt port. That extra line is not expected.

[Jan Erik Hanssen]

(In reply to comment #23)
> It's an issue of Qt port. That extra line is not expected.

Looks like this is due to Qt's implementation of insertText(), which confirms the composition, which in turn sends a compositionend event to the input element (from Editor::confirmComposition()). 

However, looking at Chromium's insertText() implementation that appears to do something similar yet that one doesn't send such an event. Not sure why.

[James Su]

Created attachment 77822 [details]
Patch

[James Su]

Comment on attachment 77822 [details]
Patch

Fix LayoutTests/fast/events/ime-composition-events-001.html on qt port.

[Jan Erik Hanssen]

(In reply to comment #26)
> (From update of attachment 77822 [details])
> Fix LayoutTests/fast/events/ime-composition-events-001.html on qt port.

The test does indeed pass on Qt now and the new code looks ok to me. Though I don't know enough about the Qt IM code to say for sure that this is the correct solution.

[Kenneth Russell]

Comment on attachment 77822 [details]
Patch

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

The code changes look okay to me (though I am not an expert on input methods) -- but please document the Qt code changes in WebKit/qt/ChangeLog.

> WebKit/qt/ChangeLog:10
> +        * Api/qwebpage.cpp:
> +        (QWebPagePrivate::inputMethodEvent):

Please document these changes.

[James Su]

Created attachment 77909 [details]
Patch

[James Su]

Comment on attachment 77909 [details]
Patch

Update WebKit/qt/ChangeLog

[Kenneth Russell]

Comment on attachment 77909 [details]
Patch

Thanks, looks good. r=me

[WebKit Commit Bot]

Comment on attachment 77909 [details]
Patch

Clearing flags on attachment: 77909

Committed r75000: <http://trac.webkit.org/changeset/75000>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.