Bug 71991 – Spellcheck should be able to run asynchronously

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Hajime Morrita]

Once spellchecking has a unified text checking emulation by Bug 65849, 
we can use SpellChecker::requestCheckingFor() to run all spellchecking async.
This will improve responsiveness of key typing.

See also:
 * http://code.google.com/p/chromium/issues/detail?id=103517

[Shinya Kawanaka]

Created attachment 115831 [details]
Patch

[WebKit Review Bot]

Comment on attachment 115831 [details]
Patch

Attachment 115831 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10369720

New failing tests:
editing/pasteboard/paste-table-001.html
css2.1/20110323/abspos-containing-block-initial-004b.htm
editing/spelling/spellcheck-attribute.html
fast/replaced/width100percent-textarea.html
editing/spelling/spelling-unified-emulation.html
editing/spelling/spelling-attribute-change.html
editing/spelling/spelling-hasspellingmarker.html
css2.1/20110323/abspos-containing-block-initial-004d.htm
editing/spelling/spelling-linebreak.html
editing/spelling/spelling-attribute-at-child.html
editing/spelling/spelling.html

[Shinya Kawanaka]

Created attachment 116164 [details]
Patch

[Hajime Morrita]

Comment on attachment 116164 [details]
Patch

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

Is it possible to add a test that modify the checking range content during the spellcheck request is in flight?

> Source/WebCore/ChangeLog:7
> +

Could you add some detailed explanation here?
The bug summary  isn't enough.

> Source/WebCore/editing/Editor.cpp:1882
> +            if (asynchronous)

It looks async check and m_spellChecker invocation can be a part of markAllMisspellingsAndBadGrammarInRanges().

> Source/WebCore/editing/SpellChecker.cpp:134
> +    if (shouldMarkGrammar)

Maybe body of this if() statement should be a part of the if clause above.

[Shinya Kawanaka]

Created attachment 116202 [details]
Patch

[Shinya Kawanaka]

Created attachment 116205 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116205 [details]
Patch

Clearing flags on attachment: 116205

Committed r101002: <http://trac.webkit.org/changeset/101002>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #7)
> (From update of attachment 116205 [details])
> Clearing flags on attachment: 116205
> 
> Committed r101002: <http://trac.webkit.org/changeset/101002>

The new editing/spelling/spellcheck-async.html fails on the GTK and on the Qt bot. Have you got any idea why?

--- /ramdisk/qt-linux-release/build/layout-test-results/editing/spelling/spellcheck-async-expected.txt 
+++ /ramdisk/qt-linux-release/build/layout-test-results/editing/spelling/spellcheck-async-actual.txt 
@@ -5,14 +5,14 @@
 PASS successfullyParsed is true
 
 TEST COMPLETE
-PASS text : "zz" has markers: [zz]
-PASS text : "apple,zz,orange" has markers: [zz]
-PASS text : "zz,zz" has markers: [zz,zz]
-PASS text : "zz zz zz" has markers: [zz,zz,zz]
-PASS text : " zz zz zz " has markers: [zz,zz,zz]
-PASS text : "zz apple orange" has markers: [zz]
-PASS text : "apple zz orange" has markers: [zz]
-PASS text : "apple orange zz" has markers: [zz]
+FAIL text : "zz" should have markers: [zz]
+FAIL text : "apple,zz,orange" should have markers: [zz]
+FAIL text : "zz,zz" should have markers: [zz,zz]
+FAIL text : "zz zz zz" should have markers: [zz,zz,zz]
+FAIL text : " zz zz zz " should have markers: [zz,zz,zz]
+FAIL text : "zz apple orange" should have markers: [zz]
+FAIL text : "apple zz orange" should have markers: [zz]
+FAIL text : "apple orange zz" should have markers: [zz]
 PASS text : "zzz" has markers: []
 PASS text : "I would like to sleep, zzz" has markers: []
 
--- /var/lib/buildbot/build/gtk-linux-32-release/build/layout-test-results/editing/spelling/spellcheck-async-expected.txt 
+++ /var/lib/buildbot/build/gtk-linux-32-release/build/layout-test-results/editing/spelling/spellcheck-async-actual.txt 
@@ -13,6 +13,6 @@
 PASS text : "zz apple orange" has markers: [zz]
 PASS text : "apple zz orange" has markers: [zz]
 PASS text : "apple orange zz" has markers: [zz]
