Bug 93611 – WebKitTestRunner needs to turn on 'setContinuousSpellCheckingEnabled'

• Component: WebKit2
• Status: RESOLVED FIXED

[Grzegorz Czajkowski]

WebKitTestRunner needs to turn on 'setContinuousSpellCheckingEnabled' setting to pass the tests from editing/spelling directory.
To do it WebKitTestRunner would create text checker client  (WKTextChecer.h) and define callback functions responsible for spell/grammar checking.

WebKit2-Gtk and WebKit2-EFL wrap WKTextChecker API internally and define their own API level for example,

EAPI void ewk_text_checker_setting_enable_continuous_spell_checking_set(Eina_Bool enable);

WebKitTestRunner should test Native C API of WebKit2.

To enable this setting we can call method from WebKit that refers to methods implemented by client:

WebTextChecker::shared()->client().setContinuousSpellCheckingEnabled(true);

What do you think? If you agree with this proposal I will submit a patch.

[Grzegorz Czajkowski]

Created attachment 162168 [details]
proposed patch

This patch enables spelling feature (in fact turns on setContinuousSpellCheckingEnabled flag) for WebKitTestRunner for all WebKit's ports which implement WKTextChecker's client.

By default this feature is disabled by WebKi2-EFL/WebKi2-GTK. To allow pass the tests from editing/spelling directory, WebKit2 C has been used. This patch doesn't ensure initialization of WKTextChecker's client. This part should be moved to port specific files as it has been proposed for WebKi2-EFL in:
 - initialization of WKTextchecker' client,
 - setting the default language for layout tests.
In both cases the native WebKit2-EFL has been used.

When WebKitTestRunner turns on the spelling feature with the resetStateToConsistentValues() method, it happens that the web process is still not launched (although it is already created). In this case, isValid() method returns false. This fix sends a message to the web process messages queue, and the message  will be handled once the web process is ready.

[Grzegorz Czajkowski]

This patch allows to pass (after gardening) 12 tests from editing/spelling directory for WebKi2-EFL.

Found 27 tests; running 27, skipping 0.
Running 1 WebKitTestRunner over 1 shard.

[1/27] editing/spelling/context-menu-suggestions.html failed
[2/27] editing/spelling/design-mode-spellcheck-off.html passed
[3/27] editing/spelling/grammar-edit-word.html crashed unexpectedly
[4/27] editing/spelling/grammar-markers.html failed unexpectedly (text diff)
[5/27] editing/spelling/grammar-paste.html failed unexpectedly (text diff)
[6/27] editing/spelling/grammar.html failed unexpectedly (text diff)
[7/27] editing/spelling/inline_spelling_markers.html passed
[8/27] editing/spelling/markers.html failed unexpectedly (text diff)
[9/27] editing/spelling/spellcheck-api-crash.html passed
[10/27] editing/spelling/spellcheck-async-mutation.html failed unexpectedly (text diff)
[11/27] editing/spelling/spellcheck-async-remove-frame.html failed unexpectedly (text diff)
[12/27] editing/spelling/spellcheck-async.html failed unexpectedly (text diff)
[13/27] editing/spelling/spellcheck-attribute.html passed
[14/27] editing/spelling/spellcheck-input-search-crash.html failed
[15/27] editing/spelling/spellcheck-paste-disabled.html failed unexpectedly (text diff)
[16/27] editing/spelling/spellcheck-paste.html failed unexpectedly (text diff)
[17/27] editing/spelling/spellcheck-queue.html failed unexpectedly (text diff)
[18/27] editing/spelling/spellcheck-sequencenum.html failed unexpectedly (text diff)
[19/27] editing/spelling/spelling-attribute-at-child.html passed
[20/27] editing/spelling/spelling-attribute-change.html passed
[21/27] editing/spelling/spelling-backspace-between-lines.html failed unexpectedly (text diff)
[22/27] editing/spelling/spelling-hasspellingmarker.html passed
[23/27] editing/spelling/spelling-insert-html.html passed
[24/27] editing/spelling/spelling-linebreak.html passed
[25/27] editing/spelling/spelling-marker-description.html failed unexpectedly (text diff)
[26/27] editing/spelling/spelling-unified-emulation.html crashed unexpectedly
[27/27] editing/spelling/spelling.html passed

