[webkit-reviews] review denied: [Bug 107682] [WK2][EFL] Unified text checker implementation : [Attachment 186818] don't call checkSpellingOfString for word separators

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Grzegorz Czajkowski
<g.czajkowski at samsung.com>'s request for review:
Bug 107682: [WK2][EFL] Unified text checker implementation
https://bugs.webkit.org/show_bug.cgi?id=107682

Attachment 186818: don't call checkSpellingOfString for word separators
https://bugs.webkit.org/attachment.cgi?id=186818&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
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?