-PASS text : "zzz" has markers: []
-PASS text : "I would like to sleep, zzz" has markers: []
+FAIL text : "zzz" should have markers: []
+FAIL text : "I would like to sleep, zzz" should have markers: []

[Shinya Kawanaka]

Hmm...

Though I have to investigate why to say the correct things..., perhaps Qt and GTK have not implemented requestCheckingFor. Or, some functions might not be implemented... Since my previous patch which resolves issues depending on this issue have also failed in Qt and GTK (their tests are skipped on Qt and Gtk now.), resolving them may fix such errors.

And sorry, I should have to add this test on Skipped lists.

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 116205 [details] [details])
> > Clearing flags on attachment: 116205
> > 
> > Committed r101002: <http://trac.webkit.org/changeset/101002>
> 
> The new editing/spelling/spellcheck-async.html fails on the GTK and on the Qt bot. Have you got any idea why?
> 
> --- /ramdisk/qt-linux-release/build/layout-test-results/editing/spelling/spellcheck-async-expected.txt 
> +++ /ramdisk/qt-linux-release/build/layout-test-results/editing/spelling/spellcheck-async-actual.txt 
> @@ -5,14 +5,14 @@
>  PASS successfullyParsed is true
> 
>  TEST COMPLETE
> -PASS text : "zz" has markers: [zz]
> -PASS text : "apple,zz,orange" has markers: [zz]
> -PASS text : "zz,zz" has markers: [zz,zz]
> -PASS text : "zz zz zz" has markers: [zz,zz,zz]
> -PASS text : " zz zz zz " has markers: [zz,zz,zz]
> -PASS text : "zz apple orange" has markers: [zz]
> -PASS text : "apple zz orange" has markers: [zz]
> -PASS text : "apple orange zz" has markers: [zz]
> +FAIL text : "zz" should have markers: [zz]
> +FAIL text : "apple,zz,orange" should have markers: [zz]
> +FAIL text : "zz,zz" should have markers: [zz,zz]
> +FAIL text : "zz zz zz" should have markers: [zz,zz,zz]
> +FAIL text : " zz zz zz " should have markers: [zz,zz,zz]
> +FAIL text : "zz apple orange" should have markers: [zz]
> +FAIL text : "apple zz orange" should have markers: [zz]
> +FAIL text : "apple orange zz" should have markers: [zz]
>  PASS text : "zzz" has markers: []
>  PASS text : "I would like to sleep, zzz" has markers: []
> 
> --- /var/lib/buildbot/build/gtk-linux-32-release/build/layout-test-results/editing/spelling/spellcheck-async-expected.txt 
> +++ /var/lib/buildbot/build/gtk-linux-32-release/build/layout-test-results/editing/spelling/spellcheck-async-actual.txt 
> @@ -13,6 +13,6 @@
>  PASS text : "zz apple orange" has markers: [zz]
>  PASS text : "apple zz orange" has markers: [zz]
>  PASS text : "apple orange zz" has markers: [zz]
> -PASS text : "zzz" has markers: []
> -PASS text : "I would like to sleep, zzz" has markers: []
> +FAIL text : "zzz" should have markers: []
> +FAIL text : "I would like to sleep, zzz" should have markers: []

[Ryosuke Niwa]

It seems like the test added by this patch has been failing on Mac:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=editing%2Fspelling%2Fspellcheck-async.html&group=%40ToT%20-%20webkit.org

[Shinya Kawanaka]

(In reply to comment #11)
> It seems like the test added by this patch has been failing on Mac:
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=editing%2Fspelling%2Fspellcheck-async.html&group=%40ToT%20-%20webkit.org

I'll check it soon.

[Shinya Kawanaka]

I've confirmed the test fails occasionally, sometimes the test passes though.
I've filed the bug of this flakiness. I'll try to fix soon.

https://bugs.webkit.org/show_bug.cgi?id=74388