12 tests ran as expected, 15 didn't:


Regressions: Unexpected text failures : (13)
  editing/spelling/grammar-markers.html = TEXT
  editing/spelling/grammar-paste.html = TEXT
  editing/spelling/grammar.html = TEXT
  editing/spelling/markers.html = TEXT
  editing/spelling/spellcheck-async-mutation.html = TEXT
  editing/spelling/spellcheck-async-remove-frame.html = TEXT
  editing/spelling/spellcheck-async.html = TEXT
  editing/spelling/spellcheck-paste-disabled.html = TEXT
  editing/spelling/spellcheck-paste.html = TEXT
  editing/spelling/spellcheck-queue.html = TEXT
  editing/spelling/spellcheck-sequencenum.html = TEXT
  editing/spelling/spelling-backspace-between-lines.html = TEXT
  editing/spelling/spelling-marker-description.html = TEXT

Regressions: Unexpected crashes : (2)
  editing/spelling/grammar-edit-word.html = CRASH
  editing/spelling/spelling-unified-emulation.html = CRASH

[Early Warning System Bot]

Comment on attachment 162168 [details]
proposed patch

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

[Build Bot]

Comment on attachment 162168 [details]
proposed patch

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

[Grzegorz Czajkowski]

> /usr/bin/gold: obj/release/TestController.o: in function WTR::TestController::resetStateToConsistentValues():TestController.cpp(.text+0x84f): error: undefined reference to 'WKTextCheckerContinuousSpellCheckingEnabledStateChanged'

Qt build is broken because it doesn't compile WKTextChecker.cpp at all. Adding WebKitTextChecker (header and source) to WebKit2/Target.pri hopefully resolve this build break.

