Bug 40092 - spellcheck does not check pasted text
: spellcheck does not check pasted text
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-06-02 17:25 PST by
Modified: 2010-12-12 22:07 PST (History)


Attachments
A draft implementation (only for discussion) (16.71 KB, patch)
2010-07-08 03:05 PST, Hironori Bono
no flags Review Patch | Details | Formatted Diff | Diff
Another draft implementation (29.23 KB, patch)
2010-07-14 01:19 PST, Hironori Bono
no flags Review Patch | Details | Formatted Diff | Diff
Draft implementation #2 (29.22 KB, patch)
2010-07-14 01:35 PST, Hironori Bono
no flags Review Patch | Details | Formatted Diff | Diff
Draft implementation #3 (29.22 KB, patch)
2010-07-14 01:47 PST, Hironori Bono
eric: review-
Review Patch | Details | Formatted Diff | Diff
Draft implementation #4 (30.45 KB, patch)
2010-07-30 00:27 PST, Hironori Bono
no flags Review Patch | Details | Formatted Diff | Diff
Draft implementation #5 (31.39 KB, patch)
2010-08-26 23:00 PST, Hironori Bono
no flags Review Patch | Details | Formatted Diff | Diff
Patch (52.11 KB, patch)
2010-09-08 02:15 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
sync up to the latest tree (52.29 KB, patch)
2010-09-08 04:33 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
An attempt to fix build on Loopard (52.25 KB, patch)
2010-09-08 18:55 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Another attempt to fix build on Loopard (52.20 KB, patch)
2010-09-08 20:42 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Saved the buildbot delay (52.36 KB, patch)
2010-09-09 03:39 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
took care of textInput event (62.27 KB, patch)
2010-10-12 02:36 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Updated to lastest (62.24 KB, patch)
2010-10-13 01:59 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Fixing Qt WebKit2 build (62.26 KB, patch)
2010-10-13 04:41 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (62.61 KB, patch)
2010-10-21 05:36 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (62.56 KB, patch)
2010-10-21 05:59 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (76.18 KB, patch)
2010-10-28 04:39 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
trying to make efl bot happy (76.23 KB, patch)
2010-10-28 04:47 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
resolving conflict (76.27 KB, patch)
2010-10-28 06:00 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
trying to fix build (77.81 KB, patch)
2010-10-28 18:40 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Updated to ToT (78.02 KB, patch)
2010-11-10 18:03 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (78.01 KB, patch)
2010-11-16 18:39 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Updated to ToT (77.51 KB, patch)
2010-12-09 01:05 PST, Hajime Morrita
ojan: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-06-02 17:25:37 PST
For example, copy "foo baz" into your clipboard and paste it into a bugzilla comment.  "foo" doesn't get a red underline because it isn't checked.

This is similar to bug 37050.
------- Comment #1 From 2010-06-03 13:41:58 PST -------
Interestingly, TextEdit on Mac OS X 10.5 has similar behavior - but that's not the case on 10.6 for me.
------- Comment #2 From 2010-07-08 03:05:19 PST -------
Created an attachment (id=60861) [details]
A draft implementation (only for discussion)

Greetings,

