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.
Interestingly, TextEdit on Mac OS X 10.5 has similar behavior - but that's not the case on 10.6 for me.
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
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
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.
Attachment 61480 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3528128
Created attachment 61485 [details] Draft implementation #2 Sorry, my previous change causes a build break on GTK. Regards, Hironori Bono
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.
Created attachment 61488 [details] Draft implementation #3 Sorry for my annoying updates. I have fixed style issues in my previous change. Regards, Hironori Bono
Attachment 61480 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3533064
Attachment 61480 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3545048
IIRC Doug did the recent spell-checking work in WebKit and might want to see this.
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 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.
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
ping?
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 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?
(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,
(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 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 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,
Created attachment 66859 [details] Patch
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?
Created attachment 66879 [details] sync up to the latest tree
Attachment 66879 [details] did not build on mac: Build output: http://queues.webkit.org/results/3966293
Created attachment 66988 [details] An attempt to fix build on Loopard
Attachment 66988 [details] did not build on mac: Build output: http://queues.webkit.org/results/3930286
Created attachment 66994 [details] Another attempt to fix build on Loopard
Created attachment 67016 [details] Saved the buildbot delay
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.
Created attachment 70518 [details] took care of textInput event
Created attachment 70592 [details] Updated to lastest
Attachment 70592 [details] did not build on qt: Build output: http://queues.webkit.org/results/4406004
Created attachment 70599 [details] Fixing Qt WebKit2 build
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.
We'll need to look at this and see how it meshes with what we're doing.
(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.
Created attachment 71426 [details] Patch
Created attachment 71427 [details] Patch
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.
(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.
(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?
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.
(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.
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.
Created attachment 72168 [details] Patch
Updated the patch. It - introduced Settings::m_asynchronousSpellCheckingEnabled, false as default. - added a LayoutTestController API to enable it. - removed EditorClient::isAsynchronousSpellCheckingEnabled(), replaced by settings.
Created attachment 72170 [details] trying to make efl bot happy
Created attachment 72177 [details] resolving conflict
Attachment 72177 [details] did not build on qt: Build output: http://queues.webkit.org/results/4848049
Attachment 72177 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4836056
Attachment 72177 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4873014
Created attachment 72283 [details] trying to fix build
Could anyone take a look ? I think we got a consensus and the patch is ready to review. Thanks in advance.
Created attachment 73564 [details] Updated to ToT
Created attachment 74074 [details] Patch
Created attachment 76023 [details] Updated to ToT
Ojan, thank you reviewing! Darin, Alexey, I'll land this if there is no objection.
Committed r73886: <http://trac.webkit.org/changeset/73886>