Provide implementation of textCheckOfParagraph which allows to check spelling for multiple words.
Created attachment 184215 [details] proposed patch
Hi, This patch implements unified text checker for WK2-EFL similarly to Mac. It uses a sync message between WebProcess and UIProcess to synchronously call TextChecker::checkTextOfParagraph(). Are you in a favour of this solution? Or maybe should we implement it asynchronously? BTW we should be aware that it's likely to change spelling implementation for wk2 a lot - at the moment it's working synchronously). Thanks
(In reply to comment #2) > Hi, > This patch implements unified text checker for WK2-EFL similarly to Mac. It uses a sync message between WebProcess and UIProcess to synchronously call TextChecker::checkTextOfParagraph(). > > Are you in a favour of this solution? > Or maybe should we implement it asynchronously? BTW we should be aware that it's likely to change spelling implementation for wk2 a lot - at the moment it's working synchronously). > Thanks I created a bug 108172 to deliver asynchronous spelling implementation, so please ignore above question. Could someone from WK2 Owners approve unified text checker implementation?
Comment on attachment 184215 [details] proposed patch According to WK2 review policy, the patch requires WK2 Owners review. Clearing Morrita's r+ :(
Comment on attachment 184215 [details] proposed patch Clearing flags on attachment: 184215 Committed r141110: <http://trac.webkit.org/changeset/141110>
All reviewed patches have been landed. Closing bug.
The unit tests are failing on the bots.
[ RUN ] EWK2UnitTestBase.ewk_text_checker_string_spelling_check_cb_set /home/buildslave-1/webkit-buildslave/efl-linux-64-release-wk2/build/Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:150: Failure Value of: text Actual: "aa " Expected: expectedMisspelledWord Which is: "aa" /home/buildslave-1/webkit-buildslave/efl-linux-64-release-wk2/build/Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:468: Failure Value of: callbacksExecutionStats.spellingCheck Actual: false Expected: true [ FAILED ] EWK2UnitTestBase.ewk_text_checker_string_spelling_check_cb_set (471 ms) [ RUN ] EWK2UnitTestBase.ewk_text_checker_word_guesses_get_cb_set /home/buildslave-1/webkit-buildslave/efl-linux-64-release-wk2/build/Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:150: Failure Value of: text Actual: "aa " Expected: expectedMisspelledWord Which is: "aa" [ FAILED ] EWK2UnitTestBase.ewk_text_checker_word_guesses_get_cb_set (471 ms) [ RUN ] EWK2UnitTestBase.ewk_text_checker_word_learn_cb_set /home/buildslave-1/webkit-buildslave/efl-linux-64-release-wk2/build/Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:150: Failure Value of: text Actual: "aa " Expected: expectedMisspelledWord Which is: "aa" [ FAILED ] EWK2UnitTestBase.ewk_text_checker_word_learn_cb_set (472 ms) [ RUN ] EWK2UnitTestBase.ewk_text_checker_word_ignore_cb_set /home/buildslave-1/webkit-buildslave/efl-linux-64-release-wk2/build/Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:150: Failure Value of: text Actual: "aa " Expected: expectedMisspelledWord Which is: "aa" [ FAILED ] EWK2UnitTestBase.ewk_text_checker_word_ignore_cb_set (470 ms)
(In reply to comment #4) > (From update of attachment 184215 [details]) > According to WK2 review policy, the patch requires WK2 Owners review. Clearing Morrita's r+ :( Wow, if I understand correctly, this patch doesn't need owners review. The patch touches only port-specific part.
(In reply to comment #9) > (In reply to comment #4) > > (From update of attachment 184215 [details] [details]) > > According to WK2 review policy, the patch requires WK2 Owners review. Clearing Morrita's r+ :( > > Wow, if I understand correctly, this patch doesn't need owners review. > The patch touches only port-specific part. I asked about that on the mailing list and the answer was that even port specific bits needs "owners" review. https://lists.webkit.org/pipermail/webkit-dev/2013-January/023263.html
Grzegorz, if you wanna land WK1 first, I think you can divide this patch between WK1 and WK2. We can land patch for WK1.
(In reply to comment #10) > > I asked about that on the mailing list and the answer was that even port specific bits needs "owners" review. > > https://lists.webkit.org/pipermail/webkit-dev/2013-January/023263.html Woa you're right. I overlooked this. Thanks for letting me know this.
Created attachment 186818 [details] don't call checkSpellingOfString for word separators This patch doesn't call checkSpellingOfString method for the word separators (in compliance with wk-efl unit tests). The word separators are skipped: - at the beginning of the given text, for example checkTextOfParagraph(' zz') -> checkSpellingOfString('zz'), - in the middle of string, generally chechSpellingOfString finishes checking at the word separators, checkTextOfParagraph('zz xx') -> checkSpellingOfString('zz') and checkSpellingOfString('xx'); (NOT checkSpellingOfString(' xx')) - for multiple operators checkTextOfParagraph('zz xx') -> checkSpellingOfString('zz') and checkSpellingOfString('xx') - at the end of the given string checkTextOfParagraph('zz ') -> checkSpellingOfString('zz') Morrita, May I ask you once again to have a look at the patch. I'd appreciate your review. Thanks
The code looks good. By the way, here is what I heard before about spellchecking on chomium: - It runs spellchecker in WebProcess (renderer in chromium's terminology) Some spellchecker can run inside the sandbox. If your spellchecker works like that, it helps reducing the delay. - WebCore supports "async" spellchecking. We added it for mitigating the delay of IPC. It might be good idea to add it for WebKit2.
(In reply to comment #14) > The code looks good. Thanks :) Hope WK'2 owners will say the final word. Anders, Benjamin, Andreas? > By the way, here is what I heard before about spellchecking on chomium: > > - It runs spellchecker in WebProcess (renderer in chromium's terminology) > Some spellchecker can run inside the sandbox. If your spellchecker works like that, > it helps reducing the delay. In WK2, the spell checking is exposed to the UIProcess (spelling library related stuff - check spelling of text, get guesses, learning/ignoring words). See WKTextChecker.h (C API). > - WebCore supports "async" spellchecking. We added it for mitigating the delay of IPC. > It might be good idea to add it for WebKit2. Yes, definitely it's our goal in bug 108172. Asynchronous spell checking in WK2 requires some core changes in WK2 as WebProcess needs to send (async) request to UIProcess and WebProcess has to be notified when spell checking is finished (relevant didSucceed() didCancel() from TextCheckingRequest). Unified text checker is required to ensure async implementation (a lot of asserts in WebCore).
Comment on attachment 186818 [details] don't call checkSpellingOfString for word separators View in context: https://bugs.webkit.org/attachment.cgi?id=186818&action=review > Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:135 > + // FIXME: avoid creating textIterator object, it may be passed as parameter. > + // isTextBreak() as a side effect, modifies the iterator. This issue > + // describes ICU doc - ubrk_isBoundary. In specific cases, for many > + // separators the method doesn't properly determinie the boundaries > + // without reseting the iterator. Lots of English issues. This wouldn't even pass a spell checker, which is what you are working on > Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:155 > +Vector<TextCheckingResult> TextChecker::checkTextOfParagraph(int64_t spellDocumentTag, const UChar* text, int length, uint64_t checkingTypes) typeFlags? checkingTypes sounds a bit weird to me > Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:184 > + // Genrrally, we end up checking at the word seperator, move to the adjacent word. Spelling > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebEditorClientEfl.cpp:68 > +void WebEditorClient::checkTextOfParagraph(const UChar* text, int length, WebCore::TextCheckingTypeMask checkingTypes, Vector<TextCheckingResult>& results) > +{ > + m_page->sendSync(Messages::WebPageProxy::CheckTextOfParagraph(String(text, length), checkingTypes), Messages::WebPageProxy::CheckTextOfParagraph::Reply(results)); > +} > +#endif We really have no way to make this async? If it cannot check this fast enough, will this not block typing on vkb?
Comment on attachment 186818 [details] don't call checkSpellingOfString for word separators View in context: https://bugs.webkit.org/attachment.cgi?id=186818&action=review >> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:135 >> + // without reseting the iterator. > > Lots of English issues. This wouldn't even pass a spell checker, which is what you are working on Thanks. Updated. >> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:155 >> +Vector<TextCheckingResult> TextChecker::checkTextOfParagraph(int64_t spellDocumentTag, const UChar* text, int length, uint64_t checkingTypes) > > typeFlags? checkingTypes sounds a bit weird to me Header files for both WK1/WK2 are using 'checkingTypes'. I'd prefer leave it as it is. >> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:184 >> + // Genrrally, we end up checking at the word seperator, move to the adjacent word. > > Spelling Thanks. Fixed >> Source/WebKit2/WebProcess/WebCoreSupport/efl/WebEditorClientEfl.cpp:68 >> +#endif > > We really have no way to make this async? If it cannot check this fast enough, will this not block typing on vkb? WK2 calls all spelling methods synchronously. My intention is to deliver async version of WebEditorClient::requestCheckingOfString(WebCore::TextCheckingRequest ). It should pass the request to the UIProcess and checkTextOfParagraph (unified text checker) should return vector as a result of spelling - all is done by UIProcess. I hope it won't block typing.
Created attachment 187061 [details] comments improvements
Comment on attachment 187061 [details] comments improvements Attachment 187061 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16429231
(In reply to comment #19) > (From update of attachment 187061 [details]) > Attachment 187061 [details] did not pass win-ews (win): > Output: http://queues.webkit.org/results/16429231 Seems unrelated. The patch doesn't touch Win code. It looks like there were some problems with build setup.
Comment on attachment 187061 [details] comments improvements View in context: https://bugs.webkit.org/attachment.cgi?id=187061&action=review > Source/WebKit2/WebProcess/WebCoreSupport/efl/WebEditorClientEfl.cpp:68 > +#if USE(UNIFIED_TEXT_CHECKING) > +void WebEditorClient::checkTextOfParagraph(const UChar* text, int length, WebCore::TextCheckingTypeMask checkingTypes, Vector<TextCheckingResult>& results) > +{ > + m_page->sendSync(Messages::WebPageProxy::CheckTextOfParagraph(String(text, length), checkingTypes), Messages::WebPageProxy::CheckTextOfParagraph::Reply(results)); > +} > +#endif If USE(UNIFIED_TEXT_CHECKING) is always 1 when building on EFL, I don't think this #if is needed.
Committed r143192: <http://trac.webkit.org/changeset/143192>
Is it possible for any of this code to move to the enchant spell checking code in WebCore, so that it can be shared?
(In reply to comment #23) > Is it possible for any of this code to move to the enchant spell checking code in WebCore, so that it can be shared? Hi, Basically the implementation doesn't invoke any spellcheker engine API (Enchant, Hunspell etc.) It just checks spelling through client's checkSpellingOfString() (in case of EFL it can be Enchant implementation or the client's implementation - similarly to GTK+ we expose callbacks to override the default implementation). I am not sure whether moving it to the WebCore::TextCheckerEnchant.{h/cpp} is a good idea. I agree with you that sharing that code would be great. In fact, the EFL's TextChecker implementation (TextCheckerEfl.cpp) looks very similarly to GTK+ one. How about sharing this file? We could treat it as common one for EFL and GTK+, for example TextCheckerCommon.{h,cpp}. CC'ing Kenneth who was interested in refactoring spelling for EFL and Mario who implemented spelling for WebKit-GTK+.