Bug 118644 – [WK2][GTK] Add unified text checker implementation

• Status: NEW
• Component: WebKitGTK

[Kwang Yul Seo]

Need to implement TextChecker::checkTextOfParagraph and TextChecker::requestCheckingOfString to enable unified text checker for WK2-GTK.

[Kwang Yul Seo]

Created attachment 206599 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Kwang Yul Seo]

(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.

[Grzegorz Czajkowski]

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}?

[Kwang Yul Seo]

(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.

[Kwang Yul Seo]

(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.

[Kwang Yul Seo]

(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.

[Grzegorz Czajkowski]

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.

[Kwang Yul Seo]

(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?

[Grzegorz Czajkowski]

(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.

[Kwang Yul Seo]

(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!