Bug 144648

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

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

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

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

Created attachment 252490 [details] Patch

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.

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.

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

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.

Created attachment 252506 [details] Patch

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.

(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).

(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.

(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.

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.

Thanks for the review!

Comment on attachment 252506 [details] Patch Clearing flags on attachment: 252506 Committed r183936: <http://trac.webkit.org/changeset/183936>

All reviewed patches have been landed. Closing bug.

(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.

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

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

(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