But no idea how to make Mac ews happy :( 
> /Volumes/Data/WebKit/Tools/WebKitTestRunner/TestController.cpp:41:35: error: WebKit2/WKTextChecker.h: No such file or directory

Any idea? Thanks.

[Grzegorz Czajkowski]

Created attachment 162689 [details]
hopefully fixes qt and mac builds

[Build Bot]

Comment on attachment 162689 [details]
hopefully fixes qt and mac builds

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

[Early Warning System Bot]

Comment on attachment 162689 [details]
hopefully fixes qt and mac builds

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

[Grzegorz Czajkowski]

It looks like WebKit2-Qt needs more files (WebTextChecker etc.) to be compiled, I guess they do not compile spellchecking feature at all.
Does it make sense to enable this feature for WebKit2-Qt? How about adding !PLATFORM(QT) in this case ?

[Simon Hausmann]

(In reply to comment #8)
> (From update of attachment 162689 [details])
> Attachment 162689 [details] did not pass qt-wk2-ews (qt):
> Output: http://queues.webkit.org/results/13772643

Looks like more files are missing from the build:

WKTextChecker.cpp:(.text.WKTextCheckerSetClient+0x1f): undefined reference to `WebKit::WebTextChecker::shared()'

That could be Source/WebKit2/UIProcess/WebTextChecker.cpp and Source/WebKit2/UIProcess/WebTextChecker.h

[Simon Hausmann]

(In reply to comment #9)
> It looks like WebKit2-Qt needs more files (WebTextChecker etc.) to be compiled, I guess they do not compile spellchecking feature at all.
> Does it make sense to enable this feature for WebKit2-Qt? How about adding !PLATFORM(QT) in this case ?

I'd rather see the Qt build get the missing files than clutting the C API header files with !PLATFORM(QT). This is about compiling and exposing the C API to allow applications to implement spell checking, so it's a rather straight-forward delegation feature. I'll add it to the build.

[Grzegorz Czajkowski]

(In reply to comment #11)
> (In reply to comment #9)
> > It looks like WebKit2-Qt needs more files (WebTextChecker etc.) to be compiled, I guess they do not compile spellchecking feature at all.
> > Does it make sense to enable this feature for WebKit2-Qt? How about adding !PLATFORM(QT) in this case ?
> 
> I'd rather see the Qt build get the missing files than clutting the C API header files with !PLATFORM(QT). This is about compiling and exposing the C API to allow applications to implement spell checking, so it's a rather straight-forward delegation feature. I'll add it to the build.

Thanks for your opinion. I will try to add them :)

[Grzegorz Czajkowski]

Created attachment 162714 [details]
add missing files to compile Spellchecking for WebKit2-Qt

[Simon Hausmann]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > It looks like WebKit2-Qt needs more files (WebTextChecker etc.) to be compiled, I guess they do not compile spellchecking feature at all.
> > > Does it make sense to enable this feature for WebKit2-Qt? How about adding !PLATFORM(QT) in this case ?
> > 
> > I'd rather see the Qt build get the missing files than clutting the C API header files with !PLATFORM(QT). This is about compiling and exposing the C API to allow applications to implement spell checking, so it's a rather straight-forward delegation feature. I'll add it to the build.
> 
> Thanks for your opinion. I will try to add them :)

Ah, thanks! You beat me to it :) The EWS is green now and I also tried it locally.

[Build Bot]

Comment on attachment 162714 [details]
add missing files to compile Spellchecking for WebKit2-Qt

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

[Grzegorz Czajkowski]

(In reply to comment #15)
> (From update of attachment 162714 [details])
> Attachment 162714 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/13788065
>
>   Undefined symbols for architecture x86_64:
>  "__ZN6WebKit14WebTextChecker6sharedEv", referenced from:

It seems that Mac port doesn't compile spellchecker feature. Do you prefer adding missing files to Mac build system (WebTextChecker etc. as adding WKTextChecker was insufficient) or just disabling this part of code #if !PLATFORM(MAC) ?

[Simon Hausmann]

Comment on attachment 162714 [details]
add missing files to compile Spellchecking for WebKit2-Qt

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

> Tools/ChangeLog:8
> +        WebKitTestRunner enables spelling feature to pass the layout tests from editing/spelling.

Just a thought: If this enables layout tests from editing/spelling, shouldn't tests be unskipped in the same commit?

[Grzegorz Czajkowski]

(In reply to comment #17)
> (From update of attachment 162714 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162714&action=review
> 
> > Tools/ChangeLog:8
> > +        WebKitTestRunner enables spelling feature to pass the layout tests from editing/spelling.
> 
> Just a thought: If this enables layout tests from editing/spelling, shouldn't tests be unskipped in the same commit?

Makes sense, but in fact setting setContinuousSpellChecking is insufficient to pass tests from editing/spelling for all WebKit ports. We need to deliver implementation of WKTextChecker's client as I proposed it for WebKit2-EFL in this patch. And of course some gardening is required - I planed to do this in the next patch.

What about enabling setContinuousSpellChecking flag only in this commit? Other stuff (gardening and attaching WKTextChecker's client for WebKit2-EFL should be done in separate commit)?

[Grzegorz Czajkowski]

Created attachment 163562 [details]
unskip the spelling tests

[Kenneth Rohde Christiansen]

Comment on attachment 163562 [details]
unskip the spelling tests

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

> Source/WebKit2/ChangeLog:13
> +        This fix sends a message to the WebProcess messages queue, and the message
> +        will be handled once the WebProcess is ready.

Where is that happening? the canSendMessage?

[Grzegorz Czajkowski]

Comment on attachment 163562 [details]
unskip the spelling tests

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

>> Source/WebKit2/ChangeLog:13
>> +        will be handled once the WebProcess is ready.
> 
> Where is that happening? the canSendMessage?

When we call API to enable spelling in TestController::resetStateToConsistentValues().
The TextChecker's state has to be updated, in that time the WebProcess is still not launched. In this case, isValid() method returns false. We can send the message to update the state to queue by checking the WebProcess activity using canSendMessage() instead of isValid.

[Build Bot]

Comment on attachment 163562 [details]
unskip the spelling tests

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

[Maciej Stachowiak]

Enrica, Alexey, do either of you understand how this should work on Mac? The patch fails to build due to the WKTextChecker API being missing on Mac. Do we have some other way of controlling continuous spellchecking? Is WKTextChecker something we want?

[Enrica Casucci]

(In reply to comment #23)
> Enrica, Alexey, do either of you understand how this should work on Mac? The patch fails to build due to the WKTextChecker API being missing on Mac. Do we have some other way of controlling continuous spellchecking? Is WKTextChecker something we want?

I think we should add Jia to this conversation, but from what I know, we have other types of problems with the spell checking and autocorrection on the Apple Mac port. Jia was working on a patch to try to address some of them. The main issue is the fact that our spelling and autocorrection system learns and in roder to get consistent results over and over we need to reset it before we run the tests.
Add Jia to the cc list so that he can comment.

[Grzegorz Czajkowski]

(In reply to comment #24)
> (In reply to comment #23)
> > Enrica, Alexey, do either of you understand how this should work on Mac? The patch fails to build due to the WKTextChecker API being missing on Mac. Do we have some other way of controlling continuous spellchecking? Is WKTextChecker something we want?
> 
> I think we should add Jia to this conversation, but from what I know, we have other types of problems with the spell checking and autocorrection on the Apple Mac port. Jia was working on a patch to try to address some of them. The main issue is the fact that our spelling and autocorrection system learns and in roder to get consistent results over and over we need to reset it before we run the tests.
> Add Jia to the cc list so that he can comment.

Any response on this? How about disabling this part of WebKitTestRunner code for Mac platform as there are other types of problems (autocoration system)?

This patch only enables/turns on 'setContinuousSpellCheckingEnabled' by WK2 API. The main purpose of this change is to pass spelling tests for WebKit ports which have spelling feature disabled by default (WK2-EFL, WK2-GTK).

[Grzegorz Czajkowski]

Created attachment 169398 [details]
disable continuous spell checker for Mac and rebase the patch

[Andreas Kling]

Ping Jia

[Mario Sanchez Prada]

(In reply to comment #25)
> [...]
> Any response on this? How about disabling this part of WebKitTestRunner code 
> for Mac platform as there are other types of problems (autocoration system)?

This sounds like good idea to me, but Apple people should confirm it too.

[Gyuyoung Kim]

Comment on attachment 169398 [details]
disable continuous spell checker for Mac and rebase the patch

Looks fine on EFL port side. Jia, any comments ?

[Hajime Morrita]

Comment on attachment 169398 [details]
disable continuous spell checker for Mac and rebase the patch

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

Looks sane. rs=me.

> LayoutTests/ChangeLog:3
> +        WebKitTestRunner needs to turn on 'setContinuousSpellCheckingEnabled'

Could you make it clear that this change is for EFL port by, for example, tagging with [EFF]?

> Source/WebKit2/ChangeLog:3
> +        WebKitTestRunner needs to turn on 'setContinuousSpellCheckingEnabled'

Ditto.

[Grzegorz Czajkowski]

Created attachment 170347 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 170347 [details]
patch for landing

Clearing flags on attachment: 170347

Committed r132333: <http://trac.webkit.org/changeset/132333>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

This has broken many tests on the EFL-WK2 bot, see <http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/4983/steps/layout-test/logs/stdio>. Sample backtrace:

STDERR: ASSERTION FAILED: misspellingLocation < len
STDERR: /home/rakuco/dev/WebKit/Source/WebCore/editing/TextCheckingHelper.cpp(263) : WTF::String WebCore::TextCheckingHelper::findFirstMisspelling(int&, bool, WTF::RefPtr<WebCore::Range>&)
STDERR: 1   0x7f1adca15ba1 WebCore::TextCheckingHelper::findFirstMisspelling(int&, bool, WTF::RefPtr<WebCore::Range>&)
STDERR: 2   0x7f1adca17e8a WebCore::TextCheckingHelper::markAllMisspellings(WTF::RefPtr<WebCore::Range>&)
STDERR: 3   0x7f1adc9d46b6 WebCore::Editor::markMisspellingsOrBadGrammar(WebCore::VisibleSelection const&, bool, WTF::RefPtr<WebCore::Range>&)
STDERR: 4   0x7f1adc9d4806 WebCore::Editor::markMisspellings(WebCore::VisibleSelection const&, WTF::RefPtr<WebCore::Range>&)
STDERR: 5   0x7f1adc9d6517 WebCore::Editor::markMisspellingsAndBadGrammar(WebCore::VisibleSelection const&, bool, WebCore::VisibleSelection const&)
STDERR: 6   0x7f1adc9da755 WebCore::Editor::respondToChangedSelection(WebCore::VisibleSelection const&, unsigned int)
STDERR: 7   0x7f1adc9e8234 WebCore::FrameSelection::setSelection(WebCore::VisibleSelection const&, unsigned int, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity)
STDERR: 8   0x7f1adc9e71c7 WebCore::FrameSelection::moveTo(WebCore::VisiblePosition const&, WebCore::EUserTriggered, WebCore::FrameSelection::CursorAlignOnScroll)
STDERR: 9   0x7f1adcd9c6f4 WebCore::DOMSelection::collapse(WebCore::Node*, int, int&)
STDERR: 10  0x7f1add678bf2 WebCore::jsDOMSelectionPrototypeFunctionCollapse(JSC::ExecState*)
STDERR: 11  0x7f1a88f2c265

[WebKit Review Bot]

Re-opened since this is blocked by bug 100234

[Grzegorz Czajkowski]

(In reply to comment #35)
> Re-opened since this is blocked by bug 100234

Those assertions fail for the debug build so it's the main reason that I couldn't see them while I was working on the patch.
IMHO this patch doesn't cause this assertion as this only enables spelling for layout tests (it's possible that they may happen while using MiniBrowser too). I think it's rather connected with spelling implementation for EFL port. 
I will take care of it.

[Grzegorz Czajkowski]

Created attachment 172565 [details]
rebased patch

The issue which cause regressions has been fixed in separate bug 101215.
I didn't change any code in this patch since Morita has reviewed it. I am not sure if we can land it without asking review once again.
I will ask cq request as soon as the patch from bug 101215 lands.

[EFL EWS Bot]

Comment on attachment 172565 [details]
rebased patch

Attachment 172565 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14747421

[Chris Dumez]

Comment on attachment 172565 [details]
rebased patch

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

> Tools/WebKitTestRunner/efl/TestControllerEfl.cpp:105
> +    Ewk_Text_Checker::initialize();

Why is this needed? It is internal code and already called in Ewk_Context constructor.
It does not seem right to do this here.

[Grzegorz Czajkowski]

Created attachment 172744 [details]
remove EFL's changes from TestControllerEfl.cpp

Latest changes of WK2-EFL have shown that the changes in TestControllerEfl.cpp are no longer needed (there I proposed to initialize client of WKTextChecker and set the default language). At the moment those stuff are initialized in Ewk_Context.

[WebKit Review Bot]

Comment on attachment 172744 [details]
remove EFL's changes from TestControllerEfl.cpp

Clearing flags on attachment: 172744

Committed r133736: <http://trac.webkit.org/changeset/133736>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.