Need to implement TextChecker::checkTextOfParagraph and TextChecker::requestCheckingOfString to enable unified text checker for WK2-GTK.
Created attachment 206599 [details] Patch
Comment on attachment 206599 [details] Patch Attachment 206599 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/970700 New failing tests: fullscreen/full-screen-iframe-with-max-width-height.html
Created attachment 206607 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
(In reply to comment #3) > Created an attachment (id=206607) [details] > Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 > > The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. > Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3 It seems the test is flakey because this patch touches only GTK-related files.
Comment on attachment 206599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206599&action=review Added some comments before formal review. > Source/WebKit2/ChangeLog:9 > + The implementation of TextChecker::checkTextOfParagraph and > + TextChecker::requestCheckingOfString is copied from the EFL port. I wouldn't mix unified text checker implementation with asynchronous implementation of text checker. Unified text checker can be used synchronously as well. It's better to make such changes gradually. > Source/WebKit2/ChangeLog:24 > + * UIProcess/gtk/TextCheckerGtk.cpp: > + (WebKit::nextWordOffset): > + Helper function to determine the word offset. > + > + (WebKit::TextChecker::checkTextOfParagraph): > + Allow to check spelling for multiple words, their misspelling location > + and length are saved to the vector. > + > + (WebKit::TextChecker::requestCheckingOfString): > + * WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp: > + > + (WebKit::WebEditorClient::checkTextOfParagraph): > + As spelling implementation is exposed to UIProcess, send a meesage to > + UIProcess to call TextChecker::checkTextOfParagraph. I'd love to share this code with EFL instead of copying it. Frankly speaking there is a slight difference between TextCheckerEfl.cpp and TextCheckerGtk.cpp. They call the client's implementation based on WK2 C API. Both files TextCheckerEfl.cpp and TextCheckerGtk.cpp don't involve any spell checking engine library, for example, enchant etc. How about making common file with the default implementation for WK2 unless the port provides own implementation (Mac). This is suggestion only and I'd like to listen the reviewers opinion on that. > Source/WebKit/gtk/WebCoreSupport/TextCheckerClientGtk.h:53 > + virtual void checkTextOfParagraph(const UChar* text, int length, WebCore::TextCheckingTypeMask checkingTypes, WTF::Vector<WebCore::TextCheckingResult>& results); Nit: can we omit 'text' param as well as it has been done for the above methods? > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:208 > +#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 Mac and Efl use the same implementation. Is is possible to move it to sharable WebProcess/WebCoreSupport/WebEditorClient.{h/cpp}?
(In reply to comment #5) Okay. I will split the patch into multiple pieces. I will upload refactoring patches to share the code first and then work on implementing unified text checker in Gtk port.
(In reply to comment #5) > Mac and Efl use the same implementation. Is is possible to move it to sharable WebProcess/WebCoreSupport/WebEditorClient.{h/cpp}? Done in Bug 119034.
(In reply to comment #5) > I'd love to share this code with EFL instead of copying it. Frankly speaking there is a slight difference between TextCheckerEfl.cpp and TextCheckerGtk.cpp. They call the client's implementation based on WK2 C API. > > Both files TextCheckerEfl.cpp and TextCheckerGtk.cpp don't involve any spell checking engine library, for example, enchant etc. How about making common file with the default implementation for WK2 unless the port provides own implementation (Mac). This is suggestion only and I'd like to listen the reviewers opinion on that. It sounds reasonable. Done in Bug 119036.
Are you going to implement unified text checker in WK1 for GTK+ as well? As far as I know only GTK+ is using legacy text checker which is about to be removed. It hasn't announced yet but we've spoken about it with Ryosuke. When the patches land (to WK1/WK2-GTK+) we could gradually get rid of legacy text checker in a favour of unified text checker in WebCore.
(In reply to comment #9) > Are you going to implement unified text checker in WK1 for GTK+ as well? As far as I know only GTK+ is using legacy text checker which is about to be removed. It hasn't announced yet but we've spoken about it with Ryosuke. > > When the patches land (to WK1/WK2-GTK+) we could gradually get rid of legacy text checker in a favour of unified text checker in WebCore. Okay. I will. To move forward, I should find a reviewer who can help landing this work. Would you suggest a reviewer?
(In reply to comment #10) > (In reply to comment #9) > > Are you going to implement unified text checker in WK1 for GTK+ as well? As far as I know only GTK+ is using legacy text checker which is about to be removed. It hasn't announced yet but we've spoken about it with Ryosuke. > > > > When the patches land (to WK1/WK2-GTK+) we could gradually get rid of legacy text checker in a favour of unified text checker in WebCore. > > Okay. I will. To move forward, I should find a reviewer who can help landing this work. Would you suggest a reviewer? I'd ask andersca to get review for bug 119034 due to changes in WK2 core. Bug 119036 touches EFL code, cdumez might be interested in reviewing.
(In reply to comment #11) > I'd ask andersca to get review for bug 119034 due to changes in WK2 core. > Bug 119036 touches EFL code, cdumez might be interested in reviewing. Okay, I will ask them. And I will CC you in the follow-up patches. Thanks a lot for your help!