Bug 107682 - [WK2][EFL] Unified text checker implementation
Summary: [WK2][EFL] Unified text checker implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Normal
Assignee: Grzegorz Czajkowski
URL:
Keywords:
Depends on: 108299 108349
Blocks: 108172 108528
  Show dependency treegraph
 
Reported: 2013-01-23 05:49 PST by Grzegorz Czajkowski
Modified: 2013-02-25 00:41 PST (History)
17 users (show)

See Also:


Attachments
proposed patch (8.79 KB, patch)
2013-01-23 06:19 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
don't call checkSpellingOfString for word separators (9.47 KB, patch)
2013-02-06 04:09 PST, Grzegorz Czajkowski
kenneth: review-
Details | Formatted Diff | Diff
comments improvements (9.51 KB, patch)
2013-02-07 04:29 PST, Grzegorz Czajkowski
andersca: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Grzegorz Czajkowski 2013-01-23 05:49:54 PST
Provide implementation of textCheckOfParagraph which allows to check spelling for multiple words.
Comment 1 Grzegorz Czajkowski 2013-01-23 06:19:20 PST
Created attachment 184215 [details]
proposed patch
Comment 2 Grzegorz Czajkowski 2013-01-24 01:09:11 PST
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
Comment 3 Grzegorz Czajkowski 2013-01-29 06:31:37 PST
(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 4 Grzegorz Czajkowski 2013-01-29 06:47:20 PST
Comment on attachment 184215 [details]
proposed patch

According to WK2 review policy, the patch requires WK2 Owners review. Clearing Morrita's r+ :(
Comment 5 WebKit Review Bot 2013-01-29 07:31:21 PST
Comment on attachment 184215 [details]
proposed patch

Clearing flags on attachment: 184215

Committed r141110: <http://trac.webkit.org/changeset/141110>
Comment 6 WebKit Review Bot 2013-01-29 07:31:26 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Thiago Marcos P. Santos 2013-01-29 10:09:07 PST
The unit tests are failing on the bots.
Comment 8 Thiago Marcos P. Santos 2013-01-29 10:10:11 PST
[ 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)
Comment 9 Hajime Morrita 2013-01-29 16:09:22 PST
(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.
Comment 10 Thiago Marcos P. Santos 2013-01-29 16:23:41 PST
(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
Comment 11 Gyuyoung Kim 2013-01-29 17:25:51 PST
Grzegorz, if you wanna land WK1 first, I think you can divide this patch between WK1 and WK2. We can land patch for WK1.
Comment 12 Hajime Morrita 2013-01-29 18:09:28 PST
(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.
Comment 13 Grzegorz Czajkowski 2013-02-06 04:09:22 PST
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
Comment 14 Hajime Morrita 2013-02-07 00:04:09 PST
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.
Comment 15 Grzegorz Czajkowski 2013-02-07 01:00:16 PST
(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 16 Kenneth Rohde Christiansen 2013-02-07 01:43:41 PST
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 17 Grzegorz Czajkowski 2013-02-07 04:27:09 PST
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.
Comment 18 Grzegorz Czajkowski 2013-02-07 04:29:55 PST
Created attachment 187061 [details]
comments improvements
Comment 19 Build Bot 2013-02-07 10:49:26 PST
Comment on attachment 187061 [details]
comments improvements

Attachment 187061 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16429231
Comment 20 Grzegorz Czajkowski 2013-02-08 00:59:00 PST
(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 21 Anders Carlsson 2013-02-13 07:55:26 PST
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.
Comment 22 Grzegorz Czajkowski 2013-02-18 03:36:29 PST
Committed r143192: <http://trac.webkit.org/changeset/143192>
Comment 23 Martin Robinson 2013-02-22 12:39:07 PST
Is it possible for any of this code to move to the enchant spell checking code in WebCore, so that it can be shared?
Comment 24 Grzegorz Czajkowski 2013-02-25 00:41:06 PST
(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+.