Bug 109577 – [WK2] Asynchronous spell checking implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Grzegorz Czajkowski]

This bug adds the core implementation of the asynchronous spell checking for WK2.

Introduction:
-------------

WebCore asynchronous spell checking implementation is based on abstract class (TextCheckingRequest) which is passed to the client to check spelling for the request and call didSuccess/didCancel when the client has done checking.

WebCore's SpellChecker object (mainly responsible for asynchronous spell checking) is connected with Frame object. As a result each frame contains own the queue of the spell checking requests.

Proposed solution:
--------------

Associate the abstract TextCheckingRequest with the unique identifier on WebProcess side.
Extract the request data from the TextCheckingRequest class and pass it to the UIProcess.
The client (TextChecker{Gtk/Mac/Efl/Qt}.cpp) gets a new defined abstract object to get the request data and to notify about the results.
After checking spelling the client calls didFinishChecking()/didCancelChecking() (similarly to WebCore::didSuccess/WebCore::didCancel)  on the abstract type.
The implementation sends message to WebProcess - the request (with the proper identifier) has been finished.
WebPage calls on the proper TextCheckingRequest object didSuccess/didCancel method.

Patch is coming.

[Grzegorz Czajkowski]

Created attachment 188070 [details]
proposed patch

[Early Warning System Bot]

Comment on attachment 188070 [details]
proposed patch

Attachment 188070 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16514166

[Build Bot]

Comment on attachment 188070 [details]
proposed patch

Attachment 188070 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16537149

[Build Bot]

Comment on attachment 188070 [details]
proposed patch

Attachment 188070 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16527173

[Grzegorz Czajkowski]

Created attachment 188873 [details]
hopefully makes EWS happier

[Build Bot]

Comment on attachment 188873 [details]
hopefully makes EWS happier

Attachment 188873 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16617154

[Anders Carlsson]

Comment on attachment 188873 [details]
hopefully makes EWS happier

View in context: https://bugs.webkit.org/attachment.cgi?id=188873&action=review

Looks good overall, I think it might make sense to land the WebCore specific changes as a separate patch though.

