Bug 81042

Summary: [WK2] WTR needs an implementation of setAsynchronousSpellCheckingEnabled
Product: WebKit Reporter: Jessie Berlin <jberlin>
Component: Tools / TestsAssignee: Grzegorz Czajkowski <g.czajkowski>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, enrica, g.czajkowski, gyuyoung.kim, jberlin, jeffrey+webkit, ossy, rakuco, sam, thorton, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar, LayoutTestFailure, PlatformOnly
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: Unspecified   
Bug Depends on: 91854, 108172, 109577    
Bug Blocks: 108528, 111295    
Attachments:
Description Flags
insufficient
none
adapt Tim's patch enrica: review+

Comment 1 Radar WebKit Bug Importer 2012-03-13 15:23:22 PDT 
<rdar://problem/11042016>
Comment 2 Jessie Berlin 2012-03-13 15:43:08 PDT 
Added to the mac-wk2 Skipped list in http://trac.webkit.org/changeset/110625
Comment 3 Csaba Osztrogonác 2012-05-21 06:49:52 PDT 
It isn't mac-wk2 spefific bug, but general wk2 bug, so I moved tests
to wk2/Skipped list and added one more test. - http://trac.webkit.org/changeset/117776
Comment 4 Tim Horton 2012-06-04 16:25:08 PDT 
Just implementing setAsynchronousSpellCheckingEnabled is not sufficient to make those tests work, at least not for the Mac port.
Comment 5 Tim Horton 2012-06-04 17:02:56 PDT 
Created attachment 145657 [details]
insufficient
Comment 6 Grzegorz Czajkowski 2012-07-17 08:04:19 PDT 
(In reply to comment #5)
> Created an attachment (id=145657) [details]
> insufficient

WebKit2-EFL needs this patch as we've almost done spellchecker feature and we would like to perform the tests from editing/spelling directory, btw:

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2010
> +    settings->setFullScreenEnabled(store.getBoolValueForKey(WebPreferencesKey::asynchronousSpellCheckingEnabledKey()));

should be settings->setAsynchronousSpellCheckingEnabled(...)
Comment 7 Tim Horton 2012-07-24 17:57:43 PDT 
(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=145657) [details] [details]
> > insufficient
> 
> WebKit2-EFL needs this patch as we've almost done spellchecker feature and we would like to perform the tests from editing/spelling directory, btw:
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=145657&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2010
> > +    settings->setFullScreenEnabled(store.getBoolValueForKey(WebPreferencesKey::asynchronousSpellCheckingEnabledKey()));
> 
> should be settings->setAsynchronousSpellCheckingEnabled(...)

Erg, good catch. That explains why it didn't work! You're welcome to pick up the patch and fix that and see if it works.
Comment 8 Grzegorz Czajkowski 2012-08-06 06:04:25 PDT 
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Created an attachment (id=145657) [details] [details] [details]
> > > insufficient
> > 
> > WebKit2-EFL needs this patch as we've almost done spellchecker feature and we would like to perform the tests from editing/spelling directory, btw:
> > 
> > View in context: https://bugs.webkit.org/attachment.cgi?id=145657&action=review
> > 
> > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2010
> > > +    settings->setFullScreenEnabled(store.getBoolValueForKey(WebPreferencesKey::asynchronousSpellCheckingEnabledKey()));
> > 
> > should be settings->setAsynchronousSpellCheckingEnabled(...)
> 
> Erg, good catch. That explains why it didn't work! You're welcome to pick up the patch and fix that and see if it works.

Ok thanks. I will submit this patch with the fix and ChangeLog entries.
Comment 9 Vicki Pfau 2012-10-11 13:12:33 PDT 
Skipping editing/spelling/grammar-markers-hidpi.html in r131090
Comment 10 Grzegorz Czajkowski 2013-02-26 06:27:21 PST 
Hi Tim,

I am adapting your patch to the trunk. The required patches (bug 108172 and bug 109577) are likely to land.
I would like to expose asynchronous spell checking setting in this patch as it may be useful for WK2 ports that are going to enable it. In my opinion it doesn't make sense to implement it as private and then make another commit to make it public.
This patch will contain gardening of asynchronous spell checking tests. I am planing to skip them for GtK, Mac, Qt unless they implement TextChecker::requestCheckingOfString() method.
Is it ok for you if I add myself as co-author of the patch?
Thanks
Comment 11 Tim Horton 2013-02-26 10:10:05 PST 
(In reply to comment #10)
> Hi Tim,
> 
> I am adapting your patch to the trunk. The required patches (bug 108172 and bug 109577) are likely to land.
> I would like to expose asynchronous spell checking setting in this patch as it may be useful for WK2 ports that are going to enable it. In my opinion it doesn't make sense to implement it as private and then make another commit to make it public.
> This patch will contain gardening of asynchronous spell checking tests. I am planing to skip them for GtK, Mac, Qt unless they implement TextChecker::requestCheckingOfString() method.
> Is it ok for you if I add myself as co-author of the patch?

Go for it!

> Thanks

Thanks for picking this up!
Comment 12 Grzegorz Czajkowski 2013-02-27 03:24:25 PST 
Created attachment 190483 [details]
adapt Tim's patch
Comment 13 Grzegorz Czajkowski 2013-02-27 05:27:28 PST 
FYI, with this patch (and its dependencies bug 109577 and 108172) the below asynchronous spell checking tests are passing for EFL:

editing/spelling/spellcheck-async-remove-frame.html
editing/spelling/spellcheck-async.html
editing/spelling/spellcheck-sequencenum.html

The remaining asynchronous tests:
editing/spelling/spellcheck-async-mutation.html
editing/spelling/spellcheck-queue.html
editing/spelling/spellcheck-paste-disabled.html
editing/spelling/spellcheck-paste.html
editing/spelling/grammar-markers.html
editing/spelling/grammar-markers-hidpi.html
editing/spelling/grammar-paste.html
editing/spelling/spelling-marker-description.html

are failing for EFL due to not implemented features (pasteboard, grammar checking and text replacement). More details in TestExpectations file.
I was trying to do some tricks to pass the failing tests like replacing missing Paste command with InsertText and as a result, they start passing.

Recently, the bunch of spell checking tests have been landed (bug 108405). Those require asynchronous spell checking and smartInsertDelete feature (bug 107840).

I'd appreciate if wk2's owners could review the mentioned patches to support asynchronous spell checking in WK2 (bug 109577, bug 108405, bug 108172 and bug 81042)
Thanks
Comment 14 Enrica Casucci 2013-03-04 13:17:51 PST 
Comment on attachment 190483 [details]
adapt Tim's patch

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

Looks good to me. Please remove all the unnecessary lines from the ChangeLog.

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

Remove.

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

Remove.

> Source/WebKit2/ChangeLog:27
> +        (InjectedBundle):

Remove.

> Tools/ChangeLog:15
> +        (WTR):

Remove.

> Tools/ChangeLog:17
> +        (TestRunner):

Ditto.
Comment 15 Grzegorz Czajkowski 2013-03-05 05:32:26 PST 
Committed r144752: <http://trac.webkit.org/changeset/144752>