Bug 107682 – [WK2][EFL] Unified text checker implementation

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Grzegorz Czajkowski]

Provide implementation of textCheckOfParagraph which allows to check spelling for multiple words.

[Grzegorz Czajkowski]

Created attachment 184215 [details]
proposed patch

[Grzegorz Czajkowski]

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

[Grzegorz Czajkowski]

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

[Grzegorz Czajkowski]

Comment on attachment 184215 [details]
proposed patch

According to WK2 review policy, the patch requires WK2 Owners review. Clearing Morrita's r+ :(

[WebKit Review Bot]

Comment on attachment 184215 [details]
proposed patch

Clearing flags on attachment: 184215

Committed r141110: <http://trac.webkit.org/changeset/141110>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Thiago Marcos P. Santos]

The unit tests are failing on the bots.

[Thiago Marcos P. Santos]

[ 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)

[Hajime Morrita]

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

[Thiago Marcos P. Santos]

(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

[Gyuyoung Kim]

Grzegorz, if you wanna land WK1 first, I think you can divide this patch between WK1 and WK2. We can land patch for WK1.

[Hajime Morrita]

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

[Grzegorz Czajkowski]

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

[Hajime Morrita]

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.

[Grzegorz Czajkowski]

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

[Kenneth Rohde Christiansen]

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?

[Grzegorz Czajkowski]

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.

[Grzegorz Czajkowski]

Created attachment 187061 [details]
comments improvements

[Build Bot]

Comment on attachment 187061 [details]
comments improvements

Attachment 187061 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16429231

[Grzegorz Czajkowski]

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

[Anders Carlsson]

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.

[Grzegorz Czajkowski]

Committed r143192: <http://trac.webkit.org/changeset/143192>

[Martin Robinson]

Is it possible for any of this code to move to the enchant spell checking code in WebCore, so that it can be shared?

[Grzegorz Czajkowski]

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