> Source/WebCore/platform/text/TextChecking.h:119
> +class TextCheckingRequest : public RefCounted<TextCheckingRequest>, public TextCheckingRequestData {

It’s a little weird to use inheritance here. Can you make TextCheckingRequestData a member instead?

[Grzegorz Czajkowski]

(In reply to comment #7)
> (From update of attachment 188873 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188873&action=review
> 
> Looks good overall, I think it might make sense to land the WebCore specific changes as a separate patch though.

Done (bug 110208).

> 
> > Source/WebCore/platform/text/TextChecking.h:119
> > +class TextCheckingRequest : public RefCounted<TextCheckingRequest>, public TextCheckingRequestData {
> 
> It’s a little weird to use inheritance here. Can you make TextCheckingRequestData a member instead?

We should be aware of further changes in WebCore (Editor.cpp/SpellChecker.cpp) and ports implementations (BlackBerry, Chromium, Mac) then.
At the moment we are transparently using 'TextCheckingRequest' as data container and callback object. Making 'TextCheckingRequestData' as member of 'TextCheckingRequest' class will cause update a lot of code for example, request->sequecene() to request->data().sequence().
IMHO the most advantage of inheritance here is not exposing the setter method for sequecene(). This member has a default value (unrequestedSequence = -1) and as a protected member can be changed by SpellCheckRequest::setCheckerAndSequence (SpellCheckRequest inherits TextCheckingRequest).
If you guys prefer 'TextCheckingRequestData' member as member of 'TextCheckingRequest' instead of inheritance just let me know, I can change it at bug 110208

[Build Bot]

Comment on attachment 188873 [details]
hopefully makes EWS happier

Attachment 188873 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16621673

[Build Bot]

Comment on attachment 188873 [details]
hopefully makes EWS happier

Attachment 188873 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16621712

[Hajime Morrita]

> If you guys prefer 'TextCheckingRequestData' member as member of 'TextCheckingRequest' instead of inheritance just let me know, I can change it at bug 110208
Let's do it. I expect the WebCore change being fairly mechanical and 
not significant, even though the number of line changed will be a bit large.

[Grzegorz Czajkowski]

Created attachment 189731 [details]
rebased after r143688

Rebased as the dependent bug 110208 was resolved.

Added one method spellDocumentTag() to TextCheckerCompetion class. It returns the WebPageProxy's spellDocumentTag as the client might be interested in from which page the request came for example, the client (in requestCheckingOfString) may call checkSpellingOfStrin(spellDocumentTag...) or checkTextOfParagraph(spellDocumentTag,...)

[Anders Carlsson]

Comment on attachment 189731 [details]
rebased after r143688

View in context: https://bugs.webkit.org/attachment.cgi?id=189731&action=review

How is async text checking enabled? (My guess is that it's a setting in WebCore?)

Patch looks good overall, but I'd like to see another patch with my suggested changes incorporated.

> Source/WebKit2/UIProcess/TextCheckerCompletion.cpp:31
> +PassRefPtr<TextCheckerCompletionImpl> TextCheckerCompletionImpl::create(uint64_t requestID, const WebCore::TextCheckingRequestData& requestData, WebPageProxy* page)

Please use "using namespace WebCore;" so you don't have to use the WebCore:: qualifier here.

> Source/WebKit2/UIProcess/TextCheckerCompletion.cpp:36
> +TextCheckerCompletionImpl::TextCheckerCompletionImpl(uint64_t requestID, const WebCore::TextCheckingRequestData& requestData, WebPageProxy* page)

Ditto.

> Source/WebKit2/UIProcess/TextCheckerCompletion.cpp:43
> +const WebCore::TextCheckingRequestData& TextCheckerCompletionImpl::textCheckingRequestData() const

Ditto.

> Source/WebKit2/UIProcess/TextCheckerCompletion.cpp:53
> +void TextCheckerCompletionImpl::didFinishCheckingText(const Vector<WebCore::TextCheckingResult>& result) const

Ditto.

> Source/WebKit2/UIProcess/TextCheckerCompletion.h:47
> +class TextCheckerCompletionImpl : public TextCheckerCompletion {

I don't understand why we need both a TextCheckerCompletion class and a TextCheckerCompletionImpl class - can't we just have a single class?

> Source/WebKit2/UIProcess/TextCheckerCompletion.h:56
> +    virtual const WebCore::TextCheckingRequestData& textCheckingRequestData() const OVERRIDE;
> +    virtual int64_t spellDocumentTag() OVERRIDE;
> +
> +    virtual void didFinishCheckingText(const Vector<WebCore::TextCheckingResult>&) const OVERRIDE;
> +    virtual void didCancelCheckingText() const OVERRIDE;

These can probably all be private.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3783
> +void WebPage::addTextCheckingRequest(uint64_t requestID, PassRefPtr<WebCore::TextCheckingRequest> request)

No need for WebCore:: here.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3788
> +void WebPage::didFinishCheckingText(uint64_t requestID, const Vector<WebCore::TextCheckingResult>& result)

Ditto.

[Grzegorz Czajkowski]

Created attachment 190027 [details]
apply Anders' suggestions

[Grzegorz Czajkowski]

(In reply to comment #13)
> (From update of attachment 189731 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189731&action=review
> 
> How is async text checking enabled? (My guess is that it's a setting in WebCore?)

Yes, there's setting in WebCore (asynchronousSpellCheckingEnabled). We need to expose it for WK2 - partially it's done at bug 81042. I am currently adapting it to the trunk.

> > Source/WebKit2/UIProcess/TextCheckerCompletion.h:56
> > +    virtual const WebCore::TextCheckingRequestData& textCheckingRequestData() const OVERRIDE;
> > +    virtual int64_t spellDocumentTag() OVERRIDE;
> > +
> > +    virtual void didFinishCheckingText(const Vector<WebCore::TextCheckingResult>&) const OVERRIDE;
> > +    virtual void didCancelCheckingText() const OVERRIDE;
> 
> These can probably all be private.

They are responsible for getting the request data and notifying the WebPage about spelling results. The client (TextChecker{Efl,Gtk,Mac,Qt}) calls these methods. You can see example of their usage at bug 108172.

[Grzegorz Czajkowski]

The reviewers might be interested in the tests result with this patch, see bug 81042, comment 13.

[Enrica Casucci]

Comment on attachment 190027 [details]
apply Anders' suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=190027&action=review

Overall it looks good to me. Just some minor comments about the changelog.

> Source/WebKit2/ChangeLog:28
> +        (CoreIPC::::decode):

Remove the 2 lines above.

> Source/WebKit2/ChangeLog:36
> +        (TextChecker):

Remove.

> Source/WebKit2/ChangeLog:40
> +        (WebKit):

Ditto.

> Source/WebKit2/ChangeLog:58
> +        (TextCheckerCompletion):

ditto

> Source/WebKit2/ChangeLog:120
> +        (WebPage):

Ditto.

[Grzegorz Czajkowski]

Thanks Enrica for your review!
I'll apply it before landing.

[Grzegorz Czajkowski]

Created attachment 190900 [details]
patch for landing

[WebKit Review Bot]

Attachment 190900 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/CMakeLists.txt', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/Scripts/webkit2/messages.py', u'Source/WebKit2/Shared/WebCoreArgumentCoders.cpp', u'Source/WebKit2/Shared/WebCoreArgumentCoders.h', u'Source/WebKit2/Target.pri', u'Source/WebKit2/UIProcess/TextChecker.h', u'Source/WebKit2/UIProcess/TextCheckerCompletion.cpp', u'Source/WebKit2/UIProcess/TextCheckerCompletion.h', u'Source/WebKit2/UIProcess/WebPageProxy.cpp', u'Source/WebKit2/UIProcess/WebPageProxy.h', u'Source/WebKit2/UIProcess/WebPageProxy.messages.in', u'Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp', u'Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp', u'Source/WebKit2/UIProcess/mac/TextCheckerMac.mm', u'Source/WebKit2/UIProcess/qt/TextCheckerQt.cpp', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.messages.in']" exit_code: 1
Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:49:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Grzegorz Czajkowski]

Created attachment 190903 [details]
fix style after rebase

[WebKit Review Bot]

Comment on attachment 190903 [details]
fix style after rebase

Rejecting attachment 190903 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 190903, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
queue/Source/WebKit/chromium/third_party/sfntly/cpp/src --revision 134 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
46>At revision 134.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://webkit-commit-queue.appspot.com/results/16851208

[Grzegorz Czajkowski]

Committed r144436: <http://trac.webkit.org/changeset/144436>