I have quickly implemented an asynchronous spellchecker which uses [NSSpellChecker requestCheckingOfString:] so we can check the spellings (and grammar) of pasted text in the background on 10.6. Unfortunately, this change needs xcode 3.2 to compile it because it depends on GCD (Grand Central Dispatch). Even though this change does not include layout tests and is not ready for review, it may be good for starting a discussion to solve this issue. (I'm writing a change for <https://bugs.webkit.org/show_bug.cgi?id=41832> to write a layout test for this issue.)

Regards,

Hironori Bono
------- Comment #3 From 2010-07-14 01:19:49 PST -------
Created an attachment (id=61480) [details]
Another draft implementation

Greetings,

Even though nobody gave me any comments to my previous one, I have added a layout test to start a review process. Is it possible to review this change?

Regards,

Hironori Bono
------- Comment #4 From 2010-07-14 01:21:22 PST -------
Attachment 61480 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/editing/Editor.cpp:1196:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/mac/WebView/WebFrame.mm:1622:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #5 From 2010-07-14 01:29:09 PST -------
Attachment 61480 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3528128
------- Comment #6 From 2010-07-14 01:35:02 PST -------
Created an attachment (id=61485) [details]
Draft implementation #2

Sorry, my previous change causes a build break on GTK.

Regards,

Hironori Bono
------- Comment #7 From 2010-07-14 01:36:51 PST -------
Attachment 61485 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/editing/Editor.cpp:1198:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/mac/WebView/WebFrame.mm:1622:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #8 From 2010-07-14 01:47:59 PST -------
Created an attachment (id=61488) [details]
Draft implementation #3

Sorry for my annoying updates. I have fixed style issues in my previous change.

Regards,

Hironori Bono
------- Comment #9 From 2010-07-14 01:57:04 PST -------
Attachment 61480 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3533064
------- Comment #10 From 2010-07-14 02:00:11 PST -------
Attachment 61480 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3545048
------- Comment #11 From 2010-07-23 09:33:16 PST -------
IIRC Doug did the recent spell-checking work in WebKit and might want to see this.
------- Comment #12 From 2010-07-23 09:38:53 PST -------
Yes, I'm definitely interested in looking at this.  Unfortunately I will not be available until 8/13, but I would like to see an asynchronous implementation.  Jia Pu is also working in this area.
------- Comment #13 From 2010-07-23 09:45:00 PST -------
(From update of attachment 61488 [details])
WebCore/editing/Editor.cpp:1172
 +      String text;
I would have put this "get me the text I need to process" block in its own function to make this one simpler.

WebCore/editing/Editor.cpp:1187
 +  Node* Editor::getSpellingNode(Node* node)
We don't use get in names in WebKit.

WebCore/editing/Editor.cpp:1236
 +      m_spellingNode = 0;
This could be a set or reset function given how you always set these both at once.  Maybe these need to be a struct held by the object?

WebCore/editing/Editor.cpp:1207
 +      if (node) {
Early return is almost always cleaner.

WebCore/editing/Editor.cpp:1220
 +              Position end(start);
We have a PositionIterator for this sort of thing.

WebCore/editing/Editor.cpp:1230
 +              String source(m_spellingText.substring(results[i].location, results[i].length));
Personally I tend to use = over assignment constructors myself.

WebCore/editing/Editor.h:70
 +          : type(DocumentMarker::Spelling), location(0), length(0) { }
Each assignment gets its own line.

WebCore/editing/Editor.h:350
 +      Node* getSpellingNode(Node*);
Again, no "get" functions in WebKit.

WebKit/mac/ChangeLog:31
 +  2010-07-14  Hironori Bono  <hbono@chromium.org>
Double ChangeLog entry!

WebKit/mac/WebCoreSupport/WebEditorClient.mm:827
 +  #if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_6 && NS_BLOCKS_AVAILABLE
NS_BLOCKS_AVAILABLE?

WebKit/mac/WebCoreSupport/WebEditorClient.mm:839
 +      [[NSSpellChecker sharedSpellChecker] requestCheckingOfString:text range:NSMakeRange(0, text.length()) types:NSTextCheckingAllSystemTypes options:0 inSpellDocumentWithTag:0 completionHandler:^(NSInteger sequenceNumber, NSArray *results, NSOrthography *orthography, NSInteger wordCount) {
Woh.  Please split this into multiple lines using local variables.

WebKit/mac/WebView/WebFrame.mm:1596
 +          switch ([[results objectAtIndex:i] resultType]) {
I would have put this switch into an inline function instead of inlining it here to make this loop easier to read.

WebKit/mac/WebView/WebFrame.mm:1614
 +  static Node* getSpellingNode(Frame* coreFrame)
No "get" in function names.
------- Comment #14 From 2010-07-30 00:27:11 PST -------
Created an attachment (id=63034) [details]
Draft implementation #4

Greetings Eric,

Thank you for your review and comments.
I have added the AsyncSpellCheckerController class to encapsulate some spellchecker-specific code and apply your comments. (I may miss some of them, though.)

(In reply to comment #13)
> WebKit/mac/WebCoreSupport/WebEditorClient.mm:827
>  +  #if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_6 && NS_BLOCKS_AVAILABLE
> NS_BLOCKS_AVAILABLE?

This define means we can use GCD (Grand Central Dispatch), which is required by [NSSpellChecker requestCheckingOfString:]. (Even though GCD is enabled by default on 10.6, we can disable it. So it may be safer to add this define in my code.)

It is definitely helpful to review the updated one and give me your comments?

Regards,

Hironori Bono
------- Comment #15 From 2010-08-22 22:17:51 PST -------
ping?
------- Comment #16 From 2010-08-26 23:00:06 PST -------
Created an attachment (id=65679) [details]
Draft implementation #5

Greetings,

Even though nobody reviewed my pervious change, I would like to update my change because my previous change does not compile any longer. Would it be possible to give me your comments?

Regards,

Hironori Bono
------- Comment #17 From 2010-08-31 19:50:21 PST -------
(From update of attachment 65679 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=65679&action=prettypatch

> WebCore/editing/Editor.cpp:321
> +#if PLATFORM(MAC)
> +    requestCheckingOfFocusedNodeText();
> +#endif
I don't understand why this code has so many #if defs.  Should this code be in a Mac-specific file?
------- Comment #18 From 2010-08-31 20:50:56 PST -------
(In reply to comment #17)
> > WebCore/editing/Editor.cpp:321
> > +#if PLATFORM(MAC)
> > +    requestCheckingOfFocusedNodeText();
> > +#endif
> I don't understand why this code has so many #if defs.  Should this code be in a Mac-specific file?

Thank you for your comment.
In fact, most code under "WebCore" does not need #if defs. Nevertheless, I enclosed them with #if and #endif since this code needs special functions like [requestCheckingOfString:] or threads. Therefore, I'm wondering if other platforms except Mac (and Chromium) would like to use this code. (The Chromium implementation is a little more complicated: it consists of a WebKit change and a Chromium change. So, I would like to send them with another change.)

Regards,
------- Comment #19 From 2010-09-03 01:12:16 PST -------
(In reply to comment #17)
> I don't understand why this code has so many #if defs.  Should this code be in a Mac-specific file?

By the way, I'm a little wondering about the intention of your comment. Would you like to fix this issue only by changing Mac-specific files, to fix this issue without using Mac-specific functions, or something else?

Regards,
------- Comment #20 From 2010-09-03 09:56:42 PST -------
(From update of attachment 65679 [details])
Our usual approach would be to put in fewer ifdefs, and compile in more of the infrastructure for the feature even on platforms that don't have it yet.

Editor.h should not have all those PLATFORM(MAC) #if statements.

It’s also not good to add new classes to existing files. We probably want to add new source files.
------- Comment #21 From 2010-09-07 20:04:41 PST -------
(From update of attachment 65679 [details])
Greetings,

I would like to cancel this review request since Morita-san kindly takes over this bug.

Regards,
------- Comment #22 From 2010-09-08 02:15:30 PST -------
Created an attachment (id=66859) [details]
Patch
------- Comment #23 From 2010-09-08 02:18:01 PST -------
Hi Darin, thank you for reviewing!
As Bono-san mentioned, I'm taking this patch over him.

> 
> Editor.h should not have all those PLATFORM(MAC) #if statements.
> 
> It’s also not good to add new classes to existing files. We probably want to add new source files.

Updated patch introduce SpellChecker class, which defined in separated files.
The patch also attempt to minimize ifdefs.
Could you take a look at it?
------- Comment #24 From 2010-09-08 04:33:44 PST -------
Created an attachment (id=66879) [details]
sync up to the latest tree
------- Comment #25 From 2010-09-08 05:00:25 PST -------
Attachment 66879 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3966293
------- Comment #26 From 2010-09-08 18:55:25 PST -------
Created an attachment (id=66988) [details]
An attempt to fix build on Loopard
------- Comment #27 From 2010-09-08 19:22:51 PST -------
Attachment 66988 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3930286
------- Comment #28 From 2010-09-08 20:42:28 PST -------
Created an attachment (id=66994) [details]
Another attempt to fix build on Loopard
------- Comment #29 From 2010-09-09 03:39:35 PST -------
Created an attachment (id=67016) [details]
Saved the buildbot delay
------- Comment #30 From 2010-09-09 18:30:07 PST -------
Long queue of windows bot prevents patches with new file from built successfully.
I'd like to leave it to avoid making the queue longer.
------- Comment #31 From 2010-10-12 02:36:16 PST -------
Created an attachment (id=70518) [details]
took care of textInput event
------- Comment #32 From 2010-10-13 01:59:41 PST -------
Created an attachment (id=70592) [details]
Updated to lastest
------- Comment #33 From 2010-10-13 02:15:51 PST -------
Attachment 70592 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4406004
------- Comment #34 From 2010-10-13 04:41:19 PST -------
Created an attachment (id=70599) [details]
Fixing Qt WebKit2 build
------- Comment #35 From 2010-10-20 14:04:27 PST -------
(From update of attachment 70599 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=70599&action=review

This generally appears sane.  It would be nice if Doug or one of the Apple folks could comment on the Obj-C.  r- for the nits.

> WebCore/dom/DocumentMarkerController.h:77
> +void showDocumentMarkers(const WebCore::DocumentMarkerController*);

Why is this outside of WebCore?

> WebCore/editing/Editor.cpp:3663
> +static Node* findFirstMarkable(Node* node)

We didn't have this logic before?

> WebCore/editing/SpellChecker.h:35
> +struct SpellCheckingResult {

Structs rarely stay structs for long...

> WebCore/editing/SpellChecker.h:48
> +class SpellChecker {

Seems this should inherit from NonCopyable.

> WebCore/editing/SpellChecker.h:53
> +    bool isAsynchronousEnabled() const;

is "Enabled" needed here?

> WebCore/editing/SpellChecker.h:54
> +    bool isAsynchronousAvailableFor(Node*) const;

canCheckAsynchronously(node)?  I'm not sure what this function does so my naming suggestions may make little sense.

> WebCore/editing/SpellChecker.h:57
> +    bool isCheckeable(Node* node) const;

isCheckable?  Also, "node" is not needed here.

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:954
> +    RetainPtr<NSArray> _results;

Seems a bit odd, but I guess this is Objective c++?  Are destructors called for obj-c++?  Isn't there a new fancy obj-c2 way to do autoretain?

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:978
> +- (WTF::Vector<WebCore::SpellCheckingResult>) _coreResults

Extra space "lt>) _coreR"

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:995
> +    for (size_t i = 0; i < [results count]; ++i) {
> +        NSTextCheckingType type = [[results objectAtIndex:i] resultType];
> +        if (type & NSTextCheckingTypeSpelling)
> +            coreResults[i].type = DocumentMarker::Spelling;
> +        else if (type & NSTextCheckingTypeGrammar)
> +            coreResults[i].type = DocumentMarker::Grammar;
> +        else
> +            coreResults[i].type = DocumentMarker::AllMarkers;
> +
> +        NSRange range = [[results objectAtIndex:i] range];
> +        coreResults[i].location = range.location;
> +        coreResults[i].length = range.length;
> +    }

This feels like a function passed to a map.  Sadly I don't think obj-c has list comprehensions.
------- Comment #36 From 2010-10-20 14:07:23 PST -------
We'll need to look at this and see how it meshes with what we're doing.
------- Comment #37 From 2010-10-20 14:09:49 PST -------
(In reply to comment #35)
> > WebKit/mac/WebCoreSupport/WebEditorClient.mm:954
> > +    RetainPtr<NSArray> _results;
> 
> Seems a bit odd, but I guess this is Objective c++?  Are destructors called for obj-c++?

Yes.

> Isn't there a new fancy obj-c2 way to do autoretain?

The fancy new Objective-C 2 way to do it would be to have property access handle the retain/release or to use garbage collection.

Using a RetainPtr is the right way to do it in WebKit.
------- Comment #38 From 2010-10-21 05:36:42 PST -------
Created an attachment (id=71426) [details]
Patch
------- Comment #39 From 2010-10-21 05:59:41 PST -------
Created an attachment (id=71427) [details]
Patch
------- Comment #40 From 2010-10-21 06:00:24 PST -------
Eric and Everybody, thanks for giving a review!
I update the patch to address Eric's point.

> > WebCore/dom/DocumentMarkerController.h:77
> > +void showDocumentMarkers(const WebCore::DocumentMarkerController*);
> 
> Why is this outside of WebCore?
It follows RenderObject.cpp:showRenderTree() style. 
The function is designed to be called from gdb, so be in global namespace matters.

> 
> > WebCore/editing/Editor.cpp:3663
> > +static Node* findFirstMarkable(Node* node)
> 
> We didn't have this logic before?
Unfortunately, no. Other part of spellchecking is immediately done once
It finds a candidate to mark. So they need no extra traversing. 
But we need it because we mark them later.

> 
> > WebCore/editing/SpellChecker.h:35
> > +struct SpellCheckingResult {
> 
> Structs rarely stay structs for long...
Agreed. Made it to a class.

> 
> > WebCore/editing/SpellChecker.h:48
> > +class SpellChecker {
> 
> Seems this should inherit from NonCopyable.
Done.

> 
> > WebCore/editing/SpellChecker.h:53
> > +    bool isAsynchronousEnabled() const;
> 
> is "Enabled" needed here?
I think so. It is asking EditorClient to the setting.

> 
> > WebCore/editing/SpellChecker.h:54
> > +    bool isAsynchronousAvailableFor(Node*) const;
> 
> canCheckAsynchronously(node)?  I'm not sure what this function does so my naming suggestions may make little sense.
Done. I agree that brief name is preferable.

> 
> > WebCore/editing/SpellChecker.h:57
> > +    bool isCheckeable(Node* node) const;
> 
> isCheckable?  Also, "node" is not needed here.
Done.

> 
> > WebKit/mac/WebCoreSupport/WebEditorClient.mm:954
> > +    RetainPtr<NSArray> _results;
> 
> Seems a bit odd, but I guess this is Objective c++?  Are destructors called for obj-c++?  Isn't there a new fancy obj-c2 way to do autoretain?
It looks OK, as Darin mentioned.

> 
> > WebKit/mac/WebCoreSupport/WebEditorClient.mm:978
> > +- (WTF::Vector<WebCore::SpellCheckingResult>) _coreResults
> 
> Extra space "lt>) _coreR"
Done.

> 
> > WebKit/mac/WebCoreSupport/WebEditorClient.mm:995
> > +    for (size_t i = 0; i < [results count]; ++i) {
> > +        NSTextCheckingType type = [[results objectAtIndex:i] resultType];
> > +        if (type & NSTextCheckingTypeSpelling)
> > +            coreResults[i].type = DocumentMarker::Spelling;
> > +        else if (type & NSTextCheckingTypeGrammar)
> > +            coreResults[i].type = DocumentMarker::Grammar;
> > +        else
> > +            coreResults[i].type = DocumentMarker::AllMarkers;
> > +
> > +        NSRange range = [[results objectAtIndex:i] range];
> > +        coreResults[i].location = range.location;
> > +        coreResults[i].length = range.length;
> > +    }
> 
> This feels like a function passed to a map.  Sadly I don't think obj-c has list comprehensions.
Agreed but there is no map() from NSArray to WTF::Vector.
So I factored out the logic  to make it more map()-like feeling.
------- Comment #41 From 2010-10-21 06:02:39 PST -------
(In reply to comment #36)
> We'll need to look at this and see how it meshes with what we're doing.
Hi Davidson!
Please take a look. Any feedback are appreciated.
I just this is not so large change. large chunks of the code is just a boilerplate.
------- Comment #42 From 2010-10-27 13:55:34 PST -------
(In reply to comment #41)
> (In reply to comment #36)
> > We'll need to look at this and see how it meshes with what we're doing.
> Hi Davidson!
> Please take a look. Any feedback are appreciated.
> I just this is not so large change. large chunks of the code is just a boilerplate.

Since we don't do autocorrection for pasted text, this patch doesn't seem interfere with our recent work on autocorrection. But I'd like to apply to apply this patch on 10.7, see if everything work together. I will let you know.

My other concern is racing condition when multiple threads call  [NSSpellChecker requestCheckingOfString:]. Is it possible?
------- Comment #43 From 2010-10-27 18:53:47 PST -------
Hi Jia, thank you for taking a look! 

> 
> Since we don't do autocorrection for pasted text, this patch doesn't seem interfere with our recent work on autocorrection. But I'd like to apply to apply this patch on 10.7, see if everything work together. I will let you know.

Hmm. on the other hand, we need this for chromium (with separate EditorClient  implementation)...
How about this? :
To land the patch, with the async check settings false.
And doing the experiments before landing it true (at 10.7 release?).
In this way, Chromium port can enable the feature 
with keeping other ports' behavior unchanged at this time.
(I hope every ports to enable the flag at a certain timing. 
But it might need more experiments as you mentioned.)

In my feeling, what really matters here is introducing the async spellcheck API.
Without it, we cannot run spellchecking against larger chunks of text 
like copy-paste, dynamic @spellcheck attribute change, etc.
And for Chromium (sorry for mentioning a specific port again...),
A grammar check could be done by a separate process so async API is unavoidable.

> 
> My other concern is racing condition when multiple threads call  [NSSpellChecker requestCheckingOfString:]. Is it possible?

Since WebKit is single-threaded, [NSSpellChecker requestCheckingOfString:] is 
only called from the main thread.
Although NSSpellChecker looks to run the actual check logic on another (worker) thread,
it is managed by NSSpellChecker. So we wouldn't care about that.

A tricky part is that the passed completionHandler is called at the worker thread.
To void a race, We go back to the main thread using [NSRunLoop performSelector:].
We touch WebCore objects only on the main thread by doing so.
------- Comment #44 From 2010-10-27 19:05:14 PST -------
(In reply to comment #43)
> Hi Jia, thank you for taking a look! 
> 
> > 
> > Since we don't do autocorrection for pasted text, this patch doesn't seem interfere with our recent work on autocorrection. But I'd like to apply to apply this patch on 10.7, see if everything work together. I will let you know.
> 
> Hmm. on the other hand, we need this for chromium (with separate EditorClient  implementation)...
> How about this? :
> To land the patch, with the async check settings false.
I just tried the patch. It seems play well with autocorrection on Mac. This is likely a desirable behavior on Mac as well, but I think we should land it with async set to false for now, just to be safe.

> In my feeling, what really matters here is introducing the async spellcheck API.
> Without it, we cannot run spellchecking against larger chunks of text 
> like copy-paste, dynamic @spellcheck attribute change, etc.

Agree.
------- Comment #45 From 2010-10-27 19:28:26 PST -------
Hi Jia,

> I just tried the patch. It seems play well with autocorrection on Mac. This is likely a desirable behavior on Mac as well, but I think we should land it with async set to false for now, just to be safe.

Thank you for trying this!
So I'll disable the default settings and add LayoutTestController method to make it enabled.
------- Comment #46 From 2010-10-28 04:39:09 PST -------
Created an attachment (id=72168) [details]
Patch
------- Comment #47 From 2010-10-28 04:42:03 PST -------
Updated the patch. It
- introduced Settings::m_asynchronousSpellCheckingEnabled, false as default.
- added a LayoutTestController API to enable it.
- removed EditorClient::isAsynchronousSpellCheckingEnabled(), replaced by settings.
------- Comment #48 From 2010-10-28 04:47:27 PST -------
Created an attachment (id=72170) [details]
trying to make efl bot happy
------- Comment #49 From 2010-10-28 06:00:18 PST -------
Created an attachment (id=72177) [details]
resolving conflict
------- Comment #50 From 2010-10-28 06:48:47 PST -------
Attachment 72177 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4848049
------- Comment #51 From 2010-10-28 09:14:37 PST -------
Attachment 72177 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4836056
------- Comment #52 From 2010-10-28 16:49:31 PST -------
Attachment 72177 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4873014
------- Comment #53 From 2010-10-28 18:40:35 PST -------
Created an attachment (id=72283) [details]
trying to fix build
------- Comment #54 From 2010-11-01 15:27:38 PST -------
Could anyone take a look ?
I think we got a consensus and the patch is ready to review.

Thanks in advance.
------- Comment #55 From 2010-11-10 18:03:03 PST -------
Created an attachment (id=73564) [details]
Updated to ToT
------- Comment #56 From 2010-11-16 18:39:49 PST -------
Created an attachment (id=74074) [details]
Patch
------- Comment #57 From 2010-12-09 01:05:11 PST -------
Created an attachment (id=76023) [details]
Updated to ToT
------- Comment #58 From 2010-12-09 17:33:12 PST -------
Ojan, thank you reviewing!
Darin, Alexey, I'll land this if there is no objection.
------- Comment #59 From 2010-12-12 22:07:04 PST -------
Committed r73886: <http://trac.webkit.org/changeset/73886>