Currently Chromium does not implement necessary methods to work with USE_UNIFIED_TEXT_CHECKING. This should be implemented.
Currently SpellChecker on Mac uses more sophisticated interface for spellchecking (requestTextOfParagraph). If Windows and Linux have the same interface, the spellchecking code will be clearer, and easier to change.
Created attachment 118740 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
s/requestTextOfParagraph/checkTextOfParagraph/g
Comment on attachment 118740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118740&action=review > Source/WebKit/chromium/public/WebSpellCheckClient.h:44 > class WebSpellCheckClient { By the way, it seems like WebSpellCheckClient is misnamed. It should probably be called WebTextChecker. Such a renaming is out-of-scope for this patch, but I just wanted to call attention to the issue. > Source/WebKit/chromium/public/WebSpellCheckClient.h:57 > + virtual void checkTextOfParagraph(const WebString& text, This appears to run synchronously, like spellCheck. Should we have an asynchronous version like requestCheckingOfText? Also, what about the optionalSuggestions out- param of spellCheck? You don't need a way to provide suggestions with this new API? It seems like WebSpellCheckClient is a bit of a mess :-( > Source/WebKit/chromium/public/WebTextChecking.h:31 > +#ifndef WebTextChecking_h nit: the file name should match the type name, so WebTextCheckingType.h would be better. > Source/WebKit/chromium/public/WebTextChecking.h:36 > +enum WebTextCheckingType { WebTextCheckingResult has a similar enum. Presumably, that one should be deleted in favor of this one? > Source/WebKit/chromium/src/EditorClientImpl.cpp:769 > +static void convertCheckingResult(const WebKit::WebTextCheckingResult& webResult, nit: please do not break up the flow of class method definitions with non-class method definitions. please move static helper functions to the top of the file. nit: you are already in the WebKit namespace, so no need for WebKit:: here. also, there is a 'using namespace WebCore' at the top of the file, so no need for WebCore:: here either. nit: consider putting a conversion operator from WebTextCheckingResult to TextCheckingResult on WebTextCheckingResult in a #if WEBKIT_IMPLEMENTATION section. > Source/WebKit/chromium/src/EditorClientImpl.cpp:793 > + perhaps you should assert that webMask is not 0 after this? it seems like TextCheckingTypeMask could have other bits set too. > Source/WebKit/chromium/src/EditorClientImpl.cpp:795 > + m_webView->spellCheckClient()->checkTextOfParagraph(WebString(text, length), webMask, &webResults); do we need to concern ourselves with the cost of constructing the WebString here? it seems like if we are doing a lot of spell checking that we might prefer to avoid the heap allocation here. have you measured this to see if it matters?
Created attachment 118995 [details] Patch
(In reply to comment #5) > (From update of attachment 118740 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118740&action=review > > > Source/WebKit/chromium/public/WebSpellCheckClient.h:44 > > class WebSpellCheckClient { > > By the way, it seems like WebSpellCheckClient is misnamed. It should probably > be called WebTextChecker. Such a renaming is out-of-scope for this patch, but > I just wanted to call attention to the issue. I'll do this another patch if necessary. > > > Source/WebKit/chromium/public/WebSpellCheckClient.h:57 > > + virtual void checkTextOfParagraph(const WebString& text, > > This appears to run synchronously, like spellCheck. Should we have an asynchronous > version like requestCheckingOfText? Also, what about the optionalSuggestions out- > param of spellCheck? You don't need a way to provide suggestions with this new API? I have a plan to add asynchronous version after this patch is landed. I want to have the same interface WebCore::TextCheckerClient::checkTextOfParagraph. So optional suggestions is not mandatory. But we can specify adding auto correction list by WebTextCheckingTypeMask. > > It seems like WebSpellCheckClient is a bit of a mess :-( > > > Source/WebKit/chromium/public/WebTextChecking.h:31 > > +#ifndef WebTextChecking_h > > nit: the file name should match the type name, so WebTextCheckingType.h would be better. Done. > > > Source/WebKit/chromium/public/WebTextChecking.h:36 > > +enum WebTextCheckingType { > > WebTextCheckingResult has a similar enum. Presumably, that one should be deleted > in favor of this one? Yeah, I want to keep WebTextCheckingType. So the previous enum should be removed. But removing it this time will break chromium build, so after this patch is landed, I'll change chromium code, then come back to WebKit to remove it. > > > Source/WebKit/chromium/src/EditorClientImpl.cpp:769 > > +static void convertCheckingResult(const WebKit::WebTextCheckingResult& webResult, > > nit: please do not break up the flow of class method definitions with non-class method definitions. > please move static helper functions to the top of the file. > > nit: you are already in the WebKit namespace, so no need for WebKit:: here. also, > there is a 'using namespace WebCore' at the top of the file, so no need for WebCore:: > here either. > > nit: consider putting a conversion operator from WebTextCheckingResult to TextCheckingResult > on WebTextCheckingResult in a #if WEBKIT_IMPLEMENTATION section. I've added conversion operator. > > > Source/WebKit/chromium/src/EditorClientImpl.cpp:793 > > + > > perhaps you should assert that webMask is not 0 after this? it seems like TextCheckingTypeMask > could have other bits set too. Done. > > Source/WebKit/chromium/src/EditorClientImpl.cpp:795 > > + m_webView->spellCheckClient()->checkTextOfParagraph(WebString(text, length), webMask, &webResults); > > do we need to concern ourselves with the cost of constructing the WebString here? > it seems like if we are doing a lot of spell checking that we might prefer to > avoid the heap allocation here. have you measured this to see if it matters? Hmm... Currently SpellCheckClient uses WebString interface instead of UChar* and int. If it is important, we should change all the interface of SpellCheckClient, I guess. Currently it does not matter, I guess.
(In reply to comment #7) > > By the way, it seems like WebSpellCheckClient is misnamed. It should probably > > be called WebTextChecker. Such a renaming is out-of-scope for this patch, but > > I just wanted to call attention to the issue. > > I'll do this another patch if necessary. OK > > This appears to run synchronously, like spellCheck. Should we have an asynchronous > > version like requestCheckingOfText? Also, what about the optionalSuggestions out- > > param of spellCheck? You don't need a way to provide suggestions with this new API? > > I have a plan to add asynchronous version after this patch is landed. > I want to have the same interface WebCore::TextCheckerClient::checkTextOfParagraph. So optional suggestions is not mandatory. But we can specify adding auto correction list by WebTextCheckingTypeMask. OK > > > Source/WebKit/chromium/public/WebTextChecking.h:36 > > > +enum WebTextCheckingType { > > > > WebTextCheckingResult has a similar enum. Presumably, that one should be deleted > > in favor of this one? > > Yeah, I want to keep WebTextCheckingType. So the previous enum should be removed. But removing it this time will break chromium build, so after this patch is landed, I'll change chromium code, then come back to WebKit to remove it. OK > > > Source/WebKit/chromium/src/EditorClientImpl.cpp:795 > > > + m_webView->spellCheckClient()->checkTextOfParagraph(WebString(text, length), webMask, &webResults); > > > > do we need to concern ourselves with the cost of constructing the WebString here? > > it seems like if we are doing a lot of spell checking that we might prefer to > > avoid the heap allocation here. have you measured this to see if it matters? > > Hmm... Currently SpellCheckClient uses WebString interface instead of UChar* and int. If it is important, we should change all the interface of SpellCheckClient, I guess. Currently it does not matter, I guess. OK... perhaps this is just something worth investigating to see if it matters. does not have to block this patch.
Comment on attachment 118995 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118995&action=review > Source/WebKit/chromium/public/WebTextCheckingResult.h:47 > +struct WebGrammarDetail { webkit API rule is to have one type definition per file
Created attachment 119126 [details] Patch
(In reply to comment #9) > (From update of attachment 118995 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118995&action=review > > > Source/WebKit/chromium/public/WebTextCheckingResult.h:47 > > +struct WebGrammarDetail { > > webkit API rule is to have one type definition per file Done. Thanks!!
Darin, could you look this?
Comment on attachment 119126 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119126&action=review > Source/WebKit/chromium/public/WebTextCheckingResult.h:48 > + // TODO(shinyak): Should be removed after we confirm Chromium does not use it. nit: WebKit style is to use FIXME instead of TODO(user) > Source/WebKit/chromium/public/WebTextCheckingResult.h:94 > + Error error; // TODO(shinyak): Should be removed after we confirm Chromium does not use it. TODO(user) -> FIXME > Source/WebKit/chromium/public/WebTextCheckingResult.h:98 > + WebVector<WebGrammarDetail> details; it seems like there is potentially a lot of copying of data here. have you considered transforming WebCore::GrammarDetail and WebCore::TextCheckingResult to be reference counted so that we can just create WebPrivatePtr<>-style wrappers for them at the API boundary?
> > Source/WebKit/chromium/public/WebTextCheckingResult.h:98 > > + WebVector<WebGrammarDetail> details; > > it seems like there is potentially a lot of copying of data here. have you > considered transforming WebCore::GrammarDetail and WebCore::TextCheckingResult > to be reference counted so that we can just create WebPrivatePtr<>-style wrappers > for them at the API boundary? Darin, I didn't know that... I think it should be better than the current implementation. I'll change the code to use it.
Created attachment 119809 [details] Patch
Darin, > I didn't know that... I think it should be better than the current implementation. I'll change the code to use it. Since GrammarDetails is a value struct, so I decided not to make it RefCounted. However, since currently we don't have a plan to use GrammarDetails for now, we have decided to omit it. I hope you to review it again.
(In reply to comment #13) > (From update of attachment 119126 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119126&action=review > > > Source/WebKit/chromium/public/WebTextCheckingResult.h:48 > > + // TODO(shinyak): Should be removed after we confirm Chromium does not use it. > > nit: WebKit style is to use FIXME instead of TODO(user) > > > Source/WebKit/chromium/public/WebTextCheckingResult.h:94 > > + Error error; // TODO(shinyak): Should be removed after we confirm Chromium does not use it. > > TODO(user) -> FIXME There were DONE.
Comment on attachment 119809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119809&action=review > Source/WebKit/chromium/src/EditorClientImpl.cpp:770 > + WebCore::TextCheckingTypeMask mask, nit: no need to mention WebCore:: or WTF:: in this .cpp file. notice the using directive at the top of the file for WebCore, and the WTF headers have a using directive.
nit: you probably want to bump your copyrights to 2012
Created attachment 123242 [details] Patch for landing
Comment on attachment 123242 [details] Patch for landing Clearing flags on attachment: 123242 Committed r105491: <http://trac.webkit.org/changeset/105491>
All reviewed patches have been landed. Closing bug.