Bug 71991 - Spellcheck should be able to run asynchronously
Summary: Spellcheck should be able to run asynchronously
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 65849 72849 72942 56368 72655 72847 72939 72940 73003
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-09 19:24 PST by Hajime Morrita
Modified: 2011-12-13 00:56 PST (History)
10 users (show)

See Also:


Attachments
Patch (19.38 KB, patch)
2011-11-18 10:36 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (11.25 KB, patch)
2011-11-21 18:23 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (9.29 KB, patch)
2011-11-22 04:31 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (9.28 KB, patch)
2011-11-22 04:40 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2011-11-09 19:24:07 PST
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
Comment 1 Shinya Kawanaka 2011-11-18 10:36:50 PST
Created attachment 115831 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-19 22:09:13 PST
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
Comment 3 Shinya Kawanaka 2011-11-21 18:23:15 PST
Created attachment 116164 [details]
Patch
Comment 4 Hajime Morrita 2011-11-21 20:42:18 PST
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.
Comment 5 Shinya Kawanaka 2011-11-22 04:31:29 PST
Created attachment 116202 [details]
Patch
Comment 6 Shinya Kawanaka 2011-11-22 04:40:56 PST
Created attachment 116205 [details]
Patch
Comment 7 WebKit Review Bot 2011-11-22 07:40:12 PST
Comment on attachment 116205 [details]
Patch

Clearing flags on attachment: 116205

Committed r101002: <http://trac.webkit.org/changeset/101002>
Comment 8 WebKit Review Bot 2011-11-22 07:40:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Csaba Osztrogonác 2011-11-22 22:54:11 PST
(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: []
Comment 10 Shinya Kawanaka 2011-11-23 18:48:50 PST
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: []
Comment 11 Ryosuke Niwa 2011-12-12 16:53:45 PST
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
Comment 12 Shinya Kawanaka 2011-12-12 19:58:19 PST
(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.
Comment 13 Shinya Kawanaka 2011-12-13 00:56:08 PST
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