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.
Created attachment 188070 [details] proposed patch
Comment on attachment 188070 [details] proposed patch Attachment 188070 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16514166
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
Comment on attachment 188070 [details] proposed patch Attachment 188070 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16527173
Created attachment 188873 [details] hopefully makes EWS happier
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
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?
(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
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
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
> 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.
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,...)
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.
Created attachment 190027 [details] apply Anders' suggestions
(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.
The reviewers might be interested in the tests result with this patch, see bug 81042, comment 13.
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.
Thanks Enrica for your review! I'll apply it before landing.
Created attachment 190900 [details] patch for landing
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.
Created attachment 190903 [details] fix style after rebase
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
Committed r144436: <http://trac.webkit.org/changeset/144436>