Bug 144648 – [GTK] All spell checking layout tests fail

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

This because spell checking is activated using the GObject API-level WebKitWebContext, but WebKitTestRunner uses the C API.

[Martin Robinson]

*** Bug 130411 has been marked as a duplicate of this bug. ***

[Martin Robinson]

*** Bug 73003 has been marked as a duplicate of this bug. ***

[Martin Robinson]

*** Bug 72248 has been marked as a duplicate of this bug. ***

[Martin Robinson]

*** Bug 108529 has been marked as a duplicate of this bug. ***

[Martin Robinson]

Created attachment 252490 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:68:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:69:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 8 files


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

[Michael Catanzaro]

In file included from ../../Source/WTF/wtf/FastMalloc.h:26:0,
                 from ../../Source/WebKit2/config.h:44,
                 from ../../Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:28:
../../Source/WTF/wtf/StdLibExtras.h: In instantiation of 'typename std::remove_reference< <template-parameter-1-1> >::type&& WTF::move(T&&) [with T = WTF::Vector<WTF::String>; typename std::remove_reference< <template-parameter-1-1> >::type = WTF::Vector<WTF::String>]':
../../Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:157:69:   required from here
../../Source/WTF/wtf/StdLibExtras.h:125:5: error: static assertion failed: T is not an lvalue reference; move() is unnecessary.
     static_assert(std::is_lvalue_reference<T>::value, "T is not an lvalue reference; move() is unnecessary.");
     ^
ninja: build stopped: subcommand failed.

[Carlos Garcia Campos]

Comment on attachment 252490 [details]
Patch

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

Cool, I have a few comments/questions.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:840
> +    Vector<String> spellCheckingLanguages = WTF::move(TextChecker::loadedSpellCheckingLanguages());

I'm not sure WTF::move has any effect this way. I think it should be loadedSpellCheckingLanguages() the one doing move in the returns. It could be clearer if the vector was an out paramater passed as a reference, though.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:848
> +    languagesToReturn = adoptGRef(g_ptr_array_new_with_free_func(g_free));
> +    for (const auto& language : spellCheckingLanguages)
> +        g_ptr_array_add(languagesToReturn.get(), g_strdup(language.utf8().data()));
> +    g_ptr_array_add(languagesToReturn.get(), nullptr);

Since languagesToReturn is static, shouldn't we avoid all this when != nullptr?

> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:56
> +        Vector<String> languages;
> +        languages.append("en_US");
> +        checker.get().updateSpellCheckingLanguages(languages);

I think you could do something like this:

checker.get().updateSpellCheckingLanguages(Vector<String> { "en_US" });

> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:69
> +    static TextCheckerState textCheckerState;
> +    static std::once_flag onceFlag;
> +    std::call_once(onceFlag, [] {
> +        textCheckerState.isContinuousSpellCheckingEnabled = false;;
> +        textCheckerState.isGrammarCheckingEnabled = false;;

We could probably simply add { false } to TextCheckerState.h. There's double ; there

> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:84
> +        contexts[i]->textCheckerStateChanged();

So, this is indeed per context, then. We can enable/disable spell checking per context?

> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:89
> -    return WebTextChecker::singleton()->client().continuousSpellCheckingAllowed();
> +    return true;

Why is this now unconditionally true?

> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:157
> +    guesses = WTF::move(enchantTextChecker().getGuessesForWord(word));

Same comment here about the move

[Martin Robinson]

Comment on attachment 252490 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:840
>> +    Vector<String> spellCheckingLanguages = WTF::move(TextChecker::loadedSpellCheckingLanguages());
> 
> I'm not sure WTF::move has any effect this way. I think it should be loadedSpellCheckingLanguages() the one doing move in the returns. It could be clearer if the vector was an out paramater passed as a reference, though.

Yeah, I think I'll simply remove the WTF::move here. The compiler should be able to optimize it away.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:848
>> +    g_ptr_array_add(languagesToReturn.get(), nullptr);
> 
> Since languagesToReturn is static, shouldn't we avoid all this when != nullptr?

It's static, but it can change between invocations of webkit_web_context_get_spell_checking_languages, so we have to update it each time.

>> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:56
>> +        checker.get().updateSpellCheckingLanguages(languages);
> 
> I think you could do something like this:
> 
> checker.get().updateSpellCheckingLanguages(Vector<String> { "en_US" });

Alright. I'll give that a shot.

>> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:69
>> +        textCheckerState.isGrammarCheckingEnabled = false;;
> 
> We could probably simply add { false } to TextCheckerState.h. There's double ; there

I'm not sure if false is an appropriate default value for all platforms though. Would it be okay to simply set it explicitly for now?

>> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:84
>> +        contexts[i]->textCheckerStateChanged();
> 
> So, this is indeed per context, then. We can enable/disable spell checking per context?

Yes, the WebCore layer supports per-context settings, but not the WebKit2 layer. It uses a singleton to deal with all web processes.

>> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:89
>> +    return true;
> 
> Why is this now unconditionally true?

In the previous code continuousSpellCheckingAllowed was unimplemented in the client, which always returned false. For enchant-based ports though, continuous spell checking is always available. It isn't a system setting.

>> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:157
>> +    guesses = WTF::move(enchantTextChecker().getGuessesForWord(word));
> 
> Same comment here about the move

I'll fix this as well.

[Martin Robinson]

Created attachment 252506 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:66:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:67:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 8 files


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

[Carlos Garcia Campos]

(In reply to comment #9)
> Comment on attachment 252490 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252490&action=review
>
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:848
> >> +    g_ptr_array_add(languagesToReturn.get(), nullptr);
> > 
> > Since languagesToReturn is static, shouldn't we avoid all this when != nullptr?
> 
> It's static, but it can change between invocations of
> webkit_web_context_get_spell_checking_languages, so we have to update it
> each time.

Ah, I see.

> >> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:69
> >> +        textCheckerState.isGrammarCheckingEnabled = false;;
> > 
> > We could probably simply add { false } to TextCheckerState.h. There's double ; there
> 
> I'm not sure if false is an appropriate default value for all platforms
> though. Would it be okay to simply set it explicitly for now?

Fair enough.

> >> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:84
> >> +        contexts[i]->textCheckerStateChanged();
> > 
> > So, this is indeed per context, then. We can enable/disable spell checking per context?
> 
> Yes, the WebCore layer supports per-context settings, but not the WebKit2
> layer. It uses a singleton to deal with all web processes.

I think this is because windows and mac don't expose that in the API, but use the global settings of the operating system. Spellcheking is enabled/disabled per context, and then all other messages happen in WebPageProxy, and the actual spellchecking is common (assuming a global configuration of languages, of course). So, my point is that we can actually enable/disable spellchecking per context, and only deprecate the languages setting API, making it a global setting somehow. That way, maybe we don't even need a new class, just a global method to deal with languages or even simply document that get/set spellchecking languages affect to all contexts (unless we plan to add more public API to the spellchecker).

[Martin Robinson]

(In reply to comment #12)

> > Yes, the WebCore layer supports per-context settings, but not the WebKit2
> > layer. It uses a singleton to deal with all web processes.
> 
> I think this is because windows and mac don't expose that in the API, but
> use the global settings of the operating system. Spellcheking is
> enabled/disabled per context, and then all other messages happen in
> WebPageProxy, and the actual spellchecking is common (assuming a global
> configuration of languages, of course). So, my point is that we can actually
> enable/disable spellchecking per context, and only deprecate the languages
> setting API, making it a global setting somehow. That way, maybe we don't
> even need a new class, just a global method to deal with languages or even
> simply document that get/set spellchecking languages affect to all contexts
> (unless we plan to add more public API to the spellchecker).

The Mac port could disable spell check per-context as well, because it is a property of the WebCore Editor implementation, but I think you're right that spell checking is a system setting for Mac. I think it might be a bit beyond the scope of this patch to implement though, because it requires modifying WebProcessPool::textCheckerStateChanged to fetch the state per-context. This patch mainly preserves original behavior.

That isn't to say it's impossible or extremely difficult, but I wonder about the value of splitting the settings or adding a lot of complication for this case.

[Carlos Garcia Campos]

(In reply to comment #13)
> (In reply to comment #12)
> 
> > > Yes, the WebCore layer supports per-context settings, but not the WebKit2
> > > layer. It uses a singleton to deal with all web processes.
> > 
> > I think this is because windows and mac don't expose that in the API, but
> > use the global settings of the operating system. Spellcheking is
> > enabled/disabled per context, and then all other messages happen in
> > WebPageProxy, and the actual spellchecking is common (assuming a global
> > configuration of languages, of course). So, my point is that we can actually
> > enable/disable spellchecking per context, and only deprecate the languages
> > setting API, making it a global setting somehow. That way, maybe we don't
> > even need a new class, just a global method to deal with languages or even
> > simply document that get/set spellchecking languages affect to all contexts
> > (unless we plan to add more public API to the spellchecker).
> 
> The Mac port could disable spell check per-context as well, because it is a
> property of the WebCore Editor implementation, but I think you're right that
> spell checking is a system setting for Mac. I think it might be a bit beyond
> the scope of this patch to implement though, because it requires modifying
> WebProcessPool::textCheckerStateChanged to fetch the state per-context. This
> patch mainly preserves original behavior.
> 
> That isn't to say it's impossible or extremely difficult, but I wonder about
> the value of splitting the settings or adding a lot of complication for this
> case.

I agree we shouldn't change the behaviour as part of this patch, this patch isn't deprecating any API either, so let's re-think the whole thing after the cleanup :-) I'll review this patch again soon.

[Carlos Garcia Campos]

Comment on attachment 252506 [details]
Patch

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

> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:47
> +#if defined(DEVELOPMENT_BUILD)

Change this before landing, please, see bug https://bugs.webkit.org/show_bug.cgi?id=144746

> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:67
> +        textCheckerState.isContinuousSpellCheckingEnabled = false;;
> +        textCheckerState.isGrammarCheckingEnabled = false;;

Still the double ; here

> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:81
> +    const Vector<WebProcessPool*>& contexts = WebProcessPool::allProcessPools();
> +    for (size_t i = 0; i < contexts.size(); ++i)

for (const auto& processPool : allProcessPools())

> Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:180
> +#if USE(UNIFIED_TEXT_CHECKING)

This is unconditionally enabled for GTK, we can get rid of the ifdefs.

[Martin Robinson]

Thanks for the review!

[Martin Robinson]

Comment on attachment 252506 [details]
Patch

Clearing flags on attachment: 252506

Committed r183936: <http://trac.webkit.org/changeset/183936>

[Martin Robinson]

All reviewed patches have been landed.  Closing bug.

[Martin Robinson]

(In reply to comment #16)
> Thanks for the review!

Ack! The comments didn't show up in my mail client for some reason. I'll land a followup patch.

[Martin Robinson]

Landed followup of shame here: http://trac.webkit.org/changeset/183949

[Michael Catanzaro]

I think this broke an API test:

  /webkit2/WebKitWebContext/spell-checker:                             **

ERROR:../../Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:248:void testWebContextSpellChecker(Test*, gconstpointer): assertion failed: (!currentLanguage)

FAIL

[Martin Robinson]

(In reply to comment #21)
> I think this broke an API test:
> 
>   /webkit2/WebKitWebContext/spell-checker:                             **
> 
> ERROR:../../Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:
> 248:void testWebContextSpellChecker(Test*, gconstpointer): assertion failed:
> (!currentLanguage)
> 
> FAIL

Patch here: https://bugs.webkit.org/show_bug.cgi?id=144828