Bug 40092 - spellcheck does not check pasted text
Summary: spellcheck does not check pasted text
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-02 17:25 PDT by Tony Chang
Modified: 2010-12-12 22:07 PST (History)
20 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2010-06-02 17:25:37 PDT
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 Alexey Proskuryakov 2010-06-03 13:41:58 PDT
Interestingly, TextEdit on Mac OS X 10.5 has similar behavior - but that's not the case on 10.6 for me.
Comment 2 Hironori Bono 2010-07-08 03:05:19 PDT
Created attachment 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 Hironori Bono 2010-07-14 01:19:49 PDT
Created attachment 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 WebKit Review Bot 2010-07-14 01:21:22 PDT
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 Early Warning System Bot 2010-07-14 01:29:09 PDT
Attachment 61480 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3528128
Comment 6 Hironori Bono 2010-07-14 01:35:02 PDT
Created attachment 61485 [details]
Draft implementation #2

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

Regards,

Hironori Bono
Comment 7 WebKit Review Bot 2010-07-14 01:36:51 PDT
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 Hironori Bono 2010-07-14 01:47:59 PDT
Created attachment 61488 [details]
Draft implementation #3

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

Regards,

Hironori Bono
Comment 9 WebKit Review Bot 2010-07-14 01:57:04 PDT
Attachment 61480 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3533064
Comment 10 WebKit Review Bot 2010-07-14 02:00:11 PDT
Attachment 61480 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3545048
Comment 11 Eric Seidel (no email) 2010-07-23 09:33:16 PDT
IIRC Doug did the recent spell-checking work in WebKit and might want to see this.
Comment 12 Douglas Davidson 2010-07-23 09:38:53 PDT
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 Eric Seidel (no email) 2010-07-23 09:45:00 PDT
Comment on attachment 61488 [details]
Draft implementation #3

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 Hironori Bono 2010-07-30 00:27:11 PDT
Created attachment 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 Hironori Bono 2010-08-22 22:17:51 PDT
ping?
Comment 16 Hironori Bono 2010-08-26 23:00:06 PDT
Created attachment 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 Adam Barth 2010-08-31 19:50:21 PDT
Comment on attachment 65679 [details]
Draft implementation #5

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 Hironori Bono 2010-08-31 20:50:56 PDT
(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 Hironori Bono 2010-09-03 01:12:16 PDT
(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 Darin Adler 2010-09-03 09:56:42 PDT
Comment on attachment 65679 [details]
Draft implementation #5

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 Hironori Bono 2010-09-07 20:04:41 PDT
Comment on attachment 65679 [details]
Draft implementation #5

Greetings,

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

Regards,
Comment 22 Hajime Morrita 2010-09-08 02:15:30 PDT
Created attachment 66859 [details]
Patch
Comment 23 Hajime Morrita 2010-09-08 02:18:01 PDT
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 Hajime Morrita 2010-09-08 04:33:44 PDT
Created attachment 66879 [details]
sync up to the latest tree
Comment 25 Eric Seidel (no email) 2010-09-08 05:00:25 PDT
Attachment 66879 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3966293
Comment 26 Hajime Morrita 2010-09-08 18:55:25 PDT
Created attachment 66988 [details]
An attempt to fix build on Loopard
Comment 27 Eric Seidel (no email) 2010-09-08 19:22:51 PDT
Attachment 66988 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3930286
Comment 28 Hajime Morrita 2010-09-08 20:42:28 PDT
Created attachment 66994 [details]
Another attempt to fix build on Loopard
Comment 29 Hajime Morrita 2010-09-09 03:39:35 PDT
Created attachment 67016 [details]
Saved the buildbot delay
Comment 30 Hajime Morrita 2010-09-09 18:30:07 PDT
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 Hajime Morrita 2010-10-12 02:36:16 PDT
Created attachment 70518 [details]
took care of textInput event
Comment 32 Hajime Morrita 2010-10-13 01:59:41 PDT
Created attachment 70592 [details]
Updated to lastest
Comment 33 Early Warning System Bot 2010-10-13 02:15:51 PDT
Attachment 70592 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4406004
Comment 34 Hajime Morrita 2010-10-13 04:41:19 PDT
Created attachment 70599 [details]
Fixing Qt WebKit2 build
Comment 35 Eric Seidel (no email) 2010-10-20 14:04:27 PDT
Comment on attachment 70599 [details]
Fixing Qt WebKit2 build

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 Douglas Davidson 2010-10-20 14:07:23 PDT
We'll need to look at this and see how it meshes with what we're doing.
Comment 37 Darin Adler 2010-10-20 14:09:49 PDT
(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 Hajime Morrita 2010-10-21 05:36:42 PDT
Created attachment 71426 [details]
Patch
Comment 39 Hajime Morrita 2010-10-21 05:59:41 PDT
Created attachment 71427 [details]
Patch
Comment 40 Hajime Morrita 2010-10-21 06:00:24 PDT
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 Hajime Morrita 2010-10-21 06:02:39 PDT
(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 Jia Pu 2010-10-27 13:55:34 PDT
(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 Hajime Morrita 2010-10-27 18:53:47 PDT
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 Jia Pu 2010-10-27 19:05:14 PDT
(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 Hajime Morrita 2010-10-27 19:28:26 PDT
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 Hajime Morrita 2010-10-28 04:39:09 PDT
Created attachment 72168 [details]
Patch
Comment 47 Hajime Morrita 2010-10-28 04:42:03 PDT
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 Hajime Morrita 2010-10-28 04:47:27 PDT
Created attachment 72170 [details]
trying to make efl bot happy
Comment 49 Hajime Morrita 2010-10-28 06:00:18 PDT
Created attachment 72177 [details]
resolving conflict
Comment 50 Early Warning System Bot 2010-10-28 06:48:47 PDT
Attachment 72177 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4848049
Comment 51 Eric Seidel (no email) 2010-10-28 09:14:37 PDT
Attachment 72177 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4836056
Comment 52 WebKit Review Bot 2010-10-28 16:49:31 PDT
Attachment 72177 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4873014
Comment 53 Hajime Morrita 2010-10-28 18:40:35 PDT
Created attachment 72283 [details]
trying to fix build
Comment 54 Hajime Morrita 2010-11-01 15:27:38 PDT
Could anyone take a look ?
I think we got a consensus and the patch is ready to review.

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