Bug 51693 - [chromium] editing/input/ime-composition-clearpreedit.html fails in chromium.
Summary: [chromium] editing/input/ime-composition-clearpreedit.html fails in chromium.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 51715
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-28 17:26 PST by James Su
Modified: 2011-01-04 13:27 PST (History)
11 users (show)

See Also:


Attachments
Patch to fix the test in chromium. (2.22 KB, patch)
2010-12-28 17:35 PST, James Su
no flags Details | Formatted Diff | Diff
Remove tab character in ChangeLog (2.23 KB, patch)
2010-12-28 17:40 PST, James Su
eric: review-
Details | Formatted Diff | Diff
Update ChangeLog (2.44 KB, patch)
2010-12-28 20:33 PST, James Su
no flags Details | Formatted Diff | Diff
Patch (18.38 KB, patch)
2010-12-29 19:18 PST, James Su
no flags Details | Formatted Diff | Diff
Patch (20.03 KB, patch)
2011-01-03 10:36 PST, James Su
no flags Details | Formatted Diff | Diff
Patch (20.13 KB, patch)
2011-01-04 11:09 PST, James Su
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Su 2010-12-28 17:26:57 PST
See http://crbug.com/67960
Comment 1 James Su 2010-12-28 17:35:31 PST
Created attachment 77593 [details]
Patch to fix the test in chromium.
Comment 2 WebKit Review Bot 2010-12-28 17:37:47 PST
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.
Comment 3 James Su 2010-12-28 17:40:48 PST
Created attachment 77594 [details]
Remove tab character in ChangeLog
Comment 4 Eric Seidel (no email) 2010-12-28 20:20:39 PST
It's unclear what this patch is doing or why removing that line is OK.  Please update your ChangeLog to explain.
Comment 5 Eric Seidel (no email) 2010-12-28 20:21:01 PST
Comment on attachment 77594 [details]
Remove tab character in ChangeLog

Please explain what you're doing and why in the ChagneLog.
Comment 6 James Su 2010-12-28 20:33:38 PST
Created attachment 77599 [details]
Update ChangeLog
Comment 7 Eric Seidel (no email) 2010-12-28 20:39:32 PST
CCing the author and reviewer of the original test.
Comment 8 Jan Erik Hanssen 2010-12-29 00:15:41 PST
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.
Comment 9 Eric Seidel (no email) 2010-12-29 01:01:30 PST
Comment on attachment 77599 [details]
Update ChangeLog

OK.
Comment 10 WebKit Commit Bot 2010-12-29 01:22:31 PST
Comment on attachment 77599 [details]
Update ChangeLog

Clearing flags on attachment: 77599

Committed r74735: <http://trac.webkit.org/changeset/74735>
Comment 11 WebKit Commit Bot 2010-12-29 01:22:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Mihai Parparita 2010-12-29 11:15:30 PST
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).
Comment 13 James Su 2010-12-29 12:28:46 PST
(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.
Comment 14 Ryosuke Niwa 2010-12-29 12:35:04 PST
(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.
Comment 15 Ryosuke Niwa 2010-12-29 12:38:33 PST
(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.
Comment 16 James Su 2010-12-29 14:01:59 PST
(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().
Comment 17 James Su 2010-12-29 14:03:11 PST
(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.
Comment 18 James Su 2010-12-29 19:18:48 PST
Created attachment 77656 [details]
Patch
Comment 19 James Su 2010-12-29 19:20:20 PST
(In reply to comment #18)
> Created an attachment (id=77656) [details]
> Patch

Can anybody help me test this patch, especially on Qt?
Comment 20 Jan Erik Hanssen 2010-12-30 04:50:15 PST
(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
Comment 21 James Su 2010-12-30 11:51:54 PST
(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.
Comment 22 Jan Erik Hanssen 2010-12-30 17:47:16 PST
(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"
Comment 23 James Su 2010-12-30 18:17:28 PST
(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.
Comment 24 Jan Erik Hanssen 2010-12-30 19:08:28 PST
(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.
Comment 25 James Su 2011-01-03 10:36:45 PST
Created attachment 77822 [details]
Patch
Comment 26 James Su 2011-01-03 10:38:34 PST
Comment on attachment 77822 [details]
Patch

Fix LayoutTests/fast/events/ime-composition-events-001.html on qt port.
Comment 27 Jan Erik Hanssen 2011-01-03 13:21:49 PST
(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.
Comment 28 Kenneth Russell 2011-01-04 10:41:53 PST
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.
Comment 29 James Su 2011-01-04 11:09:06 PST
Created attachment 77909 [details]
Patch
Comment 30 James Su 2011-01-04 11:10:11 PST
Comment on attachment 77909 [details]
Patch

Update WebKit/qt/ChangeLog
Comment 31 Kenneth Russell 2011-01-04 11:21:34 PST
Comment on attachment 77909 [details]
Patch

Thanks, looks good. r=me
Comment 32 WebKit Commit Bot 2011-01-04 13:27:18 PST
Comment on attachment 77909 [details]
Patch

Clearing flags on attachment: 77909

Committed r75000: <http://trac.webkit.org/changeset/75000>
Comment 33 WebKit Commit Bot 2011-01-04 13:27:27 PST
All reviewed patches have been landed.  Closing bug.