Bug 109577

Summary:

[WK2] Asynchronous spell checking implementation

Product:

WebKit

Reporter:

Grzegorz Czajkowski <g.czajkowski>

Component:

WebKit2

Assignee:

Grzegorz Czajkowski <g.czajkowski>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abecsi, andersca, buildbot, cmarcelo, enrica, gyuyoung.kim, menard, mifenton, morrita, rakuco, rniwa, webkit-ews, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

Unspecified

Bug Depends on:

110208

Bug Blocks:

81042, 108172

Attachments:

Description	Flags
proposed patch	webkit-ews: commit-queue-
hopefully makes EWS happier	buildbot: commit-queue-
rebased after r143688	andersca: review-
apply Anders' suggestions	enrica: review+
patch for landing	none
fix style after rebase	webkit.review.bot: commit-queue-

Grzegorz Czajkowski

Reported 2013-02-12 06:10:29 PST

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.

Attachments
proposed patch (31.27 KB, patch) 2013-02-13 06:20 PST, Grzegorz Czajkowski	webkit-ews: commit-queue-	Details Formatted Diff Diff
hopefully makes EWS happier (36.37 KB, patch) 2013-02-18 06:28 PST, Grzegorz Czajkowski	buildbot: commit-queue-	Details Formatted Diff Diff
rebased after r143688 (32.70 KB, patch) 2013-02-22 02:36 PST, Grzegorz Czajkowski	andersca: review-	Details Formatted Diff Diff
apply Anders' suggestions (31.53 KB, patch) 2013-02-25 04:04 PST, Grzegorz Czajkowski	enrica: review+	Details Formatted Diff Diff
patch for landing (31.19 KB, patch) 2013-03-01 00:35 PST, Grzegorz Czajkowski	no flags	Details Formatted Diff Diff
fix style after rebase (31.18 KB, patch) 2013-03-01 00:49 PST, Grzegorz Czajkowski	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Grzegorz Czajkowski

Comment 1 2013-02-13 06:20:40 PST

Created attachment 188070 [details] proposed patch

Early Warning System Bot

Comment 2 2013-02-13 06:34:57 PST

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 3 2013-02-13 06:49:28 PST

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 4 2013-02-13 07:48:56 PST

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

Comment 5 2013-02-18 06:28:06 PST

Created attachment 188873 [details] hopefully makes EWS happier

Build Bot

Comment 6 2013-02-18 07:15:41 PST

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 7 2013-02-18 10:41:33 PST

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

Comment 8 2013-02-19 04:49:38 PST

(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 9 2013-02-19 05:45:35 PST

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 10 2013-02-19 07:00:10 PST

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

Comment 11 2013-02-19 21:58:42 PST

> 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

Comment 12 2013-02-22 02:36:33 PST

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 13 2013-02-22 08:20:06 PST

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

Comment 14 2013-02-25 04:04:52 PST

Created attachment 190027 [details] apply Anders' suggestions

Grzegorz Czajkowski

Comment 15 2013-02-25 05:15:35 PST

(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

Comment 16 2013-02-27 05:35:22 PST

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

Enrica Casucci

Comment 17 2013-02-28 11:03:43 PST

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

Comment 18 2013-03-01 00:09:33 PST

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

Grzegorz Czajkowski

Comment 19 2013-03-01 00:35:26 PST

Created attachment 190900 [details] patch for landing

WebKit Review Bot

Comment 20 2013-03-01 00:37:29 PST

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

Comment 21 2013-03-01 00:49:40 PST

Created attachment 190903 [details] fix style after rebase

WebKit Review Bot

Comment 22 2013-03-01 02:51:20 PST

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

Comment 23 2013-03-01 03:41:42 PST

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

Note You need to log in before you can comment on or make changes